[webkit-reviews] review granted: [Bug 85005] Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering. : [Attachment 139234] add build files for platforms...

[bugzilla-daemon at webkit.org]

Adam Barth (on vacation until April 30) <abarth at webkit.org> has granted Gavin
Peters <gavinp at chromium.org>'s request for review:
Bug 85005: Add Prerenderer, PrerenderHandle and a chromium interface for
Prerendering.
https://bugs.webkit.org/show_bug.cgi?id=85005

Attachment 139234: add build files for platforms...
https://bugs.webkit.org/attachment.cgi?id=139234&action=review

------- Additional Comments from Adam Barth (on vacation until April 30)
<abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139234&action=review


This generally looks good.  The only really substantial comment is about the
static in WebPrerenderPlatform.  I'm not super excited about the
WebPrerenderPlatform name, but we've talked about that before and didn't come
up with anything better.  We might want to ask fishd if he has any naming
suggestions.  Other than that, I think we're ready to go.

> Source/Platform/chromium/public/WebPrerenderingPlatform.h:52
> +    // A prerender is canceled when it is removed from a document.

These comments look like they're swapped.

> Source/Platform/chromium/public/WebPrerenderingPlatform.h:56
> +    WEBKIT_EXPORT static void setPlatform(WebPrerenderingPlatform*);
> +    WEBKIT_EXPORT static WebPrerenderingPlatform* getPlatform();

This seems slightly odd.  Why is there a createPrerenderingPlatform() in
addition to this mechanism?  Rather than calling getPlatform() in WebKit, it
might be easier to call
WebKit::Platform::current()->prerenderingPlatform()->add(...).	That way the
embedder can decide to use a static or to use a different implementation of
WebPrerenderingPlatform at different times.  (Note:
createPrerenderingPlatform() doesn't appear to be used in your patch
currently.)

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

explicit

> Source/WebCore/platform/chromium/PrerenderHandle.cpp:51
> +	   : m_prerender(adoptRef(new Prerender(url, referrer, policy)))

bad indent

> Source/WebCore/platform/chromium/support/WebPrerender.cpp:53
> +	       : m_extraData(adoptPtr(extraData))

bad inent