Add observer interface to PrerenderContents.

Add observer interface to PrerenderContents.

This CL adds a simple Observer interface to PrerenderContents, and makes the PrerenderTracker an observer. The PrerenderHandle will become an observer in a soon to be uploaded CL....

The most interesting change here is that the prerender_unittest is now testing with a tracker, so it has to pass in reasonable route IDs.

R=mmenke@chromium.org
BUG=None


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

[gavinp, 2012-12-03 18:41:43]

Matt, WDYT?

I have one more of these (adding the Handler as an observer) before I upload the
architectural level review for events down to prerenders.

[mmenke, 2012-12-03 20:02:47]

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.cc:317: NotifyPrerenderStart();
IMPORTANT:  We do want to call this for the control group, too, right? 
Currently, we're just calling NotifyPrerenderStop for it.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.h:133: void ClearObservers();
ClearObservers() doesn't seem to have a function body, and seems like, in
general, a bad idea anyways, so I think we should remove it.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.h:284: void
NotifyPrerenderAddAlias(const GURL& alias_url);
NotifyPrerenderAddAlias is not currently implemented, and there's no
corresponding observer function.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:58: }
Isn't the PrerenderTracker shut down after the last profile is destroyed, since
it's owned by the browser process?  If so, can just DCHECK the list is empty.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:68: 
nit:  Extra line break not needed.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:121: return;
nit:  May be a little prettier if these if statements are merged, though doesn't
even save one line of code. :(

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:142:
prerender_contents->RemoveObserver(this);
Wonder if we should just clear the observer list in NotifyPrerenderStop.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:143:
observing_set_.erase(prerender_contents);
Suggest a DCHECK_EQ(1, observer_set_.count(prerender_contents));

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:149: return;
nit:  May be a little prettier if these if statements are merged, though doesn't
even save one line of code. :(

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:27: :
PrerenderContents(prerender_manager, NULL, GURL(), content::Referrer(),
Suggest a comment that the NULL is the profile argument.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:115: FinalStatus
final_status;
While you're here, could you move it to just above its first use (Per Google
style)?

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:128:
TestPrerenderContents test_contents(prerender_manager(), 0, 0);
optional:  Could just toss this in the test framework.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (left):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:365:
EXPECT_FALSE(duplicate_prerender_handle->IsValid());
Why remove these?

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:57: *child_id = -1;
This is just weird.  :(

(Not asking you change it)

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:73: static int s_next_route_id_;
I don't think I've seen a single static variable prefixed with s_ in the Chrome
code base.  Doing a quick search seems to verify that this impression is
accurate.

[gavinp, 2012-12-04 18:04:48]

How's this?

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.cc:317: NotifyPrerenderStart();
On 2012/12/03 20:02:47, Matt Menke wrote:
> IMPORTANT:  We do want to call this for the control group, too, right? 
> Currently, we're just calling NotifyPrerenderStop for it.

It's the other way, I think. We don't want to tell applications we're
prerendering when we aren't in the control group. Fixed (by conditionalizing
sending NotifyPrerenderStop()).

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.h:133: void ClearObservers();
On 2012/12/03 20:02:47, Matt Menke wrote:
> ClearObservers() doesn't seem to have a function body, and seems like, in
> general, a bad idea anyways, so I think we should remove it.

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.h:284: void
NotifyPrerenderAddAlias(const GURL& alias_url);
On 2012/12/03 20:02:47, Matt Menke wrote:
> NotifyPrerenderAddAlias is not currently implemented, and there's no
> corresponding observer function.

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:58: }
On 2012/12/03 20:02:47, Matt Menke wrote:
> Isn't the PrerenderTracker shut down after the last profile is destroyed,
since
> it's owned by the browser process?  If so, can just DCHECK the list is empty.

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:68: 
On 2012/12/03 20:02:47, Matt Menke wrote:
> nit:  Extra line break not needed.

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:121: return;
On 2012/12/03 20:02:47, Matt Menke wrote:
> nit:  May be a little prettier if these if statements are merged, though
doesn't
> even save one line of code. :(

I'm going to go ahead and ignore this nit from this review, since it doesn't
save lines, and I find this version prettier. But, if you repeat the nit, I'll
just follow it.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:142:
prerender_contents->RemoveObserver(this);
On 2012/12/03 20:02:47, Matt Menke wrote:
> Wonder if we should just clear the observer list in NotifyPrerenderStop.

That's a great idea; it avoids a ton of crashes.

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker.cc:143:
observing_set_.erase(prerender_contents);
On 2012/12/03 20:02:47, Matt Menke wrote:
> Suggest a DCHECK_EQ(1, observer_set_.count(prerender_contents));

Done. This turned out to require a significant rewrite of the unit test, and a
few changes in PrerenderContents() to make sure we don't OnPrerenderStop without
having earlier OnPrerenderStart().

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:27: :
PrerenderContents(prerender_manager, NULL, GURL(), content::Referrer(),
On 2012/12/03 20:02:47, Matt Menke wrote:
> Suggest a comment that the NULL is the profile argument.

Done, but I used a static_cast<> as the comment.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:115: FinalStatus
final_status;
On 2012/12/03 20:02:47, Matt Menke wrote:
> While you're here, could you move it to just above its first use (Per Google
> style)?

Done.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tracker_unittest.cc:128:
TestPrerenderContents test_contents(prerender_manager(), 0, 0);
On 2012/12/03 20:02:47, Matt Menke wrote:
> optional:  Could just toss this in the test framework.

Done. I also got rid of all the magic numbers, at the cost of many extra lines.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (left):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:365:
EXPECT_FALSE(duplicate_prerender_handle->IsValid());
On 2012/12/03 20:02:47, Matt Menke wrote:
> Why remove these?

No good reason. Fixed.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:57: *child_id = -1;
On 2012/12/03 20:02:47, Matt Menke wrote:
> This is just weird.  :(
> 
> (Not asking you change it)

Yes, it is.

https://codereview.chromium.org/11348357/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:73: static int s_next_route_id_;
On 2012/12/03 20:02:47, Matt Menke wrote:
> I don't think I've seen a single static variable prefixed with s_ in the
Chrome
> code base.  Doing a quick search seems to verify that this impression is
> accurate.

Done.

[mmenke, 2012-12-04 18:59:16]

Just more nits.  Though it looks like it was a pain, I like that the
PrerenderTracker tests use a bit more of the PrerenderContents mechanism, though
it does make the tests a bit less unit-y.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:371: void
PrerenderContents::set_final_status(FinalStatus final_status) {
This should now be SetFinalStatus, and should have a comment in the h file about
it sending a notification to observers.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:404: void
PrerenderContents::AddObserver(Observer* observer) {
Maybe a "DCHECK_NE(FINAL_STATUS_MAX, final_status_);"?

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:498: observer_list_.Clear();
optional:  Could DCHECK on final_status_ being / not being FINAL_STATUS_MAX in
these two functions, more for documentation than to enforce correctness.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:117: return;
On 2012/12/04 18:04:48, gavinp wrote:
> On 2012/12/03 20:02:47, Matt Menke wrote:
> > nit:  May be a little prettier if these if statements are merged, though
> doesn't
> > even save one line of code. :(
> 
> I'm going to go ahead and ignore this nit from this review, since it doesn't
> save lines, and I find this version prettier. But, if you repeat the nit, I'll
> just follow it.

I'm perfectly happy with the current order.  However, now I think we should add
a NOTREACHED() before both return statements.

In fact...Should we even be checking the return values, except maybe in a
DCHECK?  These shouldn't fail, in release builds.  Suppose there's a chance
process creation could fail, but even in that case, don't think there's any harm
in not early-exiting.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:145: return;
Same as comment above.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:153: base::AutoLock
lock(final_status_map_lock_);
Could also DCHECK on final_status_map_.count(child_route_id_pair).

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.h:130:
std::set<prerender::PrerenderContents*> observing_set_;
Doesn't seem like we need these.  This should exactly match FinalStatusMap.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:88: FinalStatus
final_status) {
nit:  OVERRIDE

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:88: FinalStatus
final_status) {
This should appear adjacent to DestroyPrerenderForRenderView, since they
override methods from the same class.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:362:
EXPECT_FALSE(prerender_tracker()->TryUse(1, 2));
Should use kDefaultChildId + 1, etc. here.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:368: kDefaultRouteId +
1);
Suggest making one of these different (Or, alternatively, making kDefaultRouteId
!= kDefaultChildId).  As it is, if we had them flipped somewhere in the tracker,
all tests would pass.

[gavinp, 2012-12-04 22:26:07]

Looks a lot cleaner. To me, at least. Matt, WDYT?

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:371: void
PrerenderContents::set_final_status(FinalStatus final_status) {
On 2012/12/04 18:59:16, Matt Menke wrote:
> This should now be SetFinalStatus, and should have a comment in the h file
about
> it sending a notification to observers.

It probably should have been SetFinalStatus a while ago, since it wasn't inline.
Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:404: void
PrerenderContents::AddObserver(Observer* observer) {
On 2012/12/04 18:59:16, Matt Menke wrote:
> Maybe a "DCHECK_NE(FINAL_STATUS_MAX, final_status_);"?

ITYM DCHECK_EQ. Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_contents.cc:498: observer_list_.Clear();
On 2012/12/04 18:59:16, Matt Menke wrote:
> optional:  Could DCHECK on final_status_ being / not being FINAL_STATUS_MAX in
> these two functions, more for documentation than to enforce correctness.

Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:117: return;
On 2012/12/04 18:59:16, Matt Menke wrote:
> On 2012/12/04 18:04:48, gavinp wrote:
> > On 2012/12/03 20:02:47, Matt Menke wrote:
> > > nit:  May be a little prettier if these if statements are merged, though
> > doesn't
> > > even save one line of code. :(
> > 
> > I'm going to go ahead and ignore this nit from this review, since it doesn't
> > save lines, and I find this version prettier. But, if you repeat the nit,
I'll
> > just follow it.
> 
> I'm perfectly happy with the current order.  However, now I think we should
add
> a NOTREACHED() before both return statements.
> 
> In fact...Should we even be checking the return values, except maybe in a
> DCHECK?  These shouldn't fail, in release builds.  Suppose there's a chance
> process creation could fail, but even in that case, don't think there's any
harm
> in not early-exiting.

Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:145: return;
On 2012/12/04 18:59:16, Matt Menke wrote:
> Same as comment above.

Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:153: base::AutoLock
lock(final_status_map_lock_);
On 2012/12/04 18:59:16, Matt Menke wrote:
> Could also DCHECK on final_status_map_.count(child_route_id_pair).

I think the DCHECK on num_erased is good enough. Isn't it?

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.h:130:
std::set<prerender::PrerenderContents*> observing_set_;
On 2012/12/04 18:59:16, Matt Menke wrote:
> Doesn't seem like we need these.  This should exactly match FinalStatusMap.

Done, and a good catch.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:88: FinalStatus
final_status) {
On 2012/12/04 18:59:16, Matt Menke wrote:
> This should appear adjacent to DestroyPrerenderForRenderView, since they
> override methods from the same class.

Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:362:
EXPECT_FALSE(prerender_tracker()->TryUse(1, 2));
On 2012/12/04 18:59:16, Matt Menke wrote:
> Should use kDefaultChildId + 1, etc. here.

Done.

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker_unittest.cc:368: kDefaultRouteId +
1);
On 2012/12/04 18:59:16, Matt Menke wrote:
> Suggest making one of these different (Or, alternatively, making
kDefaultRouteId
> != kDefaultChildId).  As it is, if we had them flipped somewhere in the
tracker,
> all tests would pass.

Done.

[mmenke, 2012-12-05 22:22:48]

Sorry, thought I'd signed off on this.  I'll take another skim tonight or (More
likely) tomorrow morning.

[mmenke, 2012-12-06 19:54:21]

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/4003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tracker.cc:153: base::AutoLock
lock(final_status_map_lock_);
On 2012/12/04 22:26:07, gavinp wrote:
> On 2012/12/04 18:59:16, Matt Menke wrote:
> > Could also DCHECK on final_status_map_.count(child_route_id_pair).
> 
> I think the DCHECK on num_erased is good enough. Isn't it?

Oops...Somehow managed to miss that.  Yea, it's just as good.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:291: return;
This is Weird.  We use prerender_manager_->IsControlGroup() for
NotifyPrerenderStop(), but a function argument here.  Should do the same thing
in both cases.  I'd just go with the callback, but caching the value of
is_control_group works, too.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.cc:56: base::AutoLock
lock(final_status_map_lock_);
This is not needed.  If PrerenderTracker is currently being used on another
thread, there's no way to prevent a crash - and I'd actually rather crash, since
that would be a bug.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.cc:135: const bool got_route_id =
prerender_contents->GetRouteId(&route_id);
optional:  Both here and above, const is not too common for
non-pointer/reference variables with function scope, unless they're compile-time
constants.  I guess this is mostly keep line length down, to make code a little
easier to read.

The style guide isn't too clear on this, but I'd argue the prevailing style rule
is pretty clear here.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.h:93: virtual void
OnPrerenderStop(PrerenderContents* prerender_contents) OVERRIDE;
optional:  While still technically accurate, not sure the comments for either of
these functions are still useful, since they're now part of the interface.

Think they're now more along the kind of things you put alongside a DCHECK.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker_unittest.cc:34:
~TestPrerenderContents() {
nit:  virtual

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker_unittest.cc:91: 
nit: Remove blank line.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:272: NotifyPrerenderStart();
We should not be running this in the control group.  This should run into a
DCHECK in debug builds, I'd think.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:273: }
These two methods should be up in the anonymous namespace with
DummyPrerenderContents itself.  Hadn't even realized code like this would
compile.

[gavinp, 2012-12-07 17:37:04]

Matt,

Please take a look. I'm moving back into WebKit land for probably rest of day so
that we don't end up blocking CQ on WebKit gardening for my chrome changes
downstream of this.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:291: return;
On 2012/12/06 19:54:22, Matt Menke wrote:
> This is Weird.  We use prerender_manager_->IsControlGroup() for
> NotifyPrerenderStop(), but a function argument here.  Should do the same thing
> in both cases.  I'd just go with the callback, but caching the value of
> is_control_group works, too.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.cc:56: base::AutoLock
lock(final_status_map_lock_);
On 2012/12/06 19:54:22, Matt Menke wrote:
> This is not needed.  If PrerenderTracker is currently being used on another
> thread, there's no way to prevent a crash - and I'd actually rather crash,
since
> that would be a bug.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.cc:135: const bool got_route_id =
prerender_contents->GetRouteId(&route_id);
On 2012/12/06 19:54:22, Matt Menke wrote:
> optional:  Both here and above, const is not too common for
> non-pointer/reference variables with function scope, unless they're
compile-time
> constants.  I guess this is mostly keep line length down, to make code a
little
> easier to read.
> 
> The style guide isn't too clear on this, but I'd argue the prevailing style
rule
> is pretty clear here.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker.h:93: virtual void
OnPrerenderStop(PrerenderContents* prerender_contents) OVERRIDE;
On 2012/12/06 19:54:22, Matt Menke wrote:
> optional:  While still technically accurate, not sure the comments for either
of
> these functions are still useful, since they're now part of the interface.
> 
> Think they're now more along the kind of things you put alongside a DCHECK.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker_unittest.cc:34:
~TestPrerenderContents() {
On 2012/12/06 19:54:22, Matt Menke wrote:
> nit:  virtual

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tracker_unittest.cc:91: 
On 2012/12/06 19:54:22, Matt Menke wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:272: NotifyPrerenderStart();
On 2012/12/06 19:54:22, Matt Menke wrote:
> We should not be running this in the control group.  This should run into a
> DCHECK in debug builds, I'd think.

Done.

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:273: }
On 2012/12/06 19:54:22, Matt Menke wrote:
> These two methods should be up in the anonymous namespace with
> DummyPrerenderContents itself.  Hadn't even realized code like this would
> compile.

Tricky. The functions reference the UnitTestPrerenderManager, which is not in
the anonymous namespace, and is defined later.

To do this, we'd have to move the definition of UnitTestPrerenderManager to
before the anonymous namespace, and probably also move many methods out of line.
Is it worth that cost?

[mmenke, 2012-12-07 19:21:16]

LGTM

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:273: }
On 2012/12/07 17:37:04, gavinp wrote:
> On 2012/12/06 19:54:22, Matt Menke wrote:
> > These two methods should be up in the anonymous namespace with
> > DummyPrerenderContents itself.  Hadn't even realized code like this would
> > compile.
> 
> Tricky. The functions reference the UnitTestPrerenderManager, which is not in
> the anonymous namespace, and is defined later.
> 
> To do this, we'd have to move the definition of UnitTestPrerenderManager to
> before the anonymous namespace, and probably also move many methods out of
line.
> Is it worth that cost?

Couldn't we just have PrerenderManager defriend UnitTestPrerenderManager, making
functions protected as needed?  Then we could just move the entire test into the
anonymous namespace.

Feel free to leave as-is.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:377: NotifyPrerenderStop();
nit:  Add braces.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:387: origin_, experiment_id_,
match_complete_status_, final_status_);
nit:  experiment_id_ is used here, experiment_id() elsewhere.  I don't care
which you use, but prefer to be consistent.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.h:282: // These call out to methods
on our Observers, using our observer_list_.
Think it's worth mentioning again that NotifyPrerenderStop() clears the list.

[gavinp, 2012-12-08 17:27:48]

Thanks Matt for the review(s)!

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11348357/diff/17001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:273: }
On 2012/12/07 19:21:16, Matt Menke wrote:
> On 2012/12/07 17:37:04, gavinp wrote:
> > On 2012/12/06 19:54:22, Matt Menke wrote:
> > > These two methods should be up in the anonymous namespace with
> > > DummyPrerenderContents itself.  Hadn't even realized code like this would
> > > compile.
> > 
> > Tricky. The functions reference the UnitTestPrerenderManager, which is not
in
> > the anonymous namespace, and is defined later.
> > 
> > To do this, we'd have to move the definition of UnitTestPrerenderManager to
> > before the anonymous namespace, and probably also move many methods out of
> line.
> > Is it worth that cost?
> 
> Couldn't we just have PrerenderManager defriend UnitTestPrerenderManager,
making
> functions protected as needed?  Then we could just move the entire test into
the
> anonymous namespace.
> 
> Feel free to leave as-is.

I think you're right, but it's out of scope for this review, so I'm leaving it
as is. I'll upload another CL to clean up this unit test in the way you
suggested.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:377: NotifyPrerenderStop();
On 2012/12/07 19:21:17, Matt Menke wrote:
> nit:  Add braces.

Done.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:387: origin_, experiment_id_,
match_complete_status_, final_status_);
On 2012/12/07 19:21:17, Matt Menke wrote:
> nit:  experiment_id_ is used here, experiment_id() elsewhere.  I don't care
> which you use, but prefer to be consistent.

Done. I moved the rest of the member references that had a fast accessor to use
that too in this method.

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11348357/diff/32006/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.h:282: // These call out to methods
on our Observers, using our observer_list_.
On 2012/12/07 19:21:17, Matt Menke wrote:
> Think it's worth mentioning again that NotifyPrerenderStop() clears the list.

Done.

[commit-bot: I haz the power, 2012-12-08 18:23:52]

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

[commit-bot: I haz the power, 2012-12-08 22:39:28]

Change committed as 171995