[Webkit-unassigned] [Bug 109422] [Qt] Add Page Visibility API support

[bugzilla-daemon at webkit.org]

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





--- Comment #54 from Benjamin Dupont <bdupont at nds.com>  2013-08-05 03:50:40 PST ---
(In reply to comment #53)
> (From update of attachment 207926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207926&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136
> > +    // Must match with values of PageVisibilityState enum from PageVisibilityState.h.
> 
> I think we should do a switch-case matching in QWebPageAdapter::setVisibilityState to make sure that the enum values are independent from WebCore. It's usually not a good idea to have the public header follow internal enums directly.
Initially I made a switch-case and then I delete it... :) For the default value, should I return the hidden value?

> 
> > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3161
> > +    \note The initial state is set to PageVisibilityStateHidden if the page visibility API is enabled,
> > +    otherwise is set to PageVisibilityApiUnavailable.
> 
> What about all applications out there not knowing about this new API, isn't it a bad default to have page told that they are hidden even though they are shown?
I agree, in the current implementation if the visibility API isn't enable the visibility attributes are available even so and set to hidden. That's why I used this default value. 

> 
> > Source/WebKit/qt/WidgetApi/qwebpage.h:220
> > +    // Must match with values of PageVisibilityState enum from QWebPageAdapter.h.
> 
> Please leave this information in QWebPageAdapter.h not to pollute the public header.
Ok.

> 
> > Source/WebKit/qt/WidgetApi/qwebpage.h:226
> > +        PageVisibilityApiUnavailable
> 
> We should simply enable the compile-time flag all the time if we're exposing the API. A feature is usually not enabled only if it isn't implemented or ready.
In this case should I remove all PAGE_VISIBILITY_API references:
Source/WTF/wtf/FeatureDefines.h
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/page/Frame.cpp
Source/WebCore/page/Frame.h
Source/WebCore/page/Page.cpp
Source/WebCore/page/Page.h
Source/WebCore/page/PageVisibilityState.cpp
Source/WebCore/page/PageVisibilityState.h
Source/WebCore/page/Settings.cpp
Source/WebCore/page/Settings.h
Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp
Tools/Scripts/webkitperl/FeatureList.pm
Tools/qmake/mkspecs/features/features.pri
And WebKit2 files? 

Or just doing the assumption that this feature is always enabled in QWebPageAdapter?

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