WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Gavin Peters
Comment 1
2012-04-26 14:32:39 PDT
Created
attachment 139072
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug