|
|
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 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNew 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 #Messages
Total messages: 42 (0 generated)
This is the chrome side of the new WebKit API for prerendering at https://bugs.webkit.org/show_bug.cgi?id=85005 . To stage this for landing, we need to land https://bugs.webkit.org/show_bug.cgi?id=84880 first.
Can you please indicate what you want each reviewer to look at? On Thu, Apr 26, 2012 at 6:13 PM, <gavinp@chromium.org> wrote: > Reviewers: John Abd-El-Malek, Matt Menke, cbentzel, jamesr, abarth, > > Message: > This is the chrome side of the new WebKit API for prerendering at > https://bugs.webkit.org/show_**bug.cgi?id=85005<https://bugs.webkit.org/show_.... > > To stage this for landing, we need to land > https://bugs.webkit.org/show_**bug.cgi?id=84880<https://bugs.webkit.org/show_.... > > 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<https://bugs.webkit.org/show_..., 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 > > > Please review this at http://codereview.chromium.**org/10198040/<http://codereview.chromium.org/101... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M chrome/browser/chrome_content_**browser_client.cc > M chrome/browser/prerender/**prerender_browsertest.cc > M chrome/browser/prerender/**prerender_contents.h > A chrome/browser/prerender/**prerender_link_manager.h > A chrome/browser/prerender/**prerender_link_manager.cc > A chrome/browser/prerender/**prerender_link_manager_**factory.h > A chrome/browser/prerender/**prerender_link_manager_**factory.cc > M chrome/browser/prerender/**prerender_manager.h > D chrome/browser/prerender/**prerender_manager_unittest.cc > A chrome/browser/prerender/**prerender_message_filter.h > A chrome/browser/prerender/**prerender_message_filter.cc > A + chrome/browser/prerender/**prerender_unittest.cc > M chrome/chrome_browser.gypi > M chrome/chrome_renderer.gypi > M chrome/chrome_tests.gypi > M chrome/common/prerender_**messages.h > M chrome/renderer/chrome_**content_renderer_client.cc > M chrome/renderer/prerender/**prerender_dispatcher.cc > A chrome/renderer/prerender/**prerender_extra_data.h > A chrome/renderer/prerender/**prerender_extra_data.cc > A chrome/renderer/prerender/**prerenderer_client.h > A chrome/renderer/prerender/**prerenderer_client.cc > A chrome/renderer/prerender/**prerendering_platform.h > A chrome/renderer/prerender/**prerendering_platform.cc > A chrome/test/data/prerender/**prerender_loader_removing_**links.html > > >
mmenke: I'd like you to look at the chrome/browser/prerender code, which you reviewed earlier in http://codereview.chromium.org/9875026/ , please? jam: we spoke a lot about isolating this to chrome, and you've seen some of the preparation CLs for this. Can you look at chrome/chrome_content_browser_client.cc, and everything in chrome/common and chrome/renderer/ please? cbentzel: I'd like you to look at chrome/renderer/prerender/ too? Thank you!
drive-by nits and comments http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:29: i != e; ++i) { why do you need to declare e in the first part? ie, why not the more typical: for (typename MapForwards::const_iterator i = map_forwards.begin(); i != map_forwards.end(); ++i) { ... } http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:41: if (forwards_count != backwards_count) { nit: {} not necessary here http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:52: PrerenderLinkManager::PrerenderLinkManager( nit: unnecessary to wrap this line http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:78: // TODO(gavinp,dominich): After the Prerender API can send events back to From this code, I don't see why the urls_to_id_map_ is needed. It looks like it's just inserting and erasing from it without using the contents. What am I missing? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:85: ids_to_url_map_.insert(std::make_pair(child_and_prerender_id, url)); Why insert these if |did_successfully_add| is false? OnCancel and OnAbandon already search the map and handles them not being there. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) || !prerender_manager.IsEnabled() ? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.cc:30: } // anon namespace nit: // end namespace http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:253: int GetNextPrerenderID() { nit: method could be const http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:255: return last_prerender_id_; nit: or 'return ++last_prerender_id_;'
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.cc:55: if (message.type() == PrerenderHostMsg_AddPrerender::ID || nit, you can just write if (IPC_MESSAGE_CLASS(message) == PrerenderMsgStart)) *thread =... http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_message_filter.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.h:10: #include "content/public/browser/browser_thread.h" nit: not needed, since by definition browser_message_filter.hi ncludes it ( you're including it for a parameter in a virtual function) http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerender_extra_data.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerender_extra_data.h:17: PrerenderExtraData(int prerender_id, nit: should be 2 space tabbing http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerender_extra_data.h:26: static const PrerenderExtraData& fromPrerender( nit: chrome style is FromPrerender http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerenderer_client.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerenderer_client.cc:13: int getNextPrerenderId() { nit: google style.. also seems unnecessary to make this a function? http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerenderer_client.h:18: class WebPrerender; here and above, you don't need to forward declare classes that you use in virtual functions, since by definition they're already forward declared in the included headers http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerendering_platform.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerendering_platform.h:13: class WebPrerender; ditto http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerendering_platform.h:28: #endif // CHROME_RENDERER_PRERENDER_PRERENDERING_PLATFORM_H_ nit: blank line above
Dominic & John, Thank you very much for your reviews. This upload addresses most of your issues, but I left some questions in. Please push back if I'm missing something....? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:29: i != e; ++i) { On 2012/04/26 22:50:58, dominich wrote: > why do you need to declare e in the first part? ie, why not the more typical: > > for (typename MapForwards::const_iterator i = map_forwards.begin(); > i != map_forwards.end(); ++i) { > ... > } Pure superstition. The standard guarantees that the standard container end() etc... will run in constant time, but not how much constant time. Running it once and taking a copy is a hedge against a bad implementation of end. We have a good standard library on all platforms, so I am removing this for end. However, in the situations where I loop on upper_bound(), I'm leaving it in place: upper_bound() has a complexity guarantee of only log(size()) on the standard associative containers, so without the copy I'm doing an O(n*log(size()) loop, which is probably worth the copy to avoid. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:41: if (forwards_count != backwards_count) { On 2012/04/26 22:50:58, dominich wrote: > nit: {} not necessary here Done. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:52: PrerenderLinkManager::PrerenderLinkManager( On 2012/04/26 22:50:58, dominich wrote: > nit: unnecessary to wrap this line Done. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:78: // TODO(gavinp,dominich): After the Prerender API can send events back to On 2012/04/26 22:50:58, dominich wrote: > From this code, I don't see why the urls_to_id_map_ is needed. It looks like > it's just inserting and erasing from it without using the contents. What am I > missing? Two links prerender the same page from the same origin; for instance, from a page, and from an iframe contained in the page. One of them cancels the prerender. This map is currently used in OnCancelPrerender() to figure out what happens to the underlying prerender, when. There are browser tests that depend on this. There was a discussion in the last bug about if I should just be keeping the map from urls, with counts: http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre... My claim remains that this work will end up being needed when events go both ways, and I didn't want to write this twice. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:85: ids_to_url_map_.insert(std::make_pair(child_and_prerender_id, url)); On 2012/04/26 22:50:58, dominich wrote: > Why insert these if |did_successfully_add| is false? OnCancel and OnAbandon > already search the map and handles them not being there. Done. What I liked about the original approach was that a debug build had a nice NOTREACHED() when the API double-canceled, but this is a release, let's drop it. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:160: e = urls_to_id_map_.upper_bound(url); I'm leaving this one here, multimap::upper_bound() is not cheap (see discussion above). http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) On 2012/04/26 22:50:58, dominich wrote: > || !prerender_manager.IsEnabled() ? Isn't that redundant with the check on PrerenderManager::IsPrerenderingPossible from the PrerenderManagerFactory::BuildServiceInstanceFor ? If it isn't redundant to it, should we fix PrerenderManagerFactory::BuildServiceInstanceFor instead? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.cc:30: } // anon namespace On 2012/04/26 22:50:58, dominich wrote: > nit: // end namespace Done, also in prerenderer_client.cc, and added the omitted one in prerender_link_manager.cc. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.cc:55: if (message.type() == PrerenderHostMsg_AddPrerender::ID || On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit, you can just write > > if (IPC_MESSAGE_CLASS(message) == PrerenderMsgStart)) > *thread =... I'm scared to do that: what if someone later adds another prerender message, say a routed one, to a browser? Would this force that surprisingly onto the UI thread? In prerendering, we have launcher contexts (i.e. any webpage), and prerender contexts (i.e. pages being prerendered), so it's really reasonable to expect this to happen in the future, and not use this message_filter, but instead one that's only installed on prerendered pages? Given that, does it make sense to leave this as it is? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_message_filter.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_message_filter.h:10: #include "content/public/browser/browser_thread.h" On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit: not needed, since by definition browser_message_filter.hi ncludes it ( > you're including it for a parameter in a virtual function) Done. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:253: int GetNextPrerenderID() { On 2012/04/26 22:50:58, dominich wrote: > nit: method could be const I don't think so, unless I make last_prerender_id_ mutable? http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:255: return last_prerender_id_; On 2012/04/26 22:50:58, dominich wrote: > nit: or 'return ++last_prerender_id_;' Done, though I bet a guinea that it compiles to the same text on all targets. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerender_extra_data.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerender_extra_data.h:17: PrerenderExtraData(int prerender_id, On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit: should be 2 space tabbing Done. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerender_extra_data.h:26: static const PrerenderExtraData& fromPrerender( On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit: chrome style is FromPrerender Done. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerenderer_client.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerenderer_client.cc:13: int getNextPrerenderId() { On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit: google style.. also seems unnecessary to make this a function? Done. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerenderer_client.h:18: class WebPrerender; On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > here and above, you don't need to forward declare classes that you use in > virtual functions, since by definition they're already forward declared in the > included headers Done. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... File chrome/renderer/prerender/prerendering_platform.h (right): http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerendering_platform.h:13: class WebPrerender; On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > ditto Done. http://codereview.chromium.org/10198040/diff/6001/chrome/renderer/prerender/p... chrome/renderer/prerender/prerendering_platform.h:28: #endif // CHROME_RENDERER_PRERENDER_PRERENDERING_PLATFORM_H_ On 2012/04/27 15:59:50, John Abd-El-Malek wrote: > nit: blank line above Done.
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:78: // TODO(gavinp,dominich): After the Prerender API can send events back to On 2012/04/27 20:31:32, gavinp wrote: > On 2012/04/26 22:50:58, dominich wrote: > > From this code, I don't see why the urls_to_id_map_ is needed. It looks like > > it's just inserting and erasing from it without using the contents. What am I > > missing? > > Two links prerender the same page from the same origin; for instance, from a > page, and from an iframe contained in the page. So they'd have the same child_id but different prerender_ids, no? > One of them cancels the > prerender. This map is currently used in OnCancelPrerender() to figure out what > happens to the underlying prerender, when. There are browser tests that depend > on this. Perhaps it would be better to propagate the prerender_id (or child_and_prerender_id) to PrerenderManager instead of using the URL as the key there. > > There was a discussion in the last bug about if I should just be keeping the map > from urls, with counts: > http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre... > > > My claim remains that this work will end up being needed when events go both > ways, and I didn't want to write this twice. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) On 2012/04/27 20:31:32, gavinp wrote: > On 2012/04/26 22:50:58, dominich wrote: > > || !prerender_manager.IsEnabled() ? > > Isn't that redundant with the check on PrerenderManager::IsPrerenderingPossible > from the PrerenderManagerFactory::BuildServiceInstanceFor ? > > If it isn't redundant to it, should we fix > PrerenderManagerFactory::BuildServiceInstanceFor instead? You're probably right, but I recently was burned by not checking for IsEnabled when Omnibox Prerendering so I'm cautious. Feel free to do the work of checking my assertion and responding accordingly :) http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:253: int GetNextPrerenderID() { On 2012/04/27 20:31:32, gavinp wrote: > On 2012/04/26 22:50:58, dominich wrote: > > nit: method could be const > > I don't think so, unless I make last_prerender_id_ mutable? You're right, of course. Review blindness. http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:31: // PrerenderLinkManager implements the API on Link elements for all documents I think it's a little odd that this should have a lifetime distinct from PrerenderManager. Should PrerenderManager perhaps own an instance of this object as that is already ProfileKeyed. I know PrerenderManager is a little full, so maybe we need to split PrerenderManager into the management part, and the Service part that can own pointers to the two managers. Or something. http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager_factory.cc:44: bool PrerenderLinkManagerFactory::ServiceHasOwnInstanceInIncognito() { Is there a reason why the link manager instance can't be shared between normal and incognito profiles? http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:253: int GetNextPrerenderID() { does this method even need to exist?
Thanks a ton Dominic... I don't quite get what you're asking about for propogating ids into the prerender manager: could you expand what the problem is we're solving and how that might go, if I haven't convinced you? Or a short VC might be best? Anytime I'm free. http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager.cc:78: // TODO(gavinp,dominich): After the Prerender API can send events back to On 2012/04/27 21:52:01, dominich wrote: > On 2012/04/27 20:31:32, gavinp wrote: > > On 2012/04/26 22:50:58, dominich wrote: > > > From this code, I don't see why the urls_to_id_map_ is needed. It looks like > > > it's just inserting and erasing from it without using the contents. What am > I > > > missing? > > > > Two links prerender the same page from the same origin; for instance, from a > > page, and from an iframe contained in the page. > > So they'd have the same child_id but different prerender_ids, no? Yes. However, notice that OnCancelPrerender has different behaviour depending on if the url is in urls_to_id_map_? It's 37 lines down from here. When you remove the first link, the prerender stays alive. When you remove the second, we kill it. > > > One of them cancels the > > prerender. This map is currently used in OnCancelPrerender() to figure out > what > > happens to the underlying prerender, when. There are browser tests that > depend > > on this. > > Perhaps it would be better to propagate the prerender_id (or > child_and_prerender_id) to PrerenderManager instead of using the URL as the key > there. That sounds to me like making this entire class part of the PrerenderManager, that's a viable strategy, but I find the PrerenderManager too confusing already; why not separate out the semantics of links from it? I'd like to see a PrerenderManager that is concerned with managing prerenders through their lifetime, and notifying observers as needed. If a launcher has different semantics, they should implement those in a separate class which is a user and/or observer of a simpler PrerenderManager. I know I haven't done that yet, PrerenderManager::AddLinkFromRel... is still there, but my hope is to just add links to the prerendermanager based on their proper unique ID, and deal with them through that. What distinguishes prerenders is, I believe, the pair<GURL, int web_storage_namespace>, although that bears some more thought. So eventually some object like that will be in the url side of these maps; and this prerender_link_manager will be > > > > There was a discussion in the last bug about if I should just be keeping the > map > > from urls, with counts: > > > http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre... > > > > > > My claim remains that this work will end up being needed when events go both > > ways, and I didn't want to write this twice. > http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) On 2012/04/27 21:52:01, dominich wrote: > On 2012/04/27 20:31:32, gavinp wrote: > > On 2012/04/26 22:50:58, dominich wrote: > > > || !prerender_manager.IsEnabled() ? > > > > Isn't that redundant with the check on > PrerenderManager::IsPrerenderingPossible > > from the PrerenderManagerFactory::BuildServiceInstanceFor ? > > > > If it isn't redundant to it, should we fix > > PrerenderManagerFactory::BuildServiceInstanceFor instead? > > You're probably right, but I recently was burned by not checking for IsEnabled > when Omnibox Prerendering so I'm cautious. Feel free to do the work of checking > my assertion and responding accordingly :) Good point. I'll follow up. http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:31: // PrerenderLinkManager implements the API on Link elements for all documents I wrote it that way, and cbentzel requested I do it this way: http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre... If you two can come to agreement, I will do what you agree on. On 2012/04/27 21:52:01, dominich wrote: > I think it's a little odd that this should have a lifetime distinct from > PrerenderManager. Should PrerenderManager perhaps own an instance of this object > as that is already ProfileKeyed. > > I know PrerenderManager is a little full, so maybe we need to split > PrerenderManager into the management part, and the Service part that can own > pointers to the two managers. Or something. http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager_factory.cc:44: bool PrerenderLinkManagerFactory::ServiceHasOwnInstanceInIncognito() { On 2012/04/27 21:52:01, dominich wrote: > Is there a reason why the link manager instance can't be shared between normal > and incognito profiles? Well, two reasons. Firstly, the PrerenderManager isn't shared, and it has a link to one. Secondly, if the same URL was prerendered by link from each type of session, they'd interact with each other. http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:253: int GetNextPrerenderID() { On 2012/04/27 21:52:01, dominich wrote: > does this method even need to exist? No, done.
On Fri, Apr 27, 2012 at 4:31 PM, <gavinp@chromium.org> wrote: > Thanks a ton Dominic... I don't quite get what you're asking about for > propogating ids into the prerender manager: could you expand what the > problem is > we're solving and how that might go, if I haven't convinced you? Or a > short VC > might be best? Anytime I'm free. I don't think it's clear what the role of the PrerenderManager is or should be, or what the PrerenderLinkManager is really for. Let's talk it out on chat and revert to this CL comment thread once I understand better. > > > > http://codereview.chromium.**org/10198040/diff/6001/chrome/** > browser/prerender/prerender_**link_manager.cc<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<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 back to > On 2012/04/27 21:52:01, dominich wrote: > >> On 2012/04/27 20:31:32, gavinp wrote: >> > On 2012/04/26 22:50:58, dominich wrote: >> > > From this code, I don't see why the urls_to_id_map_ is needed. It >> > looks like > >> > > it's just inserting and erasing from it without using the >> > contents. What am > >> I >> > > missing? >> > >> > Two links prerender the same page from the same origin; for >> > instance, from a > >> > page, and from an iframe contained in the page. >> > > So they'd have the same child_id but different prerender_ids, no? >> > > Yes. However, notice that OnCancelPrerender has different behaviour > depending on if the url is in urls_to_id_map_? It's 37 lines down from > here. When you remove the first link, the prerender stays alive. When > you remove the second, we kill it. > > > > > One of them cancels the >> > prerender. This map is currently used in OnCancelPrerender() to >> > figure out > >> what >> > happens to the underlying prerender, when. There are browser tests >> > that > >> depend >> > on this. >> > > Perhaps it would be better to propagate the prerender_id (or >> child_and_prerender_id) to PrerenderManager instead of using the URL >> > as the key > >> there. >> > > That sounds to me like making this entire class part of the > PrerenderManager, that's a viable strategy, but I find the > PrerenderManager too confusing already; why not separate out the > semantics of links from it? > > I'd like to see a PrerenderManager that is concerned with managing > prerenders through their lifetime, and notifying observers as needed. > If a launcher has different semantics, they should implement those in a > separate class which is a user and/or observer of a simpler > PrerenderManager. I know I haven't done that yet, > PrerenderManager::**AddLinkFromRel... is still there, but my hope is to > just add links to the prerendermanager based on their proper unique ID, > and deal with them through that. > > What distinguishes prerenders is, I believe, the pair<GURL, int > web_storage_namespace>, although that bears some more thought. So > eventually some object like that will be in the url side of these maps; > and this prerender_link_manager will be > > > > >> > There was a discussion in the last bug about if I should just be >> > keeping the > >> map >> > from urls, with counts: >> > >> > > http://codereview.chromium.**org/9875026/diff/2001/chrome/** > browser/prerender/prerender_**link_manager.h#pair-53<http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.h#pair-53> > >> > >> > >> > My claim remains that this work will end up being needed when events >> > go both > >> > ways, and I didn't want to write this twice. >> > > > http://codereview.chromium.**org/10198040/diff/6001/chrome/** > browser/prerender/prerender_**link_manager_factory.cc<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<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 21:52:01, dominich wrote: > >> On 2012/04/27 20:31:32, gavinp wrote: >> > On 2012/04/26 22:50:58, dominich wrote: >> > > || !prerender_manager.IsEnabled() ? >> > >> > Isn't that redundant with the check on >> PrerenderManager::**IsPrerenderingPossible >> > from the PrerenderManagerFactory::**BuildServiceInstanceFor ? >> > >> > If it isn't redundant to it, should we fix >> > PrerenderManagerFactory::**BuildServiceInstanceFor instead? >> > > You're probably right, but I recently was burned by not checking for >> > IsEnabled > >> when Omnibox Prerendering so I'm cautious. Feel free to do the work of >> > checking > >> my assertion and responding accordingly :) >> > > Good point. I'll follow up. > > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_link_manager.h<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_link_manager.h> > File chrome/browser/prerender/**prerender_link_manager.h (right): > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_link_manager.h#**newcode31<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_link_manager.h#newcode31> > chrome/browser/prerender/**prerender_link_manager.h:31: // > PrerenderLinkManager implements the API on Link elements for all > documents > I wrote it that way, and cbentzel requested I do it this way: > http://codereview.chromium.**org/9875026/diff/2001/chrome/** > browser/prerender/prerender_**link_manager.cc#pair-108<http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#pair-108> > > If you two can come to agreement, I will do what you agree on. Yes, that's exactly what I was talking about. Though I might be persuaded that it's better to have a PrerenderService that is a ProfileKeyedService that has two pointers: PrerenderManager* and PrerenderLinkManager*. Of course, this also ties them together in terms of incognito uniqueness, which may not be what we want. I'm not tied to my suggestion, I just want to make sure that I get out my thoughts so they can be considered and rejected. > > > On 2012/04/27 21:52:01, dominich wrote: > >> I think it's a little odd that this should have a lifetime distinct >> > from > >> PrerenderManager. Should PrerenderManager perhaps own an instance of >> > this object > >> as that is already ProfileKeyed. >> > > I know PrerenderManager is a little full, so maybe we need to split >> PrerenderManager into the management part, and the Service part that >> > can own > >> pointers to the two managers. Or something. >> > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_link_manager_**factory.cc<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_link_manager_factory.cc> > File chrome/browser/prerender/**prerender_link_manager_**factory.cc > (right): > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_link_manager_**factory.cc#newcode44<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_link_manager_factory.cc#newcode44> > chrome/browser/prerender/**prerender_link_manager_**factory.cc:44: bool > PrerenderLinkManagerFactory::**ServiceHasOwnInstanceInIncogni**to() { > On 2012/04/27 21:52:01, dominich wrote: > >> Is there a reason why the link manager instance can't be shared >> > between normal > >> and incognito profiles? >> > > Well, two reasons. Firstly, the PrerenderManager isn't shared, and it > has a link to one. Secondly, if the same URL was prerendered by link > from each type of session, they'd interact with each other. > > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_unittest.cc<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_unittest.cc> > File chrome/browser/prerender/**prerender_unittest.cc (right): > > http://codereview.chromium.**org/10198040/diff/18001/** > chrome/browser/prerender/**prerender_unittest.cc#**newcode253<http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/prerender_unittest.cc#newcode253> > chrome/browser/prerender/**prerender_unittest.cc:253: int > GetNextPrerenderID() { > On 2012/04/27 21:52:01, dominich wrote: > >> does this method even need to exist? >> > > No, done. > > http://codereview.chromium.**org/10198040/<http://codereview.chromium.org/101... >
OK: after more back and forth with Dominic, and together with other concerns from mmenke and cbentzel, I'll hack this up to use the refcount in the PrerenderManager, and see how that goes for another upload.
OK, I know I'm stubborn, but after getting strong review comments from Matt, Chris and Dominic all saying the same thing, I eventually give in. This new version gets rid of the double maps, and carries the new data structures available in the prerender link manager through to the prerender manager. I want to apologise now for all my stubborn resistance earlier: I like this patch better than the others. WDYT? http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/18001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:82: UrlToIdPairMap urls_to_id_map_; Yay! http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:302: EXPECT_FALSE(GetRenderViewHost()); This is here as a merge artifact. See http://codereview.chromium.org/10270003/ for the review of this move towards the StudlyCaps. It's not really dependent though, I can rebase it either way. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:244: int GetNextPrerenderID() { OK, so you know how I removed this method in my last upload? Because the new method refactors the prerender_manager, the SourceRenderViewClosed unit test actually migrated to the link manager interface, and it was either use this method, or make last_prerender_id_ protected.
First thoughts. It's definitely cleaner, but I'm still not sure that it goes far enough. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:727: PrerenderManager* prerender_manager() const { nit: should this be GetPrerenderManager? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:735: PrerenderLinkManager* prerender_link_manager() const { nit: should this be GetPrerenderLinkManager? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:191: void PrerenderContents::AddPendingPrerender(const GURL& url, I was worried about the loss of Origin here, but it's absolutely valid that you can't have pending Omnibox prerenders. However, with other prerender origins potentially coming down the pike it might be premature to remove it. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:193: pending_prerender_list_.push_back( nit: this line should be merged with the one below http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:216: prerender_manager_->AddPrerenderFromLinkRelPrerender( as above, this assumption makes me nervous. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: child_id_, route_id_, it->first, it->second, size_, is it possible that the WebView size has changed since the pending prerender was added? Maybe get the size again from the RenderViewHost here? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:281: if (size_.IsEmpty()) { will this ever happen now the size is coming from the WebView directly? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:45: // TODO(gavinp): Add tests to insure fragments work, then remove this fragment nit: ensure http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:63: if (!manager_->AddPrerenderFromLinkRelPrerender( possibly contentious comment: why not move this map to the PrerenderManager and use that as a key for all active Prerenders instead of using the URL/SessionStorage as a key? Yes, this would make this merely a wrapper interface, but I think that's reasonable. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:180: int add_count_; active_count_? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:319: if (--prerender_contents_data.add_count_ > 0) why not: if (--prerender_contents_data.add_count_ == 0) prerender_contents_data.contents_->Destroy(FINAL_STATUS_CANCELLED); http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:881: RecordFinalStatus(origin, experiment, FINAL_STATUS_DUPLICATE); this is going to mess with the stats. If the prerender is eventually used, it wouldn't be captured as USED. Maybe we shouldn't record a final status for these? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:105: // set the initial window size of the RenderViewHost used for prerendering. this comment needs to be updated. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:304: // RenderViewHost specified by |child_route_id_pair|. The |origin| specifies update comment http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:376: FindPrerenderContentsForURL(const GURL& url); should this also take a session storage namespace parameter?
On 2012/04/29 18:52:54, dominich wrote: > First thoughts. It's definitely cleaner, but I'm still not sure that it goes far > enough. > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:727: PrerenderManager* > prerender_manager() const { > nit: should this be GetPrerenderManager? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:735: PrerenderLinkManager* > prerender_link_manager() const { > nit: should this be GetPrerenderLinkManager? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_contents.cc (right): > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:191: void > PrerenderContents::AddPendingPrerender(const GURL& url, > I was worried about the loss of Origin here, but it's absolutely valid that you > can't have pending Omnibox prerenders. However, with other prerender origins > potentially coming down the pike it might be premature to remove it. > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:193: > pending_prerender_list_.push_back( > nit: this line should be merged with the one below > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:216: > prerender_manager_->AddPrerenderFromLinkRelPrerender( > as above, this assumption makes me nervous. > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:217: child_id_, route_id_, > it->first, it->second, size_, > is it possible that the WebView size has changed since the pending prerender was > added? Maybe get the size again from the RenderViewHost here? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:281: if (size_.IsEmpty()) { > will this ever happen now the size is coming from the WebView directly? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_link_manager.cc (right): > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:45: // TODO(gavinp): Add > tests to insure fragments work, then remove this fragment > nit: ensure > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:63: if > (!manager_->AddPrerenderFromLinkRelPrerender( > possibly contentious comment: why not move this map to the PrerenderManager and > use that as a key for all active Prerenders instead of using the > URL/SessionStorage as a key? > > Yes, this would make this merely a wrapper interface, but I think that's > reasonable. I thought about this some more, and I realize that I was mixing up the notion of the prerender_id coming from WebKit and a prerender_id that might be owned by PrerenderManager. Obviously prerenders from other sources cannot have the (webkit) prerender_id so moving this into PrerenderManager doesn't make sense. > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:180: int add_count_; > active_count_? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:319: if > (--prerender_contents_data.add_count_ > 0) > why not: > > if (--prerender_contents_data.add_count_ == 0) > prerender_contents_data.contents_->Destroy(FINAL_STATUS_CANCELLED); > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:881: RecordFinalStatus(origin, > experiment, FINAL_STATUS_DUPLICATE); > this is going to mess with the stats. If the prerender is eventually used, it > wouldn't be captured as USED. Maybe we shouldn't record a final status for > these? > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.h (right): > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.h:105: // set the initial window size > of the RenderViewHost used for prerendering. > this comment needs to be updated. > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.h:304: // RenderViewHost specified by > |child_route_id_pair|. The |origin| specifies > update comment > > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.h:376: > FindPrerenderContentsForURL(const GURL& url); > should this also take a session storage namespace parameter?
Thank you Dominic for taking time out of a Sunday to review my code. I hope this upload improves on it! http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:727: PrerenderManager* prerender_manager() const { On 2012/04/29 18:52:55, dominich wrote: > nit: should this be GetPrerenderManager? Yes. I'm uploading a separate CL for this, and adding you as a reviewer. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:735: PrerenderLinkManager* prerender_link_manager() const { On 2012/04/29 18:52:55, dominich wrote: > nit: should this be GetPrerenderLinkManager? Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:191: void PrerenderContents::AddPendingPrerender(const GURL& url, On 2012/04/29 18:52:55, dominich wrote: > I was worried about the loss of Origin here, but it's absolutely valid that you > can't have pending Omnibox prerenders. However, with other prerender origins > potentially coming down the pike it might be premature to remove it. I'll ping Timo, who's writing the history navigator. Otherwise I can't think of any other origins that would require this delay. Can you? http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:193: pending_prerender_list_.push_back( On 2012/04/29 18:52:55, dominich wrote: > nit: this line should be merged with the one below Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:216: prerender_manager_->AddPrerenderFromLinkRelPrerender( On 2012/04/29 18:52:55, dominich wrote: > as above, this assumption makes me nervous. I think it's easy enough to add back if we need to. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: child_id_, route_id_, it->first, it->second, size_, On 2012/04/29 18:52:55, dominich wrote: > is it possible that the WebView size has changed since the pending prerender was > added? Maybe get the size again from the RenderViewHost here? I believe that the prerender can't change size while it's not visible and prerendering, but mmenke will know better. It would be a shame if it could, since then in this case we'd be back to using the less reliable sizes from the RenderViewHost in the delayed prerendering case. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:281: if (size_.IsEmpty()) { On 2012/04/29 18:52:55, dominich wrote: > will this ever happen now the size is coming from the WebView directly? Yes. Omnibox prerendering is probably the biggest cause. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:45: // TODO(gavinp): Add tests to insure fragments work, then remove this fragment On 2012/04/29 18:52:55, dominich wrote: > nit: ensure Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:63: if (!manager_->AddPrerenderFromLinkRelPrerender( On 2012/04/29 18:52:55, dominich wrote: > possibly contentious comment: why not move this map to the PrerenderManager and > use that as a key for all active Prerenders instead of using the > URL/SessionStorage as a key? > > Yes, this would make this merely a wrapper interface, but I think that's > reasonable. We don't key on url/sessionstorage though. The key is the prerender_id, assigned in chrome/renderer/prerender/prerenderer_client.cc. The fact that adding the same url twice sometimes causes a request to map to the same underlying prerender is now a detail that only the prerender_manager is concerned with... I think a good question is: should AddPrerender... et al return a handle to the prerender they created, so I can later refer to it without searching based on the key criteria the prerender manager exposes? That would make cancel not require an url, either, which is awesome; our maps would be from a child_and_link_prerender_id to a prerender_id (from the prerender manager). http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:180: int add_count_; On 2012/04/29 18:52:55, dominich wrote: > active_count_? Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:319: if (--prerender_contents_data.add_count_ > 0) On 2012/04/29 18:52:55, dominich wrote: > why not: > > if (--prerender_contents_data.add_count_ == 0) > prerender_contents_data.contents_->Destroy(FINAL_STATUS_CANCELLED); > Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:881: RecordFinalStatus(origin, experiment, FINAL_STATUS_DUPLICATE); On 2012/04/29 18:52:55, dominich wrote: > this is going to mess with the stats. If the prerender is eventually used, it > wouldn't be captured as USED. Maybe we shouldn't record a final status for > these? Isn't this already exactly the case on pages that launch two prerenders of the same target? One is OK, one fails FINAL_STATUS_DUPLICATE, and on navigation, one gets USED? Maybe the behaviour is broken, but I don't think this change breaks it. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:105: // set the initial window size of the RenderViewHost used for prerendering. On 2012/04/29 18:52:55, dominich wrote: > this comment needs to be updated. Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:304: // RenderViewHost specified by |child_route_id_pair|. The |origin| specifies On 2012/04/29 18:52:55, dominich wrote: > update comment Done. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:376: FindPrerenderContentsForURL(const GURL& url); On 2012/04/29 18:52:55, dominich wrote: > should this also take a session storage namespace parameter? Yup, if we ever key prerenders on the session storage namespace. But they're still keyed only on URL, and we still just fail to use a prerender when a request comes in for the same URL with a different session storage namespace. This CL doesn't change that behaviour; probably it will be fixed as part of any effort to allow multiple prerenders at the same time. We might also want to handle common cases of session webstorage being different; the most sadmaking to me is different tab navigation. We kill a prerender on different tab clicks, because of the webstorage being different. But the vast majority of the time, the launching page hasn't mutated its storage since launching the prerender, or the prerender target hasn't read its storage namespace, or the prerender target is on a different site. All of these are fine, with differing amounts of effort. But now we just kill them all...
Sorry for the churn. I think it's completely necessary as we move towards a much tighter API. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:191: void PrerenderContents::AddPendingPrerender(const GURL& url, On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > I was worried about the loss of Origin here, but it's absolutely valid that > you > > can't have pending Omnibox prerenders. However, with other prerender origins > > potentially coming down the pike it might be premature to remove it. > > I'll ping Timo, who's writing the history navigator. Otherwise I can't think of > any other origins that would require this delay. Can you? None that are in active development, and it would be reasonably trivial to add it back if necessary. I agree that the reduction in API complexity given the current state of the implementation is worth it. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: child_id_, route_id_, it->first, it->second, size_, On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > is it possible that the WebView size has changed since the pending prerender > was > > added? Maybe get the size again from the RenderViewHost here? > > I believe that the prerender can't change size while it's not visible and > prerendering, but mmenke will know better. It would be a shame if it could, > since then in this case we'd be back to using the less reliable sizes from the > RenderViewHost in the delayed prerendering case. It sounds like we already have an issue that we might prerender at one size and then resize on swap. In which case this should work the same as the existing code, which is reasonable. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:63: if (!manager_->AddPrerenderFromLinkRelPrerender( On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > possibly contentious comment: why not move this map to the PrerenderManager > and > > use that as a key for all active Prerenders instead of using the > > URL/SessionStorage as a key? > > > > Yes, this would make this merely a wrapper interface, but I think that's > > reasonable. > > We don't key on url/sessionstorage though. The key is the prerender_id, > assigned in chrome/renderer/prerender/prerenderer_client.cc. The fact that > adding the same url twice sometimes causes a request to map to the same > underlying prerender is now a detail that only the prerender_manager is > concerned with... > > I think a good question is: should AddPrerender... et al return a handle to the > prerender they created, so I can later refer to it without searching based on > the key criteria the prerender manager exposes? That would make cancel not > require an url, either, which is awesome; our maps would be from a > child_and_link_prerender_id to a prerender_id (from the prerender manager). I believe this will be necessary (or convenient) for multiple prerender support. I can't decide if it should be part of this CL as it complicates the work on PrerenderManager, but it introduces significant changes to this class. I suggest leaving this CL as it is, but creating a bug to make the change to a PrerenderHandle. It will simplify this case, and also allow easier cancelation from Omnibox, and will be useful for handling edge cases in multiple prerender support. Does anyone disagree? > http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:881: RecordFinalStatus(origin, experiment, FINAL_STATUS_DUPLICATE); On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > this is going to mess with the stats. If the prerender is eventually used, it > > wouldn't be captured as USED. Maybe we shouldn't record a final status for > > these? > > Isn't this already exactly the case on pages that launch two prerenders of the > same target? One is OK, one fails FINAL_STATUS_DUPLICATE, and on navigation, > one gets USED? Maybe the behaviour is broken, but I don't think this change > breaks it. You're correct, but we do have to be careful with the refcounting here. Currently, Omnibox calls a specific method on PrerenderManager to cancel all Omnibox prerenders. We might want to be more surgical should we start multiple prerenders from the Omnibox and cancel a given URL (PrerenderHandle, whatever). When that happens, it will be tempting to minimize the API surface and remove the Omnibox-specific cancellation method and instead use MaybeCancelPrerender. However this will check the active count before cancelling but, in the case of the Omnibox, this is incorrect as we can request multiple prerenders for the same URL but from the same source, unlike the case of <link>. In that case, we want to cancel the prerender no matter what the active count. In a way, the active_count should actually be the number of independent sources that have requested a particular URL. It's fine for this CL, but would be a very simple mistake for someone (me, when I will have forgotten about this code review in a month or two) to make. I wonder if we can protect against it somehow, either by specifically not incrementing active_count for Omnibox origin, or something smarter. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:376: FindPrerenderContentsForURL(const GURL& url); On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > should this also take a session storage namespace parameter? > > Yup, if we ever key prerenders on the session storage namespace. But they're > still keyed only on URL, and we still just fail to use a prerender when a > request comes in for the same URL with a different session storage namespace. > This CL doesn't change that behaviour; probably it will be fixed as part of any > effort to allow multiple prerenders at the same time. Or the proposed CL to add a PrerenderHandle. > > We might also want to handle common cases of session webstorage being different; > the most sadmaking to me is different tab navigation. We kill a prerender on > different tab clicks, because of the webstorage being different. But the vast > majority of the time, the launching page hasn't mutated its storage since > launching the prerender, or the prerender target hasn't read its storage > namespace, or the prerender target is on a different site. All of these are > fine, with differing amounts of effort. But now we just kill them all... http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: render_view_host->GetSessionStorageNamespace()); I just noticed this fix. Thank you! http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:95: if (ids_to_url_map_.count(child_and_prerender_id)) I believe this check is redundant. You're passing in a key_type rather than an iterator so erase is safe (and will return the number of elements removed). http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:108: e = ids_to_url_map_.upper_bound(child_and_maximum_prerender_id); I wonder, given the expected size of the map, if it would be more efficient to iterate through the whole thing and check child_id and prerender_id manually. Actually, given the expected size of the map, it would probably be more efficient to store the ids_to_url_map_ as a std::vector<std::pair<child_id, prerender_id>, GURL> as they'll be contiguous in memory and hence more cacheable for these loops. Nits, of course. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:42: // assigned by the WebPrerendererClient. Is it ever not assigned by the WebPrerendererClient? http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:79: typedef std::multimap<GURL, ChildAndPrerenderIdPair> UrlToIdPairMap; This type isn't used any more, is it? http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:105: // empty, and the current tab size will be used if it is. Or a default in the case of there being no current tab. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:126: // Request cancelation of a previously added prerender. If the add_count_ of |active_count_| http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:127: // the prerender is one, it will be canceled. Otherwise, add_count_ will be 'The |active_count_| of the prerender will be decremented by one, and if it is then zero it will be canceled.' makes more sense to me, but YMMV. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:306: // active tab will be used, if available. or a default if not. http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:15: class PrerendererClient : public content::RenderViewObserver, I don't understand why this is both a RenderViewObserver and a WebPrerendererClient.
Mostly nits, though still want to go over the code once more. http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/24002/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: child_id_, route_id_, it->first, it->second, size_, On 2012/04/30 11:43:16, gavinp wrote: > On 2012/04/29 18:52:55, dominich wrote: > > is it possible that the WebView size has changed since the pending prerender > was > > added? Maybe get the size again from the RenderViewHost here? > > I believe that the prerender can't change size while it's not visible and > prerendering, but mmenke will know better. It would be a shame if it could, > since then in this case we'd be back to using the less reliable sizes from the > RenderViewHost in the delayed prerendering case. It's currently not possible for a hidden WebView to be resized, but I would be happier using the size passed along with the prerender request, rather than depend on that. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:71: typedef std::pair<GURL, content::Referrer> PendingPrerenderData; nit: Now that we have a "PrerenderExtraData", I suggest renaming this to "PrendingPrerenderInfo", just to make the names a little more distinct. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:66: return false; nit: Add braces when the antecedent takes more than one line. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:68: DCHECK(!ids_to_url_map_.count(child_and_prerender_id)); nit: DCHECK_EQ(0, ids_to_url_map_.count(child_and_prerender_id)); is generally preferred. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:86: manager_->MaybeCancelPrerender(url); Weird case 1: url is being prerendered by a prerendered page. We won't cancel, and will start prerendering if we swap in the prerendered page. Weird case 2: url is being prerendered by both a prerendered page and a non-prerendered page. On the final cancel, we'll cancel the prerender, but we'll start again when swapping in the prerendered page. Think they're beyond the scope of this CL, and probably beyond worth worrying about at all (Other than the fact we don't crash on them), but thought I'd mention them. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:56: void OnCancelPrerender(int prerender_id, int child_id); nit swap argument order, here and below. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:295: it->contents_->AddPendingPrerender(url, referrer); Random comment: Should we track size as well? Since we don't resize hidden windows, they're currently guaranteed to be the same, but should be be depending on that here? http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:86: if (!prerender_link_manager) Can this happen? http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:15: class PrerendererClient : public content::RenderViewObserver, Do we really need one of these per render view, instead of one per process? Not concerned about memory, but I don't like global variables when we don't need them. http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:6: function ExtractGetParameterBadlyAndInsecurely(param, default_value) { nit: defaultValue. Don't ask my why Javascript naming style is different from C++ style, but it is. Same goes for the rest of this file. http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:7: var re = RegExp("[&?]" + param + "=([^&?#]*)"); nit: Use single quotes. Same goes for the rest of this file.
Oops. Forgot one comment with that last batch. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:245: return ++last_prerender_id_; Using last here, but next in PrerenderClient is a little weird.
Thanks everyone for your reviews! The new upload should cover most remaining nits. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:71: typedef std::pair<GURL, content::Referrer> PendingPrerenderData; On 2012/04/30 18:35:22, Matt Menke wrote: > nit: Now that we have a "PrerenderExtraData", I suggest renaming this to > "PrendingPrerenderInfo", just to make the names a little more distinct. Done, but spelled right. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:66: return false; On 2012/04/30 18:35:22, Matt Menke wrote: > nit: Add braces when the antecedent takes more than one line. Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:68: DCHECK(!ids_to_url_map_.count(child_and_prerender_id)); On 2012/04/30 18:35:22, Matt Menke wrote: > nit: DCHECK_EQ(0, ids_to_url_map_.count(child_and_prerender_id)); is generally > preferred. Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:86: manager_->MaybeCancelPrerender(url); On 2012/04/30 18:35:22, Matt Menke wrote: > Weird case 1: url is being prerendered by a prerendered page. We won't > cancel, and will start prerendering if we swap in the prerendered page. > > Weird case 2: url is being prerendered by both a prerendered page and a > non-prerendered page. On the final cancel, we'll cancel the prerender, but > we'll start again when swapping in the prerendered page. > > Think they're beyond the scope of this CL, and probably beyond worth worrying > about at all (Other than the fact we don't crash on them), but thought I'd > mention them. These do make me sad all the same. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:95: if (ids_to_url_map_.count(child_and_prerender_id)) On 2012/04/30 15:52:05, dominich wrote: > I believe this check is redundant. You're passing in a key_type rather than an > iterator so erase is safe (and will return the number of elements removed). Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:42: // assigned by the WebPrerendererClient. On 2012/04/30 15:52:05, dominich wrote: > Is it ever not assigned by the WebPrerendererClient? No, never. Fixed. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:56: void OnCancelPrerender(int prerender_id, int child_id); On 2012/04/30 18:35:22, Matt Menke wrote: > nit swap argument order, here and below. Yes. Thanks for catching this; I should have done this in a previous review. :( Now done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:79: typedef std::multimap<GURL, ChildAndPrerenderIdPair> UrlToIdPairMap; On 2012/04/30 15:52:05, dominich wrote: > This type isn't used any more, is it? Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:295: it->contents_->AddPendingPrerender(url, referrer); On 2012/04/30 18:35:22, Matt Menke wrote: > Random comment: Should we track size as well? Since we don't resize hidden > windows, they're currently guaranteed to be the same, but should be be depending > on that here? Fair point. Done. I suspect this all changes a bit more too once we start returning a handle from this method; we'll want these "pending prerenders" to have a handle too, so they might end up moving into the prerender manager. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:105: // empty, and the current tab size will be used if it is. On 2012/04/30 15:52:05, dominich wrote: > Or a default in the case of there being no current tab. Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:126: // Request cancelation of a previously added prerender. If the add_count_ of On 2012/04/30 15:52:05, dominich wrote: > |active_count_| Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:127: // the prerender is one, it will be canceled. Otherwise, add_count_ will be On 2012/04/30 15:52:05, dominich wrote: > 'The |active_count_| of the prerender will be decremented by one, and if it is > then zero it will be canceled.' makes more sense to me, but YMMV. Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:306: // active tab will be used, if available. On 2012/04/30 15:52:05, dominich wrote: > or a default if not. Done. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:86: if (!prerender_link_manager) On 2012/04/30 18:35:22, Matt Menke wrote: > Can this happen? We return NULL if there's no prerender_manager in PrerenderLinkManagerFactory::BuildServiceInstanceFor. And, there won't be a PrerenderManagerFactory on a profile if PrerenderManager::IsPrerenderingPossible() returns false. So I think this happens if you're configured to not use prerendering. What if, while a prerender is running, you turn the option off? http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:245: return ++last_prerender_id_; On 2012/04/30 18:40:41, Matt Menke wrote: > Using last here, but next in PrerenderClient is a little weird. Since we use the last one here for the tests, I'm switching the use in PrerendererClient to be the same, and use last. http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:15: class PrerendererClient : public content::RenderViewObserver, On 2012/04/30 15:52:05, dominich wrote: > I don't understand why this is both a RenderViewObserver and a > WebPrerendererClient. This class is a WebPrerendererClient, it's called from WebKit before Prerendering, and it adds the ExtraData, which is the size and the new prerender_id. It needs to be a RenderViewObserver, so that it can find the size of the current RenderView. http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:15: class PrerendererClient : public content::RenderViewObserver, On 2012/04/30 18:35:22, Matt Menke wrote: > Do we really need one of these per render view, instead of one per process? Not > concerned about memory, but I don't like global variables when we don't need > them. The implementation of PrerendererClient::willAddPrerender() gets the size of the current RenderView for the request. That feels pretty per-RenderView to me, I think? http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:6: function ExtractGetParameterBadlyAndInsecurely(param, default_value) { On 2012/04/30 18:35:22, Matt Menke wrote: > nit: defaultValue. Don't ask my why Javascript naming style is different from > C++ style, but it is. Same goes for the rest of this file. Done. Matt, why does Javascript have a different naming style than C++? http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:7: var re = RegExp("[&?]" + param + "=([^&?#]*)"); On 2012/04/30 18:35:22, Matt Menke wrote: > nit: Use single quotes. Same goes for the rest of this file. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:977: ASSERT_TRUE(GetPrerenderLinkManager()->IsEmpty()); mmenke: this ASSERT caught a mistake I made reversing the id types in prerender_link_manager. Thanks!
http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... File chrome/renderer/prerender/prerendering_support.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... 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 file got renamed, and that's why this change.
Despite the number of nits, I'm pretty happy with the CL. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:71: typedef std::pair<GURL, content::Referrer> PendingPrerenderData; On 2012/04/30 23:55:39, gavinp wrote: > On 2012/04/30 18:35:22, Matt Menke wrote: > > nit: Now that we have a "PrerenderExtraData", I suggest renaming this to > > "PrendingPrerenderInfo", just to make the names a little more distinct. > > Done, but spelled right. I'm not going to sign off on this until you duplicate my typo, dangit. http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/38003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:86: if (!prerender_link_manager) On 2012/04/30 23:55:39, gavinp wrote: > On 2012/04/30 18:35:22, Matt Menke wrote: > > Can this happen? > > We return NULL if there's no prerender_manager in > PrerenderLinkManagerFactory::BuildServiceInstanceFor. And, there won't be a > PrerenderManagerFactory on a profile if > PrerenderManager::IsPrerenderingPossible() returns false. So I think this > happens if you're configured to not use prerendering. What if, while a > prerender is running, you turn the option off? Right. I'd forgotten that we use NULL when disabled. http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/38003/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:15: class PrerendererClient : public content::RenderViewObserver, On 2012/04/30 23:55:39, gavinp wrote: > On 2012/04/30 18:35:22, Matt Menke wrote: > > Do we really need one of these per render view, instead of one per process? > Not > > concerned about memory, but I don't like global variables when we don't need > > them. > > The implementation of PrerendererClient::willAddPrerender() gets the size of the > current RenderView for the request. That feels pretty per-RenderView to me, I > think? That should be "We don't maintain per-RenderView state". Presumably someone...or something... retrieved us via the RenderView. Anyways, not a big deal. http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/38003/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:6: function ExtractGetParameterBadlyAndInsecurely(param, default_value) { On 2012/04/30 23:55:39, gavinp wrote: > On 2012/04/30 18:35:22, Matt Menke wrote: > > nit: defaultValue. Don't ask my why Javascript naming style is different > from > > C++ style, but it is. Same goes for the rest of this file. > > Done. Matt, why does Javascript have a different naming style than C++? For the same reason I misspelled pending. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:102: gfx::Size size); Might want to make size a const. Should be optimized out regardless, but nice to have it consistent with AddPendingPrerender. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:105: gfx::Size size; nit: All these could be const. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:61: } Wonder if this block of code might not be happier living in AddPrerenderFromLinkRelPrerender. The fact it's not there has already been the cause of one bug already. Also, having this code here affects histograms, since we're no longer recording "FINAL_STATUS_SOURCE_RENDER_VIEW_CLOSED" http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:86: manager_->MaybeCancelPrerender(url); Another funky case (Again, just something to think about. This will probably be fixed by some of the planned changes already discussed, anyways): A site prerenders some url. The prerender times out. The site prerenders the url again (Or some other site does). We start prerendering again. The original prerender request is cancelled. We the cancel the prerender, even though the page is still being prerendered. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:93: // navigation away from launcher. You had a comment here before on why there's no check here for whether or not the prerender exists, like we do in OnCancelPrerender, and a TODO to add it later. I suggest you add back the comment, at least, and the TODO, if still applicable. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:305: // child process specified by |child__id|. The |origin| specifies how the nit: child_id. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:308: // cannot be found, we use a default from PrerenderCOnfig. nit: PrerenderConfig. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:308: // cannot be found, we use a default from PrerenderCOnfig. I suggest you mention that it's the PrerenderContents that does the extra size logic, so someone interested in that behavior (Or some untrusting person reviewing the accuracy of your comments) can find it more easily. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:30: } // end namespace nit: --end http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:102: BrowserThread::PostTask( nit: Fix indent. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:36: content::BrowserThread::ID* thread) OVERRIDE; Any reason why these functions are public? http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:48: virtual void OnChannelClosing() OVERRIDE; nit: Should have a comment that the OVERRIDE functions are implementing content::BrowserMessageFilter. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:640: TEST_F(PrerenderTest, LinkManagerCancel) { Suggest you add an abandon here as well. Same goes for LinkManagerAddTwiceCancelTwice. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:640: TEST_F(PrerenderTest, LinkManagerCancel) { nit: Could you add a couple linebreaks in some of these tests? I find it easy to lose my place in lines of uniformly indented, black-on-green, monospaced text. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:643: DummyPrerenderContents* prerender_contents = nit: Fix indent. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:647: EXPECT_TRUE(AddSimplePrerender(url)); nit: Suggest an "EXPECT_FALSE(IsEmptyPrerenderLinkManager());", just so we have a verification it works in the simplest LinkManager test. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:659: TEST_F(PrerenderTest, LinkManagerCancelTwice) { Does this normally happen? http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:747: // prerender manager, and so this is an add in the prerender manager sense. Comment is no longer correct. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:763: DummyPrerenderContents* first_prerender_contents = nit: Fix indent. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:785: } Suggest you add an abandon without cancellation test as well, not that it'll be terribly exciting. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/renderer_ho... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:33: #include "content/public/browser/render_widget_host_view.h" nit: Alphabetize. http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... File chrome/common/prerender_messages.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... chrome/common/prerender_messages.h:17: // Prerender Link Manager Messages nit: PrerenderLinkManager http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... chrome/common/prerender_messages.h:38: // Prerender View Host Messages What is a Prerender View Host? Generally (Like just above), messages are named for who they're sent to, not who they're sent from, and "Host" objects are generally in the browser process, but these are sent to the render process. http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... File chrome/renderer/prerender/prerender_extra_data.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... chrome/renderer/prerender/prerender_extra_data.h:32: gfx::Size size_; could make these const. http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:33: } nit: Use same indentation in both script tags. I don't know if one or the other is right, but currently it's inconsistent. http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:35: <a href='REPLACE_WITH_DESTINATION_URL'>Link To Click</a> HTML should use double quotes. Sorry, should have been clearer last time.
mmenke: are you still happy? http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:102: gfx::Size size); On 2012/05/01 16:23:21, Matt Menke wrote: > Might want to make size a const. Should be optimized out regardless, but nice > to have it consistent with AddPendingPrerender. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:105: gfx::Size size; On 2012/05/01 16:23:21, Matt Menke wrote: > nit: All these could be const. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:61: } On 2012/05/01 16:23:21, Matt Menke wrote: > Wonder if this block of code might not be happier living in > AddPrerenderFromLinkRelPrerender. The fact it's not there has already been the > cause of one bug already. Done. > > Also, having this code here affects histograms, since we're no longer recording > "FINAL_STATUS_SOURCE_RENDER_VIEW_CLOSED" Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:86: manager_->MaybeCancelPrerender(url); On 2012/05/01 16:23:21, Matt Menke wrote: > Another funky case (Again, just something to think about. This will probably be > fixed by some of the planned changes already discussed, anyways): > > A site prerenders some url. The prerender times out. The site prerenders the > url again (Or some other site does). We start prerendering again. The original > prerender request is cancelled. We the cancel the prerender, even though the > page is still being prerendered. Yes. That case should be caught by even the simplest observer interface. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:93: // navigation away from launcher. On 2012/05/01 16:23:21, Matt Menke wrote: > You had a comment here before on why there's no check here for whether or not > the prerender exists, like we do in OnCancelPrerender, and a TODO to add it > later. I suggest you add back the comment, at least, and the TODO, if still > applicable. That comment got mooted by dominich's earlier suggestion that we not create entries in the map where AddPrerenderFromLinkRelPrerender fails. Previously, the issue was that our (observer-free) WebKit implementation abandons all prerenders on navigation away. Now, though, some Prerenders don't even add, and WebKit has no idea. I'll add a single comment on this issue in front of OnCancelPrerender, since it's the first method affected by this. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:308: // cannot be found, we use a default from PrerenderCOnfig. On 2012/05/01 16:23:21, Matt Menke wrote: > nit: PrerenderConfig. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:308: // cannot be found, we use a default from PrerenderCOnfig. On 2012/05/01 16:23:21, Matt Menke wrote: > I suggest you mention that it's the PrerenderContents that does the extra size > logic, so someone interested in that behavior (Or some untrusting person > reviewing the accuracy of your comments) can find it more easily. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:30: } // end namespace On 2012/05/01 16:23:21, Matt Menke wrote: > nit: --end Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.cc:102: BrowserThread::PostTask( On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:36: content::BrowserThread::ID* thread) OVERRIDE; On 2012/05/01 16:23:21, Matt Menke wrote: > Any reason why these functions are public? None, so I fixed it. The convention is for them to be public, though. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:36: content::BrowserThread::ID* thread) OVERRIDE; On 2012/05/01 16:23:21, Matt Menke wrote: > Any reason why these functions are public? No, none. Fixed. The convention in Chrome is for them to be public, but that's clearly not required. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:48: virtual void OnChannelClosing() OVERRIDE; On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Should have a comment that the OVERRIDE functions are implementing > content::BrowserMessageFilter. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:640: TEST_F(PrerenderTest, LinkManagerCancel) { On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Could you add a couple linebreaks in some of these tests? I find it easy > to lose my place in lines of uniformly indented, black-on-green, monospaced > text. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:640: TEST_F(PrerenderTest, LinkManagerCancel) { On 2012/05/01 16:23:21, Matt Menke wrote: > Suggest you add an abandon here as well. Same goes for > LinkManagerAddTwiceCancelTwice. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:643: DummyPrerenderContents* prerender_contents = On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:647: EXPECT_TRUE(AddSimplePrerender(url)); On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Suggest an "EXPECT_FALSE(IsEmptyPrerenderLinkManager());", just so we have > a verification it works in the simplest LinkManager test. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:659: TEST_F(PrerenderTest, LinkManagerCancelTwice) { On 2012/05/01 16:23:21, Matt Menke wrote: > Does this normally happen? No, it should be impossible. This test was nice before because of the death, but that's gone now for the reasons we talked about earlier. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:747: // prerender manager, and so this is an add in the prerender manager sense. On 2012/05/01 16:23:21, Matt Menke wrote: > Comment is no longer correct. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:763: DummyPrerenderContents* first_prerender_contents = On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:785: } On 2012/05/01 16:23:21, Matt Menke wrote: > Suggest you add an abandon without cancellation test as well, not that it'll be > terribly exciting. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/renderer_ho... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/renderer_ho... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:33: #include "content/public/browser/render_widget_host_view.h" On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Alphabetize. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... File chrome/common/prerender_messages.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... chrome/common/prerender_messages.h:17: // Prerender Link Manager Messages On 2012/05/01 16:23:21, Matt Menke wrote: > nit: PrerenderLinkManager Done. http://codereview.chromium.org/10198040/diff/35007/chrome/common/prerender_me... chrome/common/prerender_messages.h:38: // Prerender View Host Messages On 2012/05/01 16:23:21, Matt Menke wrote: > What is a Prerender View Host? Generally (Like just above), messages are named > for who they're sent to, not who they're sent from, and "Host" objects are > generally in the browser process, but these are sent to the render process. Good point. Renamed to PrerenderDispatcher Messages. http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... File chrome/renderer/prerender/prerender_extra_data.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/renderer/prerender/... chrome/renderer/prerender/prerender_extra_data.h:32: gfx::Size size_; On 2012/05/01 16:23:21, Matt Menke wrote: > could make these const. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:33: } On 2012/05/01 16:23:21, Matt Menke wrote: > nit: Use same indentation in both script tags. I don't know if one or the > other is right, but currently it's inconsistent. Done. http://codereview.chromium.org/10198040/diff/35007/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:35: <a href='REPLACE_WITH_DESTINATION_URL'>Link To Click</a> On 2012/05/01 16:23:21, Matt Menke wrote: > HTML should use double quotes. Sorry, should have been clearer last time. Done.
lgtm fwiw http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:736: PrerenderLinkManager* GetPrerenderLinkManager() const { nit: It looks like this is only used for IsEmpty. As such, it could return a const PrerenderLinkManager*.
http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36035/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:736: PrerenderLinkManager* GetPrerenderLinkManager() const { On 2012/05/01 19:57:48, dominich wrote: > nit: It looks like this is only used for IsEmpty. As such, it could return a > const PrerenderLinkManager*. Good point. I implemented IsEmptyPrerenderLinkManager() in the browser_test, too, and that made the friend section a lot simpler.
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { Do you even need GetPrerenderLinkManager() now?
LGTM. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:93: // navigation away from launcher. On 2012/05/01 18:50:22, gavinp wrote: > On 2012/05/01 16:23:21, Matt Menke wrote: > > You had a comment here before on why there's no check here for whether or not > > the prerender exists, like we do in OnCancelPrerender, and a TODO to add it > > later. I suggest you add back the comment, at least, and the TODO, if still > > applicable. > > That comment got mooted by dominich's earlier suggestion that we not create > entries in the map where AddPrerenderFromLinkRelPrerender fails. Previously, > the issue was that our (observer-free) WebKit implementation abandons all > prerenders on navigation away. Now, though, some Prerenders don't even add, and > WebKit has no idea. > > I'll add a single comment on this issue in front of OnCancelPrerender, since > it's the first method affected by this. Oh, right. I saw the DVLOG for that case in OnCancelPrerender, and assumed it was a DCHECK. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_message_filter.h (right): http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... chrome/browser/prerender/prerender_message_filter.h:36: content::BrowserThread::ID* thread) OVERRIDE; On 2012/05/01 18:50:22, gavinp wrote: > On 2012/05/01 16:23:21, Matt Menke wrote: > > Any reason why these functions are public? > > No, none. Fixed. > > The convention in Chrome is for them to be public, but that's clearly not > required. Indeed. I wouldn't have commented, if OnChannelClosing wasn't private. I'm happy either way, as lon as they're together. http://codereview.chromium.org/10198040/diff/36036/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/36036/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:33: } Oh...You were asking me about that comment. I had thought you were asking me about a comment about the indentation of the document.write line (Which I apparently didn't actually make). The actual complaint was that in between these <script> tags, you indent the Javascript. Between the <script> tags above, you don't indent the Javascript. Should be consistent.
haven't looked through the entire change again, just the reply to my nits, so lgtm based on that (really deferring to other reviewers here)
http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_link_manager_factory.cc (right): http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_link_manager_factory.cc:37: if (!prerender_manager) On 2012/04/27 23:31:45, gavinp wrote: > On 2012/04/27 21:52:01, dominich wrote: > > On 2012/04/27 20:31:32, gavinp wrote: > > > On 2012/04/26 22:50:58, dominich wrote: > > > > || !prerender_manager.IsEnabled() ? > > > > > > Isn't that redundant with the check on > > PrerenderManager::IsPrerenderingPossible > > > from the PrerenderManagerFactory::BuildServiceInstanceFor ? > > > > > > If it isn't redundant to it, should we fix > > > PrerenderManagerFactory::BuildServiceInstanceFor instead? > > > > You're probably right, but I recently was burned by not checking for IsEnabled > > when Omnibox Prerendering so I'm cautious. Feel free to do the work of > checking > > my assertion and responding accordingly :) > > Good point. I'll follow up. I centralized the IsEnabled check in PrerenderManager, rather than callers. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:294: it->contents_->AddPendingPrerender(url, referrer, size); This will add a pending prerender even if the PrerenderManager is not enabled - unlikely, but could happen when a prerender is created, the user swaps the setting for whether it is allowed, and then a <link rel="prerender"> is created inside it. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:356: void PrerenderManager::CancelOmniboxPrerenders() { Does this need to change so the omnibox prerendering uses the MaybeCancel instead?
On 2012/05/02 15:54:21, cbentzel wrote: > http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... > File chrome/browser/prerender/prerender_link_manager_factory.cc (right): > > http://codereview.chromium.org/10198040/diff/6001/chrome/browser/prerender/pr... > chrome/browser/prerender/prerender_link_manager_factory.cc:37: if > (!prerender_manager) > On 2012/04/27 23:31:45, gavinp wrote: > > On 2012/04/27 21:52:01, dominich wrote: > > > On 2012/04/27 20:31:32, gavinp wrote: > > > > On 2012/04/26 22:50:58, dominich wrote: > > > > > || !prerender_manager.IsEnabled() ? > > > > > > > > Isn't that redundant with the check on > > > PrerenderManager::IsPrerenderingPossible > > > > from the PrerenderManagerFactory::BuildServiceInstanceFor ? > > > > > > > > If it isn't redundant to it, should we fix > > > > PrerenderManagerFactory::BuildServiceInstanceFor instead? > > > > > > You're probably right, but I recently was burned by not checking for > IsEnabled > > > when Omnibox Prerendering so I'm cautious. Feel free to do the work of > > checking > > > my assertion and responding accordingly :) > > > > Good point. I'll follow up. > > I centralized the IsEnabled check in PrerenderManager, rather than callers. > > http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:294: > it->contents_->AddPendingPrerender(url, referrer, size); > This will add a pending prerender even if the PrerenderManager is not enabled - > unlikely, but could happen when a prerender is created, the user swaps the > setting for whether it is allowed, and then a <link rel="prerender"> is created > inside it. > > http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:356: void > PrerenderManager::CancelOmniboxPrerenders() { > Does this need to change so the omnibox prerendering uses the MaybeCancel > instead? There was a comment block about this on one of my reviews. It _could_ but I'm a little nervous about making sure the active_count_ code works for this case. Currently we see many DUPLICATE requests for the same prerender from Omnibox as the user types. This would increment the active_count_ each time. When we then navigate to a different URL and need to cancel the prerender, we'd have to call cancel the same number of times as we added duplicates or force cancelation (which is what CancelOmniboxPrerenders does) or skip adding to the active_count_ for Omnibox prerenders. It's not clear which of those is the best solution.
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:294: it->contents_->AddPendingPrerender(url, referrer, size); On 2012/05/02 15:54:22, cbentzel wrote: > This will add a pending prerender even if the PrerenderManager is not enabled - > unlikely, but could happen when a prerender is created, the user swaps the > setting for whether it is allowed, and then a <link rel="prerender"> is created > inside it. Yes. The funny behaviour from this is: 1. User has prerendering on, and goes to a page that launches PrerenderPage 2. The PrerenderPage prerenders FirstPendingPrerender! 2. The user turns off prerendering. 3. The PrerenderPage prerenders SecondPendingPrerender, which gets put in the pending list. 4. The user then goes and turns prerendering back on. 5. The user navigates to PrerenderPage, and then the pending prerenders are added, which includes both FirstPendingPrerender and SecondPendingPrerender. If the user didn't turn prerendering on in step (4) above, then we won't launch the pending prerenders because of the IsEnabled check in AddPrerender. The behaviour previously would have been to launch only FirstPendingPrerender, right? http://codereview.chromium.org/10198040/diff/36036/chrome/test/data/prerender... File chrome/test/data/prerender/prerender_loader_removing_links.html (right): http://codereview.chromium.org/10198040/diff/36036/chrome/test/data/prerender... chrome/test/data/prerender/prerender_loader_removing_links.html:33: } On 2012/05/02 15:02:52, Matt Menke wrote: > Oh...You were asking me about that comment. I had thought you were asking me > about a comment about the indentation of the document.write line (Which I > apparently didn't actually make). > > The actual complaint was that in between these <script> tags, you indent the > Javascript. Between the <script> tags above, you don't indent the Javascript. > Should be consistent. AHA! Ok, I'll fix this.
Mostly nits, but please look at the IMPORTANT one http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { Why do you need to call IsEmpty() here? Isn't the behavior of prerenders being swapped in and their final status good enough to validate that link element removal works correctly? http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:775: void set_loader_query_and_fragment(const std::string& query_and_fragment) { Nit: I don't like having separate set_loader_path and set_loader_query_and_fragment, but I won't ask you to change in this CL. Really, it would be nice if the TestServer methods took in a GURL which is a relative URL, and then call GURL::Resolve to generate the final URL. But that should be done in a separate CL. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:288: DCHECK_EQ(1U, alias_urls_.size()); Nit: may as well move these near all of the other preconditions at the top of the method. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:296: if (size_.IsEmpty()) { Nit: Should this be done closer to the size_ = size assignment? http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:307: BrowserList::GetLastActiveWithProfile(profile_)) { Nit: not sure what this buys other than slightly tighter scoping on active_browser. But not a big deal. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:313: size_ = container_bounds.size(); IMPORTANT: This is a change in behavior from before. We would set size_ to the default tab bounds, and only override size_ if there was an active_web_contents. Now it always overrides size_ regardless of active_web_contents. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:43: DVLOG(3) << "... render_view_route_id = " << render_view_route_id May as well DVLOG(3) the size. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:69: friend class PrerenderBrowserTest; As I commented in prerender_browser_test.cc, I don't think you need to make it a friend here. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:294: it->contents_->AddPendingPrerender(url, referrer, size); On 2012/05/02 16:23:42, gavinp wrote: > On 2012/05/02 15:54:22, cbentzel wrote: > > This will add a pending prerender even if the PrerenderManager is not enabled > - > > unlikely, but could happen when a prerender is created, the user swaps the > > setting for whether it is allowed, and then a <link rel="prerender"> is > created > > inside it. > > Yes. The funny behaviour from this is: > > 1. User has prerendering on, and goes to a page that launches PrerenderPage > 2. The PrerenderPage prerenders FirstPendingPrerender! > 2. The user turns off prerendering. > 3. The PrerenderPage prerenders SecondPendingPrerender, which gets put in the > pending list. > 4. The user then goes and turns prerendering back on. > 5. The user navigates to PrerenderPage, and then the pending prerenders are > added, which includes both FirstPendingPrerender and SecondPendingPrerender. > > If the user didn't turn prerendering on in step (4) above, then we won't launch > the pending prerenders because of the IsEnabled check in AddPrerender. The > behaviour previously would have been to launch only FirstPendingPrerender, > right? Correct. http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... File chrome/common/prerender_messages.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:23: IPC_MESSAGE_CONTROL5(PrerenderHostMsg_AddPrerender, Should each of these be AddLinkRelPrerender or something similar? http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:24: int /* id, assigned by WebPrerendererClient */, nit: prerender_id would be clearer than this description. http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:49: IPC_MESSAGE_CONTROL1(PrerenderMsg_AddPrerenderURL, Not for this CL, but we may want to do more name distinguishing between this and PrerenderHostMsg_AddPrerender. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.cc:11: namespace { Nit: you could probably write this as namespace { static int s_last_prerender_id = 0; } instead of lots of vertical space. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.cc:35: render_view()->GetSize())); Why is render_view() always guaranteed to be non-NULL? Is it due to the NULL check in the PrerenderClient constructor? http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:19: virtual ~PrerendererClient(); Should this be private, since it should only be called by the RenderViewObserver base class? http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:21: virtual void willAddPrerender(WebKit::WebPrerender* prerender) OVERRIDE; Nit: add comment specifying which interface this is from.
cbentzel, what do you think of these fixes/comments? http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { On 2012/05/04 15:32:25, cbentzel wrote: > Why do you need to call IsEmpty() here? Isn't the behavior of prerenders being > swapped in and their final status good enough to validate that link element > removal works correctly? This feature was requested by mmenke. The idea is to not just test that link removal worked, but to test that the link manager can return to a well defined state (empty). FWIW, it's already helped me catch errors during the refactoring that came up in this review. http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... mmenke: WDYT? http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:288: DCHECK_EQ(1U, alias_urls_.size()); On 2012/05/04 15:32:25, cbentzel wrote: > Nit: may as well move these near all of the other preconditions at the top of > the method. Done. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:296: if (size_.IsEmpty()) { On 2012/05/04 15:32:25, cbentzel wrote: > Nit: Should this be done closer to the size_ = size assignment? Done. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:307: BrowserList::GetLastActiveWithProfile(profile_)) { On 2012/05/04 15:32:25, cbentzel wrote: > Nit: not sure what this buys other than slightly tighter scoping on > active_browser. But not a big deal. This practice is specifically recommended in our coding style: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... It felt appropriate to do while I was already editing the the block it was in. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:313: size_ = container_bounds.size(); On 2012/05/04 15:32:25, cbentzel wrote: > IMPORTANT: This is a change in behavior from before. We would set size_ to the > default tab bounds, and only override size_ if there was an active_web_contents. > Now it always overrides size_ regardless of active_web_contents. Good catch. Fixed. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:43: DVLOG(3) << "... render_view_route_id = " << render_view_route_id On 2012/05/04 15:32:25, cbentzel wrote: > May as well DVLOG(3) the size. Done. http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:69: friend class PrerenderBrowserTest; On 2012/05/04 15:32:25, cbentzel wrote: > As I commented in prerender_browser_test.cc, I don't think you need to make it a > friend here. If you and mmenke can come to consensus, I'll make the change that you both agree on. http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... File chrome/common/prerender_messages.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:23: IPC_MESSAGE_CONTROL5(PrerenderHostMsg_AddPrerender, On 2012/05/04 15:32:25, cbentzel wrote: > Should each of these be AddLinkRelPrerender or something similar? Done. http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:24: int /* id, assigned by WebPrerendererClient */, On 2012/05/04 15:32:25, cbentzel wrote: > nit: prerender_id would be clearer than this description. Done. http://codereview.chromium.org/10198040/diff/36036/chrome/common/prerender_me... chrome/common/prerender_messages.h:49: IPC_MESSAGE_CONTROL1(PrerenderMsg_AddPrerenderURL, On 2012/05/04 15:32:25, cbentzel wrote: > Not for this CL, but we may want to do more name distinguishing between this and > PrerenderHostMsg_AddPrerender. Yes. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.cc:11: namespace { On 2012/05/04 15:32:25, cbentzel wrote: > Nit: you could probably write this as > > namespace { > static int s_last_prerender_id = 0; > } > > instead of lots of vertical space. Done. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.cc:35: render_view()->GetSize())); On 2012/05/04 15:32:25, cbentzel wrote: > Why is render_view() always guaranteed to be non-NULL? Is it due to the NULL > check in the PrerenderClient constructor? It's guaranteed to be non-null because the only caller is in WebKit, and the only way WebKit can find it is through the set call above. But, even more to the point, there are no unit tests with a PrerendererClient in them, anyway, so that comment (which was copied from another constructor of a filter) is misleading. I've removed that check on the render_view in the constructor, and replaced it with a DCHECK. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... File chrome/renderer/prerender/prerenderer_client.h (right): http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:19: virtual ~PrerendererClient(); On 2012/05/04 15:32:25, cbentzel wrote: > Should this be private, since it should only be called by the RenderViewObserver > base class? Done. Also moved willAddPrerender() down as well. http://codereview.chromium.org/10198040/diff/36036/chrome/renderer/prerender/... chrome/renderer/prerender/prerenderer_client.h:21: virtual void willAddPrerender(WebKit::WebPrerender* prerender) OVERRIDE; On 2012/05/04 15:32:25, cbentzel wrote: > Nit: add comment specifying which interface this is from. Done.
http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10198040/diff/36036/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool IsEmptyPrerenderLinkManager() const { On 2012/05/04 23:10:51, gavinp wrote: > On 2012/05/04 15:32:25, cbentzel wrote: > > Why do you need to call IsEmpty() here? Isn't the behavior of prerenders being > > swapped in and their final status good enough to validate that link element > > removal works correctly? > > This feature was requested by mmenke. The idea is to not just test that link > removal worked, but to test that the link manager can return to a well defined > state (empty). FWIW, it's already helped me catch errors during the refactoring > that came up in this review. > http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/p... > > mmenke: WDYT? I still think it's a useful test to have - it's entirely possible that PrerenderLinkManager might, as a result of a bug, not remove an entry in |ids_to_url_map_| for some reason. While the unit_tests should ideally catch such a case, I think there's some added value in having it also checked in full scale integration tests.
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> > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > http://codereview.chromium.**org/10198040/diff/36036/** > chrome/browser/prerender/**prerender_browsertest.cc#**newcode744<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: > >> On 2012/05/04 15:32:25, cbentzel wrote: >> > Why do you need to call IsEmpty() here? Isn't the behavior of >> > prerenders being > >> > swapped in and their final status good enough to validate that link >> > element > >> > removal works correctly? >> > > This feature was requested by mmenke. The idea is to not just test >> > that link > >> removal worked, but to test that the link manager can return to a well >> > defined > >> state (empty). FWIW, it's already helped me catch errors during the >> > refactoring > >> that came up in this review. >> > > http://codereview.chromium.**org/10198040/diff/35007/** > chrome/browser/prerender/**prerender_browsertest.cc#**newcode977<http://codereview.chromium.org/10198040/diff/35007/chrome/browser/prerender/prerender_browsertest.cc#newcode977> > > mmenke: WDYT? >> > > I still think it's a useful test to have - it's entirely possible that > PrerenderLinkManager might, as a result of a bug, not remove an entry in > |ids_to_url_map_| for some reason. While the unit_tests should ideally > catch such a case, I think there's some added value in having it also > checked in full scale integration tests. > OK, then seems reasonable. > > http://codereview.chromium.**org/10198040/<http://codereview.chromium.org/101... >
LGTM
On 2012/05/05 00:16:50, cbentzel wrote: > LGTM Thank you everyone for your great reviews; I appreciate the time you've all taken to help get this up to top quality. Given the testing needs for the new API, I'm going to hold off on landing this until next week.
On 2012/05/05 13:51:22, gavinp wrote: > On 2012/05/05 00:16:50, cbentzel wrote: > > LGTM > > Thank you everyone for your great reviews; I appreciate the time you've all > taken to help get this up to top quality. > > Given the testing needs for the new API, I'm going to hold off on landing this > until next week. Branchpoint is Tuesday morning, so may want to hold off until after that
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/60002
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 Hunk #1 FAILED at 34. Hunk #2 succeeded at 57 (offset 4 lines). Hunk #3 succeeded at 68 (offset 4 lines). 1 out of 3 hunks FAILED -- saving rejects to file chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/70002
Presubmit check for 10198040-70002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/browser/prerender/prerender_manager.h, line 284, 90 chars Presubmit checks took 3.7s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10198040/60005
Change committed as 136611 |