Bug 131148 – [EFL][WK2] Add API to get contents size of current web page.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 228462 [details]
Patch

[Gyuyoung Kim]

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()

[Jinwoo Song]

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.

[Eunmi Lee]

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.

[Gyuyoung Kim]

(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;

[Eunmi Lee]

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.

[Gyuyoung Kim]

(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 !

[Eunmi Lee]

Created attachment 228466 [details]
Patch

[Gyuyoung Kim]

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 ?

[Eunmi Lee]

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. :)

[Eunmi Lee]

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.

[Gyuyoung Kim]

(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 ?

[Jinwoo Song]

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?

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 228478 [details]
Patch

[Gyuyoung Kim]

(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.

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 228481 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 228481 [details]
Patch

LGTM, but someone else might wanna have final look before landing.

[Jinwoo Song]

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.

[Eunmi Lee]

(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.

[Eunmi Lee]

Committed r166766: <http://trac.webkit.org/changeset/166766>