Application can want to get contents size of current web page without monitoring "contents,size,changed" signal, so I add API to get contents size directly.
Created attachment 228462 [details] Patch
Comment on attachment 228462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:732 > + EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false); Isn't it better to check if ewkView is null ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:915 > +EAPI Eina_Bool ewk_view_contents_size_get(Evas_Object *o, Evas_Coord *width, Evas_Coord *height); Missing *const* for _get()
Comment on attachment 228462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1095 > + EXPECT_EQ(contentsHeight, environment->defaultHeight()); Can't we add more test case? For example, you may load a page with a absolute width and height content.
Comment on attachment 228462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review Thanks for review :) >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:732 >> + EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false); > > Isn't it better to check if ewkView is null ? EWK_VIEW_IMPL_GET_OR_RETURN checks if ewkView is null :) >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:915 >> +EAPI Eina_Bool ewk_view_contents_size_get(Evas_Object *o, Evas_Coord *width, Evas_Coord *height); > > Missing *const* for _get() I will add. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1095 >> + EXPECT_EQ(contentsHeight, environment->defaultHeight()); > > Can't we add more test case? For example, you may load a page with a absolute width and height content. Yes, I can add more test cases.
(In reply to comment #4) > (From update of attachment 228462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review > > Thanks for review :) > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:732 > >> + EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false); > > > > Isn't it better to check if ewkView is null ? I meant it would be good if EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false) is called first rather than below 727 if (width) 728 *width = 0; 729 if (height) 730 *height = 0;
Comment on attachment 228462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:732 >>>> + EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false); >>> >>> Isn't it better to check if ewkView is null ? >> >> EWK_VIEW_IMPL_GET_OR_RETURN checks if ewkView is null :) > > I meant it would be good if EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false) is called first rather than below > 727 if (width) > 728 *width = 0; > 729 if (height) > 730 *height = 0; I understand. You mean you want to clear width and height only we fail to get impl. right? I can change codes.
(In reply to comment #6) > (From update of attachment 228462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228462&action=review > > >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:732 > >>>> + EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false); > >>> > >>> Isn't it better to check if ewkView is null ? > >> > >> EWK_VIEW_IMPL_GET_OR_RETURN checks if ewkView is null :) > > > > I meant it would be good if EWK_VIEW_IMPL_GET_OR_RETURN(ewkView, impl, false) is called first rather than below > > 727 if (width) > > 728 *width = 0; > > 729 if (height) > > 730 *height = 0; > > I understand. > > You mean you want to clear width and height only we fail to get impl. right? > I can change codes. Exactly !
Created attachment 228466 [details] Patch
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 > + *width = 0; If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ?
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 >> + *width = 0; > > If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? Ah, actually it is efl style. efl's functions to get value from pointer initialize value to 0. :)
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 >>> + *width = 0; >> >> If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? > > Ah, actually it is efl style. > > efl's functions to get value from pointer initialize value to 0. :) That is convenient to API user.
(In reply to comment #10) > (From update of attachment 228466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 > >> + *width = 0; > > > > If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? > > Ah, actually it is efl style. > > efl's functions to get value from pointer initialize value to 0. :) Even when ewkView is null ?
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1093 > + EXPECT_EQ(contentsWidth, environment->defaultWidth()); First parameter should be a expected result and second should be an actual result. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1100 > + EXPECT_EQ(contentsWidth, environment->defaultWidth() / 2); Is this right operation that content size is changed only we changed the device pixel ration?
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review >>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 >>>>> + *width = 0; >>>> >>>> If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? >>> >>> Ah, actually it is efl style. >>> >>> efl's functions to get value from pointer initialize value to 0. :) >> >> That is convenient to API user. > > Even when ewkView is null ? Yes, right. for example, evas_object_geometry_get() API does. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1093 >> + EXPECT_EQ(contentsWidth, environment->defaultWidth()); > > First parameter should be a expected result and second should be an actual result. ok, I will change. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1100 >> + EXPECT_EQ(contentsWidth, environment->defaultWidth() / 2); > > Is this right operation that content size is changed only we changed the device pixel ration? yes, if device pixel ratio is changed, view size is changed by dividing by ratio. So, contents layouts for the changed view size. of course, it is not affect to the fixed size web contents.
Created attachment 228478 [details] Patch
(In reply to comment #14) > (From update of attachment 228466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review > > >>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 > >>>>> + *width = 0; > >>>> > >>>> If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? > >>> > >>> Ah, actually it is efl style. > >>> > >>> efl's functions to get value from pointer initialize value to 0. :) > >> > >> That is convenient to API user. > > > > Even when ewkView is null ? > > Yes, right. for example, evas_object_geometry_get() API does. If so, we need to add this initialization to API description as well.
Comment on attachment 228466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228466&action=review >>>>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:731 >>>>>>> + *width = 0; >>>>>> >>>>>> If ewkView is null, I think we don't need to initialize width and height with 0. Has ewk_view_contents_size_get() a role to initialize width and height regardless of ewkView ? >>>>> >>>>> Ah, actually it is efl style. >>>>> >>>>> efl's functions to get value from pointer initialize value to 0. :) >>>> >>>> That is convenient to API user. >>> >>> Even when ewkView is null ? >> >> Yes, right. for example, evas_object_geometry_get() API does. > > If so, we need to add this initialization to API description as well. OK, I will add that to the description.
Created attachment 228481 [details] Patch
Comment on attachment 228481 [details] Patch LGTM, but someone else might wanna have final look before landing.
Comment on attachment 228481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228481&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:909 > + * If process to get size fails, values of width and height will be set to 0. I would suggest to rewrite the description like this. If it fails to get the content size, the width and height will be set to 0.
(In reply to comment #20) > (From update of attachment 228481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228481&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:909 > > + * If process to get size fails, values of width and height will be set to 0. > > I would suggest to rewrite the description like this. > If it fails to get the content size, the width and height will be set to 0. Thanks, I will modify like that.
Committed r166766: <http://trac.webkit.org/changeset/166766>