gavinp/git.webkit.org — pull request #1 by gavinp (2012-04-05 15:14:12+00:00)
Title: Remediation to review on abarth's first high-level architecture review!

(Comments from https://bugs.webkit.org/show_bug.cgi?id=82478 , and abarth's first review, cut and paste in here with my replies)

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