No chrome dependencies in TemplateURLService

No chrome dependencies in TemplateURLService

No chrome include other than search_engines and webdata.
Inject all required services via ctor.
KeywordHistoryServiceInterface is added to insulate HistoryService.

BUG=386367
TEST=git cl try
TBR=jochen@chromium.org for tests in chrome/browser/{extensions,profile_resetter}

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

[Peter Kasting, 2014-07-02 20:45:57]

LGTM

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/keyword_history_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:15: //
KeywordHistoryService provides keyword related history functionalities.
Nit: functionalities -> functionality

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:22: //
KeywordHistoryServiceInterface overrides:
Nit: No need for "overrides" here

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:31: //
content::NotificationObserver implementation.
Nit: " implementation." -> ":"

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1407: return;
Nit: Can be shortened to two lines by reversing the conditional

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1934:
google_url_tracker_->RequestServerCheck(false);
Nit: This is the second place we've done something like "if (dsp &&
dsp->HasGoogleBaseURLs() && tracker) tracker->RequestServerCheck()".  Maybe this
should be a helper function ("RequestGoogleURLTrackerServerCheckIfNecessary()").

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:47: // This interface
provides history related functionalities required by
Nit: functionalities -> functionality

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:50: class
KeywordHistoryServiceInterface {
Nit: I think this interface should be declared in its own header file.

Also, isn't there some existing naming convention for classes declared in
components/ which are then implemented concretely in chrome/?  I didn't think
"XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.

One of the componentization leads would hopefully know this.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:690:
scoped_ptr<SearchTermsData> search_terms_data_;
Tiny nit: The order of these members feels a little random... we have weak
pointers to other services (e.g. prefs_ and google_url_tracker_) mixed in with
strong pointers that we own.

I wouldn't necessarily worry about this in this change, but if you agree, it
might be worth a small followup change to just go through all this class'
members and attempt to create some sort of logical grouping/ordering.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:897:
TemplateURLService::URLVisitedDetails details;
Nit: You can use {}-initializer form here and below, or add a 2-arg constructor
for URLVisitedDetails and call that.

[hashimoto, 2014-07-03 00:05:07]

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/keyword_history_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:15: //
KeywordHistoryService provides keyword related history functionalities.
On 2014/07/02 20:45:56, Peter Kasting wrote:
> Nit: functionalities -> functionality

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:22: //
KeywordHistoryServiceInterface overrides:
On 2014/07/02 20:45:56, Peter Kasting wrote:
> Nit: No need for "overrides" here

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/keyword_history_service.h:31: //
content::NotificationObserver implementation.
On 2014/07/02 20:45:56, Peter Kasting wrote:
> Nit: " implementation." -> ":"

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1407: return;
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Nit: Can be shortened to two lines by reversing the conditional

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1934:
google_url_tracker_->RequestServerCheck(false);
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Nit: This is the second place we've done something like "if (dsp &&
> dsp->HasGoogleBaseURLs() && tracker) tracker->RequestServerCheck()".  Maybe
this
> should be a helper function
("RequestGoogleURLTrackerServerCheckIfNecessary()").

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:47: // This interface
provides history related functionalities required by
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Nit: functionalities -> functionality

Done.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:50: class
KeywordHistoryServiceInterface {
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Nit: I think this interface should be declared in its own header file.
> 
> Also, isn't there some existing naming convention for classes declared in
> components/ which are then implemented concretely in chrome/?  I didn't think
> "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> 
> One of the componentization leads would hopefully know this.

Some components have "XXXClient" classes which are implmented by chrome as
"ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)

Following this example, renamed this interface to KeywordHistoryService and the
implementation, the former KeywordHistoryService, is renamed to
ChromeKeywordHistoryService.
The interface definition is in its own file.

I'll ask blundell@ for his opinion as well.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:690:
scoped_ptr<SearchTermsData> search_terms_data_;
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Tiny nit: The order of these members feels a little random... we have weak
> pointers to other services (e.g. prefs_ and google_url_tracker_) mixed in with
> strong pointers that we own.
> 
> I wouldn't necessarily worry about this in this change, but if you agree, it
> might be worth a small followup change to just go through all this class'
> members and attempt to create some sort of logical grouping/ordering.

My logic behind this ordering is based on how often tests pass NULL to the ctor.
(It should loosely reflect how important these classes are for
TemplateURLService)
PrefService* and SearchTermsData never be NULL.  Some tests pass non-NULL
pointers as KeywordWebDataService and KeywordHistoryServiceInterface when they
are needed.  GoogleURLTracker, RapporService and dsp_change_callback_ are almost
always NULL during tests.

If you prefer another ordering, I'd be happy to make another CL for it.

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:897:
TemplateURLService::URLVisitedDetails details;
On 2014/07/02 20:45:57, Peter Kasting wrote:
> Nit: You can use {}-initializer form here and below, or add a 2-arg
constructor
> for URLVisitedDetails and call that.

Done.

[Peter Kasting, 2014-07-03 00:12:31]

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:690:
scoped_ptr<SearchTermsData> search_terms_data_;
On 2014/07/03 00:05:07, hashimoto wrote:
> On 2014/07/02 20:45:57, Peter Kasting wrote:
> > Tiny nit: The order of these members feels a little random... we have weak
> > pointers to other services (e.g. prefs_ and google_url_tracker_) mixed in
with
> > strong pointers that we own.
> > 
> > I wouldn't necessarily worry about this in this change, but if you agree, it
> > might be worth a small followup change to just go through all this class'
> > members and attempt to create some sort of logical grouping/ordering.
> 
> My logic behind this ordering is based on how often tests pass NULL to the
ctor.
> (It should loosely reflect how important these classes are for
> TemplateURLService)
> PrefService* and SearchTermsData never be NULL.  Some tests pass non-NULL
> pointers as KeywordWebDataService and KeywordHistoryServiceInterface when they
> are needed.  GoogleURLTracker, RapporService and dsp_change_callback_ are
almost
> always NULL during tests.
> 
> If you prefer another ordering, I'd be happy to make another CL for it.

Hmm.  That ordering is not at all obvious to a reader.

If you want to stick with this, I'd at least add a comment explaining this. 
Personally, it seems highly unusual to me (and not necessarily very useful...
why would a reader care which members have a higher frequency of being NULL?),
but I don't care enough to demand something else.

[hashimoto, 2014-07-03 00:36:24]

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:690:
scoped_ptr<SearchTermsData> search_terms_data_;
On 2014/07/03 00:12:31, Peter Kasting wrote:
> On 2014/07/03 00:05:07, hashimoto wrote:
> > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > Tiny nit: The order of these members feels a little random... we have weak
> > > pointers to other services (e.g. prefs_ and google_url_tracker_) mixed in
> with
> > > strong pointers that we own.
> > > 
> > > I wouldn't necessarily worry about this in this change, but if you agree,
it
> > > might be worth a small followup change to just go through all this class'
> > > members and attempt to create some sort of logical grouping/ordering.
> > 
> > My logic behind this ordering is based on how often tests pass NULL to the
> ctor.
> > (It should loosely reflect how important these classes are for
> > TemplateURLService)
> > PrefService* and SearchTermsData never be NULL.  Some tests pass non-NULL
> > pointers as KeywordWebDataService and KeywordHistoryServiceInterface when
they
> > are needed.  GoogleURLTracker, RapporService and dsp_change_callback_ are
> almost
> > always NULL during tests.
> > 
> > If you prefer another ordering, I'd be happy to make another CL for it.
> 
> Hmm.  That ordering is not at all obvious to a reader.
> 
> If you want to stick with this, I'd at least add a comment explaining this. 
> Personally, it seems highly unusual to me (and not necessarily very useful...
> why would a reader care which members have a higher frequency of being NULL?),
> but I don't care enough to demand something else.
Also this ordering divides these members into 3 categories, the current
browser's state (PrefService and SearchTermsData), dependencies to other
components (web_data_service_, history_service_ and google_url_tracker_) and
metrics/logging (RapporService and dsp_change_callback_).

I have no reason to stick with this, though.
The current ordering is the result of my not-perfectly-accomplished effort to
try to make some sense.
Should I order them based on the ownership model?

[Peter Kasting, 2014-07-03 00:38:58]

On 2014/07/03 00:36:24, hashimoto wrote:
> Also this ordering divides these members into 3 categories, the current
> browser's state (PrefService and SearchTermsData), dependencies to other
> components (web_data_service_, history_service_ and google_url_tracker_) and
> metrics/logging (RapporService and dsp_change_callback_).
> 
> I have no reason to stick with this, though.
> The current ordering is the result of my not-perfectly-accomplished effort to
> try to make some sense.
> Should I order them based on the ownership model?

Again, the important part is that the ordering be comprehensible and useful to a
fairly-inexperienced reader.  If you think this ordering fits that bill, I'd
suggest (again) adding comments about what the sections are.  Otherwise, someone
else isn't going to discern what led you to put things in this order.

This isn't so important on a class that has a half-dozen members, but this class
has a ton.

[hashimoto, 2014-07-03 01:03:14]

On 2014/07/03 00:38:58, Peter Kasting wrote:
> On 2014/07/03 00:36:24, hashimoto wrote:
> > Also this ordering divides these members into 3 categories, the current
> > browser's state (PrefService and SearchTermsData), dependencies to other
> > components (web_data_service_, history_service_ and google_url_tracker_) and
> > metrics/logging (RapporService and dsp_change_callback_).
> > 
> > I have no reason to stick with this, though.
> > The current ordering is the result of my not-perfectly-accomplished effort
to
> > try to make some sense.
> > Should I order them based on the ownership model?
> 
> Again, the important part is that the ordering be comprehensible and useful to
a
> fairly-inexperienced reader.  If you think this ordering fits that bill, I'd
> suggest (again) adding comments about what the sections are.  Otherwise,
someone
> else isn't going to discern what led you to put things in this order.
> 
> This isn't so important on a class that has a half-dozen members, but this
class
> has a ton.

OK, added comments for each section.

[hashimoto, 2014-07-03 07:13:31]

Colin, could you answer my quick question?

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:50: class
KeywordHistoryServiceInterface {
On 2014/07/03 00:05:07, hashimoto wrote:
> On 2014/07/02 20:45:57, Peter Kasting wrote:
> > Nit: I think this interface should be declared in its own header file.
> > 
> > Also, isn't there some existing naming convention for classes declared in
> > components/ which are then implemented concretely in chrome/?  I didn't
think
> > "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> > 
> > One of the componentization leads would hopefully know this.
> 
> Some components have "XXXClient" classes which are implmented by chrome as
> "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> 
> Following this example, renamed this interface to KeywordHistoryService and
the
> implementation, the former KeywordHistoryService, is renamed to
> ChromeKeywordHistoryService.
> The interface definition is in its own file.
> 
> I'll ask blundell@ for his opinion as well.

Colin, I'm going to introduce a new interface KeywordHistoryService to insulate
HistoryService and its chrome implementation is named as
ChromeKeywordHistoryService.
Does this naming sound good to you?

[blundell, 2014-07-03 08:19:54]

On 2014/07/03 07:13:31, hashimoto wrote:
> Colin, could you answer my quick question?
> 
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> File chrome/browser/search_engines/template_url_service.h (right):
> 
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service.h:50: class
> KeywordHistoryServiceInterface {
> On 2014/07/03 00:05:07, hashimoto wrote:
> > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > Nit: I think this interface should be declared in its own header file.
> > > 
> > > Also, isn't there some existing naming convention for classes declared in
> > > components/ which are then implemented concretely in chrome/?  I didn't
> think
> > > "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> > > 
> > > One of the componentization leads would hopefully know this.
> > 
> > Some components have "XXXClient" classes which are implmented by chrome as
> > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > 
> > Following this example, renamed this interface to KeywordHistoryService and
> the
> > implementation, the former KeywordHistoryService, is renamed to
> > ChromeKeywordHistoryService.
> > The interface definition is in its own file.
> > 
> > I'll ask blundell@ for his opinion as well.
> 
> Colin, I'm going to introduce a new interface KeywordHistoryService to
insulate
> HistoryService and its chrome implementation is named as
> ChromeKeywordHistoryService.
> Does this naming sound good to you?

Hi,

It looks like KeywordHistoryService is the interface via which the
search_engines component gets information from its embedder (at the time being,
that information happens to all be related to history). The standard name for
this would be as follows:

SearchEnginesClient
ChromeSearchEnginesClient

Then you can put whatever other information you need to get from the embedder
there as well. If after history is componentized the interface is empty we can
tear it out.

[hashimoto, 2014-07-03 08:35:34]

On 2014/07/03 08:19:54, blundell wrote:
> On 2014/07/03 07:13:31, hashimoto wrote:
> > Colin, could you answer my quick question?
> > 
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > File chrome/browser/search_engines/template_url_service.h (right):
> > 
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > chrome/browser/search_engines/template_url_service.h:50: class
> > KeywordHistoryServiceInterface {
> > On 2014/07/03 00:05:07, hashimoto wrote:
> > > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > > Nit: I think this interface should be declared in its own header file.
> > > > 
> > > > Also, isn't there some existing naming convention for classes declared
in
> > > > components/ which are then implemented concretely in chrome/?  I didn't
> > think
> > > > "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> > > > 
> > > > One of the componentization leads would hopefully know this.
> > > 
> > > Some components have "XXXClient" classes which are implmented by chrome as
> > > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > > 
> > > Following this example, renamed this interface to KeywordHistoryService
and
> > the
> > > implementation, the former KeywordHistoryService, is renamed to
> > > ChromeKeywordHistoryService.
> > > The interface definition is in its own file.
> > > 
> > > I'll ask blundell@ for his opinion as well.
> > 
> > Colin, I'm going to introduce a new interface KeywordHistoryService to
> insulate
> > HistoryService and its chrome implementation is named as
> > ChromeKeywordHistoryService.
> > Does this naming sound good to you?
> 
> Hi,
> 
> It looks like KeywordHistoryService is the interface via which the
> search_engines component gets information from its embedder (at the time
being,
> that information happens to all be related to history). The standard name for
> this would be as follows:
> 
> SearchEnginesClient
> ChromeSearchEnginesClient
> 
> Then you can put whatever other information you need to get from the embedder
> there as well. If after history is componentized the interface is empty we can
> tear it out.
Hi, thank you for your quick reply.

I thought SearchEnginesClient would be too general because this class only
serves to TemplateURLService.
(Maybe I shouldn't worry about this as almost all code in the search_engines
directory is related to TemplateURLService)
Also, unlike HistoryClient, this class is owned by TemplateURLService itself,
not by the embedder.
Is it OK to use a name XXXClient for a class which is not owned by the embedder?

[blundell, 2014-07-03 08:36:40]

On 2014/07/03 08:35:34, hashimoto wrote:
> On 2014/07/03 08:19:54, blundell wrote:
> > On 2014/07/03 07:13:31, hashimoto wrote:
> > > Colin, could you answer my quick question?
> > > 
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > File chrome/browser/search_engines/template_url_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > chrome/browser/search_engines/template_url_service.h:50: class
> > > KeywordHistoryServiceInterface {
> > > On 2014/07/03 00:05:07, hashimoto wrote:
> > > > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > > > Nit: I think this interface should be declared in its own header file.
> > > > > 
> > > > > Also, isn't there some existing naming convention for classes declared
> in
> > > > > components/ which are then implemented concretely in chrome/?  I
didn't
> > > think
> > > > > "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> > > > > 
> > > > > One of the componentization leads would hopefully know this.
> > > > 
> > > > Some components have "XXXClient" classes which are implmented by chrome
as
> > > > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > > > 
> > > > Following this example, renamed this interface to KeywordHistoryService
> and
> > > the
> > > > implementation, the former KeywordHistoryService, is renamed to
> > > > ChromeKeywordHistoryService.
> > > > The interface definition is in its own file.
> > > > 
> > > > I'll ask blundell@ for his opinion as well.
> > > 
> > > Colin, I'm going to introduce a new interface KeywordHistoryService to
> > insulate
> > > HistoryService and its chrome implementation is named as
> > > ChromeKeywordHistoryService.
> > > Does this naming sound good to you?
> > 
> > Hi,
> > 
> > It looks like KeywordHistoryService is the interface via which the
> > search_engines component gets information from its embedder (at the time
> being,
> > that information happens to all be related to history). The standard name
for
> > this would be as follows:
> > 
> > SearchEnginesClient
> > ChromeSearchEnginesClient
> > 
> > Then you can put whatever other information you need to get from the
embedder
> > there as well. If after history is componentized the interface is empty we
can
> > tear it out.
> Hi, thank you for your quick reply.
> 
> I thought SearchEnginesClient would be too general because this class only
> serves to TemplateURLService.
> (Maybe I shouldn't worry about this as almost all code in the search_engines
> directory is related to TemplateURLService)
> Also, unlike HistoryClient, this class is owned by TemplateURLService itself,
> not by the embedder.
> Is it OK to use a name XXXClient for a class which is not owned by the
embedder?

It's passed to TemplateURLService as a scoped_ptr I assume?

[hashimoto, 2014-07-03 08:38:04]

On 2014/07/03 08:36:40, blundell wrote:
> On 2014/07/03 08:35:34, hashimoto wrote:
> > On 2014/07/03 08:19:54, blundell wrote:
> > > On 2014/07/03 07:13:31, hashimoto wrote:
> > > > Colin, could you answer my quick question?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > File chrome/browser/search_engines/template_url_service.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > chrome/browser/search_engines/template_url_service.h:50: class
> > > > KeywordHistoryServiceInterface {
> > > > On 2014/07/03 00:05:07, hashimoto wrote:
> > > > > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > > > > Nit: I think this interface should be declared in its own header
file.
> > > > > > 
> > > > > > Also, isn't there some existing naming convention for classes
declared
> > in
> > > > > > components/ which are then implemented concretely in chrome/?  I
> didn't
> > > > think
> > > > > > "XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.
> > > > > > 
> > > > > > One of the componentization leads would hopefully know this.
> > > > > 
> > > > > Some components have "XXXClient" classes which are implmented by
chrome
> as
> > > > > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > > > > 
> > > > > Following this example, renamed this interface to
KeywordHistoryService
> > and
> > > > the
> > > > > implementation, the former KeywordHistoryService, is renamed to
> > > > > ChromeKeywordHistoryService.
> > > > > The interface definition is in its own file.
> > > > > 
> > > > > I'll ask blundell@ for his opinion as well.
> > > > 
> > > > Colin, I'm going to introduce a new interface KeywordHistoryService to
> > > insulate
> > > > HistoryService and its chrome implementation is named as
> > > > ChromeKeywordHistoryService.
> > > > Does this naming sound good to you?
> > > 
> > > Hi,
> > > 
> > > It looks like KeywordHistoryService is the interface via which the
> > > search_engines component gets information from its embedder (at the time
> > being,
> > > that information happens to all be related to history). The standard name
> for
> > > this would be as follows:
> > > 
> > > SearchEnginesClient
> > > ChromeSearchEnginesClient
> > > 
> > > Then you can put whatever other information you need to get from the
> embedder
> > > there as well. If after history is componentized the interface is empty we
> can
> > > tear it out.
> > Hi, thank you for your quick reply.
> > 
> > I thought SearchEnginesClient would be too general because this class only
> > serves to TemplateURLService.
> > (Maybe I shouldn't worry about this as almost all code in the search_engines
> > directory is related to TemplateURLService)
> > Also, unlike HistoryClient, this class is owned by TemplateURLService
itself,
> > not by the embedder.
> > Is it OK to use a name XXXClient for a class which is not owned by the
> embedder?
> 
> It's passed to TemplateURLService as a scoped_ptr I assume?

Yes, it's owned by TemplateURLService, instead of being owned by an embedder
specific factory instance.

[blundell, 2014-07-03 08:49:13]

On 2014/07/03 08:38:04, hashimoto wrote:
> On 2014/07/03 08:36:40, blundell wrote:
> > On 2014/07/03 08:35:34, hashimoto wrote:
> > > On 2014/07/03 08:19:54, blundell wrote:
> > > > On 2014/07/03 07:13:31, hashimoto wrote:
> > > > > Colin, could you answer my quick question?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > > File chrome/browser/search_engines/template_url_service.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > > chrome/browser/search_engines/template_url_service.h:50: class
> > > > > KeywordHistoryServiceInterface {
> > > > > On 2014/07/03 00:05:07, hashimoto wrote:
> > > > > > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > > > > > Nit: I think this interface should be declared in its own header
> file.
> > > > > > > 
> > > > > > > Also, isn't there some existing naming convention for classes
> declared
> > > in
> > > > > > > components/ which are then implemented concretely in chrome/?  I
> > didn't
> > > > > think
> > > > > > > "XXXInterface"/"XXX" was the convention there, but maybe I'm
wrong.
> > > > > > > 
> > > > > > > One of the componentization leads would hopefully know this.
> > > > > > 
> > > > > > Some components have "XXXClient" classes which are implmented by
> chrome
> > as
> > > > > > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > > > > > 
> > > > > > Following this example, renamed this interface to
> KeywordHistoryService
> > > and
> > > > > the
> > > > > > implementation, the former KeywordHistoryService, is renamed to
> > > > > > ChromeKeywordHistoryService.
> > > > > > The interface definition is in its own file.
> > > > > > 
> > > > > > I'll ask blundell@ for his opinion as well.
> > > > > 
> > > > > Colin, I'm going to introduce a new interface KeywordHistoryService to
> > > > insulate
> > > > > HistoryService and its chrome implementation is named as
> > > > > ChromeKeywordHistoryService.
> > > > > Does this naming sound good to you?
> > > > 
> > > > Hi,
> > > > 
> > > > It looks like KeywordHistoryService is the interface via which the
> > > > search_engines component gets information from its embedder (at the time
> > > being,
> > > > that information happens to all be related to history). The standard
name
> > for
> > > > this would be as follows:
> > > > 
> > > > SearchEnginesClient
> > > > ChromeSearchEnginesClient
> > > > 
> > > > Then you can put whatever other information you need to get from the
> > embedder
> > > > there as well. If after history is componentized the interface is empty
we
> > can
> > > > tear it out.
> > > Hi, thank you for your quick reply.
> > > 
> > > I thought SearchEnginesClient would be too general because this class only
> > > serves to TemplateURLService.
> > > (Maybe I shouldn't worry about this as almost all code in the
search_engines
> > > directory is related to TemplateURLService)
> > > Also, unlike HistoryClient, this class is owned by TemplateURLService
> itself,
> > > not by the embedder.
> > > Is it OK to use a name XXXClient for a class which is not owned by the
> > embedder?
> > 
> > It's passed to TemplateURLService as a scoped_ptr I assume?
> 
> Yes, it's owned by TemplateURLService, instead of being owned by an embedder
> specific factory instance.

Yes, that's fine. You can call it TemplateURLServiceClient if you think it will
only be used by TemplateURLService (GoogleURLTrackerClient is an existing
instance of that naming pattern in the codebase).

[hashimoto, 2014-07-03 09:05:19]

On 2014/07/03 08:49:13, blundell wrote:
> On 2014/07/03 08:38:04, hashimoto wrote:
> > On 2014/07/03 08:36:40, blundell wrote:
> > > On 2014/07/03 08:35:34, hashimoto wrote:
> > > > On 2014/07/03 08:19:54, blundell wrote:
> > > > > On 2014/07/03 07:13:31, hashimoto wrote:
> > > > > > Colin, could you answer my quick question?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > > > File chrome/browser/search_engines/template_url_service.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/367863005/diff/130001/chrome/browser/search_e...
> > > > > > chrome/browser/search_engines/template_url_service.h:50: class
> > > > > > KeywordHistoryServiceInterface {
> > > > > > On 2014/07/03 00:05:07, hashimoto wrote:
> > > > > > > On 2014/07/02 20:45:57, Peter Kasting wrote:
> > > > > > > > Nit: I think this interface should be declared in its own header
> > file.
> > > > > > > > 
> > > > > > > > Also, isn't there some existing naming convention for classes
> > declared
> > > > in
> > > > > > > > components/ which are then implemented concretely in chrome/?  I
> > > didn't
> > > > > > think
> > > > > > > > "XXXInterface"/"XXX" was the convention there, but maybe I'm
> wrong.
> > > > > > > > 
> > > > > > > > One of the componentization leads would hopefully know this.
> > > > > > > 
> > > > > > > Some components have "XXXClient" classes which are implmented by
> > chrome
> > > as
> > > > > > > "ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)
> > > > > > > 
> > > > > > > Following this example, renamed this interface to
> > KeywordHistoryService
> > > > and
> > > > > > the
> > > > > > > implementation, the former KeywordHistoryService, is renamed to
> > > > > > > ChromeKeywordHistoryService.
> > > > > > > The interface definition is in its own file.
> > > > > > > 
> > > > > > > I'll ask blundell@ for his opinion as well.
> > > > > > 
> > > > > > Colin, I'm going to introduce a new interface KeywordHistoryService
to
> > > > > insulate
> > > > > > HistoryService and its chrome implementation is named as
> > > > > > ChromeKeywordHistoryService.
> > > > > > Does this naming sound good to you?
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > It looks like KeywordHistoryService is the interface via which the
> > > > > search_engines component gets information from its embedder (at the
time
> > > > being,
> > > > > that information happens to all be related to history). The standard
> name
> > > for
> > > > > this would be as follows:
> > > > > 
> > > > > SearchEnginesClient
> > > > > ChromeSearchEnginesClient
> > > > > 
> > > > > Then you can put whatever other information you need to get from the
> > > embedder
> > > > > there as well. If after history is componentized the interface is
empty
> we
> > > can
> > > > > tear it out.
> > > > Hi, thank you for your quick reply.
> > > > 
> > > > I thought SearchEnginesClient would be too general because this class
only
> > > > serves to TemplateURLService.
> > > > (Maybe I shouldn't worry about this as almost all code in the
> search_engines
> > > > directory is related to TemplateURLService)
> > > > Also, unlike HistoryClient, this class is owned by TemplateURLService
> > itself,
> > > > not by the embedder.
> > > > Is it OK to use a name XXXClient for a class which is not owned by the
> > > embedder?
> > > 
> > > It's passed to TemplateURLService as a scoped_ptr I assume?
> > 
> > Yes, it's owned by TemplateURLService, instead of being owned by an embedder
> > specific factory instance.
> 
> Yes, that's fine. You can call it TemplateURLServiceClient if you think it
will
> only be used by TemplateURLService (GoogleURLTrackerClient is an existing
> instance of that naming pattern in the codebase).

OK, renamed it to TemplateURLServiceClient/ChromeTemplateURLServiceClient.
Thanks!

[hashimoto, 2014-07-03 10:11:13]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-03 10:11:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/367863005/230001

[hashimoto, 2014-07-03 11:11:24]

TBRing jochen@chromium.org for tests in
chrome/browser/{extensions,profile_resetter}

[hashimoto, 2014-07-03 11:11:30]

The CQ bit was unchecked by hashimoto@chromium.org

[hashimoto, 2014-07-03 11:11:33]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-03 11:12:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/367863005/230001

[commit-bot: I haz the power, 2014-07-03 12:44:49]

Change committed as 281256