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

Issue 10918189: Add PrerenderStatusEvent on Prerenders (Closed)

Created:
8 years, 3 months ago by gavinp
Modified:
8 years ago
Reviewers:
dominich, tburkard, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, gavinp+prer_chromium.org, dominich+watch_chromium.org, darin-cc_chromium.org, mmenke
Visibility:
Public.

Description

Add PrerenderStatusEvent on Prerenders Changes the events a bit as they are passed down, and unifies the PrerenderingSupport and PrerenderingDispatcher interfaces. Read this together with the WebKit issue https://bugs.webkit.org/show_bug.cgi?id=96474 . Also, a few other code cleanups/assertion cleanups I came across while coding this. This change will need to be staged carefully (the change to the WebPrerenderingSupport interface is going to require around four gardened checkins). R=mmenke@chromium.org BUG=None

Patch Set 1 #

Total comments: 12

Patch Set 2 : rebase to trunk, remove some unrelated fixes... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+256 lines, -137 lines) Patch
M chrome/browser/prerender/prerender_browsertest.cc View 1 1 chunk +34 lines, -2 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.h View 1 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_contents.cc View 1 6 chunks +14 lines, -43 lines 0 comments Download
M chrome/browser/prerender/prerender_link_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prerender/prerender_manager.h View 1 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/prerender/prerender_manager.cc View 1 7 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/prerender/prerender_unittest.cc View 1 7 chunks +7 lines, -4 lines 0 comments Download
M chrome/common/prerender_messages.h View 1 chunk +10 lines, -5 lines 0 comments Download
M chrome/renderer/prerender/prerender_dispatcher.h View 1 2 chunks +15 lines, -8 lines 0 comments Download
M chrome/renderer/prerender/prerender_dispatcher.cc View 1 1 chunk +73 lines, -37 lines 0 comments Download
M chrome/renderer/prerender/prerender_dispatcher_unittest.cc View 7 chunks +65 lines, -29 lines 0 comments Download
M chrome/test/data/prerender/prerender_loader.html View 1 2 chunks +23 lines, -1 line 0 comments Download
M chrome/test/data/prerender/prerender_page.html View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 3 (0 generated)
gavinp
Timo: I banged this out on my flight today. What do you think about it?
8 years, 3 months ago (2012-09-12 07:12:45 UTC) #1
dominich
Can you separate the renderer-side cleanup from the new event code? https://chromiumcodereview.appspot.com/10918189/diff/1/chrome/browser/prerender/prerender_contents.h File chrome/browser/prerender/prerender_contents.h (right): ...
8 years, 3 months ago (2012-09-12 15:21:03 UTC) #2
mmenke
8 years, 3 months ago (2012-09-12 16:31:45 UTC) #3
http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
chrome/browser/prerender/prerender_manager.cc:834: origin, process_id, -1, url,
referrer, size, session_storage_namespace));
Won't this break PrerenderMsg_StoppedPrerender?  We'll glom all pending
prerenders into a group with an id of -1, send multiple start events for it
(Which won't get passed on to WebKit), and then when one of them stops, we'll
delete them all, renderer-side, but keep them browser-side, while WebKit remains
happily oblivious.

http://codereview.chromium.org/10918189/diff/1/chrome/browser/prerender/prere...
chrome/browser/prerender/prerender_manager.cc:942: return new
PrerenderHandle(preexisting_prerender_data);
When we merge prerenders, only one of them will ever get updates.

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:103: extra_data.size(),
extra_data.render_view_route_id()));
So if the same prerender is added twice, then the first one is stopped, we won't
cancel the underlying prerender, and it will still be used if the second
prerender is navigated to, but we won't be sending any prerender events for it.

Is this the behavior we want?

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:112: prerenders_.erase(it);
Is the find / check / erase needed?

Doesn't just "prerenders_.erase(extra_data.prerender_id());" do the trick, here
and below?

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.h:54: // from the browser
process, which prerenders have been launched.
nit:  Comments are generally capitalized (And those ending in periods should
always be capitalized).

http://codereview.chromium.org/10918189/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.h:56: std::set<int>
running_prerenders_;
|running_prerenders_| doesn't seem to serve any purpose.  Also, if there's any
need to track running prerenders apart from prerender_aliases_, I'd suggest
combining them in a single map.

Powered by Google App Engine
This is Rietveld 408576698