Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(333)

Issue 10198040: New link rel=prerender api, using WebKit::WebPrerenderingPlatform (Closed)

Created:
8 years, 8 months ago by gavinp
Modified:
8 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, jam, joi+watch-content_chromium.org, dominich+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

New link rel=prerender api, using WebKit::WebPrerenderingPlatform This patch implements the renderer side classes corresponding to https://bugs.webkit.org/show_bug.cgi?id=85005 , and adds messaging up to the browser process for link prerender events. A new PrerenderLinkManager is introduced to handle these events for the LinkManager. BUG=84236 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136611

Patch Set 1 : fix base #

Total comments: 41

Patch Set 2 : remediate to dominich + jam reviews #

Total comments: 7

Patch Set 3 : remove the urls_to_id_map_, and follow consequences through. #

Total comments: 38

Patch Set 4 : remediate to dominich review #

Total comments: 44

Patch Set 5 : same as Patch Set 4, but updated to trunk. #

Patch Set 6 : is this closer? #

Total comments: 56

Patch Set 7 : continued remediation #

Total comments: 2

Patch Set 8 : friendship is hard #

Total comments: 37

Patch Set 9 : remediate to cbentzel review #

Patch Set 10 : merge to trunk to make cq happy #

Patch Set 11 : 80 columns #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1970 lines, -751 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_browsertest.cc View 1 2 3 4 5 6 7 9 chunks +67 lines, -6 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.h View 1 2 3 4 5 8 chunks +18 lines, -14 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.cc View 1 2 3 4 5 6 7 8 6 chunks +48 lines, -47 lines 0 comments Download
A chrome/browser/prerender/prerender_link_manager.h View 1 2 3 4 5 6 7 1 chunk +88 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_link_manager.cc View 1 2 3 4 5 6 7 8 1 chunk +118 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_link_manager_factory.h View 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_link_manager_factory.cc View 1 chunk +48 lines, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_manager.h View 1 2 3 4 5 6 7 8 9 10 8 chunks +65 lines, -36 lines 0 comments Download
M chrome/browser/prerender/prerender_manager.cc View 1 2 3 4 5 6 7 8 9 12 chunks +71 lines, -42 lines 0 comments Download
D chrome/browser/prerender/prerender_manager_unittest.cc View 1 chunk +0 lines, -601 lines 0 comments Download
A chrome/browser/prerender/prerender_message_filter.h View 1 2 3 4 5 6 1 chunk +60 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_message_filter.cc View 1 2 3 4 5 6 7 8 1 chunk +110 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_unittest.cc View 1 2 3 4 5 6 1 chunk +924 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 8 9 3 chunks +20 lines, -4 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/chrome_renderer.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/prerender_messages.h View 1 2 3 4 5 6 7 8 1 chunk +29 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/renderer/prerender/prerender_dispatcher.cc View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerender_extra_data.h View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerender_extra_data.cc View 1 1 chunk +31 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerenderer_client.h View 1 2 3 4 5 6 7 8 1 chunk +30 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerenderer_client.cc View 1 2 3 4 5 6 7 8 1 chunk +37 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerendering_support.h View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerendering_support.cc View 1 2 3 4 5 6 7 8 1 chunk +43 lines, -0 lines 0 comments Download
A chrome/test/data/prerender/prerender_loader_removing_links.html View 1 2 3 4 5 6 7 8 1 chunk +37 lines, -0 lines 0 comments Download

Messages

Total messages: 42 (0 generated)
gavinp
This is the chrome side of the new WebKit API for prerendering at https://bugs.webkit.org/show_bug.cgi?id=85005 . ...
8 years, 8 months ago (2012-04-26 22:13:47 UTC) #1
cbentzel
Can you please indicate what you want each reviewer to look at? On Thu, Apr ...
8 years, 8 months ago (2012-04-26 22:20:42 UTC) #2
gavinp
mmenke: I'd like you to look at the chrome/browser/prerender code, which you reviewed earlier in ...
8 years, 8 months ago (2012-04-26 22:34:14 UTC) #3
dominich
drive-by nits and comments http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager.cc File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager.cc#newcode29 chrome/browser/prerender/prerender_link_manager.cc:29: i != e; ++i) { ...
8 years, 8 months ago (2012-04-26 22:50:58 UTC) #4
jam
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_message_filter.cc File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_message_filter.cc#newcode55 chrome/browser/prerender/prerender_message_filter.cc:55: if (message.type() == PrerenderHostMsg_AddPrerender::ID || nit, you can just ...
8 years, 8 months ago (2012-04-27 15:59:50 UTC) #5
gavinp
Dominic & John, Thank you very much for your reviews. This upload addresses most of ...
8 years, 8 months ago (2012-04-27 20:31:32 UTC) #6
dominich
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager.cc File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager.cc#newcode78 chrome/browser/prerender/prerender_link_manager.cc:78: // TODO(gavinp,dominich): After the Prerender API can send events ...
8 years, 8 months ago (2012-04-27 21:52:00 UTC) #7
gavinp
Thanks a ton Dominic... I don't quite get what you're asking about for propogating ids ...
8 years, 8 months ago (2012-04-27 23:31:44 UTC) #8
dominich.google
On Fri, Apr 27, 2012 at 4:31 PM, <gavinp@chromium.org> wrote: > Thanks a ton Dominic... ...
8 years, 8 months ago (2012-04-27 23:47:21 UTC) #9
gavinp
OK: after more back and forth with Dominic, and together with other concerns from mmenke ...
8 years, 8 months ago (2012-04-28 00:21:59 UTC) #10
gavinp
OK, I know I'm stubborn, but after getting strong review comments from Matt, Chris and ...
8 years, 7 months ago (2012-04-29 00:35:43 UTC) #11
dominich
First thoughts. It's definitely cleaner, but I'm still not sure that it goes far enough. ...
8 years, 7 months ago (2012-04-29 18:52:54 UTC) #12
dominich
On 2012/04/29 18:52:54, dominich wrote: > First thoughts. It's definitely cleaner, but I'm still not ...
8 years, 7 months ago (2012-04-29 20:23:56 UTC) #13
gavinp
Thank you Dominic for taking time out of a Sunday to review my code. I ...
8 years, 7 months ago (2012-04-30 11:43:15 UTC) #14
dominich
Sorry for the churn. I think it's completely necessary as we move towards a much ...
8 years, 7 months ago (2012-04-30 15:52:04 UTC) #15
mmenke
Mostly nits, though still want to go over the code once more. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/prerender_contents.cc File chrome/browser/prerender/prerender_contents.cc ...
8 years, 7 months ago (2012-04-30 18:35:22 UTC) #16
mmenke
Oops. Forgot one comment with that last batch. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/prerender_unittest.cc File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/prerender_unittest.cc#newcode245 chrome/browser/prerender/prerender_unittest.cc:245: return ...
8 years, 7 months ago (2012-04-30 18:40:40 UTC) #17
gavinp
Thanks everyone for your reviews! The new upload should cover most remaining nits. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/prerender_contents.h File ...
8 years, 7 months ago (2012-04-30 23:55:38 UTC) #18
gavinp
http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/prerendering_support.h File chrome/renderer/prerender/prerendering_support.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/prerendering_support.h#newcode10 chrome/renderer/prerender/prerendering_support.h:10: #include "third_party/WebKit/Source/Platform/chromium/public/WebPrerenderingSupport.h" In the review of https://bugs.webkit.org/show_bug.cgi?id=85005 , this ...
8 years, 7 months ago (2012-05-01 00:14:23 UTC) #19
mmenke
Despite the number of nits, I'm pretty happy with the CL. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/prerender_contents.h File chrome/browser/prerender/prerender_contents.h (right): ...
8 years, 7 months ago (2012-05-01 16:23:20 UTC) #20
gavinp
mmenke: are you still happy? http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/prerender_contents.cc File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/prerender_contents.cc#newcode102 chrome/browser/prerender/prerender_contents.cc:102: gfx::Size size); On 2012/05/01 ...
8 years, 7 months ago (2012-05-01 18:50:21 UTC) #21
dominich
lgtm fwiw http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/prerender_browsertest.cc#newcode736 chrome/browser/prerender/prerender_browsertest.cc:736: PrerenderLinkManager* GetPrerenderLinkManager() const { nit: It looks ...
8 years, 7 months ago (2012-05-01 19:57:48 UTC) #22
gavinp
http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/prerender_browsertest.cc#newcode736 chrome/browser/prerender/prerender_browsertest.cc:736: PrerenderLinkManager* GetPrerenderLinkManager() const { On 2012/05/01 19:57:48, dominich wrote: ...
8 years, 7 months ago (2012-05-01 20:24:10 UTC) #23
dominich
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc#newcode744 chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { Do you even need GetPrerenderLinkManager() ...
8 years, 7 months ago (2012-05-01 20:26:35 UTC) #24
mmenke
LGTM. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/prerender_link_manager.cc File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/prerender_link_manager.cc#newcode93 chrome/browser/prerender/prerender_link_manager.cc:93: // navigation away from launcher. On 2012/05/01 18:50:22, ...
8 years, 7 months ago (2012-05-02 15:02:52 UTC) #25
jam
haven't looked through the entire change again, just the reply to my nits, so lgtm ...
8 years, 7 months ago (2012-05-02 15:22:44 UTC) #26
cbentzel
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager_factory.cc File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager_factory.cc#newcode37 chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) On 2012/04/27 23:31:45, gavinp wrote: > On ...
8 years, 7 months ago (2012-05-02 15:54:21 UTC) #27
dominich
On 2012/05/02 15:54:21, cbentzel wrote: > http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager_factory.cc > File chrome/browser/prerender/prerender_link_manager_factory.cc (right): > > http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/prerender_link_manager_factory.cc#newcode37 > ...
8 years, 7 months ago (2012-05-02 16:12:30 UTC) #28
gavinp
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_manager.cc File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_manager.cc#newcode294 chrome/browser/prerender/prerender_manager.cc:294: it->contents_->AddPendingPrerender(url, referrer, size); On 2012/05/02 15:54:22, cbentzel wrote: > ...
8 years, 7 months ago (2012-05-02 16:23:41 UTC) #29
cbentzel
Mostly nits, but please look at the IMPORTANT one http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc#newcode744 chrome/browser/prerender/prerender_browsertest.cc:744: ...
8 years, 7 months ago (2012-05-04 15:32:24 UTC) #30
gavinp
cbentzel, what do you think of these fixes/comments? http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc#newcode744 chrome/browser/prerender/prerender_browsertest.cc:744: bool ...
8 years, 7 months ago (2012-05-04 23:10:51 UTC) #31
mmenke
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc#newcode744 chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { On 2012/05/04 23:10:51, gavinp wrote: ...
8 years, 7 months ago (2012-05-04 23:16:49 UTC) #32
cbentzel
On Fri, May 4, 2012 at 7:16 PM, <mmenke@chromium.org> wrote: > > http://codereview.chromium.**org/10198040/diff/36036/** > chrome/browser/prerender/**prerender_browsertest.cc<http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/prerender_browsertest.cc> ...
8 years, 7 months ago (2012-05-05 00:16:31 UTC) #33
cbentzel
LGTM
8 years, 7 months ago (2012-05-05 00:16:50 UTC) #34
gavinp
On 2012/05/05 00:16:50, cbentzel wrote: > LGTM Thank you everyone for your great reviews; I ...
8 years, 7 months ago (2012-05-05 13:51:22 UTC) #35
mmenke
On 2012/05/05 13:51:22, gavinp wrote: > On 2012/05/05 00:16:50, cbentzel wrote: > > LGTM > ...
8 years, 7 months ago (2012-05-05 13:52:36 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/60002
8 years, 7 months ago (2012-05-11 16:02:01 UTC) #37
commit-bot: I haz the power
Can't apply patch for file chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc. While running patch -p1 --forward --force; patching file chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc ...
8 years, 7 months ago (2012-05-11 16:02:36 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/70002
8 years, 7 months ago (2012-05-11 16:18:11 UTC) #39
commit-bot: I haz the power
Presubmit check for 10198040-70002 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 7 months ago (2012-05-11 16:18:34 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/60005
8 years, 7 months ago (2012-05-11 16:29:47 UTC) #41
commit-bot: I haz the power
8 years, 7 months ago (2012-05-11 18:06:28 UTC) #42
Change committed as 136611

Powered by Google App Engine
This is Rietveld 408576698