Bug 82478 – Add Prerenderer and PrerenderHandle for signaling on <link rel=prerender ..> elements.

• Status: ASSIGNED
• Component: Page Loading

[Gavin Peters]

The prerender case isn't quite a resource load, and using resource loading to retrieve them has constrained the API.

The new Prerenderer and PrerenderHandle let <link rel=prerender> elements signal removal and unload to their loading platform.  Coming soon, I'd like to add events back from the prerendered page to the element, such as load, abort and error.

[Gavin Peters]

Created attachment 134359 [details]
First upload

[Gavin Peters]

Adam,

Here's my first upload.  This is a significant enough change that I probably should announce to WebKit dev; I'd like to do that after you give it a first pass over to eliminate the crazy.

To land this, I believe I'll have to stage it in at least two commits; the first to move WebReferrerPolicy.h to platform/ (and create a file in public that includes it), and the second will be something like this patch.

For the chromium side of this, and more discussion of staging, see http://codereview.chromium.org/9875026/

Adam: WDYT?

[Gavin Peters]

Comment on attachment 134359 [details]
First upload

View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:419
> +    // Link Prerender

Two things about these functions.  First: I think I'll need to make a WebPrerenderer object fairly soon, to handle events arriving from the platform up to the prerender link elements.  Second: I really don't like passing a WebView* here; it's required given the way that WebStorageNamespace works right now; as soon as I can, I'll try and pass it that way.

[Adam Barth]

Comment on attachment 134359 [details]
First upload

View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review

> Source/WebCore/loader/LinkLoader.cpp:148
> +void LinkLoader::removedFromDocument()

I would have picked a different name here.  The LinkLoader isn't being removed from any document.  Perhaps LinkLoader::cancelPrerender()?  I guess there's a subtly in that we might not have any prerenders to cancel...  Maybe didRemoveLinkElementFromDocument ?

> Source/WebCore/loader/LinkLoader.cpp:152
> +        m_prerenderHandle->removedFromDocument();

At this point, though, we shouldn't be talking about documents anymore.  Perhaps cancel() or abort() ?  I'm not sure which is idiomatic.

> Source/WebCore/loader/LinkLoader.h:73
> +#endif // ENABLE(LINK_PRERENDER)

You can skip the comment on the closing comment for these short #ifdefs.

> Source/WebCore/loader/Prerenderer.h:50
> +    Prerenderer(Document*);

Please add the explicit keyword.

> Source/WebCore/loader/Prerenderer.h:59
> +    const Document* m_document;

Why do you need a pointer fo the document?  ActiveDOMObject has a scriptExecutionContext() member already.

> Source/WebCore/platform/PrerenderHandle.h:45
> +    PrerenderHandle(int id);

Please add the explicit keyword.

> Source/WebCore/platform/PrerenderHandle.h:49
> +    void removedFromDocument();
> +    void unloaded();

We should phrase these more as instructions to the platform layer.  removedFromDocument and unloaded are higher-level concepts.

>> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:419
>> +    // Link Prerender
> 
> Two things about these functions.  First: I think I'll need to make a WebPrerenderer object fairly soon, to handle events arriving from the platform up to the prerender link elements.  Second: I really don't like passing a WebView* here; it's required given the way that WebStorageNamespace works right now; as soon as I can, I'll try and pass it that way.

Yeah, you can't pass a WebView* here.  That's not right.  I don't understand the connection with WebStorageNamespace.

Generally, we make the embedder to the work with IDs and instead phrase the WebKit API in in terms of objects.  For example, the first of these methods so be createPrerenderer, analogous to createURLLoader.  At this point, the word "link" shouldn't appear.  Nothing here has anything to do with links any more.  We're just talking about prerendering URLs and canceling them.

> Source/WebKit/chromium/src/Prerenderer.cpp:60
> +Prerenderer::Prerenderer(Document* document)

This object should be in WebCore.  The "Handle" object is the only one that needs to be in the WebKit layer.  Note: I'm working separately to refactor things so that we don't need to have any of this code in the WebKit layer, but that's still a bit off.

[Michael Nordman]

Comment on attachment 134359 [details]
First upload

View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review

>>> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:419
>>> +    // Link Prerender
>> 
>> Two things about these functions.  First: I think I'll need to make a WebPrerenderer object fairly soon, to handle events arriving from the platform up to the prerender link elements.  Second: I really don't like passing a WebView* here; it's required given the way that WebStorageNamespace works right now; as soon as I can, I'll try and pass it that way.
> 
> Yeah, you can't pass a WebView* here.  That's not right.  I don't understand the connection with WebStorageNamespace.
> 
> Generally, we make the embedder to the work with IDs and instead phrase the WebKit API in in terms of objects.  For example, the first of these methods so be createPrerenderer, analogous to createURLLoader.  At this point, the word "link" shouldn't appear.  Nothing here has anything to do with links any more.  We're just talking about prerendering URLs and canceling them.

You may want to consider moving these three methods into WebView/WebViewClient interfaces instead of having them here since they're dependent on the context containing the <link>. The impls of WebViewClient in chromium should have an easier time of dealing with the statefulness you have to deal with.

[Gavin Peters]

(In reply to comment #5)
> (From update of attachment 134359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review
> 
> >>> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:419
> >>> +    // Link Prerender
> >> 
> >> Two things about these functions.  First: I think I'll need to make a WebPrerenderer object fairly soon, to handle events arriving from the platform up to the prerender link elements.  Second: I really don't like passing a WebView* here; it's required given the way that WebStorageNamespace works right now; as soon as I can, I'll try and pass it that way.
> > 
> > Yeah, you can't pass a WebView* here.  That's not right.  I don't understand the connection with WebStorageNamespace.
> > 
> > Generally, we make the embedder to the work with IDs and instead phrase the WebKit API in in terms of objects.  For example, the first of these methods so be createPrerenderer, analogous to createURLLoader.  At this point, the word "link" shouldn't appear.  Nothing here has anything to do with links any more.  We're just talking about prerendering URLs and canceling them.
> 
> You may want to consider moving these three methods into WebView/WebViewClient interfaces instead of having them here since they're dependent on the context containing the <link>. The impls of WebViewClient in chromium should have an easier time of dealing with the statefulness you have to deal with.

I'm looking at this now.  I also might just add a WebPrerender class; it will be minimal now, but important later when we start sending events to the prerender link elements.  Thanks for the suggestion.

[Adam Barth]

By the way, you should be able to call directly from WebCore/platform into interfaces defined on http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/Platform.h

I'm about to move ResourceHandle from the WebKit layer into WebCore/platform/network:

https://bugs.webkit.org/show_bug.cgi?id=82657

It's very new pattern, but hopefully should remove some of the boilerplate "plumbing" in your patch.

[Gavin Peters]

(In reply to comment #4)
> (From update of attachment 134359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review
> 
> > Source/WebCore/loader/LinkLoader.cpp:148
> > +void LinkLoader::removedFromDocument()
> 
> I would have picked a different name here.  The LinkLoader isn't being removed from any document.  Perhaps LinkLoader::cancelPrerender()?  I guess there's a subtly in that we might not have any prerenders to cancel...  Maybe didRemoveLinkElementFromDocument ?

I went with something simpler, but possibly too general: released(); that then may become a call to PrerenderHandle::cancel(), as appropriate.  I'm on the fence and I could easily be talked into didRemoveLinkElementFromDocument().  I had release(), but that was too confusing compared to a RefPtr: but then maybe it's not?  It is something very similar.

> 
> > Source/WebCore/loader/LinkLoader.cpp:152
> > +        m_prerenderHandle->removedFromDocument();
> 
> At this point, though, we shouldn't be talking about documents anymore.  Perhaps cancel() or abort() ?  I'm not sure which is idiomatic.

add/cancel/abandon is what I went with.  add and not start, since it's up to the platform's prerender manager to decide which prerenders even are allowed to start.

> 
> > Source/WebCore/loader/LinkLoader.h:73
> > +#endif // ENABLE(LINK_PRERENDER)
> 
> You can skip the comment on the closing comment for these short #ifdefs.
> 

Done.

> > Source/WebCore/loader/Prerenderer.h:50
> > +    Prerenderer(Document*);
> 
> Please add the explicit keyword.
> 

Done.

> > Source/WebCore/loader/Prerenderer.h:59
> > +    const Document* m_document;
> 
> Why do you need a pointer fo the document?  ActiveDOMObject has a scriptExecutionContext() member already.
> 

I do not.  Removed: it was bad practice, I concur.

> > Source/WebCore/platform/PrerenderHandle.h:45
> > +    PrerenderHandle(int id);
> 
> Please add the explicit keyword.
> 

Mooted.

> > Source/WebCore/platform/PrerenderHandle.h:49
> > +    void removedFromDocument();
> > +    void unloaded();
> 
> We should phrase these more as instructions to the platform layer.  removedFromDocument and unloaded are higher-level concepts.
> 
> >> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:419
> >> +    // Link Prerender
> > 
> > Two things about these functions.  First: I think I'll need to make a WebPrerenderer object fairly soon, to handle events arriving from the platform up to the prerender link elements.  Second: I really don't like passing a WebView* here; it's required given the way that WebStorageNamespace works right now; as soon as I can, I'll try and pass it that way.
> 
> Yeah, you can't pass a WebView* here.  That's not right.  I don't understand the connection with WebStorageNamespace.
> 
> Generally, we make the embedder to the work with IDs and instead phrase the WebKit API in in terms of objects.  For example, the first of these methods so be createPrerenderer, analogous to createURLLoader.  At this point, the word "link" shouldn't appear.  Nothing here has anything to do with links any more.  We're just talking about prerendering URLs and canceling them.
> 

So, I went and added a lot of plumbing in response to this, and I'm really on the fence about it.  I created a PrerenderClient, and a corresponding WebPrerendererClient, which gets a WebPrerender, and adds some platform-renderer specific information to the prerender; the requestor ID and the size.  However, the only thing the requestor is used for is to, in the prerender manager to find the session storage namespace associated with the requestor (since the prerender needs to use the same session storage namespace).

Can we simplify this some?  I keep leaning towards not needing a requestor ID; I'd then need to pass a WebStorageNamespace* with the prerender commands, and then move that to an ID, and then from that ID back at the prerender_manager, find the correct session storage namespace object.  None of that's awful, and maybe preferable to the plumbing I created for WebPrerenderClient?

(Also: would WebViewClient be a better place for willAddPrerender()???)

michaeln: I especially welcome your suggestions here?

The next thing I need to think about is events coming down to the HTMLLinkElement from the prerender manager in the platform.  I have given this some, but not enough consideration: I'm going to spend some time hacking up a patch in that vein, so that I can be sure I haven't painted myself into any corners here.

> > Source/WebKit/chromium/src/Prerenderer.cpp:60
> > +Prerenderer::Prerenderer(Document* document)
> 
> This object should be in WebCore.  The "Handle" object is the only one that needs to be in the WebKit layer.  Note: I'm working separately to refactor things so that we don't need to have any of this code in the WebKit layer, but that's still a bit off.

I have integrated with your patches, which have since landed.  I think they did simplify my coding quite a bit.  Let me know if I continue to not quite get the layers...

Also, the existing chrome CL will get an upload soon that includes the changes here.  I'll notify on bug when that happens, for anyone who wants a 360 view of this.


I have made an upload of changes that meet this remediation in https://github.com/gavinp/git.webkit.org/pull/1 ; a pull request on my clone of git.webkit.org: would you like to go over there and continue to look at it, or have me upload here?

[Adam Barth]

> I have made an upload of changes that meet this remediation in https://github.com/gavinp/git.webkit.org/pull/1 ; a pull request on my clone of git.webkit.org: would you like to go over there and continue to look at it, or have me upload here?

I've left you a bunch of comments on the pull request.  IMHO, the first thing we should do is break the change down into smaller pieces.  For example, splitting the ReferrerPolicy and the ENABLE macro changes into separate patches (and probably landing them).

As for how to associate the request with the session storage identifier, that's less clear.  It's a somewhat common problem in the WebKit API that we don't have a good solution for yet.  For now, I'd use the willPrerenderURL client callback since that mirrors what we do in the rest of the loader with willSendRequest.

It's important to remove the prerender IDs from WebKit through.  The WebKit API is object-based, even though the embedder often uses IDs to refer to objects because the embedder uses multiple processes.

[Gavin Peters]

(In reply to comment #9)
> > I have made an upload of changes that meet this remediation in https://github.com/gavinp/git.webkit.org/pull/1 ; a pull request on my clone of git.webkit.org: would you like to go over there and continue to look at it, or have me upload here?
> 
> I've left you a bunch of comments on the pull request.  IMHO, the first thing we should do is break the change down into smaller pieces.  For example, splitting the ReferrerPolicy and the ENABLE macro changes into separate patches (and probably landing them).

Thanks a TON for those comments!  I'm working hard on a better upload; I've already started a new pull request, https://github.com/gavinp/git.webkit.org/pull/2 in which I've created four distinct commits, doing this in parts like you suggested.  However, your more substantive architectural suggestions are still pending: I'll ping this bug when I think that pull request is worthy of a review.

> 
> As for how to associate the request with the session storage identifier, that's less clear.  It's a somewhat common problem in the WebKit API that we don't have a good solution for yet.  For now, I'd use the willPrerenderURL client callback since that mirrors what we do in the rest of the loader with willSendRequest.

That's an interesting answer...  And I think it causes me to think: let's just leave the requestor ID as part of what's set (along with the size) in the willPrerender... callback.  That's sufficient, and works for what we're doing, so I'm going to count my musings above as satisfied by that answer.

> It's important to remove the prerender IDs from WebKit through.  The WebKit API is object-based, even though the embedder often uses IDs to refer to objects because the embedder uses multiple processes.

My next upload will reflect this.  Thank you!

[Michael Nordman]

Hi Gavin,

Not sure this is the sort of feedback you were looking for, but here goes...

An obvious high level question that's really easy to escape being asked given all the energy put into api layer traveral... what happens if we're given the same url to be prerendered twice?
  <link type=prerender url=foo>
  <link type=prerender url=foo>

About the code itelf... I've made a list of the classes involved with short notes about each below to try to get a better understanding of things.

---- sketchy notes ----

// webkit::Platform
// ultimately the handful of outputs your interested in getting from the
// bowls of webcore when <link type=prerender> come and go so you know
// when to start/stop prerendering pages.
void addPrerender(...)
void cancelPrerender(...)
void abortPrerender(...)

// webkit
WebPrerender
  - odd struct that doesn't reflect the url of the resource to be prerendered
WebPrerendererClient
  - has a willAddPrerender method
  - WebView has one of these guys presumably implemented in chromium svn code
    (+1 that client)

// loader
Prerenderer
  - a per-document active dom object thing that produces a PrerenderHandle,
  a refcounted wrapper around a Prerender, and tells the PrerenderClient about
  them
PrerenderClient 
  - a platform specific impl that in chromium bridges out thru WebPrerenderClient
  - seems like in practical terms, the 'client' is the actual prerenderer since
    the Prerender class really doesn't do much besides call it

// platform (chromium specific impls in webcore)
Prerender
PrerenderHandle (LinkLoader uses the handle)
  - a refcounted wrapper around a Prerender
  - LinkLoader uses the handle
  Q: Should document->prerenderer() be involved with LinkLoader
  calls to cancel, the 'handle' will still be in it's collection of handles
  otherwise?
  Q: What's the benefit of the wrapper? I'm wondering if this could be flattened out.

----

So that's ~7 classes (if you allow me to count the additions to webkit::Platform as a class).

I understand you'll want to add support for events that originate in the backend and are raised on pages containing prerender <links>, so you'll need to support calling back into webcore in addition to calling out from webcore. That will undoubetly bump up the number of classes and interfaces (perhaps double it).

I cant help but wonder if we can flatten the api layer traversal out and simplify things. I like fewer moving parts. With that in mind...

* Given the WebPrerenderClient that has knowlege of its containing WebView (including its size and session storage namespace id), it seems like the methods on webkit::Platform could be replace with methods on that client that don't have to accept size and id params.

* The way i understand this, you basically have two classes, the Prerenderer and PrerenderItems, where the Prerenderer is used as a factory of Items by the <link> handling world. (fwiw, in reading the code it was easy to not notice the distcintion between prerender and prerender'er').

* Can the WebCore Prerenderer and PrerenderClient classes be collapsed into a single class? WebCore::Prerenderer.

* Can the WebCore Prerender and PrerenderHandle classes be collapsed into a single class? WebCore::PrerenderItem.

// Interface impelemented by the embedder.
// Chromium's impl of WebCore::Prerender calls out via this interface.
class WebPrerenderClient {
  // The caller should delete to cancel/abort prerendering.
  // It's assumed that the impl of this interface has knowlege
  // of the containing document's size referrer, session storage ids etc.
  WebPrerenderItem* createPrerenderItem(url);
};

// Also implemented by the embedder.
// Chromium's impl of WebCore::PrerenderItem is basically a wrapper over
// this.
class WebPrerenderItem {
  // Deletion causes the prerendering to be stopped.
  ~WebPrerenderItem ();
};


I'm pretty confident the wekit keepers will not like the direction those question suggest (layering violation, right), but i gotta question the value of obscure layers of indirection imposed on bridging in and out between webcore and chromium. Consider... it's easy to see how support for events might be added in the simplified system design (make a WebPrerenderItemClient interface whose methods get called when an event should be raised, have WebCore::PrerenderItem get called thru that interface). Not so easty to say the same about the more complicated set of classes and interfaces.

[Michael Nordman]

You might want to call it plain WebPrerenderer since that's what it does.

> // Interface impelemented by the embedder.
> // Chromium's impl of WebCore::Prerender calls out via this interface.
> class WebPrerenderClient {
>   // The caller should delete to cancel/abort prerendering.
>   // It's assumed that the impl of this interface has knowlege
>   // of the containing document's size referrer, session storage ids etc.
>   WebPrerenderItem* createPrerenderItem(url);
> };
> 
> // Also implemented by the embedder.
> // Chromium's impl of WebCore::PrerenderItem is basically a wrapper over
> // this.
> class WebPrerenderItem {
>   // Deletion causes the prerendering to be stopped.
>   ~WebPrerenderItem ();
> };

[Gavin Peters]

I have just finished another upload on this patch; it's on github at https://github.com/gavinp/git.webkit.org/pull/2

This patch improves on the previous upload in a few ways.

* It's broken down into 7 commits, particularly moving a lot of boilerplate, and the removal of the old phantom-load facility out into distinct commits.  The last commit in the chain is still large, though, but at least it's topical.
* I used an ExtraData pattern to allow the WebPrerendererClient to save platform data in the prerender.  Now the WebCore prerender object is only aware of WebCore data from the LinkLoader.
* The clients are now passed properly from WebKit down into WebCore.

[Adam Barth]

I left Gavin some comments on the pull request.

[Mark Rowe (bdash)]

Comment on attachment 134359 [details]
First upload

View in context: https://bugs.webkit.org/attachment.cgi?id=134359&action=review

> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:93
>  ENABLE_MATHML = ENABLE_MATHML;

If you're updating one FeatureDefines.xcconfig you must update them all.

[Gavin Peters]

See https://bugs.webkit.org/show_bug.cgi?id=85005 for the meat of this issue, post the review on GitHub.