Prepare HistoryProvider for componentization

Prepare HistoryProvider for componentization

Obtain HistoryService and BookmarkModel from AutocompleteProviderClient.

BUG=371538
Committed: https://crrev.com/55e35e874889b9eccb85eb012f91878fd64bab70
Cr-Commit-Position: refs/heads/master@{#334567}

[blundell, 2015-06-12 15:28:09]

blundell@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-06-12 16:20:23]

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:194: profile,
InMemoryURLIndexFactory::GetForProfile(profile)));
It feels weird that right now these classes take both a profile and an
AutocompleteProviderClient that itself takes a profile.

What if this file remained unchanged and these providers themselves constructed
the AutocompleteProviderClients?  When at some point the Profile dependencies
need to be eliminated from these classes as well, we could move the construction
here.  That seems cleaner for now, at least.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:80: return
history_service ? history_service->InMemoryDatabase() : NULL;
It's not immediately obvious why in this function we assume HistoryService() can
fail, but in the next one we assume it can't.

Either comment about why this discrepancy makes sense, or make the two match,
and match whatever is actually reality (i.e. don't null-check unless we truly
should be null-checking).

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.cc:47: :
AutocompleteProvider(type), client_(client.Pass()) {
Tiny nit: I kinda feel like we should reverse the order of the arguments to this
constructor if |type| is needed for the base class but |client| isn't.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:44:
scoped_ptr<AutocompleteProviderClient> client_;
This was a pre-existing bug, but: the Google style guide bans protected data
members.  Make this private, and give subclasses access via one or more
accessors as appropriate.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:112: profile_,
ServiceAccessType::EXPLICIT_ACCESS);
Seems like this ought to be using the client's HistoryService() method?

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:242: new
ChromeAutocompleteProviderClient(profile_.get())),
Note that if you move the ChromeAutocompleteProviderClient construction into the
HQP as I proposed earlier, this change also disappears.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider_unittest.cc:267: new
ChromeAutocompleteProviderClient(profile_.get())),
...as does this change

https://codereview.chromium.org/1185723002/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_provider_client.h (right):

https://codereview.chromium.org/1185723002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_provider_client.h:52: // Returns the
HistoryService.
The order of these functions seems totally random.  For example, this seems like
it is very similar to InMemoryDatabase(), and not at all similar to Classify(),
but the latter is closer than the former.

Maybe these should be grouped: getters for various services/objects, oracle
functions, other utilities.  Or some other form of grouping that makes sense? 
Anything that isn't so random-seeming.

[blundell, 2015-06-12 16:46:59]

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:194: profile,
InMemoryURLIndexFactory::GetForProfile(profile)));
On 2015/06/12 16:20:22, Peter Kasting wrote:
> It feels weird that right now these classes take both a profile and an
> AutocompleteProviderClient that itself takes a profile.
> 
> What if this file remained unchanged and these providers themselves
constructed
> the AutocompleteProviderClients?  When at some point the Profile dependencies
> need to be eliminated from these classes as well, we could move the
construction
> here.  That seems cleaner for now, at least.

I'd prefer keep this structure even though it's kind of odd as I plan on
continuing the componentization such that the Profile dependencies will be
eliminated from the History*Provider subclasses in the near future. However, if
you feel strongly about it I can change to what you suggest.

[Peter Kasting, 2015-06-12 17:02:20]

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:194: profile,
InMemoryURLIndexFactory::GetForProfile(profile)));
On 2015/06/12 16:46:59, blundell wrote:
> On 2015/06/12 16:20:22, Peter Kasting wrote:
> > It feels weird that right now these classes take both a profile and an
> > AutocompleteProviderClient that itself takes a profile.
> > 
> > What if this file remained unchanged and these providers themselves
> constructed
> > the AutocompleteProviderClients?  When at some point the Profile
dependencies
> > need to be eliminated from these classes as well, we could move the
> construction
> > here.  That seems cleaner for now, at least.
> 
> I'd prefer keep this structure even though it's kind of odd as I plan on
> continuing the componentization such that the Profile dependencies will be
> eliminated from the History*Provider subclasses in the near future. However,
if
> you feel strongly about it I can change to what you suggest.

How near is near future?  If it's in the next week or so, this is fine.  Longer
than that and I worry it could get sidelined, in which case the other would be
nice.

This makes me think of a different question.  Should there really be separate
ChromeAutocompleteProviderClient objects for each provider, owned by the
provider?  It seems like a better model would be that the provider would take a
raw pointer to one, which would be a shared client owned by whoever owned the
provider (in this case, the AutocompleteController).  Especially as more
providers get converted, this reduces both boiler plate and overhead.

[blundell, 2015-06-12 19:30:19]

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:194: profile,
InMemoryURLIndexFactory::GetForProfile(profile)));
On 2015/06/12 17:02:20, Peter Kasting wrote:
> On 2015/06/12 16:46:59, blundell wrote:
> > On 2015/06/12 16:20:22, Peter Kasting wrote:
> > > It feels weird that right now these classes take both a profile and an
> > > AutocompleteProviderClient that itself takes a profile.
> > > 
> > > What if this file remained unchanged and these providers themselves
> > constructed
> > > the AutocompleteProviderClients?  When at some point the Profile
> dependencies
> > > need to be eliminated from these classes as well, we could move the
> > construction
> > > here.  That seems cleaner for now, at least.
> > 
> > I'd prefer keep this structure even though it's kind of odd as I plan on
> > continuing the componentization such that the Profile dependencies will be
> > eliminated from the History*Provider subclasses in the near future. However,
> if
> > you feel strongly about it I can change to what you suggest.
> 
> How near is near future?  If it's in the next week or so, this is fine. 
Longer
> than that and I worry it could get sidelined, in which case the other would be
> nice.
> 
> This makes me think of a different question.  Should there really be separate
> ChromeAutocompleteProviderClient objects for each provider, owned by the
> provider?  It seems like a better model would be that the provider would take
a
> raw pointer to one, which would be a shared client owned by whoever owned the
> provider (in this case, the AutocompleteController).  Especially as more
> providers get converted, this reduces both boiler plate and overhead.

"Near future" is within a couple of weeks *unless* there's an unexpected
blocker, so I take your point :).

But I think that your suggested model is actually the right one. OK with making
that change now and having these two classes take in both a Profile* and a
Client* for the transition period?

[Peter Kasting, 2015-06-12 19:31:34]

On 2015/06/12 19:30:19, blundell wrote:
> But I think that your suggested model is actually the right one. OK with
making
> that change now and having these two classes take in both a Profile* and a
> Client* for the transition period?

Wait, I'm confused.  Wasn't my suggested model that they only take a Profile*,
and they construct the Client* internally?

[Peter Kasting, 2015-06-12 19:32:16]

On 2015/06/12 19:31:34, Peter Kasting wrote:
> On 2015/06/12 19:30:19, blundell wrote:
> > But I think that your suggested model is actually the right one. OK with
> making
> > that change now and having these two classes take in both a Profile* and a
> > Client* for the transition period?
> 
> Wait, I'm confused.  Wasn't my suggested model that they only take a Profile*,
> and they construct the Client* internally?

Oh, did you mean my suggestion about the caller owning the client*?  Yeah,
that's fine to have the classes take both in the interim.

[blundell, 2015-06-12 19:32:48]

On 2015/06/12 19:31:34, Peter Kasting wrote:
> On 2015/06/12 19:30:19, blundell wrote:
> > But I think that your suggested model is actually the right one. OK with
> making
> > that change now and having these two classes take in both a Profile* and a
> > Client* for the transition period?
> 
> Wait, I'm confused.  Wasn't my suggested model that they only take a Profile*,
> and they construct the Client* internally?

Apologies for the confusion. I'm referring to this suggestion of yours:

"It seems like a better model would be that the provider would take
a raw pointer to [a Client], which would be a shared client owned by whoever
owned the
provider (in this case, the AutocompleteController)."

[blundell, 2015-06-15 08:06:50]

Patchset #3 (id:40001) has been deleted

[blundell, 2015-06-15 08:24:35]

Patchset #3 (id:60001) has been deleted

[blundell, 2015-06-15 08:51:33]

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:194: profile,
InMemoryURLIndexFactory::GetForProfile(profile)));
On 2015/06/12 19:30:19, blundell wrote:
> On 2015/06/12 17:02:20, Peter Kasting wrote:
> > On 2015/06/12 16:46:59, blundell wrote:
> > > On 2015/06/12 16:20:22, Peter Kasting wrote:
> > > > It feels weird that right now these classes take both a profile and an
> > > > AutocompleteProviderClient that itself takes a profile.
> > > > 
> > > > What if this file remained unchanged and these providers themselves
> > > constructed
> > > > the AutocompleteProviderClients?  When at some point the Profile
> > dependencies
> > > > need to be eliminated from these classes as well, we could move the
> > > construction
> > > > here.  That seems cleaner for now, at least.
> > > 
> > > I'd prefer keep this structure even though it's kind of odd as I plan on
> > > continuing the componentization such that the Profile dependencies will be
> > > eliminated from the History*Provider subclasses in the near future.
However,
> > if
> > > you feel strongly about it I can change to what you suggest.
> > 
> > How near is near future?  If it's in the next week or so, this is fine. 
> Longer
> > than that and I worry it could get sidelined, in which case the other would
be
> > nice.
> > 
> > This makes me think of a different question.  Should there really be
separate
> > ChromeAutocompleteProviderClient objects for each provider, owned by the
> > provider?  It seems like a better model would be that the provider would
take
> a
> > raw pointer to one, which would be a shared client owned by whoever owned
the
> > provider (in this case, the AutocompleteController).  Especially as more
> > providers get converted, this reduces both boiler plate and overhead.
> 
> "Near future" is within a couple of weeks *unless* there's an unexpected
> blocker, so I take your point :).
> 
> But I think that your suggested model is actually the right one. OK with
making
> that change now and having these two classes take in both a Profile* and a
> Client* for the transition period?

Changed to that model.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:80: return
history_service ? history_service->InMemoryDatabase() : NULL;
On 2015/06/12 16:20:23, Peter Kasting wrote:
> It's not immediately obvious why in this function we assume HistoryService()
can
> fail, but in the next one we assume it can't.
> 
> Either comment about why this discrepancy makes sense, or make the two match,
> and match whatever is actually reality (i.e. don't null-check unless we truly
> should be null-checking).

Yes, I thought this was strange as well. It dates back to before this class
existed, and it turns out to be what I guessed as the most likely explanation:
This method is called in a bunch of unittests where the HistoryService isn't
loaded. Added a comment to that effect.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.cc:47: :
AutocompleteProvider(type), client_(client.Pass()) {
On 2015/06/12 16:20:23, Peter Kasting wrote:
> Tiny nit: I kinda feel like we should reverse the order of the arguments to
this
> constructor if |type| is needed for the base class but |client| isn't.

Done.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:44:
scoped_ptr<AutocompleteProviderClient> client_;
On 2015/06/12 16:20:23, Peter Kasting wrote:
> This was a pre-existing bug, but: the Google style guide bans protected data
> members.  Make this private, and give subclasses access via one or more
> accessors as appropriate.

Done.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:112: profile_,
ServiceAccessType::EXPLICIT_ACCESS);
On 2015/06/12 16:20:23, Peter Kasting wrote:
> Seems like this ought to be using the client's HistoryService() method?

Done.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:242: new
ChromeAutocompleteProviderClient(profile_.get())),
On 2015/06/12 16:20:23, Peter Kasting wrote:
> Note that if you move the ChromeAutocompleteProviderClient construction into
the
> HQP as I proposed earlier, this change also disappears.

Obsolete now.

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider_unittest.cc:267: new
ChromeAutocompleteProviderClient(profile_.get())),
On 2015/06/12 16:20:23, Peter Kasting wrote:
> ...as does this change

Obsolete now.

https://codereview.chromium.org/1185723002/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_provider_client.h (right):

https://codereview.chromium.org/1185723002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_provider_client.h:52: // Returns the
HistoryService.
On 2015/06/12 16:20:23, Peter Kasting wrote:
> The order of these functions seems totally random.  For example, this seems
like
> it is very similar to InMemoryDatabase(), and not at all similar to
Classify(),
> but the latter is closer than the former.
> 
> Maybe these should be grouped: getters for various services/objects, oracle
> functions, other utilities.  Or some other form of grouping that makes sense? 
> Anything that isn't so random-seeming.

Agreed. I'll send you a separate CL for this as the reordering would introduce a
bunch of churn in this CL.

[Peter Kasting, 2015-06-16 00:03:43]

LGTM

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:214: new
SearchProvider(this, template_url_service, provider_client_.get());
Nit: If you're going to do it in the history providers above, it probably makes
sense to pass this client arg first in these providers too for consistency.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.h:221:
scoped_ptr<ChromeAutocompleteProviderClient> provider_client_;
Nit: I'd place this above |providers_| since it outlives them and is used when
creating them.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:80: //
loaded.
Tiny nit: I'd put this comment below the next statement.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:47: AutocompleteProviderClient*
client_;
Nit: While here: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:530: if (!client())
Can this ever be null?

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:264: // The provider
client.
Nit: This comment, as well as the similar ones nearby, don't add anything to the
code, and I'd remove them (and the blank lines between these).

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:152: //
ProviderClient we use.
Nit: Same comment as in search_provider_unittest.cc.

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_provider_client.h (right):

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_provider_client.h:55: // Returns the
bookmarks:BookmarkModel
Nit: Trailing period (but I would remove these comments entirely; see my
feedback on your CL to re-order these functions).

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/base...
File components/omnibox/base_search_provider.h (right):

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/base...
components/omnibox/base_search_provider.h:54: AutocompleteProvider::Type type);
Nit: For consistency with HistoryProvider, I'd order these (type, client,
template_url_service).

[blundell, 2015-06-16 07:21:25]

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:214: new
SearchProvider(this, template_url_service, provider_client_.get());
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Nit: If you're going to do it in the history providers above, it probably
makes
> sense to pass this client arg first in these providers too for consistency.

Done.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.h:221:
scoped_ptr<ChromeAutocompleteProviderClient> provider_client_;
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Nit: I'd place this above |providers_| since it outlives them and is used when
> creating them.

Done.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:80: //
loaded.
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Tiny nit: I'd put this comment below the next statement.

Done.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:47: AutocompleteProviderClient*
client_;
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Nit: While here: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_url_provider.cc:530: if (!client())
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Can this ever be null?

No. Removed.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:264: // The provider
client.
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Nit: This comment, as well as the similar ones nearby, don't add anything to
the
> code, and I'd remove them (and the blank lines between these).

Done.

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/1185723002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:152: //
ProviderClient we use.
On 2015/06/16 00:03:42, Peter Kasting wrote:
> Nit: Same comment as in search_provider_unittest.cc.

Done.

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_provider_client.h (right):

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_provider_client.h:55: // Returns the
bookmarks:BookmarkModel
On 2015/06/16 00:03:43, Peter Kasting wrote:
> Nit: Trailing period (but I would remove these comments entirely; see my
> feedback on your CL to re-order these functions).

Added period (I'll do the other change on the reorder CL).

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/base...
File components/omnibox/base_search_provider.h (right):

https://codereview.chromium.org/1185723002/diff/80001/components/omnibox/base...
components/omnibox/base_search_provider.h:54: AutocompleteProvider::Type type);
On 2015/06/16 00:03:43, Peter Kasting wrote:
> Nit: For consistency with HistoryProvider, I'd order these (type, client,
> template_url_service).

Done.

[blundell, 2015-06-16 07:31:50]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-06-16 07:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1185723002/#ps100001
(title: "Response to review")

[commit-bot: I haz the power, 2015-06-16 07:32:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185723002/100001

[commit-bot: I haz the power, 2015-06-16 08:46:31]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-06-16 08:47:14]

Patchset 4 (id:??) landed as
https://crrev.com/55e35e874889b9eccb85eb012f91878fd64bab70
Cr-Commit-Position: refs/heads/master@{#334567}