Add instrumentation to see how often a local predictor prerender could be

Add instrumentation to see how often a local predictor prerender could be
swapped in, but is not.

R=shishir@chromium.org

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

[Shishir, 2013-05-14 18:19:34]

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

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:486: for
(TabContentsIterator it; !it.done(); it.Next()) {
Since you are adding stats, how about adding a stat here for the number of times
you see multiple possible tabs that a prerender can go into. That way you can
compare to the other stats you added and find other cases the prerender does not
get swapped in.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:764:
RecordEvent(EVENT_TAB_HELPER_URL_SEEN);
This event will have a lot of duplicates since it is called from both
provisional load start and commit. Ideally you would want cleaner stats.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:767:
!prerender_handle_->Matches(url, NULL)) {
Can you call this with NULL if the prerender is live? There is a check in
prerender contents that would likely fail.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:776:
current_prerender_would_have_matched_ = true;
Can you add a comment as to the conditions where this can happen. Ideally if the
URL and namespace matches, this would have gotten swapped in.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.h:164: bool
current_prerender_would_have_matched_;
Comment on var - and the fact that it is only used for measurement.

[tburkard, 2013-05-14 19:28:05]

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

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:486: for
(TabContentsIterator it; !it.done(); it.Next()) {
On 2013/05/14 18:19:34, Shishir wrote:
> Since you are adding stats, how about adding a stat here for the number of
times
> you see multiple possible tabs that a prerender can go into. That way you can
> compare to the other stats you added and find other cases the prerender does
not
> get swapped in.

Done.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:764:
RecordEvent(EVENT_TAB_HELPER_URL_SEEN);
We already have other events for when a DidNavigate happens, so we can compare
against these if we want cleaner stats.

On 2013/05/14 18:19:34, Shishir wrote:
> This event will have a lot of duplicates since it is called from both
> provisional load start and commit. Ideally you would want cleaner stats.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:767:
!prerender_handle_->Matches(url, NULL)) {
On 2013/05/14 18:19:34, Shishir wrote:
> Can you call this with NULL if the prerender is live? There is a check in
> prerender contents that would likely fail.

Done.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:776:
current_prerender_would_have_matched_ = true;
On 2013/05/14 18:19:34, Shishir wrote:
> Can you add a comment as to the conditions where this can happen. Ideally if
the
> URL and namespace matches, this would have gotten swapped in.

Done.

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://codereview.chromium.org/14949012/diff/14004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.h:164: bool
current_prerender_would_have_matched_;
On 2013/05/14 18:19:34, Shishir wrote:
> Comment on var - and the fact that it is only used for measurement.

Done.

[Shishir, 2013-05-14 20:01:43]

Minor nits and a question.

LGTM.

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (left):

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:541: DCHECK(child_id_ == -1 ||
session_storage_namespace);
Why was this line written in the first place?

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

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:785: // and is checking
for other criteria (such as POSTs). However, there may
nit: Remove this however. The previous statment has one.

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:786: // also be conditions
when a swapin shoudl happen but does not. By recording
nit: should

[tburkard, 2013-05-14 20:04:40]

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (left):

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:541: DCHECK(child_id_ == -1 ||
session_storage_namespace);
On 2013/05/14 20:01:43, Shishir wrote:
> Why was this line written in the first place?

Gavin had written this check based on the way it was used in the past, no
specific reason that this needs to be enforced/should be true.

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

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:785: // and is checking
for other criteria (such as POSTs). However, there may
On 2013/05/14 20:01:43, Shishir wrote:
> nit: Remove this however. The previous statment has one.

Done.

https://codereview.chromium.org/14949012/diff/59001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:786: // also be conditions
when a swapin shoudl happen but does not. By recording
On 2013/05/14 20:01:43, Shishir wrote:
> nit: should

Done.

[tburkard, 2013-05-14 20:08:33]

Committed patchset #6 manually as r200050.