Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object.

Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object.

The goal of this CL is to eliminate the dependence of
GoogleURLTracker(MapEntry) on NavigationController. To accomplish this goal,
GoogleURLTrackerNavigationHelper is turned into a conceptually per-tab
interface. GoogleURLTracker::OnNavigationPending() now takes in the
GoogleURLTrackerNavigationHelper with which the navigation is associated rather
than the associated NavigationController; GoogleURLTracker performs per-tab
actions by calling the navigation helper associated with the tab.

A followup CL will turn GoogleURLTrackerNavigationHelper(Impl) into the
GoogleURLTrackerDriver interface and ContentURLTrackerDriver implementation.

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

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

[blundell, 2014-05-15 15:24:14]

Hi Peter,

This is the trickiest part of the componentization. I manually tested that the
infobar still works as expected in a variety of cases by manipulating the code
so that the infobar always showed on a search from the omnibox.

Thanks,

Colin

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.cc:388: if
(map_entry.navigation_helper()->IsListeningForNavigationCommit()) {
This is the core of the change: rather than there being one NavHelper to which
GoogleURLTracker passes the NavigationController of the tab in question, there
is a NavHelper for each tab and GoogleURLTracker directly calls the one of the
tab in question.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.h (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:177: const GoogleURLTrackerMapEntry&
map_entry,
The reason for the removal of const several places is that
GoogleURLTrackerMapEntry is now storing NavigationHelpers on which non-const
methods are being called, vs. NavigationControllers that were just being used as
pointer values.

[Peter Kasting, 2014-05-15 21:29:51]

Do the nav helper impls really have to be owned by the WebContents?  Why can't
they be owned by the map entries?  Then they could be more transient objects.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
File chrome/browser/google/chrome_google_url_tracker_client.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
chrome/browser/google/chrome_google_url_tracker_client.cc:56: //
InfoBarService/NavigationHelper for some notifications, e.g. navigations in
Super-tiny nit: Reverse order of infobar service/nav helper in both this comment
and the conditional, to match the declaration order above and the param order in
the call below.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.cc:44: prefs::kLastKnownGoogleURL)),
Nit: Wrap like this:

      google_url_((mode == UNIT_TEST_MODE) ?
          kDefaultGoogleHomepage :
          profile->GetPrefs()->GetString(prefs::kLastKnownGoogleURL)),

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.h (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:177: const GoogleURLTrackerMapEntry&
map_entry,
On 2014/05/15 15:24:14, blundell wrote:
> The reason for the removal of const several places is that
> GoogleURLTrackerMapEntry is now storing NavigationHelpers on which non-const
> methods are being called, vs. NavigationControllers that were just being used
as
> pointer values.

OK, but then you need to pass these objects as pointers, not non-const refs.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_map_entry.cc (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_map_entry.cc:14: 
Nit: Don't remove this blank line

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.cc:7: #include
<stddef.h>
Nit: See comments on other CL regarding this #include

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper.h (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:14:
GoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
Nit: Explicit

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:15: virtual
~GoogleURLTrackerNavigationHelper() {}
Nit: Put this destructor definition in the .cc file

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:35:
GoogleURLTracker* google_url_tracker() { return tracker_; }
Nit: See comments on other CL regarding accessor/member name harmonization

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:39: };
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:18: void
GoogleURLTrackerNavigationHelperImpl::CreateForWebContentsAndTracker(
Function definition order should match .h declaration order.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:22: return;
Is this conditional actually needed?

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:32: :
GoogleURLTrackerNavigationHelper(tracker), web_contents_(web_contents) {
Nit: One line per initializer

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:91:
DCHECK(controller->GetWebContents() == web_contents_);
Nit: I think this can be DCHECK_EQ()

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.h (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.h:47: };
Nit: While here: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_unittest.cc (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:97: 
Nit: Don't remove this blank line (really, TestGoogleURLTrackerNavigationHelper
should have another blank above it too)

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:134:
TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
Nit: Explicit

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:147: };
Nit: While here: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:257: 
Nit: No blank line here

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:403: if
(GetNavigationHelper(unique_id) &&
Nit: Factor GetNavigationHelper() calls out to a temp

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:412: if
(GetNavigationHelper(unique_id) &&
Nit: Factor GetNavigationHelper() calls out to a temp

[blundell, 2014-05-15 21:37:23]

On 2014/05/15 21:29:51, Peter Kasting wrote:
> Do the nav helper impls really have to be owned by the WebContents?  Why can't
> they be owned by the map entries?  Then they could be more transient objects.
> 

So, a design that looks like this, for example?

- OnNavigationPending() takes in the helper as a scoped_ptr
- ChromeGoogleURLTrackerClient creates a new nav helper impl for each pending
navigation it observes and passes it to the tracker via OnNavigationPending()
- In OnNavigationPending, the tracker lets the nav helper go out of scope if
there's already a map entry and otherwise passes ownership of the nav helper to
the newly-created map entry

That seems like it would work AFAICT. Thoughts?

>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
> File chrome/browser/google/chrome_google_url_tracker_client.cc (right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
> chrome/browser/google/chrome_google_url_tracker_client.cc:56: //
> InfoBarService/NavigationHelper for some notifications, e.g. navigations in
> Super-tiny nit: Reverse order of infobar service/nav helper in both this
comment
> and the conditional, to match the declaration order above and the param order
in
> the call below.
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker.cc (right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker.cc:44: prefs::kLastKnownGoogleURL)),
> Nit: Wrap like this:
> 
>       google_url_((mode == UNIT_TEST_MODE) ?
>           kDefaultGoogleHomepage :
>           profile->GetPrefs()->GetString(prefs::kLastKnownGoogleURL)),
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker.h (left):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker.h:177: const
GoogleURLTrackerMapEntry&
> map_entry,
> On 2014/05/15 15:24:14, blundell wrote:
> > The reason for the removal of const several places is that
> > GoogleURLTrackerMapEntry is now storing NavigationHelpers on which non-const
> > methods are being called, vs. NavigationControllers that were just being
used
> as
> > pointer values.
> 
> OK, but then you need to pass these objects as pointers, not non-const refs.
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_map_entry.cc (left):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_map_entry.cc:14: 
> Nit: Don't remove this blank line
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_navigation_helper.cc (right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper.cc:7: #include
> <stddef.h>
> Nit: See comments on other CL regarding this #include
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_navigation_helper.h (right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper.h:14:
> GoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
> Nit: Explicit
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper.h:15: virtual
> ~GoogleURLTrackerNavigationHelper() {}
> Nit: Put this destructor definition in the .cc file
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper.h:35:
> GoogleURLTracker* google_url_tracker() { return tracker_; }
> Nit: See comments on other CL regarding accessor/member name harmonization
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper.h:39: };
> Nit: DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_navigation_helper_impl.cc
(right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:18: void
> GoogleURLTrackerNavigationHelperImpl::CreateForWebContentsAndTracker(
> Function definition order should match .h declaration order.
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:22: return;
> Is this conditional actually needed?
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:32: :
> GoogleURLTrackerNavigationHelper(tracker), web_contents_(web_contents) {
> Nit: One line per initializer
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:91:
> DCHECK(controller->GetWebContents() == web_contents_);
> Nit: I think this can be DCHECK_EQ()
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_navigation_helper_impl.h
(right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_navigation_helper_impl.h:47: };
> Nit: While here: DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_unittest.cc (left):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:97: 
> Nit: Don't remove this blank line (really,
TestGoogleURLTrackerNavigationHelper
> should have another blank above it too)
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> File chrome/browser/google/google_url_tracker_unittest.cc (right):
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:134:
> TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
> Nit: Explicit
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:147: };
> Nit: While here: DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:257: 
> Nit: No blank line here
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:403: if
> (GetNavigationHelper(unique_id) &&
> Nit: Factor GetNavigationHelper() calls out to a temp
> 
>
https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
> chrome/browser/google/google_url_tracker_unittest.cc:412: if
> (GetNavigationHelper(unique_id) &&
> Nit: Factor GetNavigationHelper() calls out to a temp

[Peter Kasting, 2014-05-15 21:39:15]

On 2014/05/15 21:37:23, blundell wrote:
> On 2014/05/15 21:29:51, Peter Kasting wrote:
> > Do the nav helper impls really have to be owned by the WebContents?  Why
can't
> > they be owned by the map entries?  Then they could be more transient
objects.
> > 
> 
> So, a design that looks like this, for example?
> 
> - OnNavigationPending() takes in the helper as a scoped_ptr
> - ChromeGoogleURLTrackerClient creates a new nav helper impl for each pending
> navigation it observes and passes it to the tracker via OnNavigationPending()
> - In OnNavigationPending, the tracker lets the nav helper go out of scope if
> there's already a map entry and otherwise passes ownership of the nav helper
to
> the newly-created map entry
> 
> That seems like it would work AFAICT. Thoughts?

That sounds about like what I was thinking.

[blundell, 2014-05-16 11:54:17]

Thanks! Changed the NavHelper ownership model to work as we discussed.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
File chrome/browser/google/chrome_google_url_tracker_client.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/chrome...
chrome/browser/google/chrome_google_url_tracker_client.cc:56: //
InfoBarService/NavigationHelper for some notifications, e.g. navigations in
Irrelevant now.

On 2014/05/15 21:29:52, Peter Kasting wrote:
> Super-tiny nit: Reverse order of infobar service/nav helper in both this
comment
> and the conditional, to match the declaration order above and the param order
in
> the call below.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.cc:44: prefs::kLastKnownGoogleURL)),
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Wrap like this:
> 
>       google_url_((mode == UNIT_TEST_MODE) ?
>           kDefaultGoogleHomepage :
>           profile->GetPrefs()->GetString(prefs::kLastKnownGoogleURL)),

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.h (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:177: const GoogleURLTrackerMapEntry&
map_entry,
On 2014/05/15 21:29:52, Peter Kasting wrote:
> On 2014/05/15 15:24:14, blundell wrote:
> > The reason for the removal of const several places is that
> > GoogleURLTrackerMapEntry is now storing NavigationHelpers on which non-const
> > methods are being called, vs. NavigationControllers that were just being
used
> as
> > pointer values.
> 
> OK, but then you need to pass these objects as pointers, not non-const refs.

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_map_entry.cc (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_map_entry.cc:14: 
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Don't remove this blank line

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.cc:7: #include
<stddef.h>
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: See comments on other CL regarding this #include

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper.h (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:14:
GoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Explicit

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:15: virtual
~GoogleURLTrackerNavigationHelper() {}
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Put this destructor definition in the .cc file

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:35:
GoogleURLTracker* google_url_tracker() { return tracker_; }
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: See comments on other CL regarding accessor/member name harmonization

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper.h:39: };
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:18: void
GoogleURLTrackerNavigationHelperImpl::CreateForWebContentsAndTracker(
Function removed.

On 2014/05/15 21:29:52, Peter Kasting wrote:
> Function definition order should match .h declaration order.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:22: return;
Function removed.

On 2014/05/15 21:29:52, Peter Kasting wrote:
> Is this conditional actually needed?

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:32: :
GoogleURLTrackerNavigationHelper(tracker), web_contents_(web_contents) {
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: One line per initializer

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:91:
DCHECK(controller->GetWebContents() == web_contents_);
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: I think this can be DCHECK_EQ()

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.h (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_navigation_helper_impl.h:47: };
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: While here: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_unittest.cc (left):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:97: 
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Don't remove this blank line (really,
TestGoogleURLTrackerNavigationHelper
> should have another blank above it too)

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:134:
TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Explicit

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:147: };
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: While here: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:257: 
Code removed.

On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: No blank line here

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:403: if
(GetNavigationHelper(unique_id) &&
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Factor GetNavigationHelper() calls out to a temp

Done.

https://codereview.chromium.org/283413002/diff/2/chrome/browser/google/google...
chrome/browser/google/google_url_tracker_unittest.cc:412: if
(GetNavigationHelper(unique_id) &&
On 2014/05/15 21:29:52, Peter Kasting wrote:
> Nit: Factor GetNavigationHelper() calls out to a temp

Done.

[Peter Kasting, 2014-05-16 23:38:13]

LGTM

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.cc:312: if
(!map_entry->navigation_helper()->IsListeningForNavigationCommit()) {
Nit: No {}

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.cc (right):

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:78:
DCHECK_EQ(controller->GetWebContents(), web_contents_);
Nit: (expected, actual) (2 places)

[blundell, 2014-05-17 19:23:54]

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.cc:312: if
(!map_entry->navigation_helper()->IsListeningForNavigationCommit()) {
On 2014/05/16 23:38:13, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_navigation_helper_impl.cc (right):

https://codereview.chromium.org/283413002/diff/70001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_navigation_helper_impl.cc:78:
DCHECK_EQ(controller->GetWebContents(), web_contents_);
On 2014/05/16 23:38:13, Peter Kasting wrote:
> Nit: (expected, actual) (2 places)

Done.

[blundell, 2014-05-20 18:39:40]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-20 18:40:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/283413002/110001

[commit-bot: I haz the power, 2014-05-20 19:34:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-20 19:39:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 19:39:24]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[blundell, 2014-05-20 21:18:01]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:19:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/283413002/130001

[commit-bot: I haz the power, 2014-05-21 00:40:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-22 04:25:34]

Change committed as 272094

[blundell, 2014-05-23 12:07:44]

A revert of this CL has been created in
https://codereview.chromium.org/294193005/ by blundell@chromium.org.

The reason for reverting is: Introduced flake on Mac: crbug.com/376550.

[blundell, 2014-05-26 12:19:34]

Hey Peter,

Could you take a look over my change to fix the flaky unittests that caused this
CL to be reverted? The comment explains the fix.

Thanks!

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
chrome/browser/google/google_url_tracker_unittest.cc:317:
google_url_tracker_->Shutdown();
This is the fix, which fixes a latent bug that this CL tickled. GoogleURLTracker
was never removing itself as an IPAddressObserver in these unittests. Another
object adds itself as an IPAddressObserver during TestingProfile shutdown after
the deallocation of the GoogleURLTracker instance, resulting in an ObserverList
DCHECK firing if the address of this second object aliased the address of the
deallocated GoogleURLTracker instance. The changes that my CL made around memory
allocation brought this bug up from the depths.

[Peter Kasting, 2014-05-28 02:17:20]

I will try to review this tomorrow.

[Peter Kasting, 2014-05-28 19:18:50]

LGTM with question

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
chrome/browser/google/google_url_tracker_unittest.cc:317:
google_url_tracker_->Shutdown();
On 2014/05/26 12:19:34, blundell wrote:
> This is the fix, which fixes a latent bug that this CL tickled.
GoogleURLTracker
> was never removing itself as an IPAddressObserver in these unittests. Another
> object adds itself as an IPAddressObserver during TestingProfile shutdown
after
> the deallocation of the GoogleURLTracker instance, resulting in an
ObserverList
> DCHECK firing if the address of this second object aliased the address of the
> deallocated GoogleURLTracker instance. The changes that my CL made around
memory
> allocation brought this bug up from the depths.

Is there some way to use the service factory system to instantiate the
GoogleURLTracker object in SetUp(), instead of constructing one manually, so
that Shutdown() gets called automatically?  That seems both less error-prone and
more like the normal control flow (and thus probably better to test).

[blundell, 2014-05-28 19:19:51]

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/283413002/diff/150001/chrome/browser/google/g...
chrome/browser/google/google_url_tracker_unittest.cc:317:
google_url_tracker_->Shutdown();
I'm going to be componentizing this unittest whereas the factory will stay in
//chrome.

On 2014/05/28 19:18:50, Peter Kasting wrote:
> On 2014/05/26 12:19:34, blundell wrote:
> > This is the fix, which fixes a latent bug that this CL tickled.
> GoogleURLTracker
> > was never removing itself as an IPAddressObserver in these unittests.
Another
> > object adds itself as an IPAddressObserver during TestingProfile shutdown
> after
> > the deallocation of the GoogleURLTracker instance, resulting in an
> ObserverList
> > DCHECK firing if the address of this second object aliased the address of
the
> > deallocated GoogleURLTracker instance. The changes that my CL made around
> memory
> > allocation brought this bug up from the depths.
> 
> Is there some way to use the service factory system to instantiate the
> GoogleURLTracker object in SetUp(), instead of constructing one manually, so
> that Shutdown() gets called automatically?  That seems both less error-prone
and
> more like the normal control flow (and thus probably better to test).

[blundell, 2014-05-29 11:55:30]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-29 11:56:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/283413002/150001

[commit-bot: I haz the power, 2014-05-29 16:32:13]

Change committed as 273490