Bug 69127 – [EFL] Enabling the Page Visibility API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

This patch will enabling the Page Visibility API on EFL port by default.
A setter function is added.

[Dongwoo Joshua Im (dwim)]

Created attachment 109253 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 109253 [details]
Patch

I think this patch needs to have _get function as well.

[Dongwoo Joshua Im (dwim)]

I think this funcion is slightly different with traditional getter/setter. Browser(or, other application) doesn't need to query the visibility state of the page to WebKit using getter fuction, because browser itself maintain the visibility status of the page. Browser use the setter API whenever it need to inform the visibility status of the page to WebKit. (eg. when status is changed.)

[Grzegorz Czajkowski]

View in context: https://bugs.webkit.org/attachment.cgi?id=109253&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:3785
> +void ewk_view_visibility_state_set(Evas_Object* o, Ewk_Page_Visibility_State page_visible_state, Eina_Bool initial_state)

Setter functions in WebKit-EFL tend to return Eina_Bool instead of void to notify whether changes were applied.

> Source/WebKit/efl/ewk/ewk_view.cpp:3801
> +    default:

Default statement is unnecessary in this patch.

> Source/WebKit/efl/ewk/ewk_view.cpp:3805
> +    DBG("PAGE_VISIBILITY_API is disabled.\n");

\n will be added by EINA.

> Source/WebKit/efl/ewk/ewk_view.h:2170
> +/// Defines the page visibility status

Missing dot.

> Source/WebKit/efl/ewk/ewk_view.h:2182
> + * @param page_visible_state It is the visible state of the page.

"It is" is not necessary. What do you say about:
@param page_visible_state the visible state of the page to set

> Source/WebKit/efl/ewk/ewk_view.h:2183
> + * @param initial_state It is true only when this function is called at page initialization.

I would rather prefer to use:
@param initial_state @c EINA_TRUE to ... or @c EINA_FALSE to ...

[Dongwoo Joshua Im]

Dear Grzegorz.

I will update the patch regarding your comments.


But there is one issue.
What you said is, "Setter functions in WebKit-EFL tend to return Eina_Bool instead of void to notify whether changes were applied.".
The return type of this setter function is void, because the return type of the "setVisibilityState()" function of WebCore::Page is void.
There is no way to know whether changes were applied in WebCore, so I think it's better to remain the return type of this setter function as "void".

[Gyuyoung Kim]

(In reply to comment #5)
> Dear Grzegorz.
> 
> I will update the patch regarding your comments.
> 
> 
> But there is one issue.
> What you said is, "Setter functions in WebKit-EFL tend to return Eina_Bool instead of void to notify whether changes were applied.".
> The return type of this setter function is void, because the return type of the "setVisibilityState()" function of WebCore::Page is void.
> There is no way to know whether changes were applied in WebCore, so I think it's better to remain the return type of this setter function as "void".

In efl port, we have returned EINA_TRUE or EINA_FALSE despite return type of internal calling function is void. Because, we are using smart data by means of evas object for webview as below,

EWK_VIEW_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
EWK_VIEW_PRIV_GET_OR_RETURN(sd, priv, EINA_FALSE);

So, if we can't get smart data via evas object, we need to return EINA_FALSE to inform this failure to application.

[Dongwoo Joshua Im (dwim)]

Created attachment 109261 [details]
Patch

[WebKit Review Bot]

Attachment 109261 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/efl/ChangeLog'..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

Created attachment 109266 [details]
Patch

I changed the return type of the setter function as 'Eina_Bool'.

[Grzegorz Czajkowski]

View in context: https://bugs.webkit.org/attachment.cgi?id=109266&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:3800
> +        break;

Now you need the default label to return EINA_FALSE if the given state wasn't found.

> Source/WebKit/efl/ewk/ewk_view.h:2181
> + *

Additionally you can describe what are benefits to call this API and when it should be called.

> Source/WebKit/efl/ewk/ewk_view.h:2182
> + * @param page_visible_state the visible state of the page.

You didn't add "to set" :)

[Dongwoo Joshua Im (dwim)]

Created attachment 109273 [details]
Patch

I revised the patch regarding Grzegorz's comments.

[Grzegorz Czajkowski]

LGTM

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 109273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109273&action=review

> ChangeLog:12
> +        * Source/cmake/OptionsEfl.cmake:
> +        * Source/cmakeconfig.h.cmake:

The description above is too generic; please describe what is being changed in these specific files.

> Source/WebKit/efl/ChangeLog:12
> +        Browser(or, other application) should call this newly added function
> +        when page visibility is changed.

Do you mean users should call the API when the page visibility changes or when they want to change it? If it is really the former, how are they informed that this change has happened?

> Source/WebKit/efl/ChangeLog:15
> +        (ewk_view_visibility_state_set): Setter function of EFL port.

Don't you also need a getter?

> Source/WebKit/efl/ewk/ewk_view.h:2183
> + * When the visibility status of page is changed,
> + * this API should be called by the application who use WebKit, such as browser.

Weird line break here, could you make both lines have approximately the same width?

> Source/WebKit/efl/ewk/ewk_view.h:2185
> + * When the visibility status of page is changed,
> + * this API should be called by the application who use WebKit, such as browser.
> + * Then the web application will get the visibility status of the page on which it runs,
> + * it could stop or slow down if the visibility of the page is hidden.

The whole description is a bit unclear to me, can you phrase it in another way?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #13)
> (From update of attachment 109273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109273&action=review
> 
> > ChangeLog:12
> > +        * Source/cmake/OptionsEfl.cmake:
> > +        * Source/cmakeconfig.h.cmake:
> 
> The description above is too generic; please describe what is being changed in these specific files.
> 

I add some more description.

> > Source/WebKit/efl/ChangeLog:12
> > +        Browser(or, other application) should call this newly added function
> > +        when page visibility is changed.
> 
> Do you mean users should call the API when the page visibility changes or when they want to change it? If it is really the former, how are they informed that this change has happened?
> 

If browser call this API, then WebKit will fire a "visibilitychange" event.
Web application need to add an event listener to the "visibilitychange" event, and event handling code.

You can find an example code here. (http://www.w3.org/TR/page-visibility/#introduction)


> > Source/WebKit/efl/ChangeLog:15
> > +        (ewk_view_visibility_state_set): Setter function of EFL port.
> 
> Don't you also need a getter?
> 

I don't think a getter is needed, because browser will not try to get the visibility status from WebKit. The visibility status is maintained by browser.
Browser inform the status to WebKit to fire the "visibilitychange" event which the web application might be listening.

> > Source/WebKit/efl/ewk/ewk_view.h:2183
> > + * When the visibility status of page is changed,
> > + * this API should be called by the application who use WebKit, such as browser.
> 
> Weird line break here, could you make both lines have approximately the same width?
> 

I'll modify this.

> > Source/WebKit/efl/ewk/ewk_view.h:2185
> > + * When the visibility status of page is changed,
> > + * this API should be called by the application who use WebKit, such as browser.
> > + * Then the web application will get the visibility status of the page on which it runs,
> > + * it could stop or slow down if the visibility of the page is hidden.
> 
> The whole description is a bit unclear to me, can you phrase it in another way?

I'll try to write again.

[Dongwoo Joshua Im (dwim)]

Created attachment 109554 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 110107 [details]
Patch

I add a getter function, and rewrite the description in the header file.

[Leandro Pereira]

Comment on attachment 110107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110107&action=review

Informal r-. Comments below.

> ChangeLog:3
> +        [EFL] Enabling the Page Visibility API.

Should be: Enable the Page Visibility API.

> ChangeLog:9
> +        This patch will enable the Page Visibility API on EFL port.
> +        http://www.w3.org/TR/page-visibility/

Should be: Build system changes to support ENABLE(PAGE_VISIBILITY) on EFL port.

> Source/WebKit/efl/ChangeLog:12
> +        This patch will enable the Page Visibility API on EFL port.
> +        http://www.w3.org/TR/page-visibility/
> +
> +        Browser(or, other application) should call this newly added function
> +        when page visibility is changed. 

Should be something like: Implement methods to enable Page Visibility API on EFL port (http://www.w3.org/TR/page-visibility/).

> Source/WebKit/efl/ChangeLog:14
> +        * ewk/ewk_view.cpp: Add settet/getter funcitons of visibility status.

Should be something like: Add setter/getter functions to query/set page visibility state.

> Source/WebKit/efl/ChangeLog:16
> +        (ewk_view_visibility_state_set): Setter function of the visiility status on EFL port.
> +        (ewk_view_visibility_state_get): Getter function of the visiility status on EFL port.

No need to mention the EFL port here, since changeset is all about it. Also, watch out for typos. Try to be more specific, like "Sets the page visibility state" or something like that.

> Source/WebKit/efl/ChangeLog:17
> +        * ewk/ewk_view.h: Add settet/getter funcitons of visibility status.

Add public prototypes.

> Source/WebKit/efl/ewk/ewk_view.cpp:3834
> +    Ewk_Page_Visibility_State status = EWK_PAGE_VISIBILITY_STATE_VISIBLE;

You can get rid of this variable if you return immediately in the switch statement below.

> Source/WebKit/efl/ewk/ewk_view.cpp:3838
> +    EWK_VIEW_SD_GET(o, sd);
> +    if (!sd)
> +        return status;

Use EWK_VIEW_SD_GET_OR_RETURN macro.

> Source/WebKit/efl/ewk/ewk_view.cpp:3841
> +    EWK_VIEW_PRIV_GET(sd, priv);
> +    if (!priv)
> +        return status;

Use EWK_VIEW_PRIV_GET_OR_RETURN macro.

> Source/WebKit/efl/ewk/ewk_view.cpp:3845
> +        status = EWK_PAGE_VISIBILITY_STATE_VISIBLE;

return EWK_PAGE_VISIBILITY_STATE_VISIBLE will allow you to get rid of the status variable and the break statement below.

> Source/WebKit/efl/ewk/ewk_view.h:2195
> + * Sets the visibility state of the page.
> + *
> + * This function let WebKit knows the visibility status of the page.
> + * WebKit will save the current status, and fire a "visibilitychange"
> + * event which web application can listen.

Why should WebKit know about a page's visibility state? Please state this in the apidox.

[Dongwoo Joshua Im (dwim)]

Created attachment 110346 [details]
Patch

[Dongwoo Joshua Im (dwim)]

(In reply to comment #17)

I revised the patch regarding your comments.
Please review the new patch.

Thanks.

[Ryuan Choi]

Comment on attachment 110346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110346&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:3804
> +Eina_Bool ewk_view_visibility_state_set(Evas_Object* o, Ewk_Page_Visibility_State page_visible_state, Eina_Bool initial_state)

Because of style changes, you should use ewkView instead of o.

And also, please change _ to keep webkit coding style.

[Dongwoo Joshua Im (dwim)]

Created attachment 111905 [details]
Patch

Apply new coding style of efl port.

[Ryuan Choi]

(In reply to comment #21)
> Created an attachment (id=111905) [details]
> Patch
> 
> Apply new coding style of efl port.

LGTM.

[Gyuyoung Kim]

Comment on attachment 111905 [details]
Patch

LGTM too.

[Raphael Kubo da Costa (:rakuco)]

Can you please also remove the page visibility tests from LayoutTests/platform/efl/Skipped? They're 4 tests grouped together with a comment indicating they depend on the page visibility api.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #24)
> Can you please also remove the page visibility tests from LayoutTests/platform/efl/Skipped? They're 4 tests grouped together with a comment indicating they depend on the page visibility api.

Yes. I will remove that 4 lines. (exactly 6 lines including comment)
That's not a difficult thing. :)


BTW, does EFL port support layout test now?
As I know, EFL port does not support that yet.

If you have any further information about that, 
please let me know it. Thanks!

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #25)
> BTW, does EFL port support layout test now?
> As I know, EFL port does not support that yet.
> 
> If you have any further information about that, 
> please let me know it. Thanks!

EFL's DumpRenderTree is mostly in SVN (the baselines are still being committed). We just don't have a bot running the layout tests yet.

[Dongwoo Joshua Im (dwim)]

Created attachment 112813 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 112814 [details]
Patch

I remove all of the test cases of page visibility from platform/efl/Skipped.

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Gyuyoung Kim]

Comment on attachment 112814 [details]
Patch

LGTM too.

[Gyuyoung Kim]

Comment on attachment 112814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112814&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:3880
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, sd, EINA_FALSE);

Oops. this patch has a nit. We don't use abbreviation anymore. Change sd with smartData.

[Dongwoo Joshua Im (dwim)]

Created attachment 113415 [details]
Patch

I use 'smartData' instead of 'sd'.
I will carefully follow the new coding style. :)

[Dongwoo Joshua Im (dwim)]

Created attachment 113434 [details]
Patch

[Gyuyoung Kim]

LGTM.

[Adam Barth]

Comment on attachment 113434 [details]
Patch

Looks reasonable.

[WebKit Review Bot]

Comment on attachment 113434 [details]
Patch

Clearing flags on attachment: 113434

Committed r99155: <http://trac.webkit.org/changeset/99155>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.