[Webkit-unassigned] [Bug 97324] [GTK] Add support for Page Visibility

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #186591|review?                     |review-
               Flag|                            |




--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2013-02-06 16:56:23 PST ---
(From update of attachment 186591)
View in context: https://bugs.webkit.org/attachment.cgi?id=186591&action=review

Nice!  I think we should just ask via JavaScript for the page visibility state rather than making API for it. Some more minor comments follow. Didn't we agree that this would be called something like override_visibility state so that we could have a default implementation that relied on whether or not the widget was mapped?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1645
> +WebCore::PageVisibilityState WebPageProxy::visibilityState()
> +{
> +#if ENABLE(PAGE_VISIBILITY_API)
> +    return m_visibilityState;
> +#endif
> +}

Are we adding this WebCore API just for the sake of the unit tests? Why not simply ask JavaScript for the value of the page visibility state there?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955
> + * @initial_state: a flag to specify if the visibility state is initial

Hrm. I'm not sure if I totally understand the initial_state flag here. It could definitely use some more documentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2957
> + * Set the page visibility of the page in @webView to @visibility_state.

It'd be a good idea to expand this documentation and potentially include a link to the specification.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:170
> + * WebKitPageVisibilityState:
> + * @WEBKIT_PAGE_VISIBILITY_STATE_VISIBLE: The document contained by the page is at least partially visible at on at least one screen. This is the same condition under which the hidden attribute is set to false.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_HIDDEN: The document contained by the page is not visible at all on any screen.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_PRERENDER: Optional. The document is prerendered by being loaded off-screen and not visibly shown.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_PREVIEW: Optional. The document contained by the page is not visible at all and a preview of the document is visible.
> + *
> + * Enum values to specify the different visibility states for a #WebKitWebView
> + * web page.

This is a bit of a nit, but adding some newlines here would improve readability of the documentation. You could probably omit "Optional." here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1088
> +// To test page visibility API. Currently only 'visible' and 'hidden' states are implemented fully in WebCore.

If only visible and hidden are implemented in WebCore, maybe it's better to only expose them? Do the layout tests depend on all states?

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:881
> +void DumpRenderTreeSupportGtk::setPageVisibility(WebKitWebView *webView, const char* visibility)

An enum would be better than a string here. Watch that the location of your asterisks is correct in this file.

> Tools/Scripts/webkitperl/FeatureList.pm:371
> +      define => "ENABLE_PAGE_VISIBILITY_API", default => (isBlackBerry() || isEfl() || isGtk()), value => \$pageVisibilityAPISupport },

Since you are enabling it by default you shouldn't need to override it here as well.

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