[webkit-reviews] review denied: [Bug 82478] Add Prerenderer and PrerenderHandle for signaling on <link rel=prerender ..> elements. : [Attachment 134359] First upload

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 82478: Add Prerenderer and PrerenderHandle for signaling on <link
rel=prerender ..> elements.
https://bugs.webkit.org/show_bug.cgi?id=82478

Attachment 134359: First upload
https://bugs.webkit.org/attachment.cgi?id=134359&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.