[Webkit-unassigned] [Bug 69127] [EFL] Enabling the Page Visibility API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69127


Leandro Pereira <leandro at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110107|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #17 from Leandro Pereira <leandro at profusion.mobi>  2011-10-07 11:40:11 PST ---
(From update of attachment 110107)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.