Bug 81355

Created attachment 132289 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 132289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132289&action=review > Source/WebCore/ChangeLog:8 > + No need for no new tests as we are just updating the spec implementation. I might have said that this change is covered by fast/events/page-visibility-transition-test.html > Source/WebKit/chromium/public/WebPageVisibilityState.h:41 > - WebPageVisibilityStatePrerender > + WebPageVisibilityStatePreview Does this need a Chromium-side change to avoid breaking downstream?

(In reply to comment #3) > I might have said that this change is covered by fast/events/page-visibility-transition-test.html I will fix it before landing, thanks. > > > Source/WebKit/chromium/public/WebPageVisibilityState.h:41 > > - WebPageVisibilityStatePrerender > > + WebPageVisibilityStatePreview > > Does this need a Chromium-side change to avoid breaking downstream? Does it?!

Comment on attachment 132289 [details] Patch Looks like there's one instance in src/chrome/renderer/chrome_content_renderer_client.cc

(In reply to comment #5) > (From update of attachment 132289 [details]) > Looks like there's one instance in src/chrome/renderer/chrome_content_renderer_client.cc =/ How do you guys usually handle this? Should I: 1- Keep WebPageVisibilityStatePrerender in WebKit/chromium/public/ and add a check to its WebViewImpl for backward compatibility? 2- Wait for a patch to land in Chrome's trunk before landing this? or 3- Patch chrome_content_renderer_client.cc and submit it somehow?

There's a common pattern: 1) Land this patch with an ifdef to select between the new and old names (defaulting to the old name). 2) Wait for a WebKit roll. 3) Change downstream to use the new name (and to set the define in the files that need it). 4) Remove the define from WebKit. 5) Wait for a WebKit roll. 6) Remove the define from downstream. In some cases, you might need to update the version of Chromium in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS between steps (3) and (4).

> In some cases, you might need to update the version of Chromium in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS between steps (3) and (4). Note: This step isn't necessary for this patch.

This patch implements functionality that disagrees with the spec. The spec says the following about the PREVIEW state: > This attribute is optional. This attribute may return document.PAGE_PREVIEW if the Document contained by the top level browsing context is not visible at all and a preview of the the Document is visible. Note that it says "AND a preview of the Document is visible." That is not what's going on with prerender--the document is not visible, but it is also not visible in ANY form. If anything, the change according to the spec should be to change it to return "webkit-prerender", according to section 4.4. However, even that change would have implications for sites that have already started expecting the "prerender" return value. As one example, if you inspect the minified Google Analytics code (http://www.google-analytics.com/ga.js) you'll see that it has logic around the "prerender" return value. There are surely many other sites that have already come to expect the "prerender" value that was in earlier forms of the spec. This does not imply that we shouldn't make the change--only that there are ramifications to be aware of.

Comment on attachment 132289 [details] Patch Clearing the review flag while we discuss the above.

Thanks for the feedback and reviews! (In reply to comment #10) > This patch implements functionality that disagrees with the spec. I tend to disagree. I'm not trying to replace prerender with preview, but to update our implementation of this spec. For doing that then we do have to remove the prerender state and add the preview one. Perhaps I'm just missing something but I don't see where this patch can possibly be disagreeing with the spec. > Note that it says "AND a preview of the Document is visible." That is not what's going on with prerender--the document is not visible, but it is also not visible in ANY form. When to set a given visibility state is a per-port decision, isn't it? That is why chromium and efl have API for it. Again, the patch is just updating the enums and keywords all over, but it doesn't affect the ports' decision to when apply each state. Therefore, it can't be not following the spec. > This does not imply that we shouldn't make the change--only that there are ramifications to be aware of. Ok, got it. This is my first attempt to fix/implement a HTML5 spec/feature, so I'm not sure how we should handle this situation. I would just go to the "let's stick to the spec" approach, but maybe we should add webkit-prerender and keep compatibility. However, as you said, sites that were using "prerender" still would need to be updated.

Note that I did not implement this feature, so I'm not aware of all of the details of its implementation. Chrome must return "prerender" (or "webkit-prerender") because it is important for web developers to know if their page is being prerendered or not. If my understanding is correct, there is very little Chrome-specific code to do so; it lives in WebKit. This change as-is would thus require a fair bit of hackery on the Chrome side to return "prerender". Again, I don't know this code, but it seems like it would be preferable to _add_ a "preview" value to be returned if a port wants to, but still allow ports that decide to return "prerender". I'm not sure how best to transition web developers from "prerender" to "webkit-prerender" as a return value. We could start by updating the code samples to check for if the return value _contains_ "prerender", but presumably we'd want to give sites a chance to change their code before this goes live for everyone.

(In reply to comment #13) > Chrome must return "prerender" (or "webkit-prerender") because it is important for web developers to know if their page is being prerendered or not. If my understanding is correct, there is very little Chrome-specific code to do so; it lives in WebKit. This change as-is would thus require a fair bit of hackery on the Chrome side to return "prerender". As I told you already, I'm not against the prerender state, I just want to update our implementation so we follow the spec. Currently, just by looking at http://www.w3.org/TR/page-visibility/ I can't find a single mention to neither the terms "prerender" or "webkit-prerender". So maybe I'm just missing something but the spec as it is now published covers only "visible", "hidden" and "preview". And I'm not raising the usefulness of "preview" or "prerender" from web developer perspective. > > Again, I don't know this code, but it seems like it would be preferable to _add_ a "preview" value to be returned if a port wants to, but still allow ports that decide to return "prerender". > > I'm not sure how best to transition web developers from "prerender" to "webkit-prerender" as a return value. We could start by updating the code samples to check for if the return value _contains_ "prerender", but presumably we'd want to give sites a chance to change their code before this goes live for everyone. Ok, so the way to go would be keeping "prerender" (or modifying it to "webkit-prerender"?) and adding "preview" as one more visibility state? Thanks for the help so far!

(In reply to comment #14) > As I told you already, I'm not against the prerender state, I just want to update our implementation so we follow the spec. Currently, just by looking at http://www.w3.org/TR/page-visibility/ I can't find a single mention to neither the terms "prerender" or "webkit-prerender". So maybe I'm just missing something but the spec as it is now published covers only "visible", "hidden" and "preview". And I'm not raising the usefulness of "preview" or "prerender" from web developer perspective. > That's because prerender isn't (currently) part of the spec. However, prerender is a value that ports of WebKit (in particular, Chrome) will return. The spec explicitly defines the way for experimental or non-standard return values to be returned (prefix with the vendor ID). Note also that there are currently discussions about adding prerender back into the spec. > > Ok, so the way to go would be keeping "prerender" (or modifying it to "webkit-prerender"?) and adding "preview" as one more visibility state? Making it so we can return "preview" in _addition_ to "prerender" should be fine.

Created attachment 134651 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 134651 [details] Patch Looks great. Thanks.

Comment on attachment 134651 [details] Patch Thanks for the review!

Comment on attachment 134651 [details] Patch Clearing flags on attachment: 134651 Committed r112664: <http://trac.webkit.org/changeset/112664>

All reviewed patches have been landed. Closing bug.