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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Anton Obzhirov <a.obzhirov at samsung.com>  2013-02-07 09:53:09 PST ---
(From update of attachment 186591)
View in context: https://bugs.webkit.org/attachment.cgi?id=186591&action=review

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1645
>> +}
> 
> 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?

I could do that. I thought if we add webkit_web_view_set_page_visibility to WebKit2 API, shouldn’t we add symmetrically webkit_web_view_get_page_visibility?

>> 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.

Well, it basically means if the flag is set then override current visibility state with @visibility_state. Will update documentation here.

>> 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.

Ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:170
>> + * 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.

ok

>> 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?

As far as I know no, they test only visible/hidden states. I was more thinking about future releases.

>> 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.

ok

>> 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.

ok

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