Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10553029:
Handle interface to prerenders. (Closed)
|
Created:
8 years, 6 months ago by gavinp Modified:
8 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, erikwright (departed), dominich+watch_chromium.org, brettw-cc_chromium.org, James Su, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle interface to prerenders.
The prerender_manager now returns a PrerenderHandle* when creating a prerender; this is a useful object for canceling the prerender, as well as signaling navigation, etc...
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146740
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146743
Patch Set 1 #
Total comments: 47
Patch Set 2 : more standard code, simpler handle, no class explosion #
Total comments: 38
Patch Set 3 : remediated, and cleaned up in prep for uploading for review #
Total comments: 60
Patch Set 4 : remediation to reviews, and unit test improvements. #
Total comments: 111
Patch Set 5 : Remediate to review #
Total comments: 22
Patch Set 6 : moar dominich remediation #Patch Set 7 : a previous version of VSS wasn't as good as the current one. #
Total comments: 2
Patch Set 8 : latest remediation.... #Patch Set 9 : merge to trunk of previous upload; so diffs between branches can work better #Patch Set 10 : some remediation, and deflaking browser tests. #
Total comments: 105
Patch Set 11 : remediation to mmenke reviews #Patch Set 12 : lose two friends #
Total comments: 16
Patch Set 13 : remediate to latest mmenke reviews #Patch Set 14 : lose some DVLOGs #
Total comments: 2
Patch Set 15 : Lose a leak #Patch Set 16 : unconfusify ownership of pending prerenders #
Total comments: 41
Patch Set 17 : remediate to mmenke + dominich review #
Total comments: 17
Patch Set 18 : remediate to final reviews! #
Total comments: 7
Patch Set 19 : pkasting remediation #Patch Set 20 : remove local declaration #
Total comments: 3
Patch Set 21 : fix a build and a unit test leak #
Total comments: 5
Messages
Total messages: 63 (0 generated)
http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_handle.h:17: class PrerenderHandleInterface : I've now renamed this to PrerenderInterface, and put it in prerender_interface.h. There's still a PrerenderHandle, and it gets rid of the really confusing logic around Destroy().
I think you have too many things going on here, and you've over-engineered quite a bit of this. Take a step back and simplify: A PrerenderHandle was meant to be a way for users of the public API to refer to a prerender that combines a URL, SessionStorage, etc without them caring about the details. That's all it needs to be. What you have here is a very confusing concept that is exposed by the public API but never used by it. I don't understand the difference and overlap between the PrerenderContents, PrerenderContentsData, and PrerenderHandle. There are also a number of code smells being introduced by this change that suggest the design is not ideal for the problem you're trying to solve. It's possible I'm misunderstanding what you're trying to do, and having a discussion about that will be most useful. http://codereview.chromium.org/10553029/diff/1/base/memory/ref_counted.h File base/memory/ref_counted.h (right): http://codereview.chromium.org/10553029/diff/1/base/memory/ref_counted.h#newc... base/memory/ref_counted.h:34: public: no. http://codereview.chromium.org/10553029/diff/1/base/memory/weak_ptr_unittest.cc File base/memory/weak_ptr_unittest.cc (right): http://codereview.chromium.org/10553029/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:356: struct WithRefCounting : RefCounted<WithRefCounting>, I don't understand this. The WeakPtr is already refcounted and is really only useful if refcounting is not a good fit. Once you are RefCounted, I don't understand what supporting WeakPtr gives you. http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_edit.h:443: void DoPrerender(predictors::AutocompleteActionPredictor* action_predictor, i'd prefer this wasn't passed down. I know it saves a lookup from the profile but that isn't that expensive, and this adds a (weak) dependency on the predictor namespace to the header that is unnecessary. http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_match.h:279: mutable prerender::WeakPrerenderHandle prerender; I thought we discussed this at length and decided that adding the prerender to the AutocompleteMatch was a bad idea. The handle should be stored by the AutocompletePredictor when it starts the prerender, not by the autocomplete system. At least, that's how I expect it to be. If you have good reason to change that approach please let me know. The requirement for mutable also suggests to me that this is not the best place for it. http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... chrome/browser/predictors/autocomplete_action_predictor.cc:325: if (prerender_ != match.prerender) This should not be pulled from the match. The prerender manager or handle should be able to determine equivalence from the available information (URL/Session Storage) that is already part of the match without adding this. Please check with pkasting what he thinks about extending AutocompleteMatch this way, but my gut is that it's not the right way to do it. http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... chrome/browser/predictors/autocomplete_action_predictor.cc:326: prerender_ = prerender::PrerenderHandle(); if this has a side-effect of canceling a prerender then I don't like it at all. It is completely non-obvious what this does, while an explicit call to prerender_manager->CancelPrerender(prerender_) would be much clearer. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.cc:47: class PendingPrerender : public PrerenderHandleInterface { I think you're conflating two things here. There's no reason for a PendingPrerender to be related to a PrerenderHandle. They're related but not the same thing and I don't see what this adds that wasn't already captured by the PendingPrerenderInfo struct and list. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.cc:264: it->prerender->set_prerender( Why don't we clear the list of pending prerenders after starting them? Is that a bug? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.h:99: bool Init(WeakPrerenderHandle prerender); PrerenderHandle* or const PrerenderHandle& or base::WeakPtr<PrerenderHandle>. I don't know what a WeakPrerenderHandle is and explicit typing makes me happier. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.h:219: virtual PrerenderHandle AddPendingPrerender(const GURL& url, so this will copy the PrerenderHandle on return? That is surprising. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_handle.h:17: class PrerenderHandleInterface : On 2012/06/17 17:41:15, gavinp wrote: > I've now renamed this to PrerenderInterface, and put it in > prerender_interface.h. > > There's still a PrerenderHandle, and it gets rid of the really confusing logic > around Destroy(). I /really/ don't understand why you need an interface for a PrerenderHandle. There should only be one type of PrerenderHandle and it should be concrete and non-extensible. Maybe even POD. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_link_manager.cc:53: PrerenderHandle prerender = manager_->AddPrerenderFromLinkRelPrerender( the addition of this doesn't seem to have changed anything in the rest of this file. is it mean to have cleaned something up? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_link_manager.cc:55: if (!prerender) i'm not comfortable with this implicit cast to boolean. if !prerender.is_valid() is clearer. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:177: public PrerenderHandleInterface { I don't understand why this has a PrerenderHandle interface. Either it /is/ a PrerenderHandle, or it contains a PrerenderHandle, or it is related to a PrerenderHandle. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:191: class MatchPredicate; I smell over-engineering. These were already handled in a very simple to understand way in the code. Adding these seems unrelated to PrerenderHandle and, to me, adds complexity to the code that is unnecessary. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:279: start_time_(manager_->GetCurrentTime()), why is the manager the arbiter of what the time is? shouldn't this be base::TimeTicks::Now() ? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:280: expiry_time_(start_time_ + if this can be inferred from the start_time_ there's no need for it to be a member. Or, if this might be mutable as it can be extended, there's no need to keep the start_time_. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:311: return static_cast<PrerenderContentsData*>(interface); Code smell. If you need a static method to cast from the base class to the derived class you're doing something wrong. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:375: return contents_data->contents()->AddPendingPrerender(url, referrer, size); Ah, now I see why you need this to return a valid PrerenderHandle. I still think conflating the pending prerender info with the prerender handle is a bad idea and this can all be simplified by making the PrerenderHandle a much more simple class. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:612: if (contents_data->contents()->match_complete_status() == If you cache contents_data->contents() in a const PrerenderContentsData* const local var, the changes in this file will be minimized and you'll help the compiler make some decisions about how to render this code. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:934: if ((*it)->contents()->Matches(url, NULL)) nit: if (*it && (*it)->contents() && (*it)->contents()->Matches(url, NULL)) http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1036: PrerenderContentsData* data; PrerenderContentsData* data = new PrerenderContentsData(this, prerender_contents); PrerenderHandle handle(data); http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1129: PrerenderContentsData::ExpiredPredicate expired_predicate(GetCurrentTime()); this code is longer and more complicated than the original. I suggest it's not worth the change. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1178: DCHECK_GE(1, std::count_if(prerender_list_.begin(), prerender_list_.end(), pointless test - find_if would return prerender_list_.end() if it doesn't so just DCHECK on that instead. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1195: return const_cast<PrerenderManager::PrerenderContentsData*>( Code smell: const_cast? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1205: if (!*it) continue; nit: int contents_child_id, contents_route_id; if (*it && (*it)->contents()->GetChildId(&contents_child_id) && child_id == contents_child_id (*it)->contents()->GetRouteId(&contents_route_id) && route_id == contents_route_id) { return *it; } http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1299: data->contents()->Destroy(final_status); We shouldn't have a valid contents if we haven't started prerendering... http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_manager.h (left): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.h:129: void MaybeCancelPrerender(const GURL& url); explicit cancellation is the bomb. Why would you remove it? If nothing in this class takes a PrerenderHandle as a parameter, then why do you return one from the public interface? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.h:253: const PrerenderContents* FindPrerender(const GURL& url) const; Should this still be needed? I thought the point was to get rid of interfaces like this. If it is, it should take a SessionStorageNamespace at least. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.h:303: typedef std::list<base::WeakPtr<PrerenderContentsData> > Shouldn't this now be a list of PrerenderHandles from which the PrerenderContentsData can be determined?
Thanks for going through this Dominic. You're right. In getting this approach to work, I did have a class explosion, and I am now trying to minimize it. Let's see how much closer my next upload comes to what you're thinking. http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_edit.h:443: void DoPrerender(predictors::AutocompleteActionPredictor* action_predictor, On 2012/06/18 15:32:44, dominich wrote: > i'd prefer this wasn't passed down. I know it saves a lookup from the profile > but that isn't that expensive, and this adds a (weak) dependency on the > predictor namespace to the header that is unnecessary. Done. http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... chrome/browser/predictors/autocomplete_action_predictor.cc:325: if (prerender_ != match.prerender) On 2012/06/18 15:32:44, dominich wrote: > This should not be pulled from the match. The prerender manager or handle should > be able to determine equivalence from the available information (URL/Session > Storage) that is already part of the match without adding this. > > Please check with pkasting what he thinks about extending AutocompleteMatch this > way, but my gut is that it's not the right way to do it. We did talk about this at length, and I'm aware we differ here. PrerenderManager shouldn't have a way to look through the list of running prerenders and get one back besides "AddPrerender". Because omnibox prerendering is the only source here, we could just do away with the weakptr on the match, and look only at prerender_, or even keep a list/set of all launched prerenders (removing elements through an observer when they destroy). I'll remove the element from the match in my next upload. Or... have it both ways: should it call AddPrerender here? http://codereview.chromium.org/10553029/diff/1/chrome/browser/predictors/auto... chrome/browser/predictors/autocomplete_action_predictor.cc:326: prerender_ = prerender::PrerenderHandle(); On 2012/06/18 15:32:44, dominich wrote: > if this has a side-effect of canceling a prerender then I don't like it at all. > It is completely non-obvious what this does, while an explicit call to > prerender_manager->CancelPrerender(prerender_) would be much clearer. And a runtime error if you try to drop a ref to a prerender that is running? http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.cc:47: class PendingPrerender : public PrerenderHandleInterface { This class is gone in the next upload. The question is: if AddPrerenderFromLinkRelPrerender is always going to returna PrerenderHandle, how's that go? This was one approach; but a better one is to return a real one that just doesn't have its prerendercontents yet. That's where I'm leaning now. On 2012/06/18 15:32:44, dominich wrote: > I think you're conflating two things here. There's no reason for a > PendingPrerender to be related to a PrerenderHandle. They're related but not the > same thing and I don't see what this adds that wasn't already captured by the > PendingPrerenderInfo struct and list. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.cc:264: it->prerender->set_prerender( On 2012/06/18 15:32:44, dominich wrote: > Why don't we clear the list of pending prerenders after starting them? Is that a > bug? Also gone in the new approach. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_contents.h:219: virtual PrerenderHandle AddPendingPrerender(const GURL& url, On 2012/06/18 15:32:44, dominich wrote: > so this will copy the PrerenderHandle on return? That is surprising. The PrerenderHandle is a machine word in size. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_handle.h:17: class PrerenderHandleInterface : On 2012/06/18 15:32:44, dominich wrote: > On 2012/06/17 17:41:15, gavinp wrote: > > I've now renamed this to PrerenderInterface, and put it in > > prerender_interface.h. > > > > There's still a PrerenderHandle, and it gets rid of the really confusing logic > > around Destroy(). > > I /really/ don't understand why you need an interface for a PrerenderHandle. > There should only be one type of PrerenderHandle and it should be concrete and > non-extensible. Maybe even POD. Hrm. 1. A prerender can be canceled by any one of the holders of a handle (do we support multiple handles to the same prerender?), invalidating all other handles. 2. The prerender manager can cancel a prerender at any time, invalidating all handles. 3. Handles should be small and PODlike, to fit in an STL container. The abstract interface seemed like a good way to expose the interface to a prerender outside of the prerender_manager. As well, moving methods like IsPending() up here get rid of loops over the PrerenderManager::contents_data_list_. The handle seemed like a good way to let consumers grab a pointer to the running prerender, but had to be possible to invalidate it from there, and that should invalidate the other handles. So if we moved the PrerenderInterface methods out of PrerenderManager, that'd be a huge win: the PrerenderManager is pretty complicated as it is. So I'll continue with a CL that has a PrerenderInterface and a PrerenderHandle, but I want to talk about this more once I've done that upload. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_link_manager.cc:53: PrerenderHandle prerender = manager_->AddPrerenderFromLinkRelPrerender( On 2012/06/18 15:32:44, dominich wrote: > the addition of this doesn't seem to have changed anything in the rest of this > file. is it mean to have cleaned something up? No. It's meant to prepare prerendermanager to have multiple prerenders. sa well, the observer interface on prerender hadnles (in the next upload) will be how events go to the elements in webkit, so we get ready for that too. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_link_manager.cc:55: if (!prerender) On 2012/06/18 15:32:44, dominich wrote: > i'm not comfortable with this implicit cast to boolean. if !prerender.is_valid() > is clearer. Already fixed. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:177: public PrerenderHandleInterface { On 2012/06/18 15:32:44, dominich wrote: > I don't understand why this has a PrerenderHandle interface. Either it /is/ a > PrerenderHandle, or it contains a PrerenderHandle, or it is related to a > PrerenderHandle. Every other instance of Handle in our code base is as a reference, ala a weakptr or scoped ptr to an object. I think this is a PrerenderInterface, or maybe a Prerender itself... But it isn't a Handle if it isn't indirect. I suppose maybe the name Handle is bad, then. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:191: class MatchPredicate; Yes, I hate these and plan to remove them. On 2012/06/18 15:32:44, dominich wrote: > I smell over-engineering. These were already handled in a very simple to > understand way in the code. Adding these seems unrelated to PrerenderHandle and, > to me, adds complexity to the code that is unnecessary. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:612: if (contents_data->contents()->match_complete_status() == On 2012/06/18 15:32:44, dominich wrote: > If you cache contents_data->contents() in a const PrerenderContentsData* const > local var, the changes in this file will be minimized and you'll help the > compiler make some decisions about how to render this code. Yes on the minimized changes, but probably not so helpful to the compiler. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.cc:1129: PrerenderContentsData::ExpiredPredicate expired_predicate(GetCurrentTime()); On 2012/06/18 15:32:44, dominich wrote: > this code is longer and more complicated than the original. I suggest it's not > worth the change. Yes. I hate the predicates too. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_manager.h:303: typedef std::list<base::WeakPtr<PrerenderContentsData> > On 2012/06/18 15:32:44, dominich wrote: > Shouldn't this now be a list of PrerenderHandles from which the > PrerenderContentsData can be determined? I was really hoping to avoid a lot of "IsCanceled" logic. So that's why the WeakPtrs. But now I'm thinking an Observer interface with OnDestroy is the way to go; Observers can do things like take themselves out of this list. So yes, explicit cancellation, and sure, PrerenderHandles here, but they will remove themselves from the list when they get OnDestroy
Just two quick comments. I'm reluctant to spend too much time reviewing this until you've uploaded a new version, since you've said you're significantly revamping it, and I'm not the fastest reviewer. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_handle.h:17: class PrerenderHandleInterface : Seems like there are a lot of ways to implement this class. Since this is a non-POD class, you also have to DISALLOW_COPY_AND_ASSIGN. What I'd pictured was something more like: class PrerenderHandle { public: PrerenderHandle(PrerenderContents* prerender_contents); // No methods needed, most likely, though maybe an // bool IsPending() function would be useful. private: // Maybe friend PrerenderManager and add a private // function to access this. Not ideal, but think it's // better than ref counting. weak_ptr<PrerenderContents> prerender_contents_; // Copy and assign are allowed. }; Alternatively, you could just make it support weak pointer. I don't see why you need both. http://codereview.chromium.org/10553029/diff/1/chrome/browser/prerender/prere... chrome/browser/prerender/prerender_handle.h:43: typedef base::WeakPtr<PrerenderHandleInterface> WeakPrerenderHandle; This just makes things hard to follow, and is actually a bit of an anti-pattern, I believe.
This is the new way I'm going. I haven't fixed the omnibox yet, and most of the unit tests don't run because the system for time isn't quite right. However, WDYT of this approach overall?
Read the individual comments first, and then come back for my summary of thoughts: Having been through this, I like the general approach but the web of different smart pointer semantics leaves me feeling as if something is not quite right. I'm tempted to suggest that the PrerenderHandle should be just refcounted. As much as it's frowned upon, it actually makes sense for this type. Also, I think a simple API tweak to typedef the base::WeakPtr<PrerenderHandle> (or scoped_refptr<PrerenderHandle> I suppose) to PrerenderHandle, ie the type returned by the public API, will make everything friendlier. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:318: // prerender_manager->CancelOmniboxPrerenders(); will this come back in some form? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:232: virtual void AddPendingPrerender(scoped_ptr<const GURL& url, i'm pretty sure this doesn't compile ;) http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:99: linked_ptr<PrerenderHandle> prerender_handle; You don't see those very often. I would have expected a scoped_refptr I think, or at least for the PrerenderHandle to be refcounted and passed raw. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:215: make_linked_ptr(prerender_handle.release()), what is this voodoo? So you have a prerender handle passed as a scoped ptr, which has an explicit lifetime, then you release that and make it into a linked ptr which is ref-counted. Why not pass in the prerender handle directly and have it be refcounted internally instead of relying on this external code (to PrerenderHandle) to do the refcounting? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:221: for (PendingPrerenderList::const_iterator it = what no std::count_if? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:276: dummy->client_count_ = client_count_; do we not need to copy the child_id, route_id, final_status_, etc? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:577: void PrerenderContents::DecrementClientCount() { DCHECK_GT(0, client_count_); http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:594: std::bind2nd(std::equal_to<GURL>(), url))); This will break fragment mismatches. There's a very specific reason to the prior matching only working on a subset of URL features. Also, it's unreadable, and an explicit ' > 0' would be better than an implicit_cast. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:71: class LessThanByLoadStartTime { public? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:168: int client_count() const { return client_count_; } naming nit: I don't know what a client is or what this count is counting. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:175: // URL, i.e. whether there is a match. |matching_url| is optional and will be comment needs updating http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:237: virtual void AddPendingPrerender(scoped_ptr<PrerenderHandle> prerender_handle, I don't know why, but seeing a scoped_ptr<> passed in here is causing me to quirk an eyebrow. I'll revert when I've examined the .cc. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:243: bool IsPendingEntry(const PrerenderHandle* prerender_handle) const; if |prerender_handle| can never be NULL, prefer passing a reference. Even better, make PrerenderHandle have sensible copy semantics and pass-by-value. I know that seems wrong, but when I see 'Handle' I expect something that can be passed by value. YMMV. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:282: // (potentially only partially) prerendered page is shown to the user. Is it not used to potentially expire stale prerendered pages? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:283: base::TimeTicks load_start_time_; The move to protected is unexpected as I thought PrerenderContents was a leaf class. Unless this is exposed for tests? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:334: // for instance). Ah - maybe instance_count_ ? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:37: contents_->DecrementClientCount(); Where do we increment the client count? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:24: public base::NonThreadSafe { consider making this refcounted directly and see if that clears up some of the client code. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:46: PrerenderContents* contents() const { return contents_; } not sure i understand the point of an accessor at the same accessibility level as the field. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:52: // PrerenderContents. what for? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:54: base::WeakPtr<PrerenderHandle> prerender_handle = and yet another smart pointer type required for the handle. I think I'm repeating myself, and I'm sorry if I am, but I really feel that this should be: PrerenderHandle prerender_handle = manager_->AddPrerender... and it should just work. Maybe this means that somewhere there's a typedef scoped_refptr<PrerenderHandleImpl> PrerenderHandle; or something, I'm not sure. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (left): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:323: // Even if we match, the location.hash might be different. Record this as a The problem with fragment mismatch is that we don't get the right fragment passed through to us when starting a prerender (or at least we didn't...). Do we now? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:222: scoped_ptr<PrerenderHandle> scoped_prerender_handle(new PrerenderHandle); Expected implementation: return parent_prerender_contents->AddPendingPrerender(url, referrer, size); where AddPendingPrerender creates and returns the PrerenderHandle. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:237: return base::WeakPtr<PrerenderHandle>(); return PrerenderHandle::kInvalidHandle; where kInvalidHandle is a lazy static initialize PrerenderHandle marked as invalid. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:270: AddPrerender(ORIGIN_LINK_REL_PRERENDER, origin should have been tracked. We might start pending prerenders from any origin. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:272: existing_prerender_handle.Pass()); ahah! Now I see the need for the existing prerender handle. Perhaps instead of passing this through, it could be: DCHECK(!existing_prerender_handle.is_valid()); existing_prerender_handle = AddPrerender(...); existing_prerender_handle.Replace(AddPrerender(...)); which leaves the handle intact but replaces the contents. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:124: base::WeakPtr<PrerenderHandle> AddPrerenderFromOmnibox( This cements it for me. I don't like an API that returns a specific smart pointer and then... http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:130: scoped_ptr<PrerenderHandle> existing_prerender_handle, ... uses a different smart pointer to the same type in another part of the public API. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:135: content::SessionStorageNamespace* session_storage_namespace); ps - why is starting a pending prerender a public operation? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:258: PrerenderContents* FindEntry(const GURL& url); move to protected for tests? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:304: PrerenderHandleMap; if the PrerenderHandle already contains the PrerenderContents* and is the canonical way to refer to a prerender, why do we need this map going the other way? http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:328: const scoped_ptr<PrerenderHandle> preexisting_prerender_handle); I don't understand why this is passed through and can't be discovered from within the method. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:376: PrerenderHandle* FindHandle( with all those smart pointer returns, i'm surprised that this is returning a raw pointer. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:381: PrerenderContents* FindContentsForChildAndRoute(int child_id, int route_id); If we add a new prerender for the same URL/SessionStorage but from a different child_id/route_id, is that a new PrerenderHandle or is it a new client of an existing one?
I find the use of scoped_ptrs, weak_ptrs, linked_ptrs, laser_ptrs, and irish_setters (Which are also apparently pointers, despite their claim to be setters, and the lack of corresponding irish_getters) fairly confusing. I want to think about this, see if we can come up with something much simpler. Thinking it may make sense to have a prerender's pending child prerenders also fit into this mechanism as well, so it behaves somewhat like another prerendering source. Would need code to have it trigger off the new prerender when used, but if it just had a Handle like everyone else, may make life simpler. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:94: PendingPrerenderInfo(linked_ptr<PrerenderHandle> prerender_handle, I assume this is to allow copying. linked_ptr is very rarely used. The much more common thing to do in this case is to just use a vector or list of (gasp) pointers. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:46: PrerenderContents* contents() const { return contents_; } On 2012/06/22 15:36:16, dominich wrote: > not sure i understand the point of an accessor at the same accessibility level > as the field. I generally do it when something is called by a friend class, at least if the friend class is not the outer class, to make it clearer which variables are accessed semi-externally. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:115: const int kHistoryLength = 100; nit: In cc files, the general style is to put constants first. I assume this is the opposite of h files because in h files the purpose is to emphasize methods over variables, while in h files we want to emphasize constants, rather than bury them. (Know you don't like nits this early in the process, but this is a general apparent Chrome consensus style, so thought I'd mention it. It also decreases the number of changes, which I find makes subsequent review passes a little faster, but I'm slow.)
Dominic & Matt, thanks for your comments. They are particularly valuable when you two agree on the same points. I am going to pay close attention to Dominic's API and implementation details suggestions, and clean up the transfers of ownership. I think that will leave us with relatively few smart ptrs. That result, once tests work (including browser tests), I'll upload it. http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/10001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:258: PrerenderContents* FindEntry(const GURL& url); On 2012/06/22 15:36:16, dominich wrote: > move to protected for tests? Yup.
I need to go over my work in browser tests, but then I'll upload this (in pieces if I can figure out where to put the knife). http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:352: DeletePendingDeleteEntries(); Remove this.
i think this is close to landable. make sure you remove all the DVLOGs once you're ready to check in. if we need to debug well, we have debuggers and can add LOG statements locally. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:328: // won't live the full expiry period. the opened_url might match something in the redirect chain (ie, the alias urls) so do we really want to mark it as abandoned? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:88: void StartPrerendering( this'll need a comment. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:128: using PrerenderContents::PendingPrerenderInfo; Do you really need these as a derived class of PrerenderContents? I thought they'd be in scope. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:721: return nit: return GetPrerenderManager()->FindEntry( url, GetSessionStorageNamespace()) != NULL; http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1674: // DISABLED: http://crbug.com/84154 shouldn't be disabled any more? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1686: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, shouldn't be disabled any more? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1697: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, shouldn't be disabled any more? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1708: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, shouldn't be disabled any more? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1720: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, shouldn't be disabled any more? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1739: // Checks that preren a PNG works correctly. an accidental s/dering//g ? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1886: FINAL_STATUS_APP_TERMINATING, this is surprising. we still want to cancel on WINDOW_OPENER, no? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1896: FINAL_STATUS_APP_TERMINATING, ditto http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:11: #include "base/memory/linked_ptr.h" include no longer needed. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:17: #include "chrome/browser/prerender/prerender_handle.h" unnecessary include? included by .h http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:195: PrerenderHandleImpl* prerender_handle_impl = new PrerenderHandleImpl; () at the end of the 'new' call to ensure initialization. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:214: SessionStorageNamespace* my_session_storage_namespace = NULL; nit: Drop the my_. Just session_storage_namespace. It's cleaner. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:215: if (RenderViewHost* render_view_host = GetRenderViewHostMutable()) { why does getting the SessionStorageNamespace require a mutable rvh? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:229: it->prerender_handle_impl, ORIGIN_LINK_REL_PRERENDER, origin should be stored in the pending struct. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:236: PrerenderHandleImpl* prerender_handle_impl, const PrerenderHandleImpl? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:276: void PrerenderContents::MakeIntoDummyReplacementOf( CloneAsDummy, maybe? Do any of the other members matter? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:313: DCHECK(load_start_time_.is_null()); any reason why this moved earlier? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:557: DCHECK_LT(0, client_count_); this is very confusing. DCHECK_GT(client_count_, 0); http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:573: return match_count > 0; nit: return std::count_if( alias_urls_.begin(), alias_urls_.end(), std::bind2nd(std::equal_to<GURL>(), url)) != 0; http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:98: void MakeIntoDummyReplacementOf( can this be a separate CL? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:159: int client_count() const { return client_count_; } usage count? instance count? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:170: bool Matches( we didn't use the returned matching_url anywhere? if not, then change the comment please. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:228: // Returns true if |url| corresponds to a pending prerender. comment http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:237: struct PendingPrerenderInfo { I believe this can just be a forward reference here, and then actually defined in the .cc file as before. If not, why not? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:243: PrerenderHandleImpl* prerender_handle_impl; const PrerenderHandleImpl? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:249: nit: extra blank line http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:339: // Number of distinct clients of this prerendered page (distinct link elements instances? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:52: // PrerenderContents. This ends up being effectively a list, since our my non-lisp brain still wants this to be a scoped_vector<PrerenderHandleImpl> instances_; or something similar. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:57: typedef base::WeakPtr<PrerenderHandleImpl> PrerenderHandle; i love this so hard. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:12: #include "chrome/browser/prerender/prerender_handle.h" unnecessary include - included in header. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:58: DCHECK_EQ(0U, ids_to_handle_map_.count(child_and_prerender_id)); in the case of multiple prerenders, this could happen if a web dev adds multiple link rel prerender elements referencing the same URL. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:84: DVLOG(5) << "... and the handle was no good."; does this ever happen? maybe DCHECK instead. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:28: #include "chrome/browser/prerender/prerender_handle.h" unnecessary include - included by header. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:105: return final_status != FINAL_STATUS_USED && more robust: switch (final_status) { case FINAL_STATUS_USED: case FINAL_STATUS_TIMED_OUT: ... return false; case FINAL_STATUS... return true; } NOTREACHED(); return false; http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:259: if (!IsOmniboxEnabled(profile_)) nit: return IsOmniboxEnabled(profile_) ? AddPrerender(...) : PrerenderHandle(); http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:280: new_prerender_handle->AddDuplicate(existing_prerender_handle); shouldn't this be existing_prerender_handle->AddDuplicate(new_prerender_handle)? noone has a reference to the new_prerender_handle and it's not returned. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:404: prerender_contents->StartPendingPrerenders(); any reason why this was moved earlier? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:471: return; i think this changes the logic. by returning early we now don't call AddToHistory for entries that would have been added before. this might be correct (it probably is) but i wanted to point it out. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:881: PrerenderHandleImpl* prerender_handle_impl = new PrerenderHandleImpl; a constructor that takes a PrerenderContents? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:881: PrerenderHandleImpl* prerender_handle_impl = new PrerenderHandleImpl; () after the type to ensure initialization. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:899: DCHECK_GT(old_size, prerender_list_.size()); if you want to test that calling Destroy removes an entry from the list, you should do that in a unit test, not in production code. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:930: PrerenderHandleList prerender_list(prerender_list_); Change comment to 'Grab a copy of the current PrerenderContents list and then don't use it in the rest of the method.' or maybe don't take the copy. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:953: bool PrerenderManager::IsPrerenderFresh(base::TimeTicks start) const { can you write up a post for the dev.chromium.org site, or just the mailing list, about Time vs TimeTicks? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1105: DCHECK_GT(old_size, prerender_list_.size()); check Destroy removes items in a unit test, not in production code. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:266: // |session_storage_namespace| if it is prerendering. Used only in tests. #if UNIT_TEST? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:271: void TreatPrerenderAsUsedForTest( This should be a member of TestPrerenderManager as it's only used in prerender unit tests http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (left): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:601: TEST_F(PrerenderTest, PageMatchesFragmentTest) { Maybe it's worth reversing these tests and keeping them? http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:73: using PrerenderManager::kNavigationRecordWindowMs; not sure why you need these using statements. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:237: load_start_time_ = test_prerender_manager_->GetCurrentTimeTicks(); if this is all you need it for, maybe make GetCurrentTimeTicks virtual and retain the PrerenderManager pointer instead of adding a TestPrerenderManager member. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:391: void DoPrerender(predictors::AutocompleteActionPredictor* action_predictor, this should be unnecessary - the predictor is profile keyed, and we have a profile_.
http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:55: }; How not get rid of duplicate_handle_ and AddDuplicate? Just call AddClient and return the original handle. I don't see why you need more than one of them, they don't have any individual state, and you aren't doing anything in their destructors (And can't, for that matter). http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:58: DCHECK_EQ(0U, ids_to_handle_map_.count(child_and_prerender_id)); On 2012/06/28 00:34:32, dominich wrote: > in the case of multiple prerenders, this could happen if a web dev adds multiple > link rel prerender elements referencing the same URL. Could happen in the case of supporting one prerender at a time, too, since that rule is handled by the PrerenderManager. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:280: new_prerender_handle->AddDuplicate(existing_prerender_handle); Also, erm...What if a prerender is cancelled prior to the call to StartPendingPrerender?
http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:55: }; On 2012/06/28 16:01:28, Matt Menke wrote: > How not get rid of duplicate_handle_ and AddDuplicate? Just call AddClient and > return the original handle. I don't see why you need more than one of them, > they don't have any individual state, and you aren't doing anything in their > destructors (And can't, for that matter). Erm... That should be "Why not just get" (Or "How about getting", take your pick).
I've removed the **NOTFORLANDING** from the description, as I think this CL is ready to begin a formal review. There are a lot of changes from previous versions; in particular see what a PrerenderHandle is now. http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/24001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:128: using PrerenderContents::PendingPrerenderInfo; On 2012/06/28 00:34:32, dominich wrote: > Do you really need these as a derived class of PrerenderContents? I thought > they'd be in scope. They are, but TestPrerenderManager calls them, and they are protected in PrerenderContents.
first pass. please run try. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:327: prerender_handle_->OnNavigateAway(); I know that OnNavigateAway doesn't immediately delete the prerender, but that's an implementation detail. I'd much rather we call this iff we don't have a URL match. Or not call it at all. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:131: using PrerenderContents::PendingPrerenderInfo; Should these just be made protected in PrerenderContents? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:504: return current_browser()->tab_strip_model()->GetActiveTabContents()-> Is this fragile on, say, OSX where we can have a browser but no active tab contents? Do you need if(.. != NULL) checks? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1692: FINAL_STATUS_APP_TERMINATING, Why is this not USED? also below a few times. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1753: // Checks that preren a PNG works correctly. typo http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:11: #include "base/memory/linked_ptr.h" unused include http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:206: if (it->weak_prerender_handle == prerender_handle) if (it->weak_prerender_handle.get() == prerender_handle) ? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:228: it->weak_prerender_handle, ORIGIN_LINK_REL_PRERENDER, child_id_, weak_prerender_handle.get()? PrerenderManager::StartPendingPrerender takes a raw pointer. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:563: return match_count > 0; return std::count_if(alias_urls_.begin(), alias_urls_.end(), std::bind2nd(std::equal_to<GURL>(), url)) != 0; http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:226: bool IsPendingEntry(PrerenderHandle* prerender_handle) const; const PrerenderHandle& prerender_handle? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:248: typedef std::list<PendingPrerenderInfo> PendingPrerenderList; I think this should be a vector - better memory use, D-cache use, and we can control which order we push/pop. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:29: if (prerender_data_->contents) maybe if (IsPending()) or if(IsPrerendering()) makes it clearer what you're doing here. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:77: base::WeakPtr<PrerenderData> tmp_prerender_data = prerender_data_; std::swap(prerender_data_, other_prerender_handle->prerender_data_); http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:11: #include "base/memory/ref_counted.h" unnecessary ref_counted.h include? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:56: using base::SupportsWeakPtr<PrerenderHandle>::AsWeakPtr; why is this using statement necessary? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:63: PrerenderContents* get_contents() { return contents; } no need for this accessor - contents is public. If you do need it, it should be const and return const PrerenderContents* http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:67: int client_count; instance_count? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:70: explicit PrerenderHandle(); no explicit http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); Why OnNavigateAway? If you delete (as you do below) it should call OnCancel which is more correct, i think. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:38: delete prerender_handle; So the caller of AddPrerender* owns the returned handle? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:103: prerender_handle->OnNavigateAway(); This will call both OnNavigateAway and OnCancel. It's not clear that this is a good idea from the API. Once we hook this up to the link element getting events, I'm not sure it will be clear what the expected behaviour is. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:29: class PrerenderHandle; nit: indent http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:74: typedef std::map<ChildAndPrerenderIdPair, PrerenderHandle*> why isn't this a WeakPtr? could the reference be removed before it's removed from the map, and could this then be a dangling pointer? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:297: if (&(*it) == pending_prerender_data) { Is it worth checking the contents of the pending_prerender_data? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:346: return false; // Do not swap in to ourself. does this actually happen? Does it cause problems? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:643: TabContents* prerender_tab_contents = You removed a character and now it doesn't fit on a line? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:885: scoped_ptr<PrerenderHandle> prerender_handle( why scoped_ptr here? You're going to release in a few lines so you should just use a raw pointer and return it. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:900: prerender_contents->Destroy(FINAL_STATUS_EVICTED); Did you add a unit test to ensure that Destroy removes contents from active_prerender_list_ http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:932: prerender_contents(active_prerender_list_.size()); +100 for sizing the vector correctly. -5 for not finding a way to initialize it to the correct size and the right contents in one STL method. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:956: bool PrerenderManager::IsPrerenderFresh(base::TimeTicks start) const { Y U NO const base::TimeTicks start? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1002: PrerenderWeakPtrFactoryMap::value_type map_value(prerender_data, NULL); base::WeakPtrFactory<PrerenderData> prerender_data_factory = new base::WeakPtrFactory<PrerenderData>(prerender_data); PrerenderWeakPtrFactoryMap::value_type map_value(prerender_data, factory); if (!prerender_weak_ptr_factory_map_.insert(map_value).second) { delete prerender_data_factory; return new PrerenderHandle(); } return new PrerenderHandle(prerender_data_factory->GetWeakPtr()); http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1009: insert_result.first->second->GetWeakPtr(); if insert fails this will be NULL. See my version above that assumes insert will succeed. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1026: for (std::list<PrerenderData>::iterator it = active_prerender_list_.begin(); Why did you remove the typedef for std::list<PrerenderData>? (and when oh when will we be able to use auto?) http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:111: // as a pending prerender. Add comment that caller is the owner of the returned Handle. Also below. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:274: typedef PrerenderHandle::PrerenderData PrerenderData; I'm not convinced this typedef makes the code clearer. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:287: // found, it then uses a default from PrerenderConfig. If |prerender_handle| update comment - no prerender_handle parameter. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (left): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:27: namespace { please keep this. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:71: using PrerenderManager::kNavigationRecordWindowMs; if this is a friend of PrerenderManager you shouldn't need these using statements. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:101: if (!prerender_data) { nit: braces not required. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:243: if (!is_control_group) { nit: braces not required. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:675: // TODO(gavinp): Drop this test once we are fragment clean. we are fragment clean now, no? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:676: TEST_F(PrerenderTest, FragmentDropedOnLaunchTest) { typo: Dropped http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1097: void OmniboxEditModel::DoPrerender( this should still fit on one line. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:36: class AutocompleteActionPredictor; not necessary
Remediated to dominich review. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:327: prerender_handle_->OnNavigateAway(); On 2012/07/02 20:43:03, dominich wrote: > I know that OnNavigateAway doesn't immediately delete the prerender, but that's > an implementation detail. I'd much rather we call this iff we don't have a URL > match. Or not call it at all. Done. I'll add it back when prerenders become long lived. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:131: using PrerenderContents::PendingPrerenderInfo; On 2012/07/02 20:43:03, dominich wrote: > Should these just be made protected in PrerenderContents? They are already protected, this would not work otherwise. However, PrerenderBrowserTest::URLIsPendingInPrerenderManager depended on these. I moved that method into TestPrerenderContents, and now the using statements are gone. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:504: return current_browser()->tab_strip_model()->GetActiveTabContents()-> On 2012/07/02 20:43:03, dominich wrote: > Is this fragile on, say, OSX where we can have a browser but no active tab > contents? Do you need if(.. != NULL) checks? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1692: FINAL_STATUS_APP_TERMINATING, On 2012/07/02 20:43:03, dominich wrote: > Why is this not USED? The URLs do not compare as equal. > > also below a few times. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1753: // Checks that preren a PNG works correctly. On 2012/07/02 20:43:03, dominich wrote: > typo Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:11: #include "base/memory/linked_ptr.h" On 2012/07/02 20:43:03, dominich wrote: > unused include Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:206: if (it->weak_prerender_handle == prerender_handle) On 2012/07/02 20:43:03, dominich wrote: > if (it->weak_prerender_handle.get() == prerender_handle) ? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:228: it->weak_prerender_handle, ORIGIN_LINK_REL_PRERENDER, child_id_, On 2012/07/02 20:43:03, dominich wrote: > weak_prerender_handle.get()? > > PrerenderManager::StartPendingPrerender takes a raw pointer. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:563: return match_count > 0; On 2012/07/02 20:43:03, dominich wrote: > return std::count_if(alias_urls_.begin(), alias_urls_.end(), > std::bind2nd(std::equal_to<GURL>(), url)) != 0; Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:226: bool IsPendingEntry(PrerenderHandle* prerender_handle) const; On 2012/07/02 20:43:03, dominich wrote: > const PrerenderHandle& prerender_handle? const PrerenderHandle* prerender_handle. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:248: typedef std::list<PendingPrerenderInfo> PendingPrerenderList; On 2012/07/02 20:43:03, dominich wrote: > I think this should be a vector - better memory use, D-cache use, and we can > control which order we push/pop. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:29: if (prerender_data_->contents) On 2012/07/02 20:43:03, dominich wrote: > maybe if (IsPending()) or if(IsPrerendering()) makes it clearer what you're > doing here. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:77: base::WeakPtr<PrerenderData> tmp_prerender_data = prerender_data_; On 2012/07/02 20:43:03, dominich wrote: > std::swap(prerender_data_, other_prerender_handle->prerender_data_); Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:11: #include "base/memory/ref_counted.h" On 2012/07/02 20:43:03, dominich wrote: > unnecessary ref_counted.h include? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:56: using base::SupportsWeakPtr<PrerenderHandle>::AsWeakPtr; On 2012/07/02 20:43:03, dominich wrote: > why is this using statement necessary? To make the method private. I don't want to expose the method publicly without a good reason; WeakPtr<PrerenderHandle> is an implementation detail of pending prerenders, not a public API of PrerenderHandle. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:63: PrerenderContents* get_contents() { return contents; } On 2012/07/02 20:43:03, dominich wrote: > no need for this accessor - contents is public. If you do need it, it should be > const and return const PrerenderContents* It's used only for a single std::transform in PrerenderManager. Now removed and replaced with an explicit loop. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:67: int client_count; On 2012/07/02 20:43:03, dominich wrote: > instance_count? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:70: explicit PrerenderHandle(); On 2012/07/02 20:43:03, dominich wrote: > no explicit Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); On 2012/07/02 20:43:03, dominich wrote: > Why OnNavigateAway? If you delete (as you do below) it should call OnCancel > which is more correct, i think. I removed the delete. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:38: delete prerender_handle; On 2012/07/02 20:43:03, dominich wrote: > So the caller of AddPrerender* owns the returned handle? Yes. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:103: prerender_handle->OnNavigateAway(); On 2012/07/02 20:43:03, dominich wrote: > This will call both OnNavigateAway and OnCancel. It's not clear that this is a > good idea from the API. Once we hook this up to the link element getting events, > I'm not sure it will be clear what the expected behaviour is. Both OnNavigateAway and OnCancel invalidate the handle. Now documented. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:29: class PrerenderHandle; On 2012/07/02 20:43:03, dominich wrote: > nit: indent Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:74: typedef std::map<ChildAndPrerenderIdPair, PrerenderHandle*> On 2012/07/02 20:43:03, dominich wrote: > why isn't this a WeakPtr? could the reference be removed before it's removed > from the map, and could this then be a dangling pointer? No, it owns it. Made it a linked_ptr<> so that's clear. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:297: if (&(*it) == pending_prerender_data) { On 2012/07/02 20:43:03, dominich wrote: > Is it worth checking the contents of the pending_prerender_data? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:346: return false; // Do not swap in to ourself. On 2012/07/02 20:43:03, dominich wrote: > does this actually happen? Does it cause problems? Yes. See http://avclub.ytz.ca/~gavin/testing/prerender/self-launcher.html http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:643: TabContents* prerender_tab_contents = On 2012/07/02 20:43:03, dominich wrote: > You removed a character and now it doesn't fit on a line? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:885: scoped_ptr<PrerenderHandle> prerender_handle( On 2012/07/02 20:43:03, dominich wrote: > why scoped_ptr here? You're going to release in a few lines so you should just > use a raw pointer and return it. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:900: prerender_contents->Destroy(FINAL_STATUS_EVICTED); On 2012/07/02 20:43:03, dominich wrote: > Did you add a unit test to ensure that Destroy removes contents from > active_prerender_list_ All of the existing unit tests should test this, since we assert on the lists being empty at destruction... http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:932: prerender_contents(active_prerender_list_.size()); On 2012/07/02 20:43:03, dominich wrote: > +100 for sizing the vector correctly. > -5 for not finding a way to initialize it to the correct size and the right > contents in one STL method. Now removed, as get_contents is dead. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:956: bool PrerenderManager::IsPrerenderFresh(base::TimeTicks start) const { On 2012/07/02 20:43:03, dominich wrote: > Y U NO const base::TimeTicks start? Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:958: base::TimeTicks now = GetCurrentTimeTicks(); Adeed bonus: const base::TimeTicks now. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1009: insert_result.first->second->GetWeakPtr(); On 2012/07/02 20:43:03, dominich wrote: > if insert fails this will be NULL. See my version above that assumes insert will > succeed. If insert fails, then there was a pre-existing element, which must have a WeakPtrFactory<>, from line 1006 above. I've added a DCHECK. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1026: for (std::list<PrerenderData>::iterator it = active_prerender_list_.begin(); On 2012/07/02 20:43:03, dominich wrote: > Why did you remove the typedef for std::list<PrerenderData>? > > (and when oh when will we be able to use auto?) I didn't think it saved much space; mmenke is not a fan of typedef. We will be allowed to use auto on the same day we can use lambda, and put a moveable scoped_ptr into an STL container. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:111: // as a pending prerender. On 2012/07/02 20:43:03, dominich wrote: > Add comment that caller is the owner of the returned Handle. > > Also below. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:274: typedef PrerenderHandle::PrerenderData PrerenderData; On 2012/07/02 20:43:03, dominich wrote: > I'm not convinced this typedef makes the code clearer. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:287: // found, it then uses a default from PrerenderConfig. If |prerender_handle| On 2012/07/02 20:43:03, dominich wrote: > update comment - no prerender_handle parameter. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (left): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:27: namespace { On 2012/07/02 20:43:03, dominich wrote: > please keep this. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:71: using PrerenderManager::kNavigationRecordWindowMs; On 2012/07/02 20:43:03, dominich wrote: > if this is a friend of PrerenderManager you shouldn't need these using > statements. Without them, NotSoRecentlyVisited will not compile. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:101: if (!prerender_data) { On 2012/07/02 20:43:03, dominich wrote: > nit: braces not required. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:243: if (!is_control_group) { On 2012/07/02 20:43:03, dominich wrote: > nit: braces not required. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:675: // TODO(gavinp): Drop this test once we are fragment clean. On 2012/07/02 20:43:03, dominich wrote: > we are fragment clean now, no? See http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/prerender/p... It's a separate CL to remove that, which I'll do after this. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:676: TEST_F(PrerenderTest, FragmentDropedOnLaunchTest) { On 2012/07/02 20:43:03, dominich wrote: > typo: Dropped Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1097: void OmniboxEditModel::DoPrerender( On 2012/07/02 20:43:03, dominich wrote: > this should still fit on one line. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:36: class AutocompleteActionPredictor; On 2012/07/02 20:43:03, dominich wrote: > not necessary Done.
http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1692: FINAL_STATUS_APP_TERMINATING, On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > Why is this not USED? > > The URLs do not compare as equal. So this would be TIMED_OUT in production? Maybe we need to bring back FRAGMENT_MISMATCH to see how often we are prerendering something with a different ref to the navigation. WDYT? > > > > > also below a few times. > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1697: // Checks that we correctly use a prerendered page when we prerender a fragment this should be enabled and the comment should be changed to reflect our expectations. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1734: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, Can you add a test that prerenders #fragment and navigates to #fragment to ensure that we use it? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:2193: FINAL_STATUS_APP_TERMINATING, If this isn't used because the Session Storage Namespace doesn't match, then we should still track it as such. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:226: bool IsPendingEntry(PrerenderHandle* prerender_handle) const; On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > const PrerenderHandle& prerender_handle? > > const PrerenderHandle* prerender_handle. Why? Can it be NULL? http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:56: using base::SupportsWeakPtr<PrerenderHandle>::AsWeakPtr; On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > why is this using statement necessary? > > To make the method private. I don't want to expose the method publicly without a > good reason; WeakPtr<PrerenderHandle> is an implementation detail of pending > prerenders, not a public API of PrerenderHandle. Then, as per the recent thread, you might want to use WeakPtrFactory instead of inheriting from SupportsWeakPtr. Then you can control who can create weak pointers. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > Why OnNavigateAway? If you delete (as you do below) it should call OnCancel > > which is more correct, i think. > > I removed the delete. But that's the right thing to do :) We're not navigating away, we're shutting down. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:103: prerender_handle->OnNavigateAway(); On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > This will call both OnNavigateAway and OnCancel. It's not clear that this is a > > good idea from the API. Once we hook this up to the link element getting > events, > > I'm not sure it will be clear what the expected behaviour is. > > Both OnNavigateAway and OnCancel invalidate the handle. Now documented. I'm tempted to suggest that OnCancel should delete(this), but there be dragons. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:74: typedef std::map<ChildAndPrerenderIdPair, PrerenderHandle*> On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > why isn't this a WeakPtr? could the reference be removed before it's removed > > from the map, and could this then be a dangling pointer? > > No, it owns it. Made it a linked_ptr<> so that's clear. I think a comment stating ownership would be better than a linked_ptr. Linked_ptr doesn't scream ownership to me, it screams refcounting. Which is unnecessary here. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:346: return false; // Do not swap in to ourself. On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > does this actually happen? Does it cause problems? > > Yes. See http://avclub.ytz.ca/%7Egavin/testing/prerender/self-launcher.html Doesn't seem to cause problems. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:958: base::TimeTicks now = GetCurrentTimeTicks(); On 2012/07/03 16:41:02, gavinp wrote: > Adeed bonus: const base::TimeTicks now. Even better, return GetCurrentTimeTicks() - start < GetMaxAge(); no need for a local at all!!eleven! http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1026: for (std::list<PrerenderData>::iterator it = active_prerender_list_.begin(); On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > Why did you remove the typedef for std::list<PrerenderData>? > > > > (and when oh when will we be able to use auto?) > > I didn't think it saved much space; mmenke is not a fan of typedef. > so much more readable though > We will be allowed to use auto on the same day we can use lambda, and put a > moveable scoped_ptr into an STL container. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:71: using PrerenderManager::kNavigationRecordWindowMs; On 2012/07/03 16:41:02, gavinp wrote: > On 2012/07/02 20:43:03, dominich wrote: > > if this is a friend of PrerenderManager you shouldn't need these using > > statements. > > Without them, NotSoRecentlyVisited will not compile. My gut prefers accessors to usings, but this is pretty neat. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:239: pending_prerender_vector()->size() == expected_pending_prerenders_) { when your variable name includes the type, you're doing it wrong. pending_prerenders() http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:201: for (std::vector<PendingPrerenderInfo>::const_iterator it = Another reason to not remove the typedef - you would have only had to change the type in one place. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:205: if (it->weak_prerender_handle.get() == prerender_handle) i'm curious how this compiled before. it /should/ be ok to compare a weak_ptr to a raw pointer, but I don't know exactly what code the compiler would have inserted to do that, given there's no overloaded operator== that I can see. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:228: it->weak_prerender_handle.get(), ORIGIN_LINK_REL_PRERENDER, child_id_, and again - how did this compile? http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:241: base::WeakPtr<PrerenderHandle> weak_prerender_handle; why did you remove the const? once we have a pending prerender it shouldn't change. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:375: std::vector<PendingPrerenderInfo> pending_prerender_list_; so you changed the accessor to reflect the new type but not the member? ... http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:36: PrerenderHandle* prerender_handle = it->second.get(); the more i see this, the more worried i get. I don't think it should be valid for the compiler to automatically degrade a weak pointer to a raw pointer. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); I don't trust refcounting. You have a very clear ownership semantic and linked_ptr is confusing it. I'd prefer explicit deletes. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:932: std::vector<PrerenderContents*> prerender_contents; you lost 100 points for no longer sizing the vector correctly. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:277: base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> I wonder if this can go away if the PrerenderData contains a WeakPtrFactory
http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1697: // Checks that we correctly use a prerendered page when we prerender a fragment On 2012/07/03 17:08:39, dominich wrote: > this should be enabled and the comment should be changed to reflect our > expectations. Is http://code.google.com/p/chromium/issues/detail?id=84154 fixed? This will flake if it isn't. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1734: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, On 2012/07/03 17:08:39, dominich wrote: > Can you add a test that prerenders #fragment and navigates to #fragment to > ensure that we use it? That sounds like it belongs in the CL that removes http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/prerender/p... http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:2193: FINAL_STATUS_APP_TERMINATING, On 2012/07/03 17:08:39, dominich wrote: > If this isn't used because the Session Storage Namespace doesn't match, then we > should still track it as such. I disagree; I'm happy to throw out that result code. It's a special case that infects all of our matching logic to find it, and to gather a stat that is basically about how often users middle click to search results? We have a good estimate of the frequency of that now, and we can just carry it forward, and if we ever add more liberal session storage namespace handling, we'll see the benefit in A/B testing... http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:226: bool IsPendingEntry(PrerenderHandle* prerender_handle) const; On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > const PrerenderHandle& prerender_handle? > > > > const PrerenderHandle* prerender_handle. > > Why? Can it be NULL? Done. I just didn't have a PrerenderHandle& anywhere else, so it felt weird; especially since the function was really only using the address. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:56: using base::SupportsWeakPtr<PrerenderHandle>::AsWeakPtr; On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > why is this using statement necessary? > > > > To make the method private. I don't want to expose the method publicly without > a > > good reason; WeakPtr<PrerenderHandle> is an implementation detail of pending > > prerenders, not a public API of PrerenderHandle. > > Then, as per the recent thread, you might want to use WeakPtrFactory instead of > inheriting from SupportsWeakPtr. Then you can control who can create weak > pointers. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > Why OnNavigateAway? If you delete (as you do below) it should call OnCancel > > > which is more correct, i think. > > > > I removed the delete. > > But that's the right thing to do :) We're not navigating away, we're shutting > down. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:103: prerender_handle->OnNavigateAway(); On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > This will call both OnNavigateAway and OnCancel. It's not clear that this is > a > > > good idea from the API. Once we hook this up to the link element getting > > events, > > > I'm not sure it will be clear what the expected behaviour is. > > > > Both OnNavigateAway and OnCancel invalidate the handle. Now documented. > > I'm tempted to suggest that OnCancel should delete(this), but there be dragons. I was tempted to write it that way too, but came to the same conclusion. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:74: typedef std::map<ChildAndPrerenderIdPair, PrerenderHandle*> On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > why isn't this a WeakPtr? could the reference be removed before it's removed > > > from the map, and could this then be a dangling pointer? > > > > No, it owns it. Made it a linked_ptr<> so that's clear. > > I think a comment stating ownership would be better than a linked_ptr. > Linked_ptr doesn't scream ownership to me, it screams refcounting. Which is > unnecessary here. Done. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:346: return false; // Do not swap in to ourself. On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > On 2012/07/02 20:43:03, dominich wrote: > > > does this actually happen? Does it cause problems? > > > > Yes. See http://avclub.ytz.ca/%257Egavin/testing/prerender/self-launcher.html > > Doesn't seem to cause problems. You're right, my test doesn't. But I was just copying http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/prerender/p... here, and I'm made a bit anxious by removing it... I'll continue trying to create a test that exercises this. Our current browser tests don't. http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:958: base::TimeTicks now = GetCurrentTimeTicks(); On 2012/07/03 17:08:39, dominich wrote: > On 2012/07/03 16:41:02, gavinp wrote: > > Adeed bonus: const base::TimeTicks now. > > Even better, return GetCurrentTimeTicks() - start < GetMaxAge(); > > no need for a local at all!!eleven! Done. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:239: pending_prerender_vector()->size() == expected_pending_prerenders_) { On 2012/07/03 17:08:39, dominich wrote: > when your variable name includes the type, you're doing it wrong. > pending_prerenders() Done. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:205: if (it->weak_prerender_handle.get() == prerender_handle) On 2012/07/03 17:08:39, dominich wrote: > i'm curious how this compiled before. it /should/ be ok to compare a weak_ptr to > a raw pointer, but I don't know exactly what code the compiler would have > inserted to do that, given there's no overloaded operator== that I can see. See base::WeakPtr<T>::operator T*() { return get(); } The language allows one implicit conversion when searching for a function, so it was calling that conversion to get a PrerenderHandle*, then using the default bool ::operator==(PrerenderHandle*, PrerenderHandle*). http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:241: base::WeakPtr<PrerenderHandle> weak_prerender_handle; On 2012/07/03 17:08:39, dominich wrote: > why did you remove the const? once we have a pending prerender it shouldn't > change. std::vector<T> requires that T be default constructible and assignable. Technically, std::list<> requires assignable, but in practice a copy constructor is good enough. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:375: std::vector<PendingPrerenderInfo> pending_prerender_list_; On 2012/07/03 17:08:39, dominich wrote: > so you changed the accessor to reflect the new type but not the member? > > ... Done. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:37: prerender_handle->OnNavigateAway(); On 2012/07/03 17:08:39, dominich wrote: > I don't trust refcounting. You have a very clear ownership semantic and > linked_ptr is confusing it. I'd prefer explicit deletes. Done. When we switch to C++11, I'll make it a map to scoped_ptr. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:932: std::vector<PrerenderContents*> prerender_contents; On 2012/07/03 17:08:39, dominich wrote: > you lost 100 points for no longer sizing the vector correctly. added reserve(). http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:277: base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> On 2012/07/03 17:08:39, dominich wrote: > I wonder if this can go away if the PrerenderData contains a WeakPtrFactory This was a challenge due to our coding standards. I wanted PrerenderData to be pod, so it could go in a std::list<>, so we would have the active_prerender_list_ like we do. But then the copy constructor and assignment operator (required for the container), would be a bit confusing: they wouldn't copy the weak_ptr_factory(). I made the call that this while it's a bit ugly, saved prerender_handle.h from being really ugly. Would you like it better the other way?
On 2012/07/03 18:45:40, gavinp wrote: > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:1697: // Checks that we > correctly use a prerendered page when we prerender a fragment > On 2012/07/03 17:08:39, dominich wrote: > > this should be enabled and the comment should be changed to reflect our > > expectations. > > Is http://code.google.com/p/chromium/issues/detail?id=84154 fixed? This will > flake if it isn't. No. Your WebKit changes should have fixed 83109 which is related. Maybe once this and other related changes land we should (I should) take another look at this. > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:1734: > IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, > On 2012/07/03 17:08:39, dominich wrote: > > Can you add a test that prerenders #fragment and navigates to #fragment to > > ensure that we use it? > > That sounds like it belongs in the CL that removes > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/prerender/p... > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:2193: > FINAL_STATUS_APP_TERMINATING, SGTM > On 2012/07/03 17:08:39, dominich wrote: > > If this isn't used because the Session Storage Namespace doesn't match, then > we > > should still track it as such. > > I disagree; I'm happy to throw out that result code. It's a special case that > infects all of our matching logic to find it, and to gather a stat that is > basically about how often users middle click to search results? > > We have a good estimate of the frequency of that now, and we can just carry it > forward, and if we ever add more liberal session storage namespace handling, > we'll see the benefit in A/B testing... We'll see the benefit under reduced time-out which could be attributed to something else (user behaviour, etc). By having the explicit reason for dropping the prerender we have both better A/B test tracking and we know where to focus efforts. > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_contents.h (right): > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.h:226: bool > IsPendingEntry(PrerenderHandle* prerender_handle) const; > On 2012/07/03 17:08:39, dominich wrote: > > On 2012/07/03 16:41:02, gavinp wrote: > > > On 2012/07/02 20:43:03, dominich wrote: > > > > const PrerenderHandle& prerender_handle? > > > > > > const PrerenderHandle* prerender_handle. > > > > Why? Can it be NULL? > > Done. I just didn't have a PrerenderHandle& anywhere else, so it felt weird; > especially since the function was really only using the address. I'm willing to bet some other reviewer will ask you to change it back, but we'll see :) > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/10553029/diff/37001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:346: return false; // Do not swap > in to ourself. > On 2012/07/03 17:08:39, dominich wrote: > > On 2012/07/03 16:41:02, gavinp wrote: > > > On 2012/07/02 20:43:03, dominich wrote: > > > > does this actually happen? Does it cause problems? > > > > > > Yes. See > http://avclub.ytz.ca/%25257Egavin/testing/prerender/self-launcher.html > > > > Doesn't seem to cause problems. > > You're right, my test doesn't. > > But I was just copying > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/prerender/p... > here, and I'm made a bit anxious by removing it... > > I'll continue trying to create a test that exercises this. Our current browser > tests don't. > > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_contents.cc (right): > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.cc:205: if > (it->weak_prerender_handle.get() == prerender_handle) > On 2012/07/03 17:08:39, dominich wrote: > > i'm curious how this compiled before. it /should/ be ok to compare a weak_ptr > to > > a raw pointer, but I don't know exactly what code the compiler would have > > inserted to do that, given there's no overloaded operator== that I can see. > > See base::WeakPtr<T>::operator T*() { return get(); } > > The language allows one implicit conversion when searching for a function, so it > was calling that conversion to get a PrerenderHandle*, then using the default > bool ::operator==(PrerenderHandle*, PrerenderHandle*). > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_contents.h (right): > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.h:241: > base::WeakPtr<PrerenderHandle> weak_prerender_handle; > On 2012/07/03 17:08:39, dominich wrote: > > why did you remove the const? once we have a pending prerender it shouldn't > > change. > > std::vector<T> requires that T be default constructible and assignable. > Technically, std::list<> requires assignable, but in practice a copy constructor > is good enough. > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_contents.h:375: > std::vector<PendingPrerenderInfo> pending_prerender_list_; > On 2012/07/03 17:08:39, dominich wrote: > > so you changed the accessor to reflect the new type but not the member? > > > > ... > > Done. > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_link_manager.cc (right): > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:37: > prerender_handle->OnNavigateAway(); > On 2012/07/03 17:08:39, dominich wrote: > > I don't trust refcounting. You have a very clear ownership semantic and > > linked_ptr is confusing it. I'd prefer explicit deletes. > > Done. > > When we switch to C++11, I'll make it a map to scoped_ptr. > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.cc:932: > std::vector<PrerenderContents*> prerender_contents; > On 2012/07/03 17:08:39, dominich wrote: > > you lost 100 points for no longer sizing the vector correctly. > > added reserve(). > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_manager.h (right): > > http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_manager.h:277: > base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> > On 2012/07/03 17:08:39, dominich wrote: > > I wonder if this can go away if the PrerenderData contains a WeakPtrFactory > > This was a challenge due to our coding standards. I wanted PrerenderData to be > pod, so it could go in a std::list<>, so we would have the > active_prerender_list_ like we do. > > But then the copy constructor and assignment operator (required for the > container), would be a bit confusing: they wouldn't copy the weak_ptr_factory(). > I made the call that this while it's a bit ugly, saved prerender_handle.h from > being really ugly. > > Would you like it better the other way? Actually no. It's a shame that we have to track the WeakPtrFactory outside of the PrerenderData, as that feels very odd. But your reasoning makes sense.
http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:205: if (it->weak_prerender_handle.get() == prerender_handle) On 2012/07/03 18:45:41, gavinp wrote: > On 2012/07/03 17:08:39, dominich wrote: > > i'm curious how this compiled before. it /should/ be ok to compare a weak_ptr > to > > a raw pointer, but I don't know exactly what code the compiler would have > > inserted to do that, given there's no overloaded operator== that I can see. > > See base::WeakPtr<T>::operator T*() { return get(); } > > The language allows one implicit conversion when searching for a function, so it > was calling that conversion to get a PrerenderHandle*, then using the default > bool ::operator==(PrerenderHandle*, PrerenderHandle*). Aha! I do not like that one bit. However, it is not something to tackle here. http://codereview.chromium.org/10553029/diff/44003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/44003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:339: DVLOG(6) << "one"; remove these before you're done.
Thanks Dominic for your reviews! A lot has gone on during this review. Dominic, I've tried hard to address your issues, and answer them where I don't. I notice you didn't LGTM my latest upload, could you please take a moment to highlight the areas you find questionable? I think I'd appreciate that, and so would mmenke. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:205: if (it->weak_prerender_handle.get() == prerender_handle) On 2012/07/03 20:02:41, dominich wrote: > On 2012/07/03 18:45:41, gavinp wrote: > > On 2012/07/03 17:08:39, dominich wrote: > > > i'm curious how this compiled before. it /should/ be ok to compare a > weak_ptr > > to > > > a raw pointer, but I don't know exactly what code the compiler would have > > > inserted to do that, given there's no overloaded operator== that I can see. > > > > See base::WeakPtr<T>::operator T*() { return get(); } > > > > The language allows one implicit conversion when searching for a function, so > it > > was calling that conversion to get a PrerenderHandle*, then using the default > > bool ::operator==(PrerenderHandle*, PrerenderHandle*). > > Aha! > > I do not like that one bit. However, it is not something to tackle here. I concur that it's out of scope. You might be surprised to see how distinct std::weak_ptr is from base::WeakPtr. Short answer? Very distinct.
http://codereview.chromium.org/10553029/diff/44003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/44003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:339: DVLOG(6) << "one"; On 2012/07/03 20:02:41, dominich wrote: > remove these before you're done. Done.
I've pulled to trunk (sorry for the spurious diffs coming from that..), but also deflaked some tests, by having the prerender_contents make sure pending prerenders die.
Aha, this is better. I uploaded my patches again, as two separate patches. Patchset 9 is the same as Patchset 8, except merged to trunk. So patchset 10 is my remediation & deflaking, you can diff from 9-10 to see my work without the merge.
I'm pretty happy with this (Though I have yet to wade through PrerenderWeakPtrFactoryMap and friends). Here are a bunch of mostly minor comments. I still need to carefully go over prerender_manager and prerender_contents. Feel free to either wait for more comments from me before doing anything, or respond in the meantime. I'm probably not going to continue reviewing this until tomorrow morning, as I find I'm much more likely to miss stuff in the afternoon. Sorry for the delay, hope to have finished the review by tomorrow afternoon. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:239: pending_prerender_vector()->size() == expected_pending_prerenders_) { On 2012/07/03 17:08:39, dominich wrote: > when your variable name includes the type, you're doing it wrong. > pending_prerenders() Hmm... "content::Referrer& referrer", "const gfx::Size& size"... (Though I agree that it's a bad idea in this case) http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:228: it->weak_prerender_handle.get(), ORIGIN_LINK_REL_PRERENDER, child_id_, On 2012/07/03 17:08:39, dominich wrote: > and again - how did this compile? It's actually pretty common to rely on the weak pointer to pointer cast operator in the Chromium code base. I think relying on it in a comparison is a *very* bad idea because of the NULL case, however. http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/39003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:36: PrerenderHandle* prerender_handle = it->second.get(); On 2012/07/03 17:08:39, dominich wrote: > the more i see this, the more worried i get. I don't think it should be valid > for the compiler to automatically degrade a weak pointer to a raw pointer. I tend to agree, but it's needed by the callback system, in come cases, I believe. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:167: prerender_handle_.reset(); I'd suggest doing the reset first. I believe it may be able to affect the math on the number of processes we have. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:32: class SessionStorageNamespace; nit: Don't indent. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:99: gfx::Size size); Hmm... size is passed by reference to prerender_link manager, but by value here (and in PrerenderManager). I'd recommend being consistent, unless there's a reason we need it by value here. Know by value's needed in one of the PrerenderManager cases. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:16: DCHECK(CalledOnValidThread()); nit: Not needed. NonThreadSafe's destructor does this. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:23: // launcher. Think a short comment about why we can't just use OnCancel here would be reasonable. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:38: NOTREACHED(); Use brackets around these conditional statement bodies. While this code is correct, using macros without them makes me uneasy. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:70: instance_count(1) { nit: Think this looks a little weird, and I haven't seen this indentation pattern used much on the Chrome code base. I'd suggest: PrerenderHandle::PrerenderData::PrerenderData( PrerenderManager* manager, PrerenderContents* contents) : manager(manager), contents(contents), instance_count(1) { Alternatively, can put the first argument on the same line as PrerenderData (And put all the initializers on the same line, if you want). And do something similar for the function above. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:8: #include <list> Not needed. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:11: #include "base/threading/non_thread_safe.h" base/basictypes.h should be here instead of in the cc files, since it's where DISALLOW_COPY_AND_ASSIGN is defined. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:26: ~PrerenderHandle(); Suggest a comment that this calls OnCancel automatically. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:36: // the handle. Suggest a comment that it does nothing if abandoned. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:41: bool IsValid() const { return prerender_data_ != NULL; } Don't inline this - style guide strongly discourages inlining anything other than simple getters/setters, and skimming over over files, tiny IsValid and is_valid functions (Both of which are about equally as common) look to be generally not inlined, despite being somewhat simple getter-ish. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:47: // True if this prerender is currently running. nit: "running" is ambiguous (Is an idle, loaded webpage "running", or what about an idle prerender that's waiting for other prerenders to go aware before starting (Not a current state, but could theoretically want it in the future)). Suggest "is currently active" or even "has an active PrerenderContents", though that might be exposing more details than we want to consumers. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:50: bool DidFinishLoading() const; Suggest IsFinishedLoading(). If a prerender finishes loading, but then is cancelled, it did finish loading, but DidFinishLoading returns false. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:65: DCHECK_EQ(0U, ids_to_handle_map_.count(child_and_prerender_id)); Suggest putting this DCHECK at the top of the function. You'd need to move the ChildAndPrerenderIdPair line as well, or include a copy of it in the DCHECK. I'd suggest the former, as it isn't exactly a heavy weight operation, though it may be more likely to be optimized out if you put it here...Either way, it doesn't really matter. Not exactly a heavy weight operation. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:82: IdPairToPrerenderHandleMap::iterator id_handle_iter = nit: Suggest id_to_handle_iter, just to be more consistent with the map's name. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:87: } Might want to put the below 4 lines into a function. It'd have to take a IdPairToPrerenderHandleMap::iterator, unfortunately, but I feel safer having only one function that does cleanup, when possible, to make sure all deletion code remains up to date. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:13: #include "base/gtest_prod_util.h" Don't think we need this. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:83: // Map giving, per child process, for each prerender_id, the PrerenderHandle. nit: prerender id (prerender_id is not a type, but rather an argument name to some methods). http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:83: // Map giving, per child process, for each prerender_id, the PrerenderHandle. nit: I find this sentence rather hard to follow. I'd suggest "A map from child process id and prerender id to PrerenderHandles. Prerender ids are only unique within each renderer process...." http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:10: #include <string> While you're here, set doesn't look to be used in this file, and string is included in the header file. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:41: class Size; Know this is pre-existing code, but since it's being passed as an argument to a function, should be included rather than forward declared. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:109: // Returns a caller owned PrerenderHandle* if the URL was added, NULL if it nit: believe this should be caller-owned here and below. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:124: // caller owned PrerenderHandle*. nit: or NULL. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:244: friend class PrerenderHandle; All friends should be together, in the private section. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:258: content::SessionStorageNamespace* session_storage_namespace); The other functions take Sizes by value rather than reference. Should either be: By value everywhere, or by reference except when needed to be by value. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:277: base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> nit: You typedefed PrerenderHandle::PrerenderData just one line up (x2). http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:290: // PrerenderHandle*, owned by the caller. Or NULL http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:331: PrerenderHandle::PrerenderData* prerender_data); nit: All of these should just be PrerenderData, unless you want to get rid of the typedef, or move it into the cc file (Any option is fine with me, just odd to create a typedef and then not use it). http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:343: // to the renderer process |child_id| and the render view in |route_id|. This comment could just as easily mean a prerender started by the given RenderView (Though that may not be unique, in the future, I still find it confusing). Suggest something along the lines of: "If |child_id| and |route_id| correspond to a RenderView that's currenly being prerendered, returns the PrerenderData object for that prerender. Otherwise, returns NULL." http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:349: // correponding to that running prerender. nit: "that running" -> "the given active" http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: current_host->GetSessionStorageNamespace(), container_bounds.size()); nit: Any reason current_host->GetSessionStorageNamespace() was moved onto the next line? match.destination_url is shorter than container_bounds.size(). http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: current_host->GetSessionStorageNamespace(), container_bounds.size()); I'll defer to Dominich on whether it makes sense to have the ActionPredirector decide if we want to prerender, the OmniboxEditModel trigger the prerender, and the ActionPredirector keep a handle to the prerender.
lgtm module mmenke remediation http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:99: gfx::Size size); On 2012/07/09 18:06:57, Matt Menke wrote: > Hmm... size is passed by reference to prerender_link manager, but by value here > (and in PrerenderManager). I'd recommend being consistent, unless there's a > reason we need it by value here. Know by value's needed in one of the > PrerenderManager cases. IIRC, it was done like this to show that it might change/be set by the method. I'd also prefer by value everywhere for consistency, but then users of the API don't actually need to know if it's being copied or referenced so maybe it doesn't matter. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:277: base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> On 2012/07/09 18:06:57, Matt Menke wrote: > nit: You typedefed PrerenderHandle::PrerenderData just one line up (x2). i asked for the typedef to be removed to make it clearer who the PrerenderData was owned by. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: current_host->GetSessionStorageNamespace(), container_bounds.size()); On 2012/07/09 18:06:57, Matt Menke wrote: > I'll defer to Dominich on whether it makes sense to have the ActionPredirector > decide if we want to prerender, the OmniboxEditModel trigger the prerender, and > the ActionPredirector keep a handle to the prerender. This makes sense and was written after discussion - it means that the OmniboxEditModel no longer has a dependence on Prerender, but only on the Predictor. It is then up to the Predictor to know about Prerender.
http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:167: prerender_handle_.reset(); On 2012/07/09 18:06:57, Matt Menke wrote: > I'd suggest doing the reset first. I believe it may be able to affect the math > on the number of processes we have. Forgot that we always delay deletion, so it probably doesn't actually matter. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool UrlIsPendingInPrerenderManager(const std::string& html_file) const { This name no longer seems accurate to me. Maybe just UrlIsPending, with a comment that it only checks TestPrerenderContents pending URLs? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:926: FINAL_STATUS_MANAGER_SHUTDOWN); Shouldn't FINAL_STATUS_MANAGER_SHUTDOWN return true in ShouldRenderPrerenderedPageCorrectly, rather than being here? How did you notice this, anyways? Have tests been failing? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1003: content::NotificationService::AllSources(), NULL, 1); It's generally considered a good idea to create the observer before starting the navigation. While the Javascript navigation should presumably not be racy,think it's best to play it safe. Also think that an ASSERT that the active prerender contents is not loading is in order (Just before we create the observer), since that would mess up the observer. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: << "session_storage_namespace != NULL, except in tests"; optional nit: I'd suggest you just do a: DCHECK(child_id_ == -1 || session_storage_namespace); outside the loop, for legibility and consistency with the corresponding DCHECK in prerender_manager.cc. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:221: pending_prerender_list.swap(pending_prerenders_); Is this needed, or are you just being safe (Or just preserving the old behavior)? Don't see how AddPendingPrerender could be called from StartPendingPrerender, unless there's a bug, since: 1) We remove it from the list of active prerenders prior to calling this function. 2) StartPendingPrerender should not synchronously trigger another prerender. I'm a little less confident of this, since I don't know much about how we spin up processes, but 1) should be more than sufficient. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:307: session_storage_namespace_ = session_storage_namespace; Erm...What guarantee do we have that we outlive this? It doesn't look like we have any, at least in the control group/dummy cases (And in other cases, things are convoluted enough that I'd rather not depend on it), so we retain this potentially completely random pointer to who-knows-what that we then compare with other pointers. I don't like the idea of owning a reference to the namespace, either. Not sure there's any nice, simple solution to this, unfortunately. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:439: } Since these are non-owning/weak pointers, I think it's a little weird to be doing this here. Is there a reason we don't just cancel prerenders when the parent process goes away, instead? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:288: size, session_storage_namespace)); IMPORTANT: AddPerender() can return NULL. Neither this function nor SwapPrerenderDataWith() has a NULL check. Fix that. Also probably a good idea to have a unit test for it. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:288: size, session_storage_namespace)); While this code is pretty straight forward, think this line is worthy of a comment due to its importance. Maybe something along the lines of "If |url| is already being prerendered, this will result creating another handle for the existing PrerenderData. If it's not being prerendered, this will start a new prerender (Or return NULL)." http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:298: ++it) { std::find would be a little simpler. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:527: active_prerender_list_.erase(it); I'd suggest replacing InvalidatePrerenderHandlesForPrerenderData(*it) with DeletePrerenderData(list_, *it), and have it do the erasing as well. I figure if two operations must always be done together or we could end up with a difficult to detect leak, it's best to couple them in a function, if possible. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1019: void PrerenderManager::InvalidatePrerenderHandlesForPrerenderData( I think that this is pretty ugly. My suggestion (And I'm completely open to push back/discussion on this) would be to make PrerenderData have a pointer to the factory. I'd also switch PrerenderData to being an inner class of PrerenderManager at the same time, since proper use of PrerenderData would then be more dependent on PrerenderManager than PrerenderHandle. My reasoning is basically just that the fewer redundant stl containers we have, the simpler things are. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:131: // status to them. This comment looks wrong to me. From both the old (And new) code and usage, it looks to me like this destroys the (necessarily unique) prerender that process_id, view_id identifies. The description makes it sound like it destroys all prerenders initiated by process_id, view_id. While you're here, mind fixing the comment? Maybe "Destroys the prerender using the specified RenderView..." http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:198: EXPECT_TRUE(next_prerender_contents_->origin() == origin); EXPECT_EQ? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:1010: base::TimeDelta::FromSeconds(1)); nit: Fix indent http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: current_host->GetSessionStorageNamespace(), container_bounds.size()); On 2012/07/09 18:25:43, dominich wrote: > On 2012/07/09 18:06:57, Matt Menke wrote: > > I'll defer to Dominich on whether it makes sense to have the ActionPredirector > > decide if we want to prerender, the OmniboxEditModel trigger the prerender, > and > > the ActionPredirector keep a handle to the prerender. > > This makes sense and was written after discussion - it means that the > OmniboxEditModel no longer has a dependence on Prerender, but only on the > Predictor. It is then up to the Predictor to know about Prerender. Thanks for the explanation, sounds reasonable. Just wanted to make sure someone had thought about it.
Thanks Matt for the thorough review, and especially for the potentially NULL pointer. What do you think of this version? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:167: prerender_handle_.reset(); On 2012/07/09 18:06:57, Matt Menke wrote: > I'd suggest doing the reset first. I believe it may be able to affect the math > on the number of processes we have. Done. It will definitely affect the final status. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:167: prerender_handle_.reset(); On 2012/07/10 18:01:13, Matt Menke wrote: > On 2012/07/09 18:06:57, Matt Menke wrote: > > I'd suggest doing the reset first. I believe it may be able to affect the > math > > on the number of processes we have. > > Forgot that we always delay deletion, so it probably doesn't actually matter. I think it does matter, since we'll be calling OnCancel which will call PrerenderContents::Destroy(). I think the final status changes from evicted to canceled, which is more proper. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:32: class SessionStorageNamespace; On 2012/07/09 18:06:57, Matt Menke wrote: > nit: Don't indent. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.h:99: gfx::Size size); On 2012/07/09 18:06:57, Matt Menke wrote: > Hmm... size is passed by reference to prerender_link manager, but by value here > (and in PrerenderManager). I'd recommend being consistent, unless there's a > reason we need it by value here. Know by value's needed in one of the > PrerenderManager cases. Done, I went with by reference everywhere. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: bool UrlIsPendingInPrerenderManager(const std::string& html_file) const { On 2012/07/10 18:01:13, Matt Menke wrote: > This name no longer seems accurate to me. Maybe just UrlIsPending, with a > comment that it only checks TestPrerenderContents pending URLs? Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:926: FINAL_STATUS_MANAGER_SHUTDOWN); On 2012/07/10 18:01:13, Matt Menke wrote: > Shouldn't FINAL_STATUS_MANAGER_SHUTDOWN return true in > ShouldRenderPrerenderedPageCorrectly, rather than being here? > > How did you notice this, anyways? Have tests been failing? I have no idea how I found this, or what I found. It must have been required at some weird intermediate state of my patch, but removing it made no tests change. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1003: content::NotificationService::AllSources(), NULL, 1); On 2012/07/10 18:01:13, Matt Menke wrote: > It's generally considered a good idea to create the observer before starting the > navigation. While the Javascript navigation should presumably not be racy,think > it's best to play it safe. Also think that an ASSERT that the active prerender > contents is not loading is in order (Just before we create the observer), since > that would mess up the observer. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:217: << "session_storage_namespace != NULL, except in tests"; On 2012/07/10 18:01:13, Matt Menke wrote: > optional nit: I'd suggest you just do a: > > DCHECK(child_id_ == -1 || session_storage_namespace); > > outside the loop, for legibility and consistency with the corresponding DCHECK > in prerender_manager.cc. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:221: pending_prerender_list.swap(pending_prerenders_); On 2012/07/10 18:01:13, Matt Menke wrote: > Is this needed, or are you just being safe (Or just preserving the old > behavior)? Don't see how AddPendingPrerender could be called from > StartPendingPrerender, unless there's a bug, since: I'm just preserving the behaviour. I agree that (1) is more than sufficient, but why not wear a belt _and_ suspenders? > > 1) We remove it from the list of active prerenders prior to calling this > function. > 2) StartPendingPrerender should not synchronously trigger another prerender. > I'm a little less confident of this, since I don't know much about how we spin > up processes, but 1) should be more than sufficient. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:439: } On 2012/07/10 18:01:13, Matt Menke wrote: > Since these are non-owning/weak pointers, I think it's a little weird to be > doing this here. Is there a reason we don't just cancel prerenders when the > parent process goes away, instead? No great reason. Without this, in destruction we'd sometimes hit this assertion: http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/3805... So, I've removed the assertion and this code. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:16: DCHECK(CalledOnValidThread()); On 2012/07/09 18:06:57, Matt Menke wrote: > nit: Not needed. NonThreadSafe's destructor does this. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:23: // launcher. On 2012/07/09 18:06:57, Matt Menke wrote: > Think a short comment about why we can't just use OnCancel here would be > reasonable. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:38: NOTREACHED(); On 2012/07/09 18:06:57, Matt Menke wrote: > Use brackets around these conditional statement bodies. While this code is > correct, using macros without them makes me uneasy. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:70: instance_count(1) { On 2012/07/09 18:06:57, Matt Menke wrote: > nit: Think this looks a little weird, and I haven't seen this indentation > pattern used much on the Chrome code base. I'd suggest: > > PrerenderHandle::PrerenderData::PrerenderData( > PrerenderManager* manager, > PrerenderContents* contents) > : manager(manager), > contents(contents), > instance_count(1) { > > Alternatively, can put the first argument on the same line as PrerenderData (And > put all the initializers on the same line, if you want). > > And do something similar for the function above. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:8: #include <list> On 2012/07/09 18:06:57, Matt Menke wrote: > Not needed. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:11: #include "base/threading/non_thread_safe.h" On 2012/07/09 18:06:57, Matt Menke wrote: > base/basictypes.h should be here instead of in the cc files, since it's where > DISALLOW_COPY_AND_ASSIGN is defined. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:26: ~PrerenderHandle(); On 2012/07/09 18:06:57, Matt Menke wrote: > Suggest a comment that this calls OnCancel automatically. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:36: // the handle. On 2012/07/09 18:06:57, Matt Menke wrote: > Suggest a comment that it does nothing if abandoned. I've tried hard to get rid of a lot of the abandoned terminology, it was confusing. I'm not sure what you're asking for. Can you expand on your request for me? http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:41: bool IsValid() const { return prerender_data_ != NULL; } On 2012/07/09 18:06:57, Matt Menke wrote: > Don't inline this - style guide strongly discourages inlining anything other > than simple getters/setters, and skimming over over files, tiny IsValid and > is_valid functions (Both of which are about equally as common) look to be > generally not inlined, despite being somewhat simple getter-ish. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:47: // True if this prerender is currently running. On 2012/07/09 18:06:57, Matt Menke wrote: > nit: "running" is ambiguous (Is an idle, loaded webpage "running", or what > about an idle prerender that's waiting for other prerenders to go aware before > starting (Not a current state, but could theoretically want it in the future)). > Suggest "is currently active" or even "has an active PrerenderContents", though > that might be exposing more details than we want to consumers. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:50: bool DidFinishLoading() const; On 2012/07/09 18:06:57, Matt Menke wrote: > Suggest IsFinishedLoading(). If a prerender finishes loading, but then is > cancelled, it did finish loading, but DidFinishLoading returns false. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:65: DCHECK_EQ(0U, ids_to_handle_map_.count(child_and_prerender_id)); On 2012/07/09 18:06:57, Matt Menke wrote: > Suggest putting this DCHECK at the top of the function. You'd need to move the > ChildAndPrerenderIdPair line as well, or include a copy of it in the DCHECK. > I'd suggest the former, as it isn't exactly a heavy weight operation, though it > may be more likely to be optimized out if you put it here...Either way, it > doesn't really matter. Not exactly a heavy weight operation. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:82: IdPairToPrerenderHandleMap::iterator id_handle_iter = On 2012/07/09 18:06:57, Matt Menke wrote: > nit: Suggest id_to_handle_iter, just to be more consistent with the map's name. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:87: } On 2012/07/09 18:06:57, Matt Menke wrote: > Might want to put the below 4 lines into a function. It'd have to take a > IdPairToPrerenderHandleMap::iterator, unfortunately, but I feel safer having > only one function that does cleanup, when possible, to make sure all deletion > code remains up to date. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:13: #include "base/gtest_prod_util.h" On 2012/07/09 18:06:57, Matt Menke wrote: > Don't think we need this. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:77: void RemovePrerender(const IdPairToPrerenderHandleMap::iterator& id_url_iter); Apparently, I was thinking about this function too. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.h:83: // Map giving, per child process, for each prerender_id, the PrerenderHandle. On 2012/07/09 18:06:57, Matt Menke wrote: > nit: I find this sentence rather hard to follow. I'd suggest "A map from child > process id and prerender id to PrerenderHandles. Prerender ids are only unique > within each renderer process...." Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:10: #include <string> On 2012/07/09 18:06:57, Matt Menke wrote: > While you're here, set doesn't look to be used in this file, and string is > included in the header file. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:288: size, session_storage_namespace)); On 2012/07/10 18:01:13, Matt Menke wrote: > While this code is pretty straight forward, think this line is worthy of a > comment due to its importance. Maybe something along the lines of "If |url| is > already being prerendered, this will result creating another handle for the > existing PrerenderData. If it's not being prerendered, this will start a new > prerender (Or return NULL)." Done. Very good catch, I'm glad you're reviewing this. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:298: ++it) { On 2012/07/10 18:01:13, Matt Menke wrote: > std::find would be a little simpler. It would. If only we had address_of().... But we don't. See http://codereview.chromium.org/10261004/ I don't believe we can use std::find without this function. Related rant: if we had std::compose(), I could replace about half of my loops with find and for_each. Alas. I also wouldn't mind nonstd::member_of(). However, it's probably best for all of us that we don't have these... http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:527: active_prerender_list_.erase(it); On 2012/07/10 18:01:13, Matt Menke wrote: > I'd suggest replacing InvalidatePrerenderHandlesForPrerenderData(*it) with > DeletePrerenderData(list_, *it), and have it do the erasing as well. I figure > if two operations must always be done together or we could end up with a > difficult to detect leak, it's best to couple them in a function, if possible. Done, but mooted if we go with the ptr in the struct (see below). http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:1019: void PrerenderManager::InvalidatePrerenderHandlesForPrerenderData( On 2012/07/10 18:01:13, Matt Menke wrote: > I think that this is pretty ugly. My suggestion (And I'm completely open to > push back/discussion on this) would be to make PrerenderData have a pointer to > the factory. I'd also switch PrerenderData to being an inner class of > PrerenderManager at the same time, since proper use of PrerenderData would then > be more dependent on PrerenderManager than PrerenderHandle. > > My reasoning is basically just that the fewer redundant stl containers we have, > the simpler things are. Done. Please look it over and let me know what you think about the struct and its podness. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:41: class Size; On 2012/07/09 18:06:57, Matt Menke wrote: > Know this is pre-existing code, but since it's being passed as an argument to a > function, should be included rather than forward declared. Mooted, as I went to the reference. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:109: // Returns a caller owned PrerenderHandle* if the URL was added, NULL if it On 2012/07/09 18:06:57, Matt Menke wrote: > nit: believe this should be caller-owned here and below. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:124: // caller owned PrerenderHandle*. On 2012/07/09 18:06:57, Matt Menke wrote: > nit: or NULL. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:131: // status to them. On 2012/07/10 18:01:13, Matt Menke wrote: > This comment looks wrong to me. From both the old (And new) code and usage, it > looks to me like this destroys the (necessarily unique) prerender that > process_id, view_id identifies. The description makes it sound like it destroys > all prerenders initiated by process_id, view_id. > > While you're here, mind fixing the comment? Maybe "Destroys the prerender using > the specified RenderView..." Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:244: friend class PrerenderHandle; On 2012/07/09 18:06:57, Matt Menke wrote: > All friends should be together, in the private section. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:258: content::SessionStorageNamespace* session_storage_namespace); On 2012/07/09 18:06:57, Matt Menke wrote: > The other functions take Sizes by value rather than reference. Should either > be: By value everywhere, or by reference except when needed to be by value. Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:277: base::WeakPtrFactory<PrerenderHandle::PrerenderData>*> On 2012/07/09 18:25:43, dominich wrote: > On 2012/07/09 18:06:57, Matt Menke wrote: > > nit: You typedefed PrerenderHandle::PrerenderData just one line up (x2). > > i asked for the typedef to be removed to make it clearer who the PrerenderData > was owned by. Yeah, sorry. My new upload gets rid of not only the users of the typedef, but the actual typedef too. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:290: // PrerenderHandle*, owned by the caller. On 2012/07/09 18:06:57, Matt Menke wrote: > Or NULL Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:331: PrerenderHandle::PrerenderData* prerender_data); On 2012/07/09 18:06:57, Matt Menke wrote: > nit: All of these should just be PrerenderData, unless you want to get rid of > the typedef, or move it into the cc file (Any option is fine with me, just odd > to create a typedef and then not use it). Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:343: // to the renderer process |child_id| and the render view in |route_id|. On 2012/07/09 18:06:57, Matt Menke wrote: > This comment could just as easily mean a prerender started by the given > RenderView (Though that may not be unique, in the future, I still find it > confusing). Suggest something along the lines of: "If |child_id| and > |route_id| correspond to a RenderView that's currenly being prerendered, returns > the PrerenderData object for that prerender. Otherwise, returns NULL." Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:349: // correponding to that running prerender. On 2012/07/09 18:06:57, Matt Menke wrote: > nit: "that running" -> "the given active" Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:198: EXPECT_TRUE(next_prerender_contents_->origin() == origin); On 2012/07/10 18:01:13, Matt Menke wrote: > EXPECT_EQ? Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:1010: base::TimeDelta::FromSeconds(1)); On 2012/07/10 18:01:13, Matt Menke wrote: > nit: Fix indent Done. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: current_host->GetSessionStorageNamespace(), container_bounds.size()); On 2012/07/09 18:06:57, Matt Menke wrote: > nit: Any reason current_host->GetSessionStorageNamespace() was moved onto the > next line? match.destination_url is shorter than container_bounds.size(). Done.
http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:244: if (size.IsEmpty()) { who's doing this check now? if size should never be empty, please CHECK as such. This was added by a recent jam change, iirc. check why he did it before removing.
http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:244: if (size.IsEmpty()) { On 2012/07/11 17:50:45, dominich wrote: > who's doing this check now? if size should never be empty, please CHECK as such. > This was added by a recent jam change, iirc. check why he did it before > removing. The size is added at PrerendererClient::willAddPrender(), and the only way the program counter would ever have entered this test was if RenderView::GetSize() was empty. And would we then, after having empty bounds on a WebView, expect the WebContentsView to have a non-empty container bounds? This code should have been removed from PrerenderContents in http://codereview.chromium.org/10244007/ , as part of the cleanup when making the new API, and it's to my shame that I didn't do that. We can DCHECK it's not empty. I'll add that, and fix the unit tests so they don't send in empty sizes, too.
I'm happy with this. Just nits, other than the session storage thing. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:36: // the handle. On 2012/07/11 17:04:00, gavinp wrote: > On 2012/07/09 18:06:57, Matt Menke wrote: > > Suggest a comment that it does nothing if abandoned. > > I've tried hard to get rid of a lot of the abandoned terminology, it was > confusing. I'm not sure what you're asking for. Can you expand on your request > for me? Sure. I just want it clear that "OnNavigateAway(); OnCancel();" does not, in fact, cancel. Could either say that explicity, or say it does nothing when the handle isn't valid. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:298: ++it) { On 2012/07/11 17:04:00, gavinp wrote: > On 2012/07/10 18:01:13, Matt Menke wrote: > > std::find would be a little simpler. > > It would. If only we had address_of().... But we don't. See > http://codereview.chromium.org/10261004/ > > I don't believe we can use std::find without this function. > > Related rant: if we had std::compose(), I could replace about half of my loops > with find and for_each. Alas. I also wouldn't mind nonstd::member_of(). However, > it's probably best for all of us that we don't have these... Right. For some reason, I was thinking the list was of PrerenderData*'s rather than PrerenderData's. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:304: session_storage_namespace_ = session_storage_namespace; On 2012/07/10 18:01:13, Matt Menke wrote: > Erm...What guarantee do we have that we outlive this? It doesn't look like we > have any, at least in the control group/dummy cases (And in other cases, things > are convoluted enough that I'd rather not depend on it), so we retain this > potentially completely random pointer to who-knows-what that we then compare > with other pointers. I don't like the idea of owning a reference to the > namespace, either. Not sure there's any nice, simple solution to this, > unfortunately. I'm still concerned about this, since it makes the code incorrect. It looks like there's a session_storage_namespace_.id() field that we can use instead, and then we won't have to worry about lifetime issues. It's unique across the process, and will never be reused until Chrome is restarted (Well, at least it will be millenia before the field overflows...). http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:57: PrerenderManager::PrerenderData* prerender_data); nit: May fit on one line now. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:755: : manager(manager), contents(contents), instance_count(1) { nit: Fix indent. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:788: delete new_prerender_handle; Just to avoid having to delete manually, which is a little more regression prone, I'd suggest storing it in a scoped_ptr, as before, and then explicitly doing a NULL check. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:246: PrerenderContents* contents); nit: May fit on a single line. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:253: const scoped_ptr<base::WeakPtrFactory<PrerenderData> > weak_ptr_factory; I don't believe this needs to be a scoped_ptr. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:347: int route_id); nit: May fit onto one line now.
http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:298: ++it) { On 2012/07/11 18:40:05, Matt Menke wrote: > On 2012/07/11 17:04:00, gavinp wrote: > > On 2012/07/10 18:01:13, Matt Menke wrote: > > > std::find would be a little simpler. > > > > It would. If only we had address_of().... But we don't. See > > http://codereview.chromium.org/10261004/ > > > > I don't believe we can use std::find without this function. > > > > Related rant: if we had std::compose(), I could replace about half of my loops > > with find and for_each. Alas. I also wouldn't mind nonstd::member_of(). > However, > > it's probably best for all of us that we don't have these... > > Right. For some reason, I was thinking the list was of PrerenderData*'s rather > than PrerenderData's. optional: May actually be a little cleaner to make it a non-copiable class and use pointers / delete, rather than make it POD.
So I finally was convinced to just keep a list of pointers. http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:36: // the handle. On 2012/07/11 18:40:05, Matt Menke wrote: > On 2012/07/11 17:04:00, gavinp wrote: > > On 2012/07/09 18:06:57, Matt Menke wrote: > > > Suggest a comment that it does nothing if abandoned. > > > > I've tried hard to get rid of a lot of the abandoned terminology, it was > > confusing. I'm not sure what you're asking for. Can you expand on your request > > for me? > > Sure. I just want it clear that "OnNavigateAway(); OnCancel();" does not, in > fact, cancel. Could either say that explicity, or say it does nothing when the > handle isn't valid. Done. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.cc:304: session_storage_namespace_ = session_storage_namespace; On 2012/07/11 18:40:05, Matt Menke wrote: > On 2012/07/10 18:01:13, Matt Menke wrote: > > Erm...What guarantee do we have that we outlive this? It doesn't look like we > > have any, at least in the control group/dummy cases (And in other cases, > things > > are convoluted enough that I'd rather not depend on it), so we retain this > > potentially completely random pointer to who-knows-what that we then compare > > with other pointers. I don't like the idea of owning a reference to the > > namespace, either. Not sure there's any nice, simple solution to this, > > unfortunately. > > I'm still concerned about this, since it makes the code incorrect. It looks > like there's a session_storage_namespace_.id() field that we can use instead, > and then we won't have to worry about lifetime issues. It's unique across the > process, and will never be reused until Chrome is restarted (Well, at least it > will be millenia before the field overflows...). Done. This was one of your most important comments in your previous review, and I should have remediated to it then. Thanks for repeating it. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:57: PrerenderManager::PrerenderData* prerender_data); On 2012/07/11 18:40:05, Matt Menke wrote: > nit: May fit on one line now. Done. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:57: PrerenderManager::PrerenderData* prerender_data); On 2012/07/11 18:40:05, Matt Menke wrote: > nit: May fit on one line now. Done. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:755: : manager(manager), contents(contents), instance_count(1) { On 2012/07/11 18:40:05, Matt Menke wrote: > nit: Fix indent. Wait. How come this doesn't crash with a null weak_ptr_factory? There are two reasons. Firstly, we didn't have any unit or browser tests exercising the duplicate logic (which needs a handle), and the PrerenderData in the list is built using the copy constructor, so it had one. Subtle. I'm sold on the list of ptrs now. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:788: delete new_prerender_handle; On 2012/07/11 18:40:05, Matt Menke wrote: > Just to avoid having to delete manually, which is a little more regression > prone, I'd suggest storing it in a scoped_ptr, as before, and then explicitly > doing a NULL check. Done. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:246: PrerenderContents* contents); On 2012/07/11 18:40:05, Matt Menke wrote: > nit: May fit on a single line. Done. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:253: const scoped_ptr<base::WeakPtrFactory<PrerenderData> > weak_ptr_factory; On 2012/07/11 18:40:05, Matt Menke wrote: > I don't believe this needs to be a scoped_ptr. Mooted once I gave up on the lists. http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:347: int route_id); On 2012/07/11 18:40:05, Matt Menke wrote: > nit: May fit onto one line now. Done.
http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/34046/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:755: : manager(manager), contents(contents), instance_count(1) { On 2012/07/11 21:19:30, gavinp wrote: > On 2012/07/11 18:40:05, Matt Menke wrote: > > nit: Fix indent. > > Wait. How come this doesn't crash with a null weak_ptr_factory? There are two > reasons. Firstly, we didn't have any unit or browser tests exercising the > duplicate logic (which needs a handle), and the PrerenderData in the list is > built using the copy constructor, so it had one. > > Subtle. > > I'm sold on the list of ptrs now. Gah...How in the world did I miss that?
http://codereview.chromium.org/10553029/diff/28037/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/28037/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:226: new PrerenderHandle(new PrerenderData(this)); Wait...Who owns the new PrerenderData? If the prerender isn't used, it'll be abandoned due to the channel closing, and leaked, looks like. Other cases look correct to me (prerender used and prerender successfully started/fails to start).
http://codereview.chromium.org/10553029/diff/28037/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/28037/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:226: new PrerenderHandle(new PrerenderData(this)); On 2012/07/11 21:44:13, Matt Menke wrote: > Wait...Who owns the new PrerenderData? If the prerender isn't used, it'll be > abandoned due to the channel closing, and leaked, looks like. Now fixed; added OnNavigateAway to the PrerenderData. > > Other cases look correct to me (prerender used and prerender successfully > started/fails to start).
And here's a better fix. I decided to just keep the pending_prerender_list_ around, it definitively has ownership, and uses a linked_ptr so that is a strong assertion. Mixing them in to the main prerender_list_ was a bit too confusing: many iterations over active prerenders gain annoying special cases.
http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:161: prerender_handle_.reset(); I'm not a fan of the double reset. Would something like: PrerenderHandle* handle = NULL; if (... GetForProfile(...)) { handle = prerender_manager->AddPrerenderFromOmnibox(...); } prerender_handle_.reset(handle); satisfy mmenke and gavinp? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: // This only checks to see if the URL is pending in our TestPrerenderContents. nit: This comment doesn't tell me anything I can't see from the code - why is it important? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:165: const content::SessionStorageNamespace* session_storage_namespace); why is this method no longer const? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:265: const std::vector<PendingPrerenderInfo>* pending_prerenders() const { we could return a const reference here instead, right? That would make more sense to me, given it can't be NULL. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:18: DCHECK(CalledOnValidThread()); worth a DCHECK(IsValid());? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:26: DCHECK(CalledOnValidThread()); worth a DCHECK(IsValid());? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:51: bool IsFinishedLoading() const; nit: comment for this method. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:66: ids_to_handle_map_.insert( In release, there might already be a child_id/prerender_id pair in the map. In that case insert would return a pair with second set to false - is it still correct to return true in that case? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:132: DCHECK(prerender_handle); why is this DCHECK important? It's legal to delete NULL, and you probably want to DCHECK when adding the prerender to the map instead of when removing it. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:763: DCHECK_EQ(1, handle_count_); I don't understand why the handle can only be 1 in this case, but can be arbitrary when canceling. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:771: void PrerenderManager::PrerenderData::OnCancel() { DCHECK(handle_count_ >= 0); ? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:428: std::list<linked_ptr<PrerenderData> > active_prerender_list_; are you using linked_ptr as ownership annotation here, or do you need refcounting semantics?
Still looking over it. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:161: prerender_handle_.reset(); On 2012/07/12 16:16:48, dominich wrote: > I'm not a fan of the double reset. Would something like: > > PrerenderHandle* handle = NULL; > if (... GetForProfile(...)) { > handle = prerender_manager->AddPrerenderFromOmnibox(...); > } > prerender_handle_.reset(handle); > > satisfy mmenke and gavinp? This affects the final status. Resetting the old prerender handle results in a cancellation, while swapping will result in an eviction. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:744: // This only checks to see if the URL is pending in our TestPrerenderContents. On 2012/07/12 16:16:48, dominich wrote: > nit: This comment doesn't tell me anything I can't see from the code - why is it > important? Because it draws attention to the fact, for when support for multiple prerenders is added, or when thinking about potential pending prerender PrerenderData leaks. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:999: // observe one navigation. Hmm...Looks like we don't have a guarantee that the initial navigation (For the page with the link rel=prerender tag on it) has issued its DidStopLoading event before we call this. We only ensure that the prerendered page's DidStopLoading event has occurred, unless I'm missing something. While this doesn't currently cause any issues - the link rel element is below the javascript functions, and we wait for the prerender contents destruction, rather than any navigation event - the test could become flaky with this change. I suppose spinning up a process and waiting for it's DidStopLoadingEvent is generally much slower than waiting for a page that's almost completely parsed already to finish), but best to avoid a race. The fact that the old code didn't have this issue may just be do to luck. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:331: // The session storage namespace id for use in Matching. We must save it nit: matching http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:260: private: nit: Fix indent.
http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:999: // observe one navigation. On 2012/07/12 16:26:51, Matt Menke wrote: > Hmm...Looks like we don't have a guarantee that the initial navigation (For the > page with the link rel=prerender tag on it) has issued its DidStopLoading event > before we call this. We only ensure that the prerendered page's DidStopLoading > event has occurred, unless I'm missing something. > > While this doesn't currently cause any issues - the link rel element is below > the javascript functions, and we wait for the prerender contents destruction, > rather than any navigation event - the test could become flaky with this change. > I suppose spinning up a process and waiting for it's DidStopLoadingEvent is > generally much slower than waiting for a page that's almost completely parsed > already to finish), but best to avoid a race. The fact that the old code didn't > have this issue may just be do to luck. Bonus challenge: How many errors can you spot in the above comment? Let's see... the test could become -> tests could be it's -> it do -> due
On Thu, Jul 12, 2012 at 9:26 AM, <mmenke@chromium.org> wrote: > Still looking over it. > > > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/predictors/**autocomplete_action_predictor.**cc<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/autocomplete_action_predictor.cc> > File chrome/browser/predictors/**autocomplete_action_predictor.**cc > (right): > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/predictors/**autocomplete_action_predictor.**cc#newcode161<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/autocomplete_action_predictor.cc#newcode161> > chrome/browser/predictors/**autocomplete_action_predictor.**cc:161: > prerender_handle_.reset(); > On 2012/07/12 16:16:48, dominich wrote: > >> I'm not a fan of the double reset. Would something like: >> > > PrerenderHandle* handle = NULL; >> if (... GetForProfile(...)) { >> handle = prerender_manager->**AddPrerenderFromOmnibox(...); >> } >> prerender_handle_.reset(**handle); >> > > satisfy mmenke and gavinp? >> > > This affects the final status. Resetting the old prerender handle > results in a cancellation, while swapping will result in an eviction. That probably should be in a comment then as some well-meaning hacker might well refactor this code and miss that subtlety. > > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_browsertest.cc<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_browsertest.cc> > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_browsertest.cc#**newcode744<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_browsertest.cc#newcode744> > chrome/browser/prerender/**prerender_browsertest.cc:744: // This only > checks to see if the URL is pending in our TestPrerenderContents. > On 2012/07/12 16:16:48, dominich wrote: > >> nit: This comment doesn't tell me anything I can't see from the code - >> > why is it > >> important? >> > > Because it draws attention to the fact, for when support for multiple > prerenders is added, or when thinking about potential pending prerender > PrerenderData leaks. > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_browsertest.cc#**newcode999<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_browsertest.cc#newcode999> > chrome/browser/prerender/**prerender_browsertest.cc:999: // observe one > navigation. > Hmm...Looks like we don't have a guarantee that the initial navigation > (For the page with the link rel=prerender tag on it) has issued its > DidStopLoading event before we call this. We only ensure that the > prerendered page's DidStopLoading event has occurred, unless I'm missing > something. > > While this doesn't currently cause any issues - the link rel element is > below the javascript functions, and we wait for the prerender contents > destruction, rather than any navigation event - the test could become > flaky with this change. I suppose spinning up a process and waiting for > it's DidStopLoadingEvent is generally much slower than waiting for a > page that's almost completely parsed already to finish), but best to > avoid a race. The fact that the old code didn't have this issue may > just be do to luck. > > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_contents.h<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_contents.h> > File chrome/browser/prerender/**prerender_contents.h (right): > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_contents.h#**newcode331<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_contents.h#newcode331> > chrome/browser/prerender/**prerender_contents.h:331: // The session > storage namespace id for use in Matching. We must save it > nit: matching > > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_manager.h<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_manager.h> > File chrome/browser/prerender/**prerender_manager.h (right): > > http://codereview.chromium.**org/10553029/diff/46013/** > chrome/browser/prerender/**prerender_manager.h#newcode260<http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/prerender_manager.h#newcode260> > chrome/browser/prerender/**prerender_manager.h:260: private: > nit: Fix indent. > > http://codereview.chromium.**org/10553029/<http://codereview.chromium.org/105... >
http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:992: FINAL_STATUS_APP_TERMINATING) { Should we be using prerender_contents->quit_message_loop_on_destruction_ here instead? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1001: content::NotificationService::AllSources(), NULL, 1); Think we should use a WindowedNotificationObserver instead of the TestNavigationObserver - The windowed one will work if the event it's watching for happens before we have it wait, so is less flake-prone. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:201: PrerenderManager::~PrerenderManager() { If we can ensure PrerenderLinkManager is destroyed before PrerenderManager, might be nice to make sure there are no pending prerenders left with a DCHECK. Not needed in this CL. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:814: new_prerender_handle.get()); nit: may fit on one line. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:256: void OnCancel(); Think these are worth commenting on what they do. The fact that OnNavigateAway destroys PrerenderData for pending prerenders while letting active prerenders just time out, in particular, is non-obvious, as is the fact that "Cancel" means just one PrerenderHandle is being "cancelled". http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:265: int handle_count_; Suggest a comment for this, too. Something along the lines of "Number of PrerenderHandles created for |this|, including ones that have NavigatedAway, and excluding Cancelled ones. Will always be 1 for pending prerenders, as they can only merged with other prerender requests when they're started".
Thanks everyone. How close is this? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:161: prerender_handle_.reset(); Added a comment explaining why the reset first is important. On 2012/07/12 16:26:51, Matt Menke wrote: > On 2012/07/12 16:16:48, dominich wrote: > > I'm not a fan of the double reset. Would something like: > > > > PrerenderHandle* handle = NULL; > > if (... GetForProfile(...)) { > > handle = prerender_manager->AddPrerenderFromOmnibox(...); > > } > > prerender_handle_.reset(handle); > > > > satisfy mmenke and gavinp? > > This affects the final status. Resetting the old prerender handle results in a > cancellation, while swapping will result in an eviction. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:992: FINAL_STATUS_APP_TERMINATING) { On 2012/07/12 17:23:21, Matt Menke wrote: > Should we be using prerender_contents->quit_message_loop_on_destruction_ here > instead? Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1001: content::NotificationService::AllSources(), NULL, 1); On 2012/07/12 17:23:21, Matt Menke wrote: > Think we should use a WindowedNotificationObserver instead of the > TestNavigationObserver - The windowed one will work if the event it's watching > for happens before we have it wait, so is less flake-prone. That turned out to be tricky: because some JS navigations go cross process, and others don't... We could in theory listen for the new renderer, then start listening for navigation, but that's pretty ornate. Instead, up in PrerenderTestURLImpl, I use a WindowedNoticationObserver to wait for the launching page to complete; that makes the stop event we look for here unambiguous, I think. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:165: const content::SessionStorageNamespace* session_storage_namespace); No good reason. Fixed. On 2012/07/12 16:16:48, dominich wrote: > why is this method no longer const? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:265: const std::vector<PendingPrerenderInfo>* pending_prerenders() const { On 2012/07/12 16:16:48, dominich wrote: > we could return a const reference here instead, right? That would make more > sense to me, given it can't be NULL. Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_contents.h:331: // The session storage namespace id for use in Matching. We must save it On 2012/07/12 16:26:51, Matt Menke wrote: > nit: matching Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:26: DCHECK(CalledOnValidThread()); On 2012/07/12 16:16:48, dominich wrote: > worth a DCHECK(IsValid());? Done, and I slightly changed the semantics around these handles: you now must call ONE OF OnCancel() or OnNavigateAway() before destruction, and you can't call these methods twice. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:51: bool IsFinishedLoading() const; On 2012/07/12 16:16:48, dominich wrote: > nit: comment for this method. Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:66: ids_to_handle_map_.insert( On 2012/07/12 16:16:48, dominich wrote: > In release, there might already be a child_id/prerender_id pair in the map. In > that case insert would return a pair with second set to false - is it still > correct to return true in that case? In practice this should never occur, since in WebCore::LinkLoader and WebCore::Prerenderer we get a unique prerender_id for each call to add. I think we should DCHECK that case, cancel the prerender that's already there, and delete the handle. Unless child_ids are ever re-used? http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:132: DCHECK(prerender_handle); On 2012/07/12 16:16:48, dominich wrote: > why is this DCHECK important? It's legal to delete NULL, and you probably want > to DCHECK when adding the prerender to the map instead of when removing it. Removed the DCHECK. You're right, it was silly. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:763: DCHECK_EQ(1, handle_count_); On 2012/07/12 16:16:48, dominich wrote: > I don't understand why the handle can only be 1 in this case, but can be > arbitrary when canceling. Ah, it can't. This was an assertion about the handle_count_ when doing a NavigateAway for a pending prerender. I have added another DCHECK about this in the cancel case, so it's more clear the parallelism. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:771: void PrerenderManager::PrerenderData::OnCancel() { On 2012/07/12 16:16:48, dominich wrote: > DCHECK(handle_count_ >= 0); ? Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:814: new_prerender_handle.get()); On 2012/07/12 17:23:21, Matt Menke wrote: > nit: may fit on one line. Not unless I rename the variable to |existing_prerender_hndle| . http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:256: void OnCancel(); On 2012/07/12 17:23:21, Matt Menke wrote: > Think these are worth commenting on what they do. The fact that OnNavigateAway > destroys PrerenderData for pending prerenders while letting active prerenders > just time out, in particular, is non-obvious, as is the fact that "Cancel" means > just one PrerenderHandle is being "cancelled". I'm changing their names to make it more clear that these calls are per handle, and adding comments. I avoided describing the current exact semantics; I think they'll change quickly as we start extending prerendering... Aside: Part of why I wanted this handle was to make isolating such changes from callers easy and practical for this experimental period. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:260: private: On 2012/07/12 16:26:51, Matt Menke wrote: > nit: Fix indent. Done. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.h:428: std::list<linked_ptr<PrerenderData> > active_prerender_list_; On 2012/07/12 16:16:48, dominich wrote: > are you using linked_ptr as ownership annotation here, or do you need > refcounting semantics? Ownership annotation, and to save the deleting iteration in the destructor.
LGTM Comment for a separate (small) CL: I just noticed that we don't register PrerenderLinkManager in ProfileDependencyManager. I suggest we do (I had thought all ProfileKeyedServiceFactories had to do this, actually...) and have the PrerenderLinkManager depend on the PrerenderManager. Main reason for the dependency is to ensure consistent shutdown order. Ideally, shouldn't matter, but best to be consistent, in case it ever does matter, so we don't have things break in response to changes in the order in which ProfileDependencyManager deletes things. http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/46013/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:814: new_prerender_handle.get()); On 2012/07/13 12:02:22, gavinp wrote: > On 2012/07/12 17:23:21, Matt Menke wrote: > > nit: may fit on one line. > > Not unless I rename the variable to |existing_prerender_hndle| . Was that before or after you renamed the handle, adding a character? :) http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:298: bool WillQuitMessageLoopOnDestruction() const { nit: quit_message_loop_on_destruction() http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:894: // We construct this launch_nav_observer so that we can be certain our nit: "this WindowedNotificationObserver" or "launch_nav_observer". http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:912: loader_nav_observer.Wait(); Suggest a comment that this must be after the ui_test_utils::RunMessageLoop(); line, since otherwise the prerender's DidStopLoading event could end up exiting the loader_nav_observer's message loop. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1016: string16(), ASCIIToUTF16(javascript_function_name)); nit: I was wrong about creating the observer first, was thinking of the Windowed observer. TestNavigationObserver isn't that smart. I'd suggest just moving this line back up outside the loop, so you only need one copy of the line. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:25: // PrerenderHandle* to the client (or NULL if they are unable to start a lame joke: "unable or unwilling" http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:38: delete prerender_handle; Do we have a unit test that exercises this code? Only runs when we're destroying the PrerenderManager, too, so not much to worry about in terms of leaks, but nice to make sure we don't DCHECK.
LGTM some nits, a conversation starter, but no blocking issues. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:165: if (prerender::PrerenderManager* prerender_manager = This is much nicer. Explicit intent FTW. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:22: if (!IsValid()) I like this a lot. I wonder if we should be stricter about it - DCHECK to ensure people aren't calling it twice. A code path that calls both OnNavigateAway and OnCancel might expect the second call to override the first call, like setting the final status.
Thanks Dominic and Matt for being so generous with your time. This is the version I plan to CQ+. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/predictors/... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/predictors/... chrome/browser/predictors/autocomplete_action_predictor.cc:165: if (prerender::PrerenderManager* prerender_manager = On 2012/07/13 15:01:43, dominich wrote: > This is much nicer. Explicit intent FTW. I wonder, shouldn't we be using OnCancel here: we will end up evicting a lot of prerenders. I'm torn. And then perhaps OnNavigateAway() when we use it? This isn't vital to get right now, but the multiple prerenders CL needs to get it right. And this code duplicates our current functionality, which is a massive virtue. So let's stick with it for now. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:298: bool WillQuitMessageLoopOnDestruction() const { On 2012/07/13 14:49:10, Matt Menke wrote: > nit: quit_message_loop_on_destruction() Done, though I avoided that as I felt the Will helped documentation. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:894: // We construct this launch_nav_observer so that we can be certain our On 2012/07/13 14:49:10, Matt Menke wrote: > nit: "this WindowedNotificationObserver" or "launch_nav_observer". Done. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:912: loader_nav_observer.Wait(); On 2012/07/13 14:49:10, Matt Menke wrote: > Suggest a comment that this must be after the ui_test_utils::RunMessageLoop(); > line, since otherwise the prerender's DidStopLoading event could end up exiting > the loader_nav_observer's message loop. Added. But is that really true? The prerender is on a different web contents, so we're not going to get notifications from it, are we? I think what we're guarding against here is the prerender finishing and exiting the message loop redundantly? http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:1016: string16(), ASCIIToUTF16(javascript_function_name)); On 2012/07/13 14:49:10, Matt Menke wrote: > nit: I was wrong about creating the observer first, was thinking of the > Windowed observer. TestNavigationObserver isn't that smart. I'd suggest just > moving this line back up outside the loop, so you only need one copy of the > line. Done. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.cc:22: if (!IsValid()) On 2012/07/13 15:01:43, dominich wrote: > I like this a lot. I wonder if we should be stricter about it - DCHECK to ensure > people aren't calling it twice. A code path that calls both OnNavigateAway and > OnCancel might expect the second call to override the first call, like setting > the final status. I had the same thought, and yesterday had that coded up in my repo. I moved away from it, because of course, PrerenderHandles can become invalid behind your back too (say the system comes under memory pressure). Anywhere that prerenders were being canceled or navigated away from, I had to include this check just in case of that. Same thing in the call to OnCancel or OnNavigateAway in destructors. It felt like our API was doing the wrong thing: taking our objects complexity and pushing it out into the users. I do think that requiring an explicit choice about NavigateAway or Cancel before deletion is good though. Neither default made me happy. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_handle.h (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_handle.h:25: // PrerenderHandle* to the client (or NULL if they are unable to start a On 2012/07/13 14:49:11, Matt Menke wrote: > lame joke: "unable or unwilling" I think you accurately labeled that joke. http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:38: delete prerender_handle; On 2012/07/13 14:49:11, Matt Menke wrote: > Do we have a unit test that exercises this code? Only runs when we're > destroying the PrerenderManager, too, so not much to worry about in terms of > leaks, but nice to make sure we don't DCHECK. In the unit test destructor, we call OnChannelClosing. Without this code, that causes some of the tests to fail due to MANAGER_SHUTDOWN. I think that the CL that gets profile dependency right can let us remove the OnChannelClosing, and then we'll use this code in every unit test. If you remove the OnChannelClosing call _and_ this code, the unit test leaks about 32 handles.
http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... chrome/browser/prerender/prerender_browsertest.cc:912: loader_nav_observer.Wait(); On 2012/07/13 16:42:43, gavinp wrote: > On 2012/07/13 14:49:10, Matt Menke wrote: > > Suggest a comment that this must be after the ui_test_utils::RunMessageLoop(); > > line, since otherwise the prerender's DidStopLoading event could end up > exiting > > the loader_nav_observer's message loop. > > Added. But is that really true? The prerender is on a different web contents, so > we're not going to get notifications from it, are we? I think what we're > guarding against here is the prerender finishing and exiting the message loop > redundantly? The problem isn't that we could get its message, it's that its exit call would mess things up. Consider this: load_nav_observer starts waiting. PrerenderContents is destroyed, tries to exit message loop. First load completes, tries to exit message loop. (This can happen since it uses ::Quit instead of ::QuitNow. Order could also be reversed). We exit message loop. We call RunMessageLoop. We hang forever. Alternatively: load_nav_observer starts waiting. PrerenderContents is destroyed, tries to exit message loop. We exit message loop. loader_nav_observer notices it didn't exit the message loop. It is sad. :(
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10553029/34057
On 2012/07/13 17:01:29, Matt Menke wrote: > http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > http://codereview.chromium.org/10553029/diff/39041/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_browsertest.cc:912: > loader_nav_observer.Wait(); > On 2012/07/13 16:42:43, gavinp wrote: > > On 2012/07/13 14:49:10, Matt Menke wrote: > > > Suggest a comment that this must be after the > ui_test_utils::RunMessageLoop(); > > > line, since otherwise the prerender's DidStopLoading event could end up > > exiting > > > the loader_nav_observer's message loop. > > > > Added. But is that really true? The prerender is on a different web contents, > so > > we're not going to get notifications from it, are we? I think what we're > > guarding against here is the prerender finishing and exiting the message loop > > redundantly? > > The problem isn't that we could get its message, it's that its exit call would > mess things up. Consider this: > > load_nav_observer starts waiting. > PrerenderContents is destroyed, tries to exit message loop. > First load completes, tries to exit message loop. (This can happen since it > uses ::Quit instead of ::QuitNow. Order could also be reversed). > We exit message loop. > We call RunMessageLoop. > We hang forever. > > Alternatively: > > load_nav_observer starts waiting. > PrerenderContents is destroyed, tries to exit message loop. > We exit message loop. > loader_nav_observer notices it didn't exit the message loop. It is sad. :( Aha. I buy both of those scenarios, and now I see why you want to wrap our exit-the-loop logic with the windowed observer. Thanks for expanding on it!
Presubmit check for 10553029-34057 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
If this change requires manual test instructions to QA team, add
TEST=[instructions].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/omnibox
Presubmit checks took 1.8s to calculate.
sky, what do you think of the changes to omnibox_edit_model.cc?
-sky +pkasting (sky is on vacation). pkasting, what do you think of the changes to omnibox_edit_model.cc?
LGTM http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1114: if (TabContents* tab = controller_->GetTabContents()) { Nit: Don't combine set + test like this. The old code that first set |tab|, then checked it, was the right wayu to go. (I also slightly preferred the early return of the old code.) http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1117: content::RenderViewHost* current_host = Nit: Could just inline these next two temps into the following statement... not sure having them separate buys a ton http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1121: Nit: Unnecessary blank line
Thanks pkasting for a quick review! I do think I disagree about what the style guide says about set and test though. I would like to resolve that. I did check to see if we could lose some of the funny line wrapping if we got rid of the if(), but we don't really gain any line joins. If we did then I would have had an easy way out... :/ http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1114: if (TabContents* tab = controller_->GetTabContents()) { On 2012/07/13 19:35:09, Peter Kasting wrote: > Nit: Don't combine set + test like this. The old code that first set |tab|, > then checked it, was the right wayu to go. (I also slightly preferred the early > return of the old code.) I can definitely change this if there's consensus, but I think http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... suggests this is a slightly preferred formulation for our style. I know I've had conversations with brettw and jar about its usage before. Should we resolve this or let it go by? http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1117: content::RenderViewHost* current_host = On 2012/07/13 19:35:09, Peter Kasting wrote: > Nit: Could just inline these next two temps into the following statement... not > sure having them separate buys a ton Done. http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1121: On 2012/07/13 19:35:09, Peter Kasting wrote: > Nit: Unnecessary blank line Done.
http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/34057/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1114: if (TabContents* tab = controller_->GetTabContents()) { On 2012/07/13 19:59:10, gavinp wrote: > On 2012/07/13 19:35:09, Peter Kasting wrote: > > Nit: Don't combine set + test like this. The old code that first set |tab|, > > then checked it, was the right wayu to go. (I also slightly preferred the > early > > return of the old code.) > > I can definitely change this if there's consensus, but I think > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... > suggests this is a slightly preferred formulation for our style. I know I've had > conversations with brettw and jar about its usage before. > > Should we resolve this or let it go by? I haven't seen this used in Chromium code and I really dislike it. I think the Google style guide example in the while() loop is borderline, but arguably OK in that this is effectively a re-written for() loop, where having a loop-scoped variable is common. Doing it in an if() statement on the other hand just asks to be misread. If I saw this in one of my readability reviews I'd ask the author to fix it.
Thanks again everyone! And thanks Peter for jumping in late. I think without the local declaration, the omnibox code ended up prettier because it lost the indent. So thanks for pushing on that. http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_manager.cc:840: DCHECK_GE(1, it->get()->handle_count_); This DCHECK was failing in the NavigateAway case; it showed up in try jobs. http://codereview.chromium.org/10553029/diff/52001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/10553029/diff/52001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1128: StartPrerendering(match.destination_url, I think this did end up looking a lot better after losing the indent, with match.destination_url moving up a line.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/10553029/52001
Change committed as 146735
It bounced out of the tree (twice) today, due to some amazing fun with std::pair::pair on some of our windows builders. http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, null)); The windows try bots were OK with this, but something is different in some windows builders, and the pair type constructor will not accept NULL, or even implicit_cast<PrererenderHandle*>(NULL). I suppose implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that kinda makes me sick.
http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, null)); On 2012/07/14 21:15:33, gavinp wrote: > The windows try bots were OK with this, but something is different in some > windows builders, and the pair type constructor will not accept NULL, or even > implicit_cast<PrererenderHandle*>(NULL). I suppose > implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that kinda > makes me sick. Did you try std::make_pair?
http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_unittest.cc (right): http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... chrome/browser/prerender/prerender_unittest.cc:1072: prerender_manager()->FindAndUseEntry(url)); During its brief time in the tree, valgrind correctly identified this as a leak. http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:11: #include "base/basictypes.h" Whoops. I'll fix this shortly. http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, null)); On 2012/07/14 21:53:34, Peter Kasting wrote: > On 2012/07/14 21:15:33, gavinp wrote: > > The windows try bots were OK with this, but something is different in some > > windows builders, and the pair type constructor will not accept NULL, or even > > implicit_cast<PrererenderHandle*>(NULL). I suppose > > implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that kinda > > makes me sick. > > Did you try std::make_pair? Yes, with that it won't compile in clang or gcc. I've had very bad luck with std::make_pair() where there's NULLs involved. Probably coincidentally, the instance of NULL is like this: std::make_pair(foo, static_cast<Bar*>(NULL)); Which works too. Would you prefer that? I don't like using static_cast<>, but I also don't like "null".
On 2012/07/14 22:12:29, gavinp wrote: > http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_unittest.cc (right): > > http://codereview.chromium.org/10553029/diff/52001/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_unittest.cc:1072: > prerender_manager()->FindAndUseEntry(url)); > During its brief time in the tree, valgrind correctly identified this as a leak. > > http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_link_manager.cc (right): > > http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:11: #include > "base/basictypes.h" > Whoops. I'll fix this shortly. > > http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, > null)); > On 2012/07/14 21:53:34, Peter Kasting wrote: > > On 2012/07/14 21:15:33, gavinp wrote: > > > The windows try bots were OK with this, but something is different in some > > > windows builders, and the pair type constructor will not accept NULL, or > even > > > implicit_cast<PrererenderHandle*>(NULL). I suppose > > > implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that > kinda > > > makes me sick. > > > > Did you try std::make_pair? > > Yes, with that it won't compile in clang or gcc. I've had very bad luck with > std::make_pair() where there's NULLs involved. ... This got mangled, I meant to also say: "There is only one NULL in make_pair in the chrome code base." > Probably coincidentally, the > instance of NULL is like this: > > std::make_pair(foo, static_cast<Bar*>(NULL)); > > Which works too. Would you prefer that? I don't like using static_cast<>, but I > also don't like "null".
http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, null)); On 2012/07/14 22:12:30, gavinp wrote: > On 2012/07/14 21:53:34, Peter Kasting wrote: > > On 2012/07/14 21:15:33, gavinp wrote: > > > The windows try bots were OK with this, but something is different in some > > > windows builders, and the pair type constructor will not accept NULL, or > even > > > implicit_cast<PrererenderHandle*>(NULL). I suppose > > > implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that > kinda > > > makes me sick. > > > > Did you try std::make_pair? > > Yes, with that it won't compile in clang or gcc. I've had very bad luck with > std::make_pair() where there's NULLs involved. Probably coincidentally, the > instance of NULL is like this: > > std::make_pair(foo, static_cast<Bar*>(NULL)); > > Which works too. Would you prefer that? I don't like using static_cast<>, but I > also don't like "null". That seems slightly better to me. We have to static_cast NULL in some other places too. I wonder if C++11's nullptr would help here...
On 2012/07/15 00:50:15, Peter Kasting wrote: > http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... > File chrome/browser/prerender/prerender_link_manager.cc (right): > > http://codereview.chromium.org/10553029/diff/54003/chrome/browser/prerender/p... > chrome/browser/prerender/prerender_link_manager.cc:72: child_and_prerender_id, > null)); > On 2012/07/14 22:12:30, gavinp wrote: > > On 2012/07/14 21:53:34, Peter Kasting wrote: > > > On 2012/07/14 21:15:33, gavinp wrote: > > > > The windows try bots were OK with this, but something is different in some > > > > windows builders, and the pair type constructor will not accept NULL, or > > even > > > > implicit_cast<PrererenderHandle*>(NULL). I suppose > > > > implicit_cast<PrerenderHandle*const&>(NULL) would have worked, but that > > kinda > > > > makes me sick. > > > > > > Did you try std::make_pair? > > > > Yes, with that it won't compile in clang or gcc. I've had very bad luck with > > std::make_pair() where there's NULLs involved. Probably coincidentally, the > > instance of NULL is like this: > > > > std::make_pair(foo, static_cast<Bar*>(NULL)); > > > > Which works too. Would you prefer that? I don't like using static_cast<>, but > I > > also don't like "null". > > That seems slightly better to me. We have to static_cast NULL in some other > places too. > > I wonder if C++11's nullptr would help here... The final commit after the win builder fix was Change committed as 146743 |
