<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 29 October 2014 20:26, Bryce Harrington <span dir="ltr"><<a href="mailto:bryce@osg.samsung.com" target="_blank">bryce@osg.samsung.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Mon, Oct 27, 2014 at 09:14:40AM +0100, Marek Chalupa wrote:<br>
> The callback returns always with the same serial,<br>
> which is not right (it's serial, not constant...).<br>
> This test highlights the bug.<br>
><br>
> Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> ---<br>
>  tests/display-test.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++<br>
>  1 file changed, 55 insertions(+)<br>
><br>
> diff --git a/tests/display-test.c b/tests/display-test.c<br>
> index a1e45b1..eb4ba1c 100644<br>
> --- a/tests/display-test.c<br>
> +++ b/tests/display-test.c<br>
> @@ -593,3 +593,58 @@ TEST(threading_read_after_error_tst)<br>
><br>
>       display_destroy(d);<br>
>  }<br>
> +<br>
> +static void<br>
> +sync_callback(void *data, struct wl_callback *wl_callback, uint32_t serial)<br>
> +{<br>
> +     int *got = data;<br>
> +     static uint32_t last_serial = 0;<br>
> +<br>
> +     /* if this is the first callback, just copy the value of serial */<br>
> +     if (*got == 0)<br>
> +             last_serial = serial;<br>
> +     else<br>
> +             ++last_serial;<br>
> +<br>
> +     /* since we only call display_sync, nothing else can increase the<br>
> +      * serial, so the serails must be sequential */<br>
<br>
</div></div>sp. serials<br>
<span class=""><br>
> +     assert(serial == last_serial<br>
> +            && "Serial is not sequential");<br>
> +<br>
> +     ++(*got);<br>
<br>
</span>I'm a bit confused about the intention of 'got', is it just a counter<br>
for how many times the callback is called?  Would sync_callback_count<br>
be a clearer variable name?<br></blockquote><div><br></div><div>Yes, it is just a counter. I could have chosen better name -- like the one you propose, I'll fix it.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> +}<br>
> +<br>
> +static const struct wl_callback_listener sync_listener = {<br>
> +     sync_callback<br>
> +};<br>
> +<br>
> +#define CB_NUM 1000<br>
> +static void<br>
> +callback_serial_tst_main(void)<br>
> +{<br>
> +     int i, got = 0;<br>
> +     struct wl_callback *cb;<br>
> +     struct client *client = client_connect();<br>
<br>
</span>'client' maybe not a good choice of variable name since it's also the<br>
type name.  'c' seems to be convention here, so maybe:<br></blockquote><div><br><br></div><div>It's used in other tests too, but I don't really mind using 'c' or 'cli' or something else instead of client, so can fix it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
        struct client *c = client_connect();<br>
<span class=""><br>
> +     for (i = 0; i < CB_NUM; ++i) {<br>
> +             cb = wl_display_sync(client->wl_display);<br>
> +             wl_callback_add_listener(cb, &sync_listener, &got);<br>
> +     }<br>
> +<br>
> +     wl_display_flush(client->wl_display);<br>
> +     wl_display_roundtrip(client->wl_display);<br>
> +<br>
> +     assert(got == CB_NUM && "Lost some callback");<br>
> +<br>
> +     client_disconnect(client);<br>
> +}<br>
> +<br>
> +TEST(callback_serial_tst)<br>
> +{<br>
> +     struct display *d = display_create();<br>
> +<br>
> +     client_create(d, callback_serial_tst_main);<br>
> +     display_run(d);<br>
> +<br>
> +     display_destroy(d);<br>
> +}<br>
> --<br>
> 1.9.3<br>
><br>
</span>> _______________________________________________<br>
> wayland-devel mailing list<br>
> <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
<br>
Other than the few quibbles, looks quite good.<br>
And cheers for adding a test with the bugfix.  :-)<br>
<br>
Reviewed-by: Bryce Harrington <<a href="mailto:b.harrington@samsung.com">b.harrington@samsung.com</a>><br>
<span class=""><font color="#888888"><br>
Bryce<br></font></span></blockquote><div><br></div><div>Thanks for reviewing!<br><br>Few days ago I found out that on bugzilla is a bug regarding the serials.<br><a href="https://bugs.freedesktop.org/show_bug.cgi?id=83488">https://bugs.freedesktop.org/show_bug.cgi?id=83488</a><br><br>I added a comment there and I'll wait with the changes to this patch until I get some feedback<br></div><div>(it's quite possible that this patch won't be push at all).<br><br></div><div>Thanks,<br></div><div>Marek<br></div></div><br></div></div>