[Webkit-unassigned] [Bug 54181] Implement Page Visibility API.

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Shishir Agrawal <shishir at chromium.org>  2011-04-20 17:33:49 PST ---
(From update of attachment 84908)
View in context: https://bugs.webkit.org/attachment.cgi?id=84908&action=review

>> Source/WebCore/ChangeLog:6
>> +        https://bugs.webkit.org/show_bug.cgi?id=54181
> 
> We should probably add a link to the spec in the changelog for posterity sake.

Done in the flag change.

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::Page::visibilityState): Retursn current visibility state.
> 
> s/Retursn/Returns/

Removed.

>> Source/WebCore/dom/Document.cpp:1383
>> +        return "visible";
> 
> I believe this constant should be defined with:
> DEFINE_STATIC_LOCAL(const String, visible, ("visible"));
> 
> Also, it isn't clear to me why we report visible when there is no frame or page. Doesn't that likely mean it isn't visible? Perhaps add a comment or something to the changelog explaining why this is considered visible.

Done.

>> Source/WebCore/dom/Document.cpp:1392
>> +void Document::triggerVisibilityStateChangeEvent()
> 
> These methods are usually named dispatch* rather than trigger*.

Changed.

>> Source/WebCore/page/Page.cpp:918
>> +    if (!Document::isVisibilityStateValueValid(visibilityState))
> 
> Passing strings around and checking if they are valid seems hacky. Why don't we create a VisibilityState enum, pass that around, then just convert to strings in the visibilityState() getter (similar to the way ReadyState works). I understand that there may be port-specific values (like "prerender"), but it still would be best to have one enum that is the union of all values used by any port. This forces ports to at least consider all current values when adding new ones.

Done.

>> Source/WebCore/svg/graphics/SVGImage.cpp:267
>> +        // holding Frames and won't know to break the cycle. But 
> 
> Is this comment change intentional? The sentence trails off.

Removed.

>> Source/WebKit/chromium/public/WebView.h:366
>> +#if defined(ENABLE_PAGE_VISIBILITY_API) && ENABLE_PAGE_VISIBILITY_API
> 
> we don't include #ifdefs like these in the public WebKit API headers.  instead, we always
> include the feature in the headers.  the implementation, in WebViewImpl.cpp can be instead
> conditionally compiled.
> 
> If you look at this method as an API that should stand on its own, it has a problem.  No
> where do you define the set of allowed visibility states.  Are you sure the set of states
> should be represented as a string in this API and not an enum?  If the possible set of 
> values here is just visible or hidden, then perhaps this should just be made into a bool
> setter.  In that case, I would also consider moving this down to WebWidget.  This is b/c
> the same "is_hidden" flag is available in Chromium's RenderWidget class, which is a peer
> to WebWidget.

Added an enum. We may have more states than just hidden. Also we may not use the exact same semantics of the is_hidden flag because it is not always accurate.

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