Separate same domain and cross domain <link rel=...> prerenders for reporting.

Separate same domain and cross domain <link rel=...> prerenders for reporting.

Especially with the 15min TTL experiment running, we'd like to see how web prerendering behaves differently in the cross vs. same domain case, so we can consider if they should be treated differently.

R=mmenke@chromium.org,dominich@chromium.org
BUG=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162609

[gavinp, 2012-09-13 19:56:47]

wdyt?

[dominich, 2012-09-13 20:03:36]

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:231:
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN,
this isn't necessarily true. Pending prerenders can be same-domain.

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
why is this the default here?

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

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:254: if
(source_web_contents->GetURL().host() == url.host())
shorter: is_same_domain = source_web_contents->GetURL().host() == url.host();

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:922: origin =
ORIGIN_GWS_PRERENDER;
GWS could be same-domain.

[gavinp, 2012-09-14 02:00:42]

wdyt?

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:231:
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN,
On 2012/09/13 20:03:36, dominich wrote:
> this isn't necessarily true. Pending prerenders can be same-domain.

Good catch. Done.

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
On 2012/09/13 20:03:36, dominich wrote:
> why is this the default here?

Why was ORIGIN_LINK_REL_PRERENDER the default before?

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

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:254: if
(source_web_contents->GetURL().host() == url.host())
On 2012/09/13 20:03:36, dominich wrote:
> shorter: is_same_domain = source_web_contents->GetURL().host() == url.host();

Done.

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:922: origin =
ORIGIN_GWS_PRERENDER;
On 2012/09/13 20:03:36, dominich wrote:
> GWS could be same-domain.

That's true... But the goal here is to gather statistics on web prerenders, and
how they differ same vs cross origin. In gws, we already have so many ways we
limit prerendering... If we find there's huge cross/same domain differences in
web prerendering, perhaps we'll want to drill down in GWS, but for now I'm happy
leaving GWS monolithic. Does that make sense?

[dominich, 2012-09-14 14:40:45]

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
On 2012/09/14 02:00:42, gavinp wrote:
> On 2012/09/13 20:03:36, dominich wrote:
> > why is this the default here?
> 
> Why was ORIGIN_LINK_REL_PRERENDER the default before?

I have no idea, but it seems very wrong to me. We probably don't have a
last_origin_ if we're within window, but we should then have a 'current origin'.
I think this is a bug, and I think we should fix it, given that we're now
looking to analyze the same/cross domain delta.

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

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:922: origin =
ORIGIN_GWS_PRERENDER;
On 2012/09/14 02:00:42, gavinp wrote:
> On 2012/09/13 20:03:36, dominich wrote:
> > GWS could be same-domain.
> 
> That's true... But the goal here is to gather statistics on web prerenders,
and
> how they differ same vs cross origin. In gws, we already have so many ways we
> limit prerendering... If we find there's huge cross/same domain differences in
> web prerendering, perhaps we'll want to drill down in GWS, but for now I'm
happy
> leaving GWS monolithic. Does that make sense?

I'm suggesting changing the conditional, not the eventual value.

if ((origin == ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN ||
     origin == ORIGIN_LINK_REL_PRERENDER_SAMEDOMAIN) &&
    IsGoogleSearchResultURL(referrer.url)) {
  origin = ORIGIN_GWS_PRERENDER;
}

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:310: const Origin origin,
nit: no need for const here.

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:193: const Origin origin,
nit: no need for const here

[gavinp, 2012-09-14 21:10:46]

+Timo, I have a specific question for you below.

Otherwise, the remediation continues...

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
I'm adding Timo to this review to explain, since I couldn't understand this code
when I read it. Timo, what should we do here?

On 2012/09/14 14:40:45, dominich wrote:
> On 2012/09/14 02:00:42, gavinp wrote:
> > On 2012/09/13 20:03:36, dominich wrote:
> > > why is this the default here?
> > 
> > Why was ORIGIN_LINK_REL_PRERENDER the default before?
> 
> I have no idea, but it seems very wrong to me. We probably don't have a
> last_origin_ if we're within window, but we should then have a 'current
origin'.
> I think this is a bug, and I think we should fix it, given that we're now
> looking to analyze the same/cross domain delta.

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

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:922: origin =
ORIGIN_GWS_PRERENDER;
DOH. Reading comprehension fail. Thanks. I have now added that.

On 2012/09/14 14:40:45, dominich wrote:
> On 2012/09/14 02:00:42, gavinp wrote:
> > On 2012/09/13 20:03:36, dominich wrote:
> > > GWS could be same-domain.
> > 
> > That's true... But the goal here is to gather statistics on web prerenders,
> and
> > how they differ same vs cross origin. In gws, we already have so many ways
we
> > limit prerendering... If we find there's huge cross/same domain differences
in
> > web prerendering, perhaps we'll want to drill down in GWS, but for now I'm
> happy
> > leaving GWS monolithic. Does that make sense?
> 
> I'm suggesting changing the conditional, not the eventual value.
> 
> if ((origin == ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN ||
>      origin == ORIGIN_LINK_REL_PRERENDER_SAMEDOMAIN) &&
>     IsGoogleSearchResultURL(referrer.url)) {
>   origin = ORIGIN_GWS_PRERENDER;
> }

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:310: const Origin origin,
On 2012/09/14 14:40:45, dominich wrote:
> nit: no need for const here.

It's not needed. But,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...
tells me to to do this, and the only "overkill" example involves three const and
is not even described as "certainly" overkill, versus this use of just one
const.

[dominich, 2012-09-14 21:13:44]

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:310: const Origin origin,
On 2012/09/14 21:10:47, gavinp wrote:
> On 2012/09/14 14:40:45, dominich wrote:
> > nit: no need for const here.
> 
> It's not needed. But,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...
> tells me to to do this, and the only "overkill" example involves three const
and
> is not even described as "certainly" overkill, versus this use of just one
> const.

If it was up to me, I'd const ALL the things. However, Chromium reviewers have
warned me off adding const to arguments passed by value as it doesn't add
anything to the API, and modern compilers don't rely on the keyword anyway.

Consider this a nit, and we'll move on.

[mmenke, 2012-09-17 20:39:27]

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
On 2012/09/14 21:10:47, gavinp wrote:
> I'm adding Timo to this review to explain, since I couldn't understand this
code
> when I read it. Timo, what should we do here?
> 
> On 2012/09/14 14:40:45, dominich wrote:
> > On 2012/09/14 02:00:42, gavinp wrote:
> > > On 2012/09/13 20:03:36, dominich wrote:
> > > > why is this the default here?
> > > 
> > > Why was ORIGIN_LINK_REL_PRERENDER the default before?
> > 
> > I have no idea, but it seems very wrong to me. We probably don't have a
> > last_origin_ if we're within window, but we should then have a 'current
> origin'.
> > I think this is a bug, and I think we should fix it, given that we're now
> > looking to analyze the same/cross domain delta.
> 

I'm not Timo (Though I've always wanted to play him on TV), but I think now that
we allow multiple prerenders, this may be a rather dysfunctional function.  Some
places, at least, we can get the relevant origin directly from the prerender.

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/26001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:310: const Origin origin,
On 2012/09/14 21:13:44, dominich wrote:
> On 2012/09/14 21:10:47, gavinp wrote:
> > On 2012/09/14 14:40:45, dominich wrote:
> > > nit: no need for const here.
> > 
> > It's not needed. But,
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...
> > tells me to to do this, and the only "overkill" example involves three const
> and
> > is not even described as "certainly" overkill, versus this use of just one
> > const.
> 
> If it was up to me, I'd const ALL the things. However, Chromium reviewers have
> warned me off adding const to arguments passed by value as it doesn't add
> anything to the API, and modern compilers don't rely on the keyword anyway.
> 
> Consider this a nit, and we'll move on.

I actually think the style guide is ambiguous.  It lists 3 specific situations
where it makes sense to do so, and this is not one of them.  Does it "make
sense" to declare everything passed by value as const?  Is it "overkill"?  It's
unclear.

I tend to just go with the prevailing style.

[gavinp, 2012-09-20 04:18:10]

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/7001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:398: return
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN;
On 2012/09/17 20:39:27, Matt Menke wrote:
> On 2012/09/14 21:10:47, gavinp wrote:
> > I'm adding Timo to this review to explain, since I couldn't understand this
> code
> > when I read it. Timo, what should we do here?
> > 
> > On 2012/09/14 14:40:45, dominich wrote:
> > > On 2012/09/14 02:00:42, gavinp wrote:
> > > > On 2012/09/13 20:03:36, dominich wrote:
> > > > > why is this the default here?
> > > > 
> > > > Why was ORIGIN_LINK_REL_PRERENDER the default before?
> > > 
> > > I have no idea, but it seems very wrong to me. We probably don't have a
> > > last_origin_ if we're within window, but we should then have a 'current
> > origin'.
> > > I think this is a bug, and I think we should fix it, given that we're now
> > > looking to analyze the same/cross domain delta.
> > 
> 
> I'm not Timo (Though I've always wanted to play him on TV), but I think now
that
> we allow multiple prerenders, this may be a rather dysfunctional function. 
Some
> places, at least, we can get the relevant origin directly from the prerender.

Oh boy, I've now reviewed this code, and these histograms are really broken
right now, since this entire file operates using what amounts to global
variables.

I'm in the process of fixing them, but there's ten distinct PLTs in
"FirstFooAfterBarWithExtraQuux"...  I'm trying to decide how much to preserve
and how much to shred.

Timo, how many of these are you using? Which ones are most interesting?

[tburkard, 2012-09-21 00:36:10]

Sorry, hadn't seen this review.

Just discussed with Gavin what to do (to make histograms work for 5min TTL
and multiple prerenders):
- Keep 30s window for Miss/windowed timings (because we expect prerenders
will have completed loading within 30s)
- Declare a wash if there are 2 different origins in the 30s window. If
there are multiple prerenders from the same origin, no wash (still need a
global last/current origin to do this)
- Match/MatchComplete: Attribute to the origin of the respective prerender
- origin web: is split into web same domain / web cross domain

[gavinp, 2012-10-05 15:26:00]

The histogram global variable issue I'm fixing in
http://codereview.chromium.org/11028037/ , so I'll rebase this review on top of
that shortly.

[gavinp, 2012-10-05 15:59:35]

Rebasing this made it smaller and more on topic.
 
mmenke, tburkard: WDYT?

dominich: PTAL if you don't mind finishing up this long dormant review...

[mmenke, 2012-10-05 21:15:44]

Patch set 8 looks fine to me, with one minor comment.  Looks like you messed up
your remediation, though there's no rush to fix.

https://codereview.chromium.org/10933065/diff/33001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/10933065/diff/33001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:230: bool is_same_domain = false;
Having an origin here instead of a bool would remove the need for the redundant
bool->origin logic.

https://codereview.chromium.org/10933065/diff/33001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:242: is_same_domain =
(source_web_contents->GetURL().host() == url.host());
Nothing actionable, but just noting that cross domain and cross site are
different - I think we're right to use domain here, but need to make sure we
don't assume they're the same in the future.

[mmenke, 2012-10-05 21:16:38]

On 2012/10/05 21:15:44, Matt Menke wrote:
> Patch set 8 looks fine to me, with one minor comment.  Looks like you messed
up
> your remediation, though there's no rush to fix.

Err...your update to the remediation in the other CL, rather.

[tburkard, 2012-10-05 21:55:03]

Will start on this, but I'd like to see change 11028037 ready & committed before
I finalize my review comments.

[dominich, 2012-10-08 17:03:04]

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:106: } else if (origin ==
ORIGIN_OMNIBOX) { \
would this read better as:

} else if (origin == ORIGIN_OMNIBOX ||
           origin == ORIGIN_LINK_REL_PRERENDER_SAMEDOMAIN ||
           origin == ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN) {
  HISTOGRAM;
} ...

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:121:
last_origin_(ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN),
worth initializing to ORIGIN_MAX so it's invalid?

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:242: is_same_domain =
(source_web_contents->GetURL().host() == url.host());
does this return true if you have https://google.com and http://google.com?
Should it?

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:706: *origin = NULL;
this could be ORIGIN_MAX. Or a new ORIGIN_INVALID (== -1)

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:710: *origin = &it->second.origin;
this could lead to lifetime issues depending on what the caller does with it.

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:207: const Origin** origin) const;
this can stay as Origin* and still be optional and NULLable.

[gavinp, 2012-10-08 17:09:36]

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:207: const Origin** origin) const;
On 2012/10/08 17:03:04, dominich wrote:
> this can stay as Origin* and still be optional and NULLable.

 Yes. Bad merge/rebase; I'll fix and reupload.

[gavinp, 2012-10-10 19:17:44]

I rebased, and did some code changes to remediate, but this review should
probably be on hold under the upstream CL (fixing prerender_histograms for
multiple prerenders) gets landed. Feel free to add comments of course, and I'll
periodically update with rebases...

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:106: } else if (origin ==
ORIGIN_OMNIBOX) { \
It might, but then when we ran we'd have an assert fail, as three histograms
with three names each used the same static instance variable. Oh boy....

On 2012/10/08 17:03:04, dominich wrote:
> would this read better as:
> 
> } else if (origin == ORIGIN_OMNIBOX ||
>            origin == ORIGIN_LINK_REL_PRERENDER_SAMEDOMAIN ||
>            origin == ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN) {
>   HISTOGRAM;
> } ...

http://codereview.chromium.org/10933065/diff/34021/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:121:
last_origin_(ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN),
On 2012/10/08 17:03:04, dominich wrote:
> worth initializing to ORIGIN_MAX so it's invalid?

Done.

[gavinp, 2012-10-17 14:38:46]

This is now rebased on top of the multi-prerender fixes that are in the commit
queue.

[mmenke, 2012-10-17 15:12:16]

LGTM.  Just minor comments, though if you inline GetHistogramName, I'll want to
take another look, just to be on the safe side.

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:219:
PrerenderManager::kNoExperiment),
The factory actually gets an origin when it creates a TestPrerenderContents. 
Why not just use it?

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:58: return
ComposeHistogramName("webcross", name);
nit:  Move these after ORIGIN_NONE, to match declaration order, and the order in
PREFIXED_HISTOGRAM_INTERNAL.

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:101: histogram_name); \
Would you mind just inlining GetHistogramName into this function?  Having two
functions that have to always have the exact same switch statement just seems a
bad idea.  Feel free to do it in another CL (Or not at all - I'm not going to
block you on it).

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:273: :
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN,
rather than duplicating this logic, suggest replacing is_same_domain with an
origin, and defaulting to CROSSDOMAIN.

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:996: origin ==
ORIGIN_LINK_REL_PRERENDER_SAMEDOMAIN) &&
Random comment:  Wonder if we even need an origin check here.  If it's not a
link rel prerender, won't referrer.url be blank?  Suppose if we end up keeping
the predictor, possibly not.

[tburkard, 2012-10-17 19:17:54]

Reviewing this afternoon.

[tburkard, 2012-10-17 19:21:11]

lgtm

[gavinp, 2012-10-17 20:57:49]

Matt & Timo, thanks for your reviews!

dominich: are you going to take another poke at this?

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:219:
PrerenderManager::kNoExperiment),
On 2012/10/17 15:12:16, Matt Menke wrote:
> The factory actually gets an origin when it creates a TestPrerenderContents. 
> Why not just use it?

Done.

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:58: return
ComposeHistogramName("webcross", name);
On 2012/10/17 15:12:16, Matt Menke wrote:
> nit:  Move these after ORIGIN_NONE, to match declaration order, and the order
in
> PREFIXED_HISTOGRAM_INTERNAL.

Done.

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:101: histogram_name); \
On 2012/10/17 15:12:16, Matt Menke wrote:
> Would you mind just inlining GetHistogramName into this function?  Having two
> functions that have to always have the exact same switch statement just seems
a
> bad idea.  Feel free to do it in another CL (Or not at all - I'm not going to
> block you on it).

I have a lot of refactoring I could imagine happening here, so I'm going to hold
off on that, at least until our discussion about how much of this complexity we
want runs out. So thanks for not blocking me, and please feel free to nag me if
we work in this area again...

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10933065/diff/65001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:273: :
ORIGIN_LINK_REL_PRERENDER_CROSSDOMAIN,
On 2012/10/17 15:12:16, Matt Menke wrote:
> rather than duplicating this logic, suggest replacing is_same_domain with an
> origin, and defaulting to CROSSDOMAIN.

Done.

[dominich, 2012-10-17 21:04:21]

LGTM

My comments should have been against a previous CL, and they're not blocking.

http://codereview.chromium.org/10933065/diff/78002/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10933065/diff/78002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:55: case ORIGIN_NONE:
is it valid to have no origin?

http://codereview.chromium.org/10933065/diff/78002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:127: last_origin_(ORIGIN_MAX),
why is ORIGIN_NONE != ORIGIN_MAX? This suggests that no origin is valid, which
surprises me.

[commit-bot: I haz the power, 2012-10-17 23:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/10933065/78002

[commit-bot: I haz the power, 2012-10-18 01:50:40]

Change committed as 162609