Bug 96474 – Add status events on <link rel=prerender> elements.

• Component: New Bugs
• Status: RESOLVED FIXED

[Gavin Peters]

Add PrerenderStatusEvent on <link rel=prerender> elements.

[Gavin Peters]

Created attachment 163530 [details]
Patch

[Gavin Peters]

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.

[Adam Barth]

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

[Adam Barth]

Where did we get with standards on this patch?

[Adam Barth]

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.

[Adam Barth]

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.

[Gavin Peters]

Created attachment 170230 [details]
Patch

[Gavin Peters]

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!

[Gavin Peters]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14496809

[Early Warning System Bot]

Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14490952

[EFL EWS Bot]

Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14530078

[Build Bot]

Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14524134

[Build Bot]

Comment on attachment 170230 [details]
Patch

Attachment 170230 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14526132

[Gavin Peters]

Just a note that EWS failures were expected: the change needs to be staged to land properly.

[Adam Barth]

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"

[Gavin Peters]

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.

[Adam Barth]

(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!

[Gavin Peters]

There's now a standards discussion started at http://wiki.whatwg.org/wiki/Link_prerender_events , and on the whatwg mailing list.

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 173064 [details]
Patch

[Gavin Peters]

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

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14760879

[Early Warning System Bot]

Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14771293

[EFL EWS Bot]

Comment on attachment 173064 [details]
Patch

Attachment 173064 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14755975

[Gavin Peters]

Comment on attachment 170230 [details]
Patch

I spoke too soon: this patch is failing tests.

[Gavin Peters]

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/

[Adam Barth]

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?

[Gavin Peters]

Created attachment 178031 [details]
Patch

[Gavin Peters]

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.

[Adam Barth]

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.

[Gavin Peters]

Created attachment 178357 [details]
Patch

[Gavin Peters]

Created attachment 178358 [details]
Patch

[Gavin Peters]

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.

[WebKit Review Bot]

Comment on attachment 178358 [details]
Patch

Clearing flags on attachment: 178358

Committed r137045: <http://trac.webkit.org/changeset/137045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch broke EFL build:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6825/steps/compile-webkit/logs/stdio

[Chris Dumez]

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)

[Gavin Peters]

(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?

[Chris Dumez]

Build for !ENABLE(LINK_PRERENDER) platforms was fixed by http://trac.webkit.org/changeset/137047