Bug 85005 – Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gavin Peters]

Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.

[Gavin Peters]

Created attachment 139072 [details]
Patch

[Gavin Peters]

This is my remediation to this review from Adam Barth: https://github.com/gavinp/git.webkit.org/commit/99d15d3b9c783395015ab74588fefbadc8a7c51e

It was not LGTM by him there, although he said "This looks great. There are a few minor things and then naming suggestions above and then we'll be all set. Thanks so much." just before he went on vacation.  Sadly, I had the chromium platform side of this impl to move through review after that.

jamesr: could you look at my interfaces in Platform/ and WebKit/, and tell me what you think?  As well, note that https://bugs.webkit.org/show_bug.cgi?id=84880 probably makes a lot more sense if you read it after this, although of course it has to commit first (and this patch is written with respect to it).

japhet: could you look at my WebCore/loader and WebCore/platform work, and tell me what you think?

[Gavin Peters]

Comment on attachment 139072 [details]
Patch

This patch won't build until after http://codereview.chromium.org/10198040/ lands, I'll update EWS on this issue after that.

[Gavin Peters]

+jmason: blackberry's ResourceRequest.h defines TargetIsPrerender, are you using it?

[Gavin Peters]

Created attachment 139105 [details]
fix WebURLRequest::TargetType

[Joe Mason]

(In reply to comment #4)
> +jmason: blackberry's ResourceRequest.h defines TargetIsPrerender, are you using it?

Nope.  Do you mind removing it from blackberry/ResourceRequest.h and blackberry/ResourceRequestBlackBerry.cpp while you're at it?

[Gavin Peters]

Created attachment 139182 [details]
remove TargetIsPrender for blackberry

[Gavin Peters]

(In reply to comment #6)
> (In reply to comment #4)
> > +jmason: blackberry's ResourceRequest.h defines TargetIsPrerender, are you using it?
> 
> Nope.  Do you mind removing it from blackberry/ResourceRequest.h and blackberry/ResourceRequestBlackBerry.cpp while you're at it?

Happy to!  I just updated my patch to do this too.  I somewhat expected this answer.  :)

[Gavin Peters]

Comment on attachment 139182 [details]
remove TargetIsPrender for blackberry

I tested, but didn't upload build files for other platforms than Chromium.  Forthcoming.

[Gavin Peters]

Created attachment 139234 [details]
add build files for platforms...

[Gavin Peters]

This latest patch, with the build files for platform, is rebased relative to the current WebKit trunk rather than the thunk headers in Bug 84880, I believe that makes the patch easier to review, and also it might even pass some EWS (although until http://codereview.chromium.org/10198040/ lands, it will break chrome).

[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 139234 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/chromium/support/WebPrerender.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joe Mason]

Comment on attachment 139234 [details]
add build files for platforms...

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

>> Source/WebCore/platform/chromium/support/WebPrerender.cpp:37
>> +#include "Prerender.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

This looks like bug 85075

[Gavin Peters]

abarth, it looks like this patch stayed alive during your whole vacation...  So you can take a peek at the remediation.  Let me know what you think.

[Adam Barth]

Comment on attachment 139234 [details]
add build files for platforms...

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

[Darin Fisher (:fishd, Google)]

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

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

this repeats the word "platform" at callsites:

  WebPrerenderingPlatform::getPlatform()

what does it mean for these static accessors to exist when Platform.h also
has a create function?

[Darin Fisher (:fishd, Google)]

Comment on attachment 139234 [details]
add build files for platforms...

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

> Source/Platform/chromium/public/WebPrerenderingPlatform.h:42
> +    virtual ~WebPrerenderingPlatform() { }

Should this really be public?  Who is responsible for deleting an
instance of type WebPrerenderingPlatform?

[Gavin Peters]

(In reply to comment #17)
> View in context: https://bugs.webkit.org/attachment.cgi?id=139234&action=review
> 
> > Source/Platform/chromium/public/WebPrerenderingPlatform.h:55
> > +    WEBKIT_EXPORT static void setPlatform(WebPrerenderingPlatform*);
> 
> this repeats the word "platform" at callsites:
> 
>   WebPrerenderingPlatform::getPlatform()
> 
> what does it mean for these static accessors to exist when Platform.h also
> has a create function?

It means I left the method in Platform.h in by accident; I'll remove that in my next upload.  Having the static setters in WebPrerenderingPlatform.h means that the change implementing it in chromium doesn't need to be in webkit/ or content/ , the only places where we implement Platform.

[Gavin Peters]

(In reply to comment #18)
> (From update of attachment 139234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139234&action=review
> 
> > Source/Platform/chromium/public/WebPrerenderingPlatform.h:42
> > +    virtual ~WebPrerenderingPlatform() { }
> 
> Should this really be public?  Who is responsible for deleting an
> instance of type WebPrerenderingPlatform?

It should be protected.  The provider deletes it, but has to make sure its life is the length of the WebKit instance, or longer.

[Darin Fisher (:fishd, Google)]

Off-line, Adam suggested naming the interface WebPrerenderingSupport instead of WebPrerenderingPlatform.  I agree with him that that name would be better.  We have other interfaces that end with *Support, and this seems similar in nature to those.

[Gavin Peters]

Created attachment 139533 [details]
rename to WebPrerenderingSupport

[Gavin Peters]

Comment on attachment 139533 [details]
rename to WebPrerenderingSupport

This upload includes the rename, and is now rebased on top of https://bugs.webkit.org/show_bug.cgi?id=84880 , which has also been updated to reflect the rename.

This change cannot land without WebKit/chromium/DEPS being advanced past the rev in which http://codereview.chromium.org/10198040/ lands, and I'll make sure that happens.

[Gavin Peters]

Created attachment 141733 [details]
rebase for ews and commit...

[Gavin Peters]

Comment on attachment 141733 [details]
rebase for ews and commit...

whoops; removing review flag.  This is reviewed, but I want to ews test and get ready for CQ.

[WebKit Review Bot]

Attachment 141733 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/chromium/support/WebPrerender.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 141733 [details]
rebase for ews and commit...

Clearing flags on attachment: 141733

Committed r117029: <http://trac.webkit.org/changeset/117029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.