Bug 85005 - Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
Summary: Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on: 84871
Blocks: 85021
  Show dependency treegraph
 
Reported: 2012-04-26 14:26 PDT by Gavin Peters
Modified: 2012-05-14 21:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (77.11 KB, patch)
2012-04-26 14:32 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
fix WebURLRequest::TargetType (77.26 KB, patch)
2012-04-26 17:40 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
remove TargetIsPrender for blackberry (78.71 KB, patch)
2012-04-27 06:04 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
add build files for platforms... (93.91 KB, patch)
2012-04-27 11:09 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
rename to WebPrerenderingSupport (88.20 KB, patch)
2012-04-30 15:43 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
rebase for ews and commit... (87.88 KB, patch)
2012-05-14 08:16 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2012-04-26 14:26:13 PDT
Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
Comment 1 Gavin Peters 2012-04-26 14:32:39 PDT
Created attachment 139072 [details]
Patch
Comment 2 Gavin Peters 2012-04-26 14:39:09 PDT
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?
Comment 3 Gavin Peters 2012-04-26 15:35:49 PDT
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.
Comment 4 Gavin Peters 2012-04-26 17:37:59 PDT
+jmason: blackberry's ResourceRequest.h defines TargetIsPrerender, are you using it?
Comment 5 Gavin Peters 2012-04-26 17:40:30 PDT
Created attachment 139105 [details]
fix WebURLRequest::TargetType
Comment 6 Joe Mason 2012-04-27 05:16:24 PDT
(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?
Comment 7 Gavin Peters 2012-04-27 06:04:13 PDT
Created attachment 139182 [details]
remove TargetIsPrender for blackberry
Comment 8 Gavin Peters 2012-04-27 06:05:22 PDT
(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.  :)
Comment 9 Gavin Peters 2012-04-27 07:23:07 PDT
Comment on attachment 139182 [details]
remove TargetIsPrender for blackberry

I tested, but didn't upload build files for other platforms than Chromium.  Forthcoming.
Comment 10 Gavin Peters 2012-04-27 11:09:39 PDT
Created attachment 139234 [details]
add build files for platforms...
Comment 11 Gavin Peters 2012-04-27 11:13:51 PDT
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).
Comment 12 WebKit Review Bot 2012-04-27 11:16:09 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 13 WebKit Review Bot 2012-04-27 11:16:43 PDT
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.
Comment 14 Joe Mason 2012-04-27 11:28:44 PDT
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
Comment 15 Gavin Peters 2012-04-27 20:30:49 PDT
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.
Comment 16 Adam Barth 2012-04-30 11:12:50 PDT
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
Comment 17 Darin Fisher (:fishd, Google) 2012-04-30 11:44:49 PDT
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?
Comment 18 Darin Fisher (:fishd, Google) 2012-04-30 11:51:45 PDT
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?
Comment 19 Gavin Peters 2012-04-30 11:59:32 PDT
(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.
Comment 20 Gavin Peters 2012-04-30 12:00:15 PDT
(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.
Comment 21 Darin Fisher (:fishd, Google) 2012-04-30 12:45:12 PDT
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.
Comment 22 Gavin Peters 2012-04-30 15:43:25 PDT
Created attachment 139533 [details]
rename to WebPrerenderingSupport
Comment 23 Gavin Peters 2012-04-30 15:44:58 PDT
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.
Comment 24 Gavin Peters 2012-05-14 08:16:22 PDT
Created attachment 141733 [details]
rebase for ews and commit...
Comment 25 Gavin Peters 2012-05-14 08:19:10 PDT
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.
Comment 26 WebKit Review Bot 2012-05-14 08:19:56 PDT
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.
Comment 27 WebKit Review Bot 2012-05-14 21:42:11 PDT
Comment on attachment 141733 [details]
rebase for ews and commit...

Clearing flags on attachment: 141733

Committed r117029: <http://trac.webkit.org/changeset/117029>
Comment 28 WebKit Review Bot 2012-05-14 21:42:17 PDT
All reviewed patches have been landed.  Closing bug.