Add a PLT histogram for prerender local predictor prefetches.

Add a PLT histogram for prerender local predictor prefetches.
R=jkarlin@chromium.org, asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291413

[jkarlin, 2014-08-21 14:34:15]

Looks good overall, just a few comments.

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:474: bool
SeenPLTForURL(const GURL& url, base::TimeDelta plt) {
Rename to MarkPLTSeen?  Then reformat the comments to look more like MarkURLSeen
as well, as those are clearer.

// Marks the PLT for the provided URL as seen in the context specified.  Returns
true iff ... i.e. the marking was successful.

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:506: bool plt_recorded_;
Why not seen_plt_ like the other members?  PrefetchList doesn't actually do the
recording.

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:23962: +    PLT for pages matched to a
prefetch initiated by the Local Predictor.
More detail please.  What does it mean for a page to match a prefetch initiated
by the local predictor.

[tburkard, 2014-08-21 15:28:11]

Hi Josh,

thanks for your comments. Please let me know if there is anything else.

Thanks.

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:474: bool
SeenPLTForURL(const GURL& url, base::TimeDelta plt) {
On 2014/08/21 14:34:14, jkarlin wrote:
> Rename to MarkPLTSeen?  Then reformat the comments to look more like
MarkURLSeen
> as well, as those are clearer.
> 
> // Marks the PLT for the provided URL as seen in the context specified. 
Returns
> true iff ... i.e. the marking was successful.

Done.

https://codereview.chromium.org/495953003/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:506: bool plt_recorded_;
On 2014/08/21 14:34:14, jkarlin wrote:
> Why not seen_plt_ like the other members?  PrefetchList doesn't actually do
the
> recording.

Done.

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:23962: +    PLT for pages matched to a
prefetch initiated by the Local Predictor.
On 2014/08/21 14:34:14, jkarlin wrote:
> More detail please.  What does it mean for a page to match a prefetch
initiated
> by the local predictor.  

Done.

[jkarlin, 2014-08-21 16:22:53]

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:23962: +    PLT for pages matched to a
prefetch initiated by the Local Predictor.
On 2014/08/21 15:28:11, tburkard wrote:
> On 2014/08/21 14:34:14, jkarlin wrote:
> > More detail please.  What does it mean for a page to match a prefetch
> initiated
> > by the local predictor.  
> 
> Done.

Where?

https://codereview.chromium.org/495953003/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/495953003/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:469: // before in, i.e.
the sighting was successful.
"before in," please fix.

[tburkard, 2014-08-22 09:49:34]

Thanks.

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:23962: +    PLT for pages matched to a
prefetch initiated by the Local Predictor.
On 2014/08/21 16:22:53, jkarlin wrote:
> On 2014/08/21 15:28:11, tburkard wrote:
> > On 2014/08/21 14:34:14, jkarlin wrote:
> > > More detail please.  What does it mean for a page to match a prefetch
> > initiated
> > > by the local predictor.  
> > 
> > Done.
> 
> Where?

Click on histograms.xml in the PS2 or PS3, and you can see how I significantly
expanded the description. Not sure why it did not show up for you (or in your
response comment).

https://codereview.chromium.org/495953003/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/495953003/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:469: // before in, i.e.
the sighting was successful.
On 2014/08/21 16:22:53, jkarlin wrote:
> "before in," please fix.

Done.

[jkarlin, 2014-08-22 10:40:32]

lgtm

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/495953003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:23962: +    PLT for pages matched to a
prefetch initiated by the Local Predictor.
On 2014/08/22 09:49:34, tburkard wrote:
> On 2014/08/21 16:22:53, jkarlin wrote:
> > On 2014/08/21 15:28:11, tburkard wrote:
> > > On 2014/08/21 14:34:14, jkarlin wrote:
> > > > More detail please.  What does it mean for a page to match a prefetch
> > > initiated
> > > > by the local predictor.  
> > > 
> > > Done.
> > 
> > Where?
> 
> Click on histograms.xml in the PS2 or PS3, and you can see how I significantly
> expanded the description. Not sure why it did not show up for you (or in your
> response comment).

Weird.  I wonder why it doesn't have a "delta from PS1" link either.

[Alexei Svitkine (slow), 2014-08-22 14:38:29]

lgtm

[tburkard, 2014-08-22 14:40:36]

The CQ bit was checked by tburkard@chromium.org

[commit-bot: I haz the power, 2014-08-22 14:41:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tburkard@chromium.org/495953003/40001

[commit-bot: I haz the power, 2014-08-22 15:36:51]

Committed patchset #3 (40001) as 291413