Bug 54181 – Implement Page Visibility API.

• Component: New Bugs
• Status: RESOLVED FIXED

[Shishir Agrawal]

<< Proposal details >>

Hi all,

There is currently no good way for a web page to detect that it is
completely invisible to the user (for example, that it is a background tab),
although some imperfect heuristics do exist. In the future, there may be
cases where such detection is even more important, for example in the
prerendering feature that Chromium is currently in the early stages of
experimentation with.

Note that an earlier version of this proposal was sent to the what-wg
mailing list for comment earlier (
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-December/029382.html),
and received comments that led to several revisions. The following proposal
introduces only a minor change on top of the result of that discussion
(specifically, renaming document.visibility to document.visibilityState and
document.isVisible to document.visible, in order to encourage developers to
prefer using the simpler boolean property).

Use cases

* A page wants to detect when it is being prerendered so it can behave
appropriately.
* A puzzle game has a timer that keeps track of how long the user has taken
to solve the puzzle.  It wants to pause the timer when the user has hidden
the tab.
* A web app that uses polling to fetch dynamic content can pause polling
when it knows the page is hidden from the user.
* A streaming video site doesn’t want to start the video until the user
actually views the tab for the first time (i.e. video shouldn’t start
automatically if a user opens the tab in the background).
* An in-browser collaborative editing environment wants to update a user’s
status to away when the editing surface is not visible to the user.


With these use-cases in mind, there are a number of requirements.

Requirements

* Easy for developers to write scripts that fall back on old behaviors for
browsers that do not implement this API
* Ability to query the document’s current visibility state
* Events fired when the document transitions between visibility states
* Ability for browser vendors to add new visibility states in the future


Proposed API

document.visible

Returns true if document.visibilityState’s current value is in the set of
visibility states considered to be visible (see the next section for
information on document.visibilityState).  In practice document.visible is
merely a convenience property that is well-suited to simple uses. In most
implementations, only the “visible” state is considered visible--although
some implementations may consider other values to be visible as well (for
example, an implementation that makes use of nearly-full-size thumbnail
previews may consider “preview” to be a visible state).


document.visibilityState

A read-only property that returns a string, one of the values described in
the next section.  Developers can use the existence of this property to know
that they can rely on the rest of this API, too.

* Values returned by all conforming implementations
    * “visible” : the full-size page content may be at least partially
visible on at least one screen.
    * “hidden” : the full-size page content is not visible to the user at
all.
* Additional values potentially returned by some implementations in some
cases
    * “prerender” : the page is currently being loaded off-screen and might
never be shown to the user.
    * “cache” : the page is currently “frozen” in a cache and not displayed
on screen (e.g. the back-forward cache).
    * “preview” : the page is currently visible only in a lower-resolution
thumbnail.

States in the second set are not guaranteed to be returned in all cases
where they might otherwise appear to apply--it is left to the discretion of
the implementation.

Additional return values may be added to this API in the future.

It is up to the implementation to interpret what these values mean in the
precise context of interface and platform.  As an example, a
current-generation desktop browser might interpret the values the following
way:
“visible” : the tab is focused in its non-minimized window (regardless of
the focus state of the containing window).
“hidden” : the tab is backgrounded within its window, or the containing
window is minimized.


visibilitychange

A simple event, fired at the document object immediately after
document.visibilityState transitions between visibility states.  The event
has a property, fromState, that is set to the value of
document.visibilityState just before it was changed to the current value.
 Note that visibility has nothing to do with whether the document’s contents
have fully loaded or not, which implies that for any given visibility
transition event, onload may or may not have already fired.

[Shishir Agrawal]

Created attachment 82102 [details]
Patch

[Shishir Agrawal]

Created attachment 82295 [details]
Patch

Fixing a unused var warning that resulted from the previous patch.

[Tony Gentilcore]

The main question I have is whether this API needs to start out prefixed: webkitVisibilityState instead of visibilityState. The whatwg thread didn't seem very finalized so I bet this will go through changes.

Some questions on the API proposal:
- I think we should remove the .visible property. It is just a
convenience method that a web app can trivially determine from
visibilityState. Typically web APIs shoot for the smallest API surface
area composed of the most atomic building blocks.
- I think visibilityChange should be named visibilityStateChange. That
ties it more definitively to the visibilityState attribute, similar to
readyState and readyStateChange.
- I don't understand how visibilityState=="cache" could ever be
reached. If the page is frozen in a cache, then by definition no
JavaScript can be running to observe the property.

[Shishir Agrawal]

Hi Tony,

I have added the webkit prefix and also changed visibilityChange to visibilityStateChange in the latest patch.

- I think we should remove the .visible property. It is just a
convenience method that a web app can trivially determine from
visibilityState. Typically web APIs shoot for the smallest API surface
area composed of the most atomic building blocks.

There seems to be some debate about this. Some people feel we should only keep the bool value since most of the web developers would only use it and not worry about the actual states.

[Shishir Agrawal]

Created attachment 84908 [details]
Patch

Addressing Tony comments about the webkit prefix.

[Simon Fraser (smfr)]

Has this API been proposed to a working group? Are other browsers going to implement it?

[Shishir Agrawal]

The web performance working group is looking into standardizing the spec of the visibility api. I am not sure if other browsers plan to implement this yet.

[Tony Gentilcore]

This was discussed at the web perf working group face-to-face last Friday. The group decided to update its charter to include working on the problem of allowing a page to determine its visibility state. The intent is to start with this as a first draft (co-edited by Google/Microsoft). It seems reasonable to me to begin landing a vendor prefixed and flag guarded implementation.

Minutes: http://lists.w3.org/Archives/Public/public-web-perf/2011Apr/0023.html

[Tony Gentilcore]

Comment on attachment 84908 [details]
Patch

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 84908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84908&action=review

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

[Shishir Agrawal]

Created attachment 89874 [details]
Patch

[Shishir Agrawal]

Comment on attachment 89874 [details]
Patch

Uploaded to help debug linking issues.

[Shishir Agrawal]

Created attachment 90455 [details]
Patch

Addressing Tony's and Darin's comments.

[Shishir Agrawal]

Comment on attachment 84908 [details]
Patch

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.

[Build Bot]

Attachment 90455 [details] did not build on win:
Build output: http://queues.webkit.org/results/8487210

[Tony Gentilcore]

Comment on attachment 90455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90455&action=review

> LayoutTests/fast/events/page-visibility-iframe-test.html:15
> +function log(string) {

Rather than reimplementing these in tests, it would be better to include ../js/resources/js-test-pre.js and use debug() if you just want to output lines. If possible, it is even better to use shouldBe*() methods.

> LayoutTests/fast/events/page-visibility-iframe-test.html:30
> +    layoutTestController.dumpAsText();

You can also remove this once you include js-test-pre.

Also, usually there is a description() call (or just inline text) which has a sentence about the goal of the test.

> LayoutTests/fast/events/page-visibility-transition-test.html:35
> +//  1`- hidden (should fire event).

Stray tick after the 1 (here and elsewhere)

> LayoutTests/fast/events/page-visibility-transition-test.html:50
> +            log("PASS 1: hidden");

Nowhere is webkitIsVisible() tested. All the existing tests could be updated to test both properties. Something along the lines of:

shouldBe("document.webkitVisibilityState()", "hidden");
shouldBeFalse("document.webkitIsVisible()");

> Source/WebCore/dom/Document.cpp:1338
> +    // with the frame, we will consider the document to be invisible.

This comment should answer "why" rather than just restating the condition below.

> Source/WebCore/dom/Document.h:381
> +    PageVisibilityState webkitVisibilityStateEnum() const;

This should be private, and I'd recommend just renaming it to visibilityState() or something.

> Source/WebCore/page/Page.cpp:955
> +void Page::setInitialVisibilityState(PageVisibilityState visibilityState)

This is never used. Is that in a subsequent patch?

> Source/WebKit/chromium/features.gypi:78
> +        'ENABLE_PAGE_VISIBILITY_API=1',

Have you run all the tests? I'm surprised that the new DOM API doesn't cause an existing test to fail. There are a some tests which just iterate through all the properties on window, etc.

[Shishir Agrawal]

Created attachment 91388 [details]
Patch

Addressing Tony's comments.

[Shishir Agrawal]

Comment on attachment 90455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90455&action=review

>> LayoutTests/fast/events/page-visibility-iframe-test.html:15
>> +function log(string) {
> 
> Rather than reimplementing these in tests, it would be better to include ../js/resources/js-test-pre.js and use debug() if you just want to output lines. If possible, it is even better to use shouldBe*() methods.

Done.

>> LayoutTests/fast/events/page-visibility-iframe-test.html:30
>> +    layoutTestController.dumpAsText();
> 
> You can also remove this once you include js-test-pre.
> 
> Also, usually there is a description() call (or just inline text) which has a sentence about the goal of the test.

Done

>> Source/WebCore/dom/Document.cpp:1338
>> +    // with the frame, we will consider the document to be invisible.
> 
> This comment should answer "why" rather than just restating the condition below.

Changed this to visible. Right now I am not sure what the right value for this is. I have only seen this happen in case of thumbnails and ideally they should have a visibility state of their own.

>> Source/WebCore/dom/Document.h:381
>> +    PageVisibilityState webkitVisibilityStateEnum() const;
> 
> This should be private, and I'd recommend just renaming it to visibilityState() or something.

Done.

>> Source/WebCore/page/Page.cpp:955
>> +void Page::setInitialVisibilityState(PageVisibilityState visibilityState)
> 
> This is never used. Is that in a subsequent patch?

Aah thanks for catching this. Comitted to wrong branch. Brought the changes in.

>> Source/WebKit/chromium/features.gypi:78
>> +        'ENABLE_PAGE_VISIBILITY_API=1',
> 
> Have you run all the tests? I'm surprised that the new DOM API doesn't cause an existing test to fail. There are a some tests which just iterate through all the properties on window, etc.

I see roughly 225 failures with and without my changes. How do I tell if my change is responsible. There are a lot of SVG failures too. Should I make sure tests in some directory pass?

[Simon Fraser (smfr)]

This seems like the kind of new feature that warrants an email to webkit-dev (per discussion at the contributors conference).

[Shishir Agrawal]

Hi Simon,

Here is the thread from webkit-dev
https://lists.webkit.org/pipermail/webkit-dev/2011-March/016162.html

Thanks
Shishir

[Tony Gentilcore]

Comment on attachment 91388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91388&action=review

> LayoutTests/fast/events/page-visibility-iframe-test.html:10
> +if (window.accessibilityController)

Is the accessibilityController guard here and in the other test intentional?

> LayoutTests/fast/events/page-visibility-iframe-test.html:48
> +        debug("Too many visibility transitions");  

Looks like a testFailed() would be better than debug().

> LayoutTests/fast/events/page-visibility-iframe-test.html:51
> +    testPassed("TEST COMPLETE");

This and the notifyDone() call below shouldn't be necessary. You should just need a "var successfullyParsed = true" at the end of the script and call finishJSTest(). Some good background here:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

> LayoutTests/fast/events/page-visibility-transition-test.html:36
> +//  1`- hidden (should fire event).

Still a stray character after the "1"

> LayoutTests/fast/events/page-visibility-transition-test.html:62
> +    }

Should this have a testFailed for >3 changes like the other test?

> LayoutTests/fast/events/resources/page-visibility-iframe-test-subframe.html:35
> +    }

All this work in the subframe makes the test harder to understand and leads to code duplication. Is it possible to structure the test so that the subframe is just a generic empty page and the parent always reaches down through the contentDocument? e.g.

<script>
function startTest()
{
    frames[0].contentDocument.addEventListener("webkitvisibilitystatechange", onChildVisibilityChange, false);
}
</script>
<iframe onload=startTest()>

> LayoutTests/platform/gtk/Skipped:1400
> +media/adopt-node-crash.html

Stray diff?

> Source/WebCore/dom/Document.cpp:1335
> +    // with the frame, we will consider the document to be invisible. This

s/invisible/visible/

> Source/WebCore/dom/Document.cpp:1336
> +    // seems to happen in cases such as thumbnails which we consider as

The thumbnails you're mentioning are chromium-port specific, right? Perhaps a better justification is that visible is considered the default unless the embedder has explicitly indicated that it is hidden. No frame/page means no indication from the embedder.

> Source/WebCore/page/Frame.cpp:709
> +        child->dispatchVisibilityStateChangeEvent();

I'd still like to see some testing of how this API behaves when documents are moved between frames. For instance, it is interesting to verify things like:
- The properties are correct when a document is moved from a visible to hidden frame
- Subsequent child dispatching works properly if a child frame is added or removed during a sibling's visibilitystatechange event.

[Shishir Agrawal]

Comment on attachment 91388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91388&action=review

>> LayoutTests/fast/events/page-visibility-iframe-test.html:10
>> +if (window.accessibilityController)
> 
> Is the accessibilityController guard here and in the other test intentional?

Removed.

>> LayoutTests/fast/events/page-visibility-iframe-test.html:48
>> +        debug("Too many visibility transitions");  
> 
> Looks like a testFailed() would be better than debug().

Done.

>> LayoutTests/fast/events/page-visibility-iframe-test.html:51
>> +    testPassed("TEST COMPLETE");
> 
> This and the notifyDone() call below shouldn't be necessary. You should just need a "var successfullyParsed = true" at the end of the script and call finishJSTest(). Some good background here:
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

Thanks for the pointer. Fixed all tests.

>> LayoutTests/fast/events/page-visibility-transition-test.html:36
>> +//  1`- hidden (should fire event).
> 
> Still a stray character after the "1"

Removed.

>> LayoutTests/fast/events/page-visibility-transition-test.html:62
>> +    }
> 
> Should this have a testFailed for >3 changes like the other test?

Done.

>> LayoutTests/fast/events/resources/page-visibility-iframe-test-subframe.html:35
>> +    }
> 
> All this work in the subframe makes the test harder to understand and leads to code duplication. Is it possible to structure the test so that the subframe is just a generic empty page and the parent always reaches down through the contentDocument? e.g.
> 
> <script>
> function startTest()
> {
>     frames[0].contentDocument.addEventListener("webkitvisibilitystatechange", onChildVisibilityChange, false);
> }
> </script>
> <iframe onload=startTest()>

Done

>> LayoutTests/platform/gtk/Skipped:1400
>> +media/adopt-node-crash.html
> 
> Stray diff?

Removed.

>> Source/WebCore/dom/Document.cpp:1335
>> +    // with the frame, we will consider the document to be invisible. This
> 
> s/invisible/visible/

Done.

>> Source/WebCore/dom/Document.cpp:1336
>> +    // seems to happen in cases such as thumbnails which we consider as
> 
> The thumbnails you're mentioning are chromium-port specific, right? Perhaps a better justification is that visible is considered the default unless the embedder has explicitly indicated that it is hidden. No frame/page means no indication from the embedder.

Added a comment about the visiiblity needing to be explicitly set by the embedder.

>> Source/WebCore/page/Frame.cpp:709
>> +        child->dispatchVisibilityStateChangeEvent();
> 
> I'd still like to see some testing of how this API behaves when documents are moved between frames. For instance, it is interesting to verify things like:
> - The properties are correct when a document is moved from a visible to hidden frame
> - Subsequent child dispatching works properly if a child frame is added or removed during a sibling's visibilitystatechange event.

Added two new tests.

[Shishir Agrawal]

Created attachment 92357 [details]
Patch

Addressing Tony's comments and adding more tests.

[Tony Gentilcore]

Comment on attachment 92357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92357&action=review

Everything else LGTM to me now. Darin, could you double check that you are happy with the chromium API updates?

> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:13
> +window.jsTestIsAsync = true;

Just a consistency thing, we usually just do var jsTestIsAsync instead of window. Same comments in this file apply to other tests.

> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:14
> +window.successfullyParsed = false;

This isn't necessary

> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:52
> +    window.successfullyParsed = true;

This should just be a var successfullyParsed at the end of the inline script block here. finishJSTest will still control the TEST COMPLETE message.

> Source/WebKit/chromium/features.gypi:78
> +        'ENABLE_PAGE_VISIBILITY_API=1',

The upstream tests will run fine with this enabled now. But chromium's build/features_override.gypi will override this flag to disable it. That means when the tests run on the chromium bots they will fail.

So the cleanest course to land this is to:
1 disable it in this patch and skip the new tests for chromium as well
2 submit a chromium patch to enable it there
3 submit a follow-up webkit patch to enable it here and unskip the tests

[Shishir Agrawal]

Comment on attachment 92357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92357&action=review

>> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:13
>> +window.jsTestIsAsync = true;
> 
> Just a consistency thing, we usually just do var jsTestIsAsync instead of window. Same comments in this file apply to other tests.

Done in all.

>> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:14
>> +window.successfullyParsed = false;
> 
> This isn't necessary

Done in all.

>> LayoutTests/fast/events/page-visibility-iframe-delete-test.html:52
>> +    window.successfullyParsed = true;
> 
> This should just be a var successfullyParsed at the end of the inline script block here. finishJSTest will still control the TEST COMPLETE message.

Changed in all.

[Shishir Agrawal]

Created attachment 92449 [details]
Patch

Disabling the ENABLE_PAGE_VISIBILITY_API flag in chromium/features.gypi and skipping the tests in chromium.
Also addressing Tony's comments.

[Brett Wilson (Google)]

Comment on attachment 92449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92449&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:2573
> +    switch (visibilityState) {

It seems like this 10-line block could be written as:
  ASSERT(visibilityState == WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden);
  m_page->setVisibilityState(visibilityState, isInitialState);

[Brett Wilson (Google)]

I just looked at the WebKit stuff and I just had that one comment. I'm not a WebKit reviewer, but FWIW the WebKit stuff LGTM with that.

[Shishir Agrawal]

Comment on attachment 92449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92449&action=review

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2573
>> +    switch (visibilityState) {
> 
> It seems like this 10-line block could be written as:
>   ASSERT(visibilityState == WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden);
>   m_page->setVisibilityState(visibilityState, isInitialState);

Not really, since the two enums are different. The input is WebPageVisibilityState and the other one is PageVisibilityState.

[Brett Wilson (Google)]

Ah, I missed that. Grep that directory for COMPILE_ASSERT_MATCHING_ENUM. What we've done for other places is a cast + a compiler assert to avoid this type of conversion code.

[Shishir Agrawal]

Created attachment 92502 [details]
Patch

Addressing Bretts comments about the enum conversion

[Shishir Agrawal]

Created attachment 92523 [details]
Patch

Missed changelog files in last patch.

[Shishir Agrawal]

Created attachment 92746 [details]
Patch

Updating the patch for commit.

[Shishir Agrawal]

Hi Tony,

Since Brett has also reviewed the CL for the chrome port changes, can you commit this?

Thanks
Shishir

[WebKit Commit Bot]

Comment on attachment 92746 [details]
Patch

Clearing flags on attachment: 92746

Committed r86047: <http://trac.webkit.org/changeset/86047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 92746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92746&action=review

> Source/WebCore/page/PageVisibilityState.cpp:38
> +String GetPageVisibilityStateString(PageVisibilityState state)

Function names must begin with a lowercase letter.

[Tony Gentilcore]

> > Source/WebCore/page/PageVisibilityState.cpp:38
> > +String GetPageVisibilityStateString(PageVisibilityState state)
> 
> Function names must begin with a lowercase letter.

Good catch, I'll land a fix.

[WebKit Review Bot]

http://trac.webkit.org/changeset/86047 might have broken GTK Linux 32-bit Debug
The following tests are not passing:
svg/W3C-SVG-1.1/animate-elem-46-t.svg

[Steve Block]

Comment on attachment 92746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92746&action=review

> Source/WebKit/chromium/public/WebView.h:365
> +                                    bool isInitialState) { }

Hi Shishir, can you explain why this method has a default implementation, while almost all of the other methods in this class are pure virtual? Thanks!

[Darin Fisher (:fishd, Google)]

Comment on attachment 92746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92746&action=review

>> Source/WebKit/chromium/public/WebView.h:365
>> +                                    bool isInitialState) { }
> 
> Hi Shishir, can you explain why this method has a default implementation, while almost all of the other methods in this class are pure virtual? Thanks!

Right, it should be pure.  Methods implemented only by WebKit should be pure.  Methods implemented by the embedder should have default implementations (to help with rolling out new APIs).