Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8583001:
For the MatchComplete PPLT change in prerendering, (Closed)
|
Created:
9 years, 1 month ago by tburkard Modified:
9 years, 1 month ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFor the MatchComplete PPLT change in prerendering,
fix a bug caused by redirects (such as in the GWS case).
We must copy all alias URLs from the original prerender
into the dummy.
R=cbentzel, dominich
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110365
Patch Set 1 #
Total comments: 8
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #
Total comments: 1
Patch Set 6 : '' #
Total comments: 1
Messages
Total messages: 12 (0 generated)
http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.h:315: DISALLOW_COPY_AND_ASSIGN(PrerenderContents); You could allow copy constructors and assign operators and use those to make a dummy copy of the PrerenderContents. http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_manager.cc:726: PrerenderContents* dummy_replacement_prerender_contents = Consider a 'clone' method on PrerenderContents that can take care of all of this.
http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.h:315: DISALLOW_COPY_AND_ASSIGN(PrerenderContents); I don't want all the initialization of the old object though (such as TabContents) etc. The only thing that's identical is the URL & the alias URL, none of the other TabContents & plumbing. On 2011/11/16 18:57:13, dominich wrote: > You could allow copy constructors and assign operators and use those to make a > dummy copy of the PrerenderContents. http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_manager.cc:726: PrerenderContents* dummy_replacement_prerender_contents = See other comment: it's only the URLs, the other stuff should expressly not be copied. On 2011/11/16 18:57:13, dominich wrote: > Consider a 'clone' method on PrerenderContents that can take care of all of > this.
http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:514: AddAliasUrl(*it); Do you just want to directly add this to alias_urls_? I'm worried about some of the behavior like HasRecentlyBeenNavigatedTo kicking in. http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_manager.cc:734: dymmy_replacement_prerender_contents-> typo here - won't compile.
http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:727: CreatePrerenderContents( Creating an entire PrerenderContents just to store a few things seems wasteful. I realise that if you were to pull this data out into a separate struct and store that instead it would complicate the tracking and destruction, but maybe it would be worth it compared to creating a whole dummy contents. http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:500: Destroy(FINAL_STATUS_RECENTLY_VISITED); What happens if we call Destroy here when adding the AliasURL? http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:514: AddAliasURL*it); CHECK the return value here - it would be awful if this failed.
http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:514: AddAliasUrl(*it); On 2011/11/16 19:06:29, cbentzel wrote: > Do you just want to directly add this to alias_urls_? I'm worried about some of > the behavior like HasRecentlyBeenNavigatedTo kicking in. Done. http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_manager.cc:734: dymmy_replacement_prerender_contents-> On 2011/11/16 19:06:29, cbentzel wrote: > typo here - won't compile. Done. http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:727: CreatePrerenderContents( per our discussion. already done for the control group, so just using hte same paradigm here. On 2011/11/16 19:11:26, dominich wrote: > Creating an entire PrerenderContents just to store a few things seems wasteful. > I realise that if you were to pull this data out into a separate struct and > store that instead it would complicate the tracking and destruction, but maybe > it would be worth it compared to creating a whole dummy contents. http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:500: Destroy(FINAL_STATUS_RECENTLY_VISITED); On 2011/11/16 19:11:26, dominich wrote: > What happens if we call Destroy here when adding the AliasURL? not running these checks anymore, adding directly, see chris's concern/my response. Done. http://codereview.chromium.org/8583001/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:514: AddAliasURL*it); not running these checks anymore, adding directly, see chris's concern/my response. On 2011/11/16 19:11:26, dominich wrote: > CHECK the return value here - it would be awful if this failed.
http://codereview.chromium.org/8583001/diff/7002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/7002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:515: prerender_tracker_->AddPrerenderURLOnUIThread(*it); This is only used to potentially defer the processing of requests (ie, in the case that the prerender causes a prerender). These URLs should already be in the prerender tracker if they are in the other prerender contents. You shouldn't need to add them again. If you do want to add them here, please make sure they're removed correctly as it could cause some subtle bugs where requests are delayed.
http://codereview.chromium.org/8583001/diff/7002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/8583001/diff/7002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.cc:515: prerender_tracker_->AddPrerenderURLOnUIThread(*it); Good catch! I thought prerender trackers were on a per PC basis, but its a global one. So I agree with you and I removed this. On 2011/11/16 20:15:15, dominich wrote: > This is only used to potentially defer the processing of requests (ie, in the > case that the prerender causes a prerender). These URLs should already be in the > prerender tracker if they are in the other prerender contents. You shouldn't > need to add them again. > > If you do want to add them here, please make sure they're removed correctly as > it could cause some subtle bugs where requests are delayed.
LGTM modulo nit http://codereview.chromium.org/8583001/diff/2005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/8583001/diff/2005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:158: void AddAliasURLsFromOtherPrerenderContents(PrerenderContents* other_pc); nit: const PrerenderContents*. Or even const PrerenderContents& as it shouldn't be NULL.
Looks like the browser tests are broken on the Linux try job. http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:727: CreatePrerenderContents( On 2011/11/16 19:11:26, dominich wrote: > Creating an entire PrerenderContents just to store a few things seems wasteful. > I realise that if you were to pull this data out into a separate struct and > store that instead it would complicate the tracking and destruction, but maybe > it would be worth it compared to creating a whole dummy contents. I'm somewhat interested in this approach of extracting a few pieces of state out to a separate class/struct, especially if it could be used for management of the control group.
I actually had to put the code for the prerender_tracker back in: unit tests are failing, because all URLs in alias_urls MUST be in prerender_tracker (will be removed in Destructor and CHECKs on existence). http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/8583001/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:727: CreatePrerenderContents( I think we should do some general cleanup of not just this, but everything prerender related, and talk through the design, rather than doing little bits and pieces here and there. I really need these stats soon, so please let me commit. Thanks. On 2011/11/16 21:14:25, cbentzel wrote: > On 2011/11/16 19:11:26, dominich wrote: > > Creating an entire PrerenderContents just to store a few things seems > wasteful. > > I realise that if you were to pull this data out into a separate struct and > > store that instead it would complicate the tracking and destruction, but maybe > > it would be worth it compared to creating a whole dummy contents. > > I'm somewhat interested in this approach of extracting a few pieces of state out > to a separate class/struct, especially if it could be used for management of the > control group.
LGTM assuming tests pass this round. http://codereview.chromium.org/8583001/diff/8014/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/8583001/diff/8014/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:158: void AddAliasURLsFromOtherPrerenderContents(PrerenderContents* other_pc); Nit: Could be a const PrerenderContents& in this case. Also - we tend to spell out other_prerender_contents or other_pc. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
