Add a PrefetchList to Prerender Local Predictor, to emulate how effective

Add a PrefetchList to Prerender Local Predictor, to emulate how effective
prefetches would be. The hope is to obtain higher numbers than with prerender
control group prerenders, since these still get cancelled for a fair number
of reasons.

R=asvitkine@chromium.org, davidben@chromium.org

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

[tburkard, 2014-08-05 10:45:57]

It'd be great if I could commit this today, since we desperately need the
numbers that these hisograms will show :)

Thanks.

[davidben, 2014-08-05 18:51:12]

lgtm with some style nits. Also a comment below about num_issued.

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:391:
StringToInt(GetLocalPredictorSpecValue(kPrefetchListTimeoutKeyName), &result);
Nit: Check for StringToInt returning false here, so we're not reading an
uninitialized value if that fails.

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:1171: }
This will count candidate URLs toward num_issued even if they fail this check.
Is this intentional?

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:409: };
Nit: newline here

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:413: }
Nit: newline here

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:435: }
Nit: newline here

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:467: explicit
ListEntry(string url)
Nit: I think we usually put methods before members. Also if these are public
fields of a struct, they probably shouldn't have the underscore. (Writing
url(url) in the initialization list works fine.)

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:473: };
Nit: newline here

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:490: }
Nit: newline here

[Alexei Svitkine (slow), 2014-08-05 19:20:22]

histograms lgtm, couple style comments

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:467: explicit
ListEntry(string url)
Pass the param by const ref.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:488: delete entry;
Can you use a scoped_ptr instead?

[tburkard, 2014-08-05 20:05:55]

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:391:
StringToInt(GetLocalPredictorSpecValue(kPrefetchListTimeoutKeyName), &result);
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: Check for StringToInt returning false here, so we're not reading an
> uninitialized value if that fails.

See comment in base/strings/string_number_conversions.h
StringToInt will always set result to something, the return value only indicates
if it was a "perfect" conversion or not.

So result will always have a defined value, and the check afterwards will ensure
it is in the correct range.

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:1171: }
On 2014/08/05 18:51:12, David Benjamin wrote:
> This will count candidate URLs toward num_issued even if they fail this check.
> Is this intentional?

Yes, that's fine, because they are ordered in descending priority. Therefore,
once the first one fails this check, all next ones should fail, too.

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:409: };
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: newline here

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:413: }
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: newline here

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:435: }
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: newline here

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:467: explicit
ListEntry(string url)
On 2014/08/05 19:20:22, Alexei Svitkine wrote:
> Pass the param by const ref.

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:467: explicit
ListEntry(string url)
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: I think we usually put methods before members. Also if these are public
> fields of a struct, they probably shouldn't have the underscore. (Writing
> url(url) in the initialization list works fine.)

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:473: };
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: newline here

Done.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:488: delete entry;
On 2014/08/05 19:20:22, Alexei Svitkine wrote:
> Can you use a scoped_ptr instead?

Not obvious to me how this would work well here.
Can you point me to some existing code using a scoped_ptr inside STL containers?
I can change it in a follow-up CL (since I want to commit this today for
tomorrow's build and it's already 10pm in my timezone).

Thanks.

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:490: }
On 2014/08/05 18:51:12, David Benjamin wrote:
> Nit: newline here

Done.

[tburkard, 2014-08-05 20:08:56]

Committed patchset #2 manually as 287582 (presubmit successful).

[Alexei Svitkine (slow), 2014-08-05 20:16:22]

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

https://codereview.chromium.org/441923002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:488: delete entry;
On 2014/08/05 20:05:54, tburkard wrote:
> On 2014/08/05 19:20:22, Alexei Svitkine wrote:
> > Can you use a scoped_ptr instead?
> 
> Not obvious to me how this would work well here.
> Can you point me to some existing code using a scoped_ptr inside STL
containers?
> I can change it in a follow-up CL (since I want to commit this today for
> tomorrow's build and it's already 10pm in my timezone).
> 
> Thanks.

Sorry, I meant in the body of the loop, not in the container - i.e.:

scoped_ptr<ListEntry> entry(entry_list_.front());
entry_list_.pop_front();

Then you don't need the delete (which is more error prone - e.g. if you decide
to add a break or continue statement later to the loop).

As for containers with owned pointers, I think we just have a ScopedVector,
which I didn't read your code careful enough to understand whether it's a good
fit here or not.