[webkit-reviews] review denied: [Bug 54181] Implement Page Visibility API. : [Attachment 84908] Patch

[bugzilla-daemon at webkit.org]

Tony Gentilcore <tonyg at chromium.org> has denied Shishir Agrawal
<shishir at chromium.org>'s request for review:
Bug 54181: Implement Page Visibility API.
https://bugs.webkit.org/show_bug.cgi?id=54181

Attachment 84908: Patch
https://bugs.webkit.org/attachment.cgi?id=84908&action=review

------- Additional Comments from Tony Gentilcore <tonyg at chromium.org>
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/FeatureD
efines.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.