Summary: | Implement Page Visibility API. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shishir Agrawal <shishir> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, brettw, buildbot, cmarcelo, commit-queue, donggwan.kim, eric, fishd, kenneth, mike, mjs, ojan, sam, simon.fraser, simonjam, steveblock, tonyg, webkit.review.bot, zalan | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
URL: | http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0175.html | ||||||||||||||||||||||||||
Bug Depends on: | 58464, 60475 | ||||||||||||||||||||||||||
Bug Blocks: | 60576 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Shishir Agrawal
2011-02-10 00:41:18 PST
Created attachment 82102 [details]
Patch
Created attachment 82295 [details]
Patch
Fixing a unused var warning that resulted from the previous patch.
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. 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. Created attachment 84908 [details]
Patch
Addressing Tony comments about the webkit prefix.
Has this API been proposed to a working group? Are other browsers going to implement it? 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. 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 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. 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. Created attachment 89874 [details]
Patch
Comment on attachment 89874 [details]
Patch
Uploaded to help debug linking issues.
Created attachment 90455 [details]
Patch
Addressing Tony's and Darin's comments.
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. Attachment 90455 [details] did not build on win: Build output: http://queues.webkit.org/results/8487210 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. Created attachment 91388 [details]
Patch
Addressing Tony's comments.
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? This seems like the kind of new feature that warrants an email to webkit-dev (per discussion at the contributors conference). Hi Simon, Here is the thread from webkit-dev https://lists.webkit.org/pipermail/webkit-dev/2011-March/016162.html Thanks Shishir 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. 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. Created attachment 92357 [details]
Patch
Addressing Tony's comments and adding more tests.
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 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. 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.
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); 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. 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. 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. Created attachment 92502 [details]
Patch
Addressing Bretts comments about the enum conversion
Created attachment 92523 [details]
Patch
Missed changelog files in last patch.
Created attachment 92746 [details]
Patch
Updating the patch for commit.
Hi Tony, Since Brett has also reviewed the CL for the chrome port changes, can you commit this? Thanks Shishir Comment on attachment 92746 [details] Patch Clearing flags on attachment: 92746 Committed r86047: <http://trac.webkit.org/changeset/86047> 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.
> > 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.
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 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! 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). |