Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9226037:
Cancel prerenders from Omnibox if we navigate to a different URL than predicted (Closed)
|
Created:
8 years, 11 months ago by dominich Modified:
8 years, 10 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org, James Su, dominich+watch_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCancel prerenders from Omnibox if we navigate to a different URL than predicted.
I also took the opportunity to align the order of methods in prerender_manager.cc to that of the header (as it should be) and make a couple of methods const.
BUG=110799, 111350
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119699
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119928
Patch Set 1 #Patch Set 2 : Remove debug logging #
Total comments: 10
Patch Set 3 : Reorg prerender_manager.cc to match header. Add tests for cancellation. #
Total comments: 6
Patch Set 4 : rebase and constify #Patch Set 5 : Fix leak #
Messages
Total messages: 17 (0 generated)
LGTM https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... File chrome/browser/autocomplete/network_action_predictor.cc (right): https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... chrome/browser/autocomplete/network_action_predictor.cc:242: prerender::PrerenderManager* prerender_manager = Nit: Does it make sense to keep this as a member? (It's not much different either way, so I don't really have a preference.) https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... chrome/browser/autocomplete/network_action_predictor.cc:244: if (prerender_manager && !prerender_manager->IsPrerendering(opened_url)) When is the manager NULL? During testing? (I try to limit NULL checks in our code to cases that truly need them.) https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:293: bool PrerenderManager::IsPrerendering(const GURL& url) { Nit: Can you make this file's definition order match the declaration order? I notice this was declared after CancelOmniboxPrerenders() in the header but here appears above it. https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:508: // then destroy them. Nit: Our normal practice for this would be to do one of the following: for (it = list.begin(); it != list.end(); ) { // OPTION 1 if (should_destroy(it)) it = it->Destroy(); // Requires that Destroy() ultimately return the result of list.erase() else ++it; // OR, OPTION 2 cur = it++; if (should_destroy(cur)) cur->Destroy(); } These have the advantage of being somewhat shorter and cheaper in addition to being more common.
Tests? Perhaps add a new FINAL_STATUS option to see how frequent this form of abandonment this is? http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... File chrome/browser/autocomplete/network_action_predictor.cc (right): http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... chrome/browser/autocomplete/network_action_predictor.cc:244: if (prerender_manager && !prerender_manager->IsPrerendering(opened_url)) On 2012/01/21 00:22:52, Peter Kasting wrote: > When is the manager NULL? During testing? > > (I try to limit NULL checks in our code to cases that truly need them.) Right now we do NULL prerender_managers for incognito mode - this should change soon, but isn't there yet.
http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... File chrome/browser/autocomplete/network_action_predictor.cc (right): http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... chrome/browser/autocomplete/network_action_predictor.cc:244: if (prerender_manager && !prerender_manager->IsPrerendering(opened_url)) On 2012/01/21 02:29:09, cbentzel wrote: > On 2012/01/21 00:22:52, Peter Kasting wrote: > > When is the manager NULL? During testing? > > > > (I try to limit NULL checks in our code to cases that truly need them.) > > Right now we do NULL prerender_managers for incognito mode - this should change > soon, but isn't there yet. Ah. Perhaps this should get a "// |prerender_manager| may be NULL in incognito mode." comment then?
On 2012/01/21 02:29:08, cbentzel wrote: > Tests? On the way. > > Perhaps add a new FINAL_STATUS option to see how frequent this form of > abandonment this is? It should be about the same as (100 - USED%). I'm not too interested in the exact breakdown of cancellation reasons for Omnibox, just the difference between cancellation for reasons we can control (misprediction) vs reasons we can't (HTML5, etc). > > http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... > File chrome/browser/autocomplete/network_action_predictor.cc (right): > > http://codereview.chromium.org/9226037/diff/4001/chrome/browser/autocomplete/... > chrome/browser/autocomplete/network_action_predictor.cc:244: if > (prerender_manager && !prerender_manager->IsPrerendering(opened_url)) > On 2012/01/21 00:22:52, Peter Kasting wrote: > > When is the manager NULL? During testing? > > > > (I try to limit NULL checks in our code to cases that truly need them.) > > Right now we do NULL prerender_managers for incognito mode - this should change > soon, but isn't there yet. Also if Prerendering is disabled. I'll add a comment.
https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... File chrome/browser/autocomplete/network_action_predictor.cc (right): https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... chrome/browser/autocomplete/network_action_predictor.cc:242: prerender::PrerenderManager* prerender_manager = On 2012/01/21 00:22:52, Peter Kasting wrote: > Nit: Does it make sense to keep this as a member? (It's not much different > either way, so I don't really have a preference.) I prefer not to. It can't go away unexpectedly currently but that might change so I prefer to keep these things loosely coupled. https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/autoc... chrome/browser/autocomplete/network_action_predictor.cc:244: if (prerender_manager && !prerender_manager->IsPrerendering(opened_url)) On 2012/01/21 22:46:07, Peter Kasting wrote: > On 2012/01/21 02:29:09, cbentzel wrote: > > On 2012/01/21 00:22:52, Peter Kasting wrote: > > > When is the manager NULL? During testing? > > > > > > (I try to limit NULL checks in our code to cases that truly need them.) > > > > Right now we do NULL prerender_managers for incognito mode - this should > change > > soon, but isn't there yet. > > Ah. Perhaps this should get a "// |prerender_manager| may be NULL in incognito > mode." comment then? Done. https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:293: bool PrerenderManager::IsPrerendering(const GURL& url) { On 2012/01/21 00:22:52, Peter Kasting wrote: > Nit: Can you make this file's definition order match the declaration order? I > notice this was declared after CancelOmniboxPrerenders() in the header but here > appears above it. Done. https://chromiumcodereview.appspot.com/9226037/diff/4001/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:508: // then destroy them. On 2012/01/21 00:22:52, Peter Kasting wrote: > Nit: Our normal practice for this would be to do one of the following: > > for (it = list.begin(); it != list.end(); ) { > // OPTION 1 > if (should_destroy(it)) > it = it->Destroy(); // Requires that Destroy() ultimately return the result > of list.erase() > else > ++it; > > // OR, OPTION 2 > cur = it++; > if (should_destroy(cur)) > cur->Destroy(); > } > > These have the advantage of being somewhat shorter and cheaper in addition to > being more common. Done.
https://chromiumcodereview.appspot.com/9226037/diff/8001/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9226037/diff/8001/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:297: for (std::list<PrerenderContentsData>::iterator it = prerender_list_.begin(); Nit: I wonder if a typedef for this might be useful
LGTM - no need for an additional round of reviews from me. http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (left): http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:338: DCHECK(session_storage_namespace); Why was this removed? http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:756: // private Nit: I haven't really seen the // private comment in the code base. Not sure if it needs to be introduced. http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:868: PrerenderContents* PrerenderManager::GetEntry(const GURL& url) { I didn't check if all of the methods in this block are identical to what existed before, assumed it was just a move in the file. http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:224: // TODO(dominich): This should be a const method but FindEntry is not const. Why not just make it const in this CL? You may need a const one which returns a const PrerenderContents* and a non-const one which returns a PrerenderContents*.
pkasting: I'm assuming your LGTM still stands as only a comment was added since the last one. Feel free to let me know if you want a chance to go through it again. cbentzel: I'll wait for your replies to the below before committing, but i won't add another patch unless you insist on any of the below changes after reading my replies. On 2012/01/25 12:14:01, cbentzel wrote: > LGTM - no need for an additional round of reviews from me. > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_manager.cc (left): > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.cc:338: > DCHECK(session_storage_namespace); > Why was this removed? The unit tests now test omnibox prerenders. I couldn't find a way to get a tab with a contents with a session storage namespace. Since the other path of adding a prerender doesn't require a session storage namespace, i didn't think it was that terrible to remove. I thought about moving the DCHECK to the callsite but that felt incorrect. I could have overridden AddOmniboxPrerender in the TestPrerenderManager but that would then be a direct copy of the PrerenderManager version just without the DCHECK. Is there a better way to handle this? > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.cc:756: // private > Nit: I haven't really seen the // private comment in the code base. Not sure if > it needs to be introduced. That was more for me to track where I was in the file as I moved things around. I actually quite like it given how many methods are in this file, but I can remove it if you feel strongly. > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.cc:868: PrerenderContents* > PrerenderManager::GetEntry(const GURL& url) { > I didn't check if all of the methods in this block are identical to what existed > before, assumed it was just a move in the file. Exactly. > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_manager.h (right): > > http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.h:224: // TODO(dominich): This should > be a const method but FindEntry is not const. > Why not just make it const in this CL? You may need a const one which returns a > const PrerenderContents* and a non-const one which returns a PrerenderContents*. I thought there'd been enough changes in this CL already without introducing const. And I have a feeling I tried before and ran into some issues. http://crbug.com/111350
http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9226037/diff/8001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:224: // TODO(dominich): This should be a const method but FindEntry is not const. On 2012/01/25 12:14:01, cbentzel wrote: > Why not just make it const in this CL? You may need a const one which returns a > const PrerenderContents* and a non-const one which returns a PrerenderContents*. I tried it out and it just worked, so that's done now.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominich@chromium.org/9226037/17001
Change committed as 119699
cbentzel: Fixed leak, PTAL Leak was caused by querying PrerenderManager to get a non-cancelled prerender through GetEntry. It should have been FindEntry which does not remove the entry from the internal list. Other tests using GetEntry are safe as long as they expect to cancel or not start the prerender. This does seem a little fragile, and I'm wondering if we can simplify the PrerenderManager API to just have FindEntry.
LGTM. Please run on an appropriate memory trybot first to ensure that the leak is gone.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominich@chromium.org/9226037/23002
Change committed as 119928 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
