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

[bugzilla-daemon at webkit.org]

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


Tony Gentilcore <tonyg at chromium.org> changed:

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




--- Comment #9 from Tony Gentilcore <tonyg at chromium.org>  2011-04-11 17:53:59 PST ---
(From update of attachment 84908)
View in context: https://bugs.webkit.org/attachment.cgi?id=84908&action=review

Some initial comments/questions.

BTW, I believe you should also add the flag to:
http://trac.webkit.org/browser/trunk/WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops

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

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

s/Retursn/Returns/

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

> Source/WebCore/dom/Document.cpp:1392
> +void Document::triggerVisibilityStateChangeEvent()

These methods are usually named dispatch* rather than trigger*.

> Source/WebCore/page/Frame.cpp:676
> +    for (Frame* child = tree()->firstChild(); child; child = child->tree()->nextSibling())

This is really begging for a layout test. Since JavaScript runs when the event is dispatched, it has a chance to manipulate the frame tree. In other words, we should be sure it works as expected when the visibilitystatechange event adds or removes a frame.

We can still add tests along with the code change, you can verify they pass locally, but skip them or expect them to fail when landing. On a higher note, since this patch touches so many build files it will be difficult to keep in sync. Up to you, but it might be easier to just add the flag-guard first, then do the implementation separately. Also, if the flag were flipped for chromium before the implementation patch so that the tests could be enabled for chromium when it lands.

> Source/WebCore/page/Page.cpp:120
> +#if ENABLE(PAGE_VISIBILITY_API)

Varying the number of arguments in the ctor seems a little awkward. Is it really necessary to know this in the ctor? Perhaps the default is always visible and then there is a setter to update it? There could be a param to suppress dispatching the change event on the initial set.

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

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

> Source/WebKit/chromium/public/WebView.h:368
> +    virtual void setVisibilityState(const WebString&) = 0;

fishd likes to review all changes to the chromium/webkit api. He should take a look at this patch 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.