Introduce ability to register callback with GoogleURLTracker

Introduce ability to register callback with GoogleURLTracker

This callback interface will replace the NOTIFICATION_GOOGLE_URL_UPDATED
notification once all clients have been converted. This CL converts
TemplateURLService and SearchProviderInstallData.

BUG=373236, 373254, 373258, 373260, 373253
TBR=brettw

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

[blundell, 2014-05-15 20:58:00]

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:247:
BrowserThread::PostTask(
This was the most straightforward way that I could find to migrate the test to
the new code organization given that GoogleURLObserver and
GoogleURLChangeNotifier are internal classes to SearchProviderInstallData. Let
me know if you see anything else as a more appealing option.

[Peter Kasting, 2014-05-15 21:12:52]

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:135: return callback_list_.Add(cb);
Add() is not a cheap accessor (based on its name), so please define this
out-of-line.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:189: callback_list_.Notify(old_url,
new_url);
Again, define this out-of-line.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:192: base::CallbackList<void(GURL,
GURL)> callback_list_;
Nit: Consider a typedef for this type and/or base::CallbackList<void(GURL,
GURL)>::Subscription.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:129: void
OnGoogleURLUpdated(GURL old_url, GURL new_url);
Both this function and the TemplateURLService one should be private.  We don't
want external classes randomly calling this method.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:154: if
(google_url_tracker) {
Will this ever actually be NULL?  Perhaps in tests?

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:247:
BrowserThread::PostTask(
On 2014/05/15 20:58:00, blundell wrote:
> This was the most straightforward way that I could find to migrate the test to
> the new code organization given that GoogleURLObserver and
> GoogleURLChangeNotifier are internal classes to SearchProviderInstallData. Let
> me know if you see anything else as a more appealing option.

At least put all this code in a protected helper in
SearchProviderInstallDataTest instead of pasting it twice.

[blundell, 2014-05-16 09:09:55]

Thanks!

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:135: return callback_list_.Add(cb);
On 2014/05/15 21:12:52, Peter Kasting wrote:
> Add() is not a cheap accessor (based on its name), so please define this
> out-of-line.

Done.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:189: callback_list_.Notify(old_url,
new_url);
On 2014/05/15 21:12:52, Peter Kasting wrote:
> Again, define this out-of-line.

Done.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.h:192: base::CallbackList<void(GURL,
GURL)> callback_list_;
On 2014/05/15 21:12:52, Peter Kasting wrote:
> Nit: Consider a typedef for this type and/or base::CallbackList<void(GURL,
> GURL)>::Subscription.

Done.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:129: void
OnGoogleURLUpdated(GURL old_url, GURL new_url);
On 2014/05/15 21:12:52, Peter Kasting wrote:
> Both this function and the TemplateURLService one should be private.  We don't
> want external classes randomly calling this method.

Done.

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:154: if
(google_url_tracker) {
In tests, yep.

On 2014/05/15 21:12:52, Peter Kasting wrote:
> Will this ever actually be NULL?  Perhaps in tests?

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:247:
BrowserThread::PostTask(
On 2014/05/15 21:12:52, Peter Kasting wrote:
> On 2014/05/15 20:58:00, blundell wrote:
> > This was the most straightforward way that I could find to migrate the test
to
> > the new code organization given that GoogleURLObserver and
> > GoogleURLChangeNotifier are internal classes to SearchProviderInstallData.
Let
> > me know if you see anything else as a more appealing option.
> 
> At least put all this code in a protected helper in
> SearchProviderInstallDataTest instead of pasting it twice.

Done.

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/search/in...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/search/in...
chrome/browser/search/instant_unittest_base.cc:75:
TemplateURLServiceFactory::GetForProfile(profile())->OnGoogleURLUpdated(
This change is needed for the instant unittests after the TemplateURLService
change.

[Peter Kasting, 2014-05-16 22:24:23]

LGTM

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:154: if
(google_url_tracker) {
On 2014/05/16 09:09:55, blundell wrote:
> In tests, yep.

Can you add a comment to that effect?

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:138: static const char
kDefaultGoogleHomepage[];
Nit: While here: Google style guide says static consts go right below typedefs
and enums.

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1917: if (loaded_) {
Nit: No {}

[blundell, 2014-05-17 11:32:53]

TBR=brettw for //chrome/browser/search

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://codereview.chromium.org/287103002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:154: if
(google_url_tracker) {
On 2014/05/16 22:24:23, Peter Kasting wrote:
> On 2014/05/16 09:09:55, blundell wrote:
> > In tests, yep.
> 
> Can you add a comment to that effect?

Done.

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/287103002/diff/40002/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:138: static const char
kDefaultGoogleHomepage[];
On 2014/05/16 22:24:23, Peter Kasting wrote:
> Nit: While here: Google style guide says static consts go right below typedefs
> and enums.

Done.

[blundell, 2014-05-17 11:33:34]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-17 11:34:03]

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

[commit-bot: I haz the power, 2014-05-17 19:13:19]

Change committed as 271257