Hi Pekka, thank you for reviewing and also your remarks. I adjusted my patch accordingly and resent it to ML.
Best regards Michael Teyfel Engineering Software Base (ADITG/ESB) Tel. +49 5121 49 6932 -----Original Message----- From: Pekka Paalanen [mailto:[email protected]] Sent: Mittwoch, 26. Juli 2017 11:18 To: Teyfel, Michael (ADITG/ESB) Cc: [email protected]; Ucan, Emre (ADITG/ESB); Friedrich, Eugen (ADITG/ESB) Subject: Re: [PATCH weston] ivi-shell: Added tests for screen-remove-layer API On Tue, 25 Jul 2017 15:59:06 +0200 Michael Teyfel <[email protected]> wrote: > Two cases are tested: success and fail case of the screen-remove-layer API. > > Signed-off-by: Michael Teyfel <[email protected]> > --- > tests/ivi_layout-internal-test.c | 62 > ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) Hi Michael, thanks for these, they look mostly good, just some polishing required. Comments inline. > > diff --git a/tests/ivi_layout-internal-test.c > b/tests/ivi_layout-internal-test.c > index 37a2356..64380ec 100644 > --- a/tests/ivi_layout-internal-test.c > +++ b/tests/ivi_layout-internal-test.c > @@ -341,6 +341,66 @@ test_layer_source_rectangle(struct test_context > *ctx) } > > static void > +test_screen_remove_layer(struct test_context *ctx) { > + const struct ivi_layout_interface *lyt = ctx->layout_interface; > + struct ivi_layout_layer *ivilayer; > + struct weston_output *output; > + struct ivi_layout_layer **array; > + int32_t length = 0; > + > + ivilayer = lyt->layer_create_with_dimension(IVI_TEST_LAYER_ID(0), 200, > 300); > + iassert(ivilayer != NULL); > + > + if (wl_list_empty(&ctx->compositor->output_list)) > + return; This leaks the ivilayer, possibly affecting also further tests. Move the check before the layer creation. > + > + output = wl_container_of(ctx->compositor->output_list.next, output, > +link); > + > + iassert(lyt->screen_add_layer(output, ivilayer) == IVI_SUCCEEDED); > + lyt->commit_changes(); > + > + iassert(lyt->get_layers_on_screen(output, &length, &array) == > IVI_SUCCEEDED); > + iassert(length == 1); > + iassert(array[0] == ivilayer); > + > + iassert(lyt->screen_remove_layer(output ,ivilayer) == > +IVI_SUCCEEDED); Space on the wrong side of comma. > + lyt->commit_changes(); > + 'array' gets leaked. > + iassert(lyt->get_layers_on_screen(output, &length, &array) == > IVI_SUCCEEDED); > + iassert(length == 0); 'array' would get leaked if there were any layers, but since we fail the test in that case, it doesn't matter. > + > + lyt->layer_destroy(ivilayer); > +} > + > +static void > +test_screen_bad_remove_layer(struct test_context *ctx) { > + const struct ivi_layout_interface *lyt = ctx->layout_interface; > + struct ivi_layout_layer *ivilayer; > + struct weston_output *output; > + > + ivilayer = lyt->layer_create_with_dimension(IVI_TEST_LAYER_ID(0), 200, > 300); > + iassert(ivilayer != NULL); > + > + if (wl_list_empty(&ctx->compositor->output_list)) > + return; The same leak of ivilayer as before. > + > + output = wl_container_of(ctx->compositor->output_list.next, output, > +link); > + > + iassert(lyt->screen_remove_layer(NULL, ivilayer) == IVI_FAILED); > + lyt->commit_changes(); > + > + iassert(lyt->screen_remove_layer(output, NULL) == IVI_FAILED); > + lyt->commit_changes(); > + > + iassert(lyt->screen_remove_layer(NULL, NULL) == IVI_FAILED); > + lyt->commit_changes(); > + > + lyt->layer_destroy(ivilayer); > +} > + > +static void > test_layer_bad_remove(struct test_context *ctx) { > const struct ivi_layout_interface *lyt = ctx->layout_interface; @@ > -951,6 +1011,8 @@ run_internal_tests(void *data) > test_layer_position(ctx); > test_layer_destination_rectangle(ctx); > test_layer_source_rectangle(ctx); > + test_screen_remove_layer(ctx); > + test_screen_bad_remove_layer(ctx); These two would be better grouped with the other screen tests, I think. > test_layer_bad_remove(ctx); > test_layer_bad_visibility(ctx); > test_layer_bad_opacity(ctx); Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
