Bug 131148 - [EFL][WK2] Add API to get contents size of current web page.
Summary: [EFL][WK2] Add API to get contents size of current web page.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eunmi Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-02 21:21 PDT by Eunmi Lee
Modified: 2014-04-03 22:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.17 KB, patch)
2014-04-02 21:26 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2014-04-02 22:39 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2014-04-03 00:20 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2014-04-03 00:39 PDT, Eunmi Lee
gyuyoung.kim: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2014-04-02 21:21:47 PDT
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.
Comment 1 Eunmi Lee 2014-04-02 21:26:59 PDT
Created attachment 228462 [details]
Patch
Comment 2 Gyuyoung Kim 2014-04-02 21:33:18 PDT
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 3 Jinwoo Song 2014-04-02 21:33:36 PDT
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 4 Eunmi Lee 2014-04-02 21:39:19 PDT
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.
Comment 5 Gyuyoung Kim 2014-04-02 21:45:18 PDT
(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 6 Eunmi Lee 2014-04-02 21:59:51 PDT
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.
Comment 7 Gyuyoung Kim 2014-04-02 22:12:00 PDT
(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 !
Comment 8 Eunmi Lee 2014-04-02 22:39:55 PDT
Created attachment 228466 [details]
Patch
Comment 9 Gyuyoung Kim 2014-04-02 22:57:20 PDT
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 10 Eunmi Lee 2014-04-02 23:03:01 PDT
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 11 Eunmi Lee 2014-04-02 23:04:46 PDT
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.
Comment 12 Gyuyoung Kim 2014-04-02 23:05:14 PDT
(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 13 Jinwoo Song 2014-04-02 23:42:50 PDT
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 14 Eunmi Lee 2014-04-03 00:13:25 PDT
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.
Comment 15 Eunmi Lee 2014-04-03 00:20:16 PDT
Created attachment 228478 [details]
Patch
Comment 16 Gyuyoung Kim 2014-04-03 00:26:05 PDT
(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 17 Eunmi Lee 2014-04-03 00:27:45 PDT
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.
Comment 18 Eunmi Lee 2014-04-03 00:39:59 PDT
Created attachment 228481 [details]
Patch
Comment 19 Gyuyoung Kim 2014-04-03 20:13:00 PDT
Comment on attachment 228481 [details]
Patch

LGTM, but someone else might wanna have final look before landing.
Comment 20 Jinwoo Song 2014-04-03 20:38:07 PDT
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.
Comment 21 Eunmi Lee 2014-04-03 22:05:28 PDT
(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.
Comment 22 Eunmi Lee 2014-04-03 22:06:01 PDT
Committed r166766: <http://trac.webkit.org/changeset/166766>