Bug 96474

Summary: Add status events on <link rel=prerender> elements.
Product: WebKit Reporter: Gavin Peters <gavinp>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
gavinp: review-
Patch
none
Patch
none
Patch
none
Patch none

Description Gavin Peters 2012-09-12 00:07:19 PDT
Add PrerenderStatusEvent on <link rel=prerender> elements.
Comment 1 Gavin Peters 2012-09-12 00:09:48 PDT
Created attachment 163530 [details]
Patch
Comment 2 Gavin Peters 2012-09-12 00:12:44 PDT
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 3 Adam Barth 2012-09-12 11:22:29 PDT
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
Comment 4 Adam Barth 2012-09-12 11:23:16 PDT
Where did we get with standards on this patch?
Comment 5 Adam Barth 2012-09-12 11:24:13 PDT
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 6 Adam Barth 2012-10-19 16:44:18 PDT
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.
Comment 7 Gavin Peters 2012-10-23 13:56:51 PDT
Created attachment 170230 [details]
Patch
Comment 8 Gavin Peters 2012-10-23 13:58:25 PDT
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!
Comment 9 Gavin Peters 2012-10-23 13:59:37 PDT
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.
Comment 10 WebKit Review Bot 2012-10-23 13:59:56 PDT
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 11 WebKit Review Bot 2012-10-23 14:00:16 PDT
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 12 Early Warning System Bot 2012-10-23 14:03:53 PDT
Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14496809
Comment 13 Early Warning System Bot 2012-10-23 14:05:50 PDT
Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14490952
Comment 14 EFL EWS Bot 2012-10-23 14:16:19 PDT
Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14530078
Comment 15 Build Bot 2012-10-23 14:34:50 PDT
Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14524134
Comment 16 Build Bot 2012-10-23 15:27:23 PDT
Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14526132
Comment 17 Gavin Peters 2012-10-23 18:02:31 PDT
Just a note that EWS failures were expected: the change needs to be staged to land properly.
Comment 18 Adam Barth 2012-10-24 23:23:12 PDT
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 19 Gavin Peters 2012-11-07 07:33:45 PST
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.
Comment 20 Adam Barth 2012-11-07 10:11:00 PST
(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!
Comment 21 Gavin Peters 2012-11-08 07:41:32 PST
There's now a standards discussion started at http://wiki.whatwg.org/wiki/Link_prerender_events , and on the whatwg mailing list.
Comment 22 Gavin Peters 2012-11-08 10:14:56 PST
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.
Comment 23 Gavin Peters 2012-11-08 10:24:38 PST
Created attachment 173064 [details]
Patch
Comment 24 Gavin Peters 2012-11-08 10:25:21 PST
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...
Comment 25 WebKit Review Bot 2012-11-08 10:26:40 PST
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 26 Early Warning System Bot 2012-11-08 10:31:05 PST
Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14760879
Comment 27 Early Warning System Bot 2012-11-08 10:34:12 PST
Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14771293
Comment 28 EFL EWS Bot 2012-11-08 10:39:58 PST
Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14755975
Comment 29 Gavin Peters 2012-11-08 10:50:25 PST
Comment on attachment 170230 [details]
Patch

I spoke too soon: this patch is failing tests.
Comment 30 Gavin Peters 2012-11-08 11:09:06 PST
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 31 Adam Barth 2012-11-08 18:56:52 PST
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?
Comment 32 Gavin Peters 2012-12-06 10:40:14 PST
Created attachment 178031 [details]
Patch
Comment 33 Gavin Peters 2012-12-06 10:41:56 PST
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 34 Adam Barth 2012-12-06 11:32:48 PST
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.
Comment 35 Gavin Peters 2012-12-08 09:06:58 PST
Created attachment 178357 [details]
Patch
Comment 36 Gavin Peters 2012-12-08 09:11:01 PST
Created attachment 178358 [details]
Patch
Comment 37 Gavin Peters 2012-12-08 09:13:00 PST
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 38 WebKit Review Bot 2012-12-08 12:04:42 PST
Comment on attachment 178358 [details]
Patch

Clearing flags on attachment: 178358

Committed r137045: <http://trac.webkit.org/changeset/137045>
Comment 39 WebKit Review Bot 2012-12-08 12:04:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 41 Chris Dumez 2012-12-08 13:07:24 PST
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)
Comment 42 Gavin Peters 2012-12-08 13:08:30 PST
(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?
Comment 43 Chris Dumez 2012-12-08 13:38:11 PST
Build for !ENABLE(LINK_PRERENDER) platforms was fixed by http://trac.webkit.org/changeset/137047