Add code to keep track of what fraction of pageviews are top sites.

Add code to keep track of what fraction of pageviews are top sites.
This code will be removed again once histograms have been collected.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98734

[tburkard, 2011-08-29 15:22:10]

Hi Chris and Dominic,

after being puzzled by buildbot results for a while, here is the same change
again.  It should be identical to the last version I sent to you.  After
extensive trybot testing (and confirming some flakiness in trybots for the
revision I am using for this), I am feeling confident that this is not breaking
anything.

Thanks.

[tburkard, 2011-08-29 15:23:21]

On 2011/08/29 15:22:10, tburkard wrote:
> Hi Chris and Dominic,
> 
> after being puzzled by buildbot results for a while, here is the same change
> again.  It should be identical to the last version I sent to you.  After
> extensive trybot testing (and confirming some flakiness in trybots for the
> revision I am using for this), I am feeling confident that this is not
breaking
> anything.
> 
> Thanks.

Chris, what you reviewed earlier this morning was a "test version" to figure out
some trybot issues.  This new change here should have your concerns (testing /
debug code) addressed.

[dominich, 2011-08-29 15:57:10]

LGTM

I hope it sticks this time!

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:192:
&prerender::PrerenderManager::MostVisitedSites::
nit: do you need the full scope here? You are within the
prerender::PrerenderManager::MostVisitedSites scope so I'd imagine you'd be fine
with just OnMostVisitedURLsAvailable.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:199: for (int i = 0; i <
static_cast<int>(data.size()); i++)
nit: size_t for loop index. I know it's not usual to do that, but in this case I
think it's reasonable.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:216: if (profile_)
nit: I prefer

return (profile_ ? profile_->GetTopSites() : NULL);

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_observer.cc (right):

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_observer.cc:267: if (!profile)
nit: I prefer

return (profile ? profile->GetPrerenderManager() : NULL);

[cbentzel, 2011-08-29 16:18:08]

LGTM

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:177: history::TopSites* ts =
GetTopSites();
Nit: Use |top_sites| instead of |ts|

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:219: }
Nit: newline between here and topsites_consumer_

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:227: return most_visited_.get() &&
most_visited_->IsTopSite(url);
most_visited_ should always be non-NULL.

[tburkard, 2011-08-29 19:14:19]

Ok, so I couldn't figure out how to get to reliably pass the tests without the
3s timeout, so I put it back in.

However, I made one important change: I only do the 3s timeout when TopSites
does not exist yet!  Otherwise, I do the init right away.

I think this is a good compromise.

Thanks.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:177: history::TopSites* ts =
GetTopSites();
On 2011/08/29 16:18:08, cbentzel wrote:
> Nit: Use |top_sites| instead of |ts|

Done.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:192:
&prerender::PrerenderManager::MostVisitedSites::
Yes, that's what I had tried first and then the compiler asked me to do the full
scope :)
On 2011/08/29 15:57:10, dominich wrote:
> nit: do you need the full scope here? You are within the
> prerender::PrerenderManager::MostVisitedSites scope so I'd imagine you'd be
fine
> with just OnMostVisitedURLsAvailable.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:219: }
On 2011/08/29 16:18:08, cbentzel wrote:
> Nit: newline between here and topsites_consumer_

Done.

http://codereview.chromium.org/7775003/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:227: return most_visited_.get() &&
most_visited_->IsTopSite(url);
On 2011/08/29 16:18:08, cbentzel wrote:
> most_visited_ should always be non-NULL. 

Done.

[tburkard, 2011-08-29 22:15:15]

Chris, I added the comment and filed the bug (http://crbug.com/89543).

Please let me know if you are now ok with this.

Thanks.

[tburkard, 2011-08-29 23:39:42]

On 2011/08/29 22:15:15, tburkard wrote:
> Chris, I added the comment and filed the bug (http://crbug.com/89543).
> 
> Please let me know if you are now ok with this.
> 
> Thanks.

Based on our chat, you said you would be ok once I create the bug and the
reference to it in the source, which I did.  So since I would like to commit
this now and you are not around, I will commit.

Thanks.

[jar (doing other things), 2011-09-01 18:14:51]

drive by commentary....

http://codereview.chromium.org/7775003/diff/13003/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7775003/diff/13003/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:180: // See http://crbug.com/94654
Any time you have to wait for something like 3 seconds to assure the unit tests
won't crash, you know you are doing something wrong.  This is generally
guaranteed to create a flaky test, as bots regularly stall for more than 3
seconds (which will mean that your delay will have been for naught).

It also shows that you haven't properly sequenced your code to wait for proper
initialization in other areas... which hints at an even larger problem.

[cbentzel, 2011-09-01 18:29:08]

Yes, I should not have LGTM'ed this. Your commentary is correct.

On Thu, Sep 1, 2011 at 2:14 PM,  <jar@chromium.org> wrote:
> drive by commentary....
>
>
>
http://codereview.chromium.org/7775003/diff/13003/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.cc (right):
>
>
http://codereview.chromium.org/7775003/diff/13003/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.cc:180: // See
> http://crbug.com/94654
> Any time you have to wait for something like 3 seconds to assure the
> unit tests won't crash, you know you are doing something wrong.  This is
> generally guaranteed to create a flaky test, as bots regularly stall for
> more than 3 seconds (which will mean that your delay will have been for
> naught).
>
> It also shows that you haven't properly sequenced your code to wait for
> proper initialization in other areas... which hints at an even larger
> problem.
>
> http://codereview.chromium.org/7775003/
>