Introduce AutocompleteProviderDelegate

Introduce AutocompleteProviderDelegate

This delegate isolates SearchProvider from chrome dependencies

Add AutocompleteProviderDelegate and its chrome implementation ChromeAutocompleteProviderDelegate.

BUG=388515
TBR=blundell@chromium.org for the new DEPS line

Committed: https://crrev.com/663b9f47b6611de54f4e358d50baf7e91717c41c
Cr-Commit-Position: refs/heads/master@{#291839}

[Peter Kasting, 2014-08-25 19:11:01]

LGTM

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc
(right):

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:20:
Profile* profile) : profile_(profile),
Nit: Put ": profile_(profile)," on a subsequent line

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:75:
HistoryService* const history_service =
Nit: I'd just inline this into the next statement.

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:85: if
(service == NULL ||
Nit: Convert this:

  if (x)
    return false;
  return true;

To this:

  return !x;

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
File components/omnibox/autocomplete_provider_delegate.h (right):

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
components/omnibox/autocomplete_provider_delegate.h:29: virtual
net::URLRequestContextGetter* GetRequestContext() = 0;
Nit: Some of your APIs are called "GetX()" (like this one) and others just "X()"
(like InMemoryDatabase() below).  Be consistent.  (I'd probably just remove all
"Get"s.)

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
components/omnibox/autocomplete_provider_delegate.h:69: // Start prefetching the
image at the given |url|.
Nit: Starts

[hashimoto, 2014-08-26 00:52:30]

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc
(right):

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:20:
Profile* profile) : profile_(profile),
On 2014/08/25 19:11:00, Peter Kasting wrote:
> Nit: Put ": profile_(profile)," on a subsequent line

Done.

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:75:
HistoryService* const history_service =
On 2014/08/25 19:11:00, Peter Kasting wrote:
> Nit: I'd just inline this into the next statement.

Done.

https://codereview.chromium.org/500023002/diff/2/chrome/browser/autocomplete/...
chrome/browser/autocomplete/chrome_autocomplete_provider_delegate.cc:85: if
(service == NULL ||
On 2014/08/25 19:11:00, Peter Kasting wrote:
> Nit: Convert this:
> 
>   if (x)
>     return false;
>   return true;
> 
> To this:
> 
>   return !x;

Done.

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
File components/omnibox/autocomplete_provider_delegate.h (right):

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
components/omnibox/autocomplete_provider_delegate.h:29: virtual
net::URLRequestContextGetter* GetRequestContext() = 0;
On 2014/08/25 19:11:00, Peter Kasting wrote:
> Nit: Some of your APIs are called "GetX()" (like this one) and others just
"X()"
> (like InMemoryDatabase() below).  Be consistent.  (I'd probably just remove
all
> "Get"s.)

Removed "Get"s.

https://codereview.chromium.org/500023002/diff/2/components/omnibox/autocompl...
components/omnibox/autocomplete_provider_delegate.h:69: // Start prefetching the
image at the given |url|.
On 2014/08/25 19:11:00, Peter Kasting wrote:
> Nit: Starts

Done.

[hashimoto, 2014-08-26 02:00:24]

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

[hashimoto, 2014-08-26 02:00:45]

TBRing blundell@chromium.org for the new DEPS line

[hashimoto, 2014-08-26 02:00:50]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-08-26 02:02:13]

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

[commit-bot: I haz the power, 2014-08-26 04:29:44]

Committed patchset #2 (70001) as 133f6adcefb80f87384e779413556e08185e16d8

[blundell, 2014-08-26 07:40:48]

couple of minor comments for a followup. Thanks!

https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
File components/omnibox/autocomplete_provider_delegate.h (right):

https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
components/omnibox/autocomplete_provider_delegate.h:24: class
AutocompleteProviderDelegate {
In keeping with naming conventions of components, this should be
AutocompleteProviderClient (and of course the //chrome impl should be renamed
accordingly).

https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
components/omnibox/autocomplete_provider_delegate.h:24: class
AutocompleteProviderDelegate {
Please provide a class comment.

[hashimoto, 2014-08-27 04:50:59]

https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
File components/omnibox/autocomplete_provider_delegate.h (right):

https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
components/omnibox/autocomplete_provider_delegate.h:24: class
AutocompleteProviderDelegate {
On 2014/08/26 07:40:48, blundell wrote:
> In keeping with naming conventions of components, this should be
> AutocompleteProviderClient (and of course the //chrome impl should be renamed
> accordingly).

In this context, both "client" and "delegate" can fit and "delegate" has been
used for long and more widely in Chromium's codebase.
Is there any strong reason to prefer "client"?

Also, extensions/browser uses "client" for the class which is responsible to
provide embedder's feature required generally by all code under the directory,
while "delegate" is used for classes which provide features needed by each
class.
IMO this distinction makes a lot of sense.

[blundell, 2014-08-27 08:38:14]

On 2014/08/27 04:50:59, hashimoto wrote:
>
https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
> File components/omnibox/autocomplete_provider_delegate.h (right):
> 
>
https://codereview.chromium.org/500023002/diff/70001/components/omnibox/autoc...
> components/omnibox/autocomplete_provider_delegate.h:24: class
> AutocompleteProviderDelegate {
> On 2014/08/26 07:40:48, blundell wrote:
> > In keeping with naming conventions of components, this should be
> > AutocompleteProviderClient (and of course the //chrome impl should be
renamed
> > accordingly).
> 
> In this context, both "client" and "delegate" can fit and "delegate" has been
> used for long and more widely in Chromium's codebase.
> Is there any strong reason to prefer "client"?

Yes, components strictly use "Client" (not "Delegate") for interfaces that the
embedder implements.

> 
> Also, extensions/browser uses "client" for the class which is responsible to
> provide embedder's feature required generally by all code under the directory,
> while "delegate" is used for classes which provide features needed by each
> class.
> IMO this distinction makes a lot of sense.

This is reasonable as well, but not the way that components' conventions have
evolved, so I'd strongly prefer to stick to the existing convention.

[commit-bot: I haz the power, 2014-09-10 02:40:26]

Patchset 2 (id:??) landed as
https://crrev.com/663b9f47b6611de54f4e358d50baf7e91717c41c
Cr-Commit-Position: refs/heads/master@{#291839}