[webkit-reviews] review denied: [Bug 69127] [EFL] Enabling the Page Visibility API : [Attachment 110107] Patch

[bugzilla-daemon at webkit.org]

Leandro Pereira <leandro at profusion.mobi> has denied Dongwoo Joshua Im
<dw.im at samsung.com>'s request for review:
Bug 69127: [EFL] Enabling the Page Visibility API
https://bugs.webkit.org/show_bug.cgi?id=69127

Attachment 110107: Patch
https://bugs.webkit.org/attachment.cgi?id=110107&action=review

------- Additional Comments from Leandro Pereira <leandro at profusion.mobi>
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.