Summary: | Add status events on <link rel=prerender> elements. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gavin Peters <gavinp> | ||||||||||||||
Component: | New Bugs | Assignee: | Gavin Peters <gavinp> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, cdumez, dglazkov, fishd, gustavo, jamesr, japhet, philn, syoichi, tkent+wkapi, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Gavin Peters
2012-09-12 00:07:19 PDT
Created attachment 163530 [details]
Patch
See https://chromiumcodereview.appspot.com/10918189 for the chromium side of this. Adam, WDYT? Because of the change to WebPrerenderingSupport, I'll have to stage this, but I wanted to upload this for review as a whole before I split it for that. Comment on attachment 163530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163530&action=review > Source/Platform/chromium/public/WebPrerender.h:69 > + WEBKIT_EXPORT void sendPrerenderStatusEvent(const WebString&); In principle, the platform layer shouldn't have any concept of events, which are a DOM concept. Is this more of a notification from the platform to WebKit? Perhaps it should be named in that style, e.g., didChangeStatus(const WebString& description). > Source/WebCore/dom/PrerenderStatusEvent.cpp:32 > +#include "config.h" Should this entire file be behind an ENABLE(LINK_PRERENDER) flag? > Source/WebCore/dom/PrerenderStatusEvent.idl:34 > +#if !defined(LANGUAGE_CPP) || !LANGUAGE_CPP What's up with these ifdefs? > Source/WebCore/dom/PrerenderStatusEvent.idl:39 > + readonly attribute [TreatReturnedNullStringAs=Null, InitializedByEventConstructor] DOMString status; bad indent > Source/WebCore/html/HTMLLinkElement.h:95 > + virtual void linkLoaded() OVERRIDE; > + virtual void linkLoadingErrored() OVERRIDE; > +#if ENABLE(LINK_PRERENDER) > + virtual void linkPrerenderStatus(const String&) OVERRIDE; I wonder if we should rename these to be more in notification style (e.g., didChangeLinkPrerenderStatus) > Source/WebCore/loader/Prerenderer.cpp:71 > -PassRefPtr<PrerenderHandle> Prerenderer::render(const KURL& url) > +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient* prerenderClient, const KURL& url) prerenderClient -> client > Source/WebCore/platform/chromium/Prerender.cpp:48 > + Looks like you've got an extra line here. > Source/WebCore/platform/chromium/Prerender.h:82 > + PrerenderClient*const m_client; We don't usually worry about the const-ness of clients Where did we get with standards on this patch? Comment on attachment 163530 [details]
Patch
It's a bit strange that the platform provides a string that's directly exposed to the web platform. I would have expected there to be an enum or something like that.
Comment on attachment 163530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163530&action=review > Source/WebCore/dom/PrerenderStatusEvent.idl:41 > + void initPrerenderStatusEvent(in [Optional=DefaultIsUndefined] DOMString typeArg, These sorts of functions are old-fashioned these days. We can just skip them in favor of just using the event constructor. Created attachment 170230 [details]
Patch
Comment on attachment 163530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163530&action=review >> Source/WebCore/dom/PrerenderStatusEvent.cpp:32 >> +#include "config.h" > > Should this entire file be behind an ENABLE(LINK_PRERENDER) flag? I deleted the file. >> Source/WebCore/html/HTMLLinkElement.h:95 >> + virtual void linkPrerenderStatus(const String&) OVERRIDE; > > I wonder if we should rename these to be more in notification style (e.g., didChangeLinkPrerenderStatus) Done. >> Source/WebCore/loader/Prerenderer.cpp:71 >> +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient* prerenderClient, const KURL& url) > > prerenderClient -> client Oh hum. There's already a PrerendererClient, and this client is a PrerenderClient for the Prerender. If I name this automatic client, then what about the method client(), which gives the PrerendererClient? >> Source/WebCore/platform/chromium/Prerender.h:82 >> + PrerenderClient*const m_client; > > We don't usually worry about the const-ness of clients Now I don't, either! Adam, This is a newer version patched up according to your suggestions. How do you think about this approach, and the names I chose? I'd like your opinion on them before I take this to a whatwg proposal. 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. Attachment 170230 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/loader/LinkLoader.h:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 26 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 170230 [details] Patch Attachment 170230 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14496809 Comment on attachment 170230 [details] Patch Attachment 170230 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14490952 Comment on attachment 170230 [details] Patch Attachment 170230 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14530078 Comment on attachment 170230 [details] Patch Attachment 170230 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14524134 Comment on attachment 170230 [details] Patch Attachment 170230 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14526132 Just a note that EWS failures were expected: the change needs to be staged to land properly. Comment on attachment 170230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170230&action=review This looks great. We just need tests and a spec. > Source/Platform/ChangeLog:8 > + The new PrerenderStatusEvent is sent to link elements when prerenders are started by the embedder, and also sent when they are stopped. Pages using this feature can now serialize launching prerenders, and track timing performance. Would you mind wrapping these lines to something sensible? They're just a bit hard to read like this. > Source/Platform/chromium/public/WebPrerender.h:72 > + WEBKIT_EXPORT void didSendLoadForPrerender(); > + WEBKIT_EXPORT void didSendDOMContentLoadedForPrerender(); These are slightly odd concepts to have at the Platform layer because they're DOM concepts, but it's for the DOM of the prerender, so I guess that makes sense. > Source/WebCore/dom/EventNames.h:249 > + macro(webkitPrerenderStart) \ > + macro(webkitPrerenderStop) \ > + macro(webkitPrerenderLoad) \ > + macro(webkitPrerenderDOMContentLoaded) These should be in all lower case. webkitRegionLayoutUpdate is wrong. > Source/WebCore/html/HTMLLinkElement.cpp:380 > +#if ENABLE(LINK_PRERENDER) > +void HTMLLinkElement::didStartLinkPrerender() > +{ > + dispatchEvent(Event::create(eventNames().webkitPrerenderStartEvent, false, false)); > +} > + > +void HTMLLinkElement::didStopLinkPrerender() > +{ > + dispatchEvent(Event::create(eventNames().webkitPrerenderStopEvent, false, false)); > +} > + > +void HTMLLinkElement::didSendLoadForLinkPrerender() > +{ > + dispatchEvent(Event::create(eventNames().webkitPrerenderLoadEvent, false, false)); > +} > + > +void HTMLLinkElement::didSendDOMContentLoadedForLinkPrerender() > +{ > + dispatchEvent(Event::create(eventNames().webkitPrerenderDOMContentLoadedEvent, false, false)); > +} > +#endif Do we want to dispatch these asynchronously? Will the calling code in the embedder realize that they can execute arbitrary JavaScript? It's often better to do that with a clean stack. > Source/WebCore/loader/Prerenderer.cpp:71 > -PassRefPtr<PrerenderHandle> Prerenderer::render(const KURL& url) > +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient* prerenderClient, const KURL& url) I would have just called this parameter "client" Comment on attachment 170230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170230&action=review Tests you say? And a spec? OK, but just this once... I'll advance this and upload it in parallel with sharing the spec on whatwg as you advised me in email. >> Source/Platform/ChangeLog:8 >> + The new PrerenderStatusEvent is sent to link elements when prerenders are started by the embedder, and also sent when they are stopped. Pages using this feature can now serialize launching prerenders, and track timing performance. > > Would you mind wrapping these lines to something sensible? They're just a bit hard to read like this. Done. I get confused by the ad-hoc wrapping in ChangeLog, so rather than make something up, I just didn't wrap. I'll pick one and run with it on my next upload... >> Source/Platform/chromium/public/WebPrerender.h:72 >> + WEBKIT_EXPORT void didSendDOMContentLoadedForPrerender(); > > These are slightly odd concepts to have at the Platform layer because they're DOM concepts, but it's for the DOM of the prerender, so I guess that makes sense. Tricky, yes. These are better names than before. I will try and think of better ones. >> Source/WebCore/dom/EventNames.h:249 >> + macro(webkitPrerenderDOMContentLoaded) > > These should be in all lower case. webkitRegionLayoutUpdate is wrong. Done. >> Source/WebCore/html/HTMLLinkElement.cpp:380 >> +#endif > > Do we want to dispatch these asynchronously? Will the calling code in the embedder realize that they can execute arbitrary JavaScript? It's often better to do that with a clean stack. In the Chromium browser, these are launched from an async event, so the embedder will not be affected either way, and debugging is a bit easier without the asynchronous launch. On that basis, I slightly favour not launching asynchronously, only by a hair since other embedders could benefit from protection. Given that, what do you think? >> Source/WebCore/loader/LinkLoader.h:53 >> + , public PrerenderClient { > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Oh foo. >> Source/WebCore/loader/Prerenderer.cpp:71 >> +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient* prerenderClient, const KURL& url) > > I would have just called this parameter "client" That doesn't compile without changing the call to Prerenderer::client() to explicitly scope the class method. There's too many clients here; the Prerenderer has a client, and this client parameter is for the prerender. Should I rename the Prerenderer::client() method? I thought that was the one that won the fight. (In reply to comment #19) > (From update of attachment 170230 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170230&action=review > > >> Source/WebCore/html/HTMLLinkElement.cpp:380 > >> +#endif > > > > Do we want to dispatch these asynchronously? Will the calling code in the embedder realize that they can execute arbitrary JavaScript? It's often better to do that with a clean stack. > > In the Chromium browser, these are launched from an async event, so the embedder will not be affected either way, and debugging is a bit easier without the asynchronous launch. On that basis, I slightly favour not launching asynchronously, only by a hair since other embedders could benefit from protection. Given that, what do you think? Ok, the way you have it probably fine. That's how we handle notifications from the network stack. > >> Source/WebCore/loader/Prerenderer.cpp:71 > >> +PassRefPtr<PrerenderHandle> Prerenderer::render(PrerenderClient* prerenderClient, const KURL& url) > > > > I would have just called this parameter "client" > > That doesn't compile without changing the call to Prerenderer::client() to explicitly scope the class method. > > There's too many clients here; the Prerenderer has a client, and this client parameter is for the prerender. > > Should I rename the Prerenderer::client() method? I thought that was the one that won the fight. The way you have it is probably fine. Thanks! There's now a standards discussion started at http://wiki.whatwg.org/wiki/Link_prerender_events , and on the whatwg mailing list. I spent some time thinking about testing. I am not sure there's a great story in layout tests here. The interface that sends these events is on WebKit::WebPrerender, part of Platform. I can definitely add thunks in DRTTestRunner for each of these four events, and they'd hope over into the MockWebPrerenderingSupport in DRT.... But that seems like a lot to test this event creation. Perhaps the right story is that the chrome browser_tests test this. The chrome CL (which is not as far along as this one) has tests at http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prerender_browsertest.cc which are full system tests. Created attachment 173064 [details]
Patch
And this latest upload cleans up the ChangeList, and explains away the lack of tests with a hand wave. How's this? I'd like to get to work on staging it... Attachment 173064 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/loader/LinkLoader.h:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 26 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 173064 [details] Patch Attachment 173064 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14760879 Comment on attachment 173064 [details] Patch Attachment 173064 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14771293 Comment on attachment 173064 [details] Patch Attachment 173064 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14755975 Comment on attachment 170230 [details]
Patch
I spoke too soon: this patch is failing tests.
False alarm. After rebasing the chrome patches, I'm passing the browser test now, although no try jobs until more remediation. See http://codereview.chromium.org/10918189/ Comment on attachment 173064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173064&action=review The general approach looks good. We need some tests and for the patch to compile. :) > Source/WebCore/platform/PrerenderClient.h:35 > +#if ENABLE(LINK_PRERENDER) If you like, you can move the ENABLE(LINK_PRERENDER) inside the class definition. That way you can remove the ifdef in the LinkLoader parent class definition. The compiler will optimize away the empty base class. > Source/WebCore/platform/chromium/Prerender.cpp:96 > + if (m_client) Is m_client ever 0? It looks like you initialize it in the constructor and never clear it. > Source/WebCore/platform/chromium/Prerender.h:85 > + PrerenderClient* m_client; Is the lifetime of m_client always longer than that of this class? This class is RefCounted, which makes me worry that it might outlive m_client... In fact, it looks like the embedder can take a reference to Prerender via WebPrerender and keep it alive for an arbitrary length of time. We probably need some way to 0 out this pointer when the PrerenderClient dies. Maybe you have that already and I missed it? Created attachment 178031 [details]
Patch
Comment on attachment 178031 [details]
Patch
Adam, what do you think? I am much happier with this (lifetime is easier to understand). And I also added lots of unit tests.
Comment on attachment 178031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178031&action=review The bulk of this change is fine, but your unit tests are talking to WebCore directly. We try to avoid that whenever possible. Would you be willing to re-work you tests to use the WebKit API? > Source/WebCore/platform/PrerenderClient.h:39 > +#if ENABLE(LINK_PRERENDER) Usually we put these ifdefs around the whole header, but I guess this saves ifdefs elsewhere? > Source/WebKit/chromium/tests/PrerenderingTest.cpp:59 > + TestPrerendererClient(WebView* webView) explicit > Source/WebKit/chromium/tests/PrerenderingTest.cpp:107 > +WebView* staticWebViewForTest() > +{ > + static WebView* webView = 0; > + if (!webView) { > + webView = FrameTestHelpers::createWebViewAndLoad("about:blank"); > + webView->setFocus(true); > + } > + return webView; > +} Please don't use a static WebView. That can cause state leaks between tests. If you copied this pattern from another test, can we remove this pattern from that test in a followup patch? > Source/WebKit/chromium/tests/PrerenderingTest.cpp:113 > +TestPrerendererClient* staticPrerendererClientForTest() > +{ > + static TestPrerendererClient* prerendererClient = new TestPrerendererClient(staticWebViewForTest()); > + return prerendererClient; > +} Please do not use statics. > Source/WebKit/chromium/tests/PrerenderingTest.cpp:322 > + RefPtr<WebCore::PrerenderHandle> handle = prerenderer()->render(&client, toKURL("http://www.foo.com")); We shouldn't talk directly to WebCore in unit tests. We should use the WebKit API to do all of our testing. > Source/WebKit/chromium/tests/PrerenderingTest.cpp:401 > + prerenderer()->suspend(WebCore::ActiveDOMObject::PageWillBeSuspended); This isn't the right way to test this stuff. You should test it via the API. There won't be any way to test suspect today in Chromium because Chromium never calls suspend. Created attachment 178357 [details]
Patch
Created attachment 178358 [details]
Patch
Comment on attachment 178358 [details]
Patch
Thanks for all your reviews Adam. The new unit test is WebKit only, without referencing WebCore. Based on your earlier r+, I'm cq+ing this.
Comment on attachment 178358 [details] Patch Clearing flags on attachment: 178358 Committed r137045: <http://trac.webkit.org/changeset/137045> All reviewed patches have been landed. Closing bug. This patch broke EFL build: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6825/steps/compile-webkit/logs/stdio Comment on attachment 178358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178358&action=review > Source/WebCore/loader/LinkLoader.cpp:93 > +void LinkLoader::didStartPrerender() Looks like those should be protected by a #if ENABLE(LINK_PRERENDER) (In reply to comment #41) > (From update of attachment 178358 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178358&action=review > > > Source/WebCore/loader/LinkLoader.cpp:93 > > +void LinkLoader::didStartPrerender() > > Looks like those should be protected by a #if ENABLE(LINK_PRERENDER) Yes. Want me to land a patch doing that? Build for !ENABLE(LINK_PRERENDER) platforms was fixed by http://trac.webkit.org/changeset/137047 |