RESOLVED FIXED Bug 85005
Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
https://bugs.webkit.org/show_bug.cgi?id=85005
Summary Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
Gavin Peters
Reported 2012-04-26 14:26:13 PDT
Add Prerenderer, PrerenderHandle and a chromium interface for Prerendering.
Attachments
Patch (77.11 KB, patch)
2012-04-26 14:32 PDT, Gavin Peters
no flags
fix WebURLRequest::TargetType (77.26 KB, patch)
2012-04-26 17:40 PDT, Gavin Peters
no flags
remove TargetIsPrender for blackberry (78.71 KB, patch)
2012-04-27 06:04 PDT, Gavin Peters
no flags
add build files for platforms... (93.91 KB, patch)
2012-04-27 11:09 PDT, Gavin Peters
no flags
rename to WebPrerenderingSupport (88.20 KB, patch)
2012-04-30 15:43 PDT, Gavin Peters
no flags
rebase for ews and commit... (87.88 KB, patch)
2012-05-14 08:16 PDT, Gavin Peters
no flags
Gavin Peters
Comment 1 2012-04-26 14:32:39 PDT
Gavin Peters
Comment 2 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?
Gavin Peters
Comment 3 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.
Gavin Peters
Comment 4 2012-04-26 17:37:59 PDT
+jmason: blackberry's ResourceRequest.h defines TargetIsPrerender, are you using it?
Gavin Peters
Comment 5 2012-04-26 17:40:30 PDT
Created attachment 139105 [details] fix WebURLRequest::TargetType
Joe Mason
Comment 6 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?
Gavin Peters
Comment 7 2012-04-27 06:04:13 PDT
Created attachment 139182 [details] remove TargetIsPrender for blackberry
Gavin Peters
Comment 8 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. :)
Gavin Peters
Comment 9 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.
Gavin Peters
Comment 10 2012-04-27 11:09:39 PDT
Created attachment 139234 [details] add build files for platforms...
Gavin Peters
Comment 11 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).
WebKit Review Bot
Comment 12 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.
WebKit Review Bot
Comment 13 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.
Joe Mason
Comment 14 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
Gavin Peters
Comment 15 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.
Adam Barth
Comment 16 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
Darin Fisher (:fishd, Google)
Comment 17 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?
Darin Fisher (:fishd, Google)
Comment 18 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?
Gavin Peters
Comment 19 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.
Gavin Peters
Comment 20 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.
Darin Fisher (:fishd, Google)
Comment 21 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.
Gavin Peters
Comment 22 2012-04-30 15:43:25 PDT
Created attachment 139533 [details] rename to WebPrerenderingSupport
Gavin Peters
Comment 23 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.
Gavin Peters
Comment 24 2012-05-14 08:16:22 PDT
Created attachment 141733 [details] rebase for ews and commit...
Gavin Peters
Comment 25 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.
WebKit Review Bot
Comment 26 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.
WebKit Review Bot
Comment 27 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>
WebKit Review Bot
Comment 28 2012-05-14 21:42:17 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.