autocomplete: Add AutocompleteProvider::Type enum.

autocomplete: Add AutocompleteProvider::Type enum.

This drops the AutocompleteProvider class's name field in
favor of an enum describing its type.  It also updates
AutocompleteController to take a bitmap describing the types
of providers that it should create, making it easier to use
AutocompleteController in places besides the omnibox.

BUG=141877, 147724
TEST=none
TBR=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156004

[Daniel Erat, 2012-09-07 21:45:55]

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:48: :
AutocompleteProvider(NULL, profile, AutocompleteProvider::TYPE_SEARCH),
I'm using TYPE_SEARCH pretty arbitrarily here.  Let me know if you'd prefer I
added a TYPE_UNKNOWN, TYPE_TEST, or similar value instead.

[Peter Kasting, 2012-09-07 23:23:02]

I think we can also eliminate set_search_provider() by converting
search_provider_unittest.cc to use this new framework.  Instead of keeping a
scoped_refptr() to the SearchProvider we create an AutocompleteController with
only one provider.  Not sure whether we then want to try to convert the rest of
that file's framework to talk only to the controller or whether we still need
the provider directly in some places.

I'm trying to get as close as I can to eliminating the UNIT_TEST constructor and
setters in AutocompleteController entirely, as well as the XXX_provider()
accessors.  I'm not sure how achievable all of that ultimately is.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.cc:27: : controller_(new
AutocompleteController(profile,
Nit: I would probably just wrap after NULL and indent 4.  Same goes for most of
the other files in this CL.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:117: if (provider_types &
AutocompleteProvider::TYPE_HISTORY_CONTENTS)
Nit: {}

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:127: CommandLine* cl =
CommandLine::ForCurrentProcess();
Nit: If you inline this into the conditional, you can combine the nested
conditionals into one.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:136: #ifdef UNIT_TEST
Can we kill these?

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.cc:66:
AsOmniboxEventProviderType() const {
Nit: Can this function at least move to the metrics code?

You can ask mpearson if there's any way to eliminate this entirely and just use
the AutocompleteProvider type enum directly.  (Or maybe the metrics code can
just cast one to the other and then we can use comments or assertions or both to
try and ensure the two stay in sync.)

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider.h (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:179: TYPE_EXTENSION_APP    =
1 << 1,
Nit: I guess this should be EXTENSION_APPS for consistency with existing code

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:190: static const char*
TypeToString(Type type);
Nit: Google style guide says constructors go before static methods.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:228: const char* name()
const { return TypeToString(type_); }
Nit: This isn't a true cheap-accessor, so it should be Name().  Or we can scrap
entirely -- forcing callers to call TypeToString() is OK with me.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:48: :
AutocompleteProvider(NULL, profile, AutocompleteProvider::TYPE_SEARCH),
On 2012/09/07 21:45:55, Daniel Erat wrote:
> I'm using TYPE_SEARCH pretty arbitrarily here.  Let me know if you'd prefer I
> added a TYPE_UNKNOWN, TYPE_TEST, or similar value instead.

This is fine.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:338:
KeywordProvider* keyword_provider = new KeywordProvider(NULL, &profile_);
I think this block of code can be replaced with a simple construction of
AutocompleteController that you pass KEYWORD_PROVIDER to.  This in turn should
eliminate the need for set_keyword_provider().

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/search_builder.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search_builder.cc:196:
AutocompleteClassifier::kDefaultOmniboxProviders;
Nit: You might want at least a TODO about supporting fewer providers here?  For
example, do you want the keyword provider or the zerosuggest provider?

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search_builder.cc:207:
controller_->Start(search_box_->text(), string16(), false, false, true,
Nice simplification!

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:87: :
ALLOW_THIS_IN_INITIALIZER_LIST(
Nit: How about reducing the vertical whitespace slightly to:

    : ALLOW_THIS_IN_INITIALIZER_LIST(autocomplete_controller_(
        new AutocompleteController(profile, this,
            AutocompleteClassifier::kDefaultOmniboxProviders))),

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/home_page_overlay_handler.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/home_page_overlay_handler.cc:40:
AutocompleteClassifier::kDefaultOmniboxProviders));
This file and the other webui/options file probably want to share the same
constant, but I don't know that it should be the "default provider set"
constant.  I think only BUILTIN, the three HISTORY providers, and SEARCH should
be listed here, and SEARCH should have a comment saying we only keep it for
navsuggest results.

[Daniel Erat, 2012-09-08 16:37:36]

FYI xiyuan for app list stuff

CC mpearson for metrics enum question

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.cc:27: : controller_(new
AutocompleteController(profile,
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: I would probably just wrap after NULL and indent 4.  Same goes for most
of
> the other files in this CL.

Done.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:117: if (provider_types &
AutocompleteProvider::TYPE_HISTORY_CONTENTS)
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: {}

Done.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:127: CommandLine* cl =
CommandLine::ForCurrentProcess();
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: If you inline this into the conditional, you can combine the nested
> conditionals into one.

Done (combining the conditionals; kept the |cl| variable since it's used again
later).

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:136: #ifdef UNIT_TEST
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Can we kill these?

Done.

But in a fairly ugly manner: the UNIT_TEST implementation of the c'tor, which
needs to appear in this header, iterates through the passed-in providers and
updates these pointers.  This header includes search_provider.h and
keyword_provider.h so it can do downcasts.  I'm not sure that clang won't
complain about this being a non-trivial inline c'tor.

Let me know if you have other suggestions (hopefully not involving major
reworking of the tests to not use this c'tor -- my end goal is still to check in
my contacts changes!).  I could replace the ifdef with a comment saying not to
use the second form outside of tests and then move the implementation to the .cc
file.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.cc:66:
AsOmniboxEventProviderType() const {
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: Can this function at least move to the metrics code?
> 
> You can ask mpearson if there's any way to eliminate this entirely and just
use
> the AutocompleteProvider type enum directly.  (Or maybe the metrics code can
> just cast one to the other and then we can use comments or assertions or both
to
> try and ensure the two stay in sync.)

SearchProvider::AddProviderInfo() is calling it, too.  Mind if I go with a TODO
for now?

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider.h (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:179: TYPE_EXTENSION_APP    =
1 << 1,
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: I guess this should be EXTENSION_APPS for consistency with existing code

I noticed this too.  The class is ExtensionAppController, though, and the file
is extension_app_provider.h.  I don't see the string being used anywhere, so
I've changed it to "ExtensionApp".

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:190: static const char*
TypeToString(Type type);
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: Google style guide says constructors go before static methods.

Done.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.h:228: const char* name()
const { return TypeToString(type_); }
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: This isn't a true cheap-accessor, so it should be Name().  Or we can
scrap
> entirely -- forcing callers to call TypeToString() is OK with me.

Done.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:338:
KeywordProvider* keyword_provider = new KeywordProvider(NULL, &profile_);
On 2012/09/07 23:23:02, Peter Kasting wrote:
> I think this block of code can be replaced with a simple construction of
> AutocompleteController that you pass KEYWORD_PROVIDER to.  This in turn should
> eliminate the need for set_keyword_provider().

Done.  Think I was misled by the comment above ("Create both a keyword and
search provider").

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/search_builder.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search_builder.cc:196:
AutocompleteClassifier::kDefaultOmniboxProviders;
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: You might want at least a TODO about supporting fewer providers here? 
For
> example, do you want the keyword provider or the zerosuggest provider?

Done.

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:87: :
ALLOW_THIS_IN_INITIALIZER_LIST(
On 2012/09/07 23:23:02, Peter Kasting wrote:
> Nit: How about reducing the vertical whitespace slightly to:
> 
>     : ALLOW_THIS_IN_INITIALIZER_LIST(autocomplete_controller_(
>         new AutocompleteController(profile, this,
>             AutocompleteClassifier::kDefaultOmniboxProviders))),

Done (although this will never look non-broken to me.  The Google vim config
doesn't like it either). :-P

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/home_page_overlay_handler.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/home_page_overlay_handler.cc:40:
AutocompleteClassifier::kDefaultOmniboxProviders));
On 2012/09/07 23:23:02, Peter Kasting wrote:
> This file and the other webui/options file probably want to share the same
> constant, but I don't know that it should be the "default provider set"
> constant.  I think only BUILTIN, the three HISTORY providers, and SEARCH
should
> be listed here, and SEARCH should have a comment saying we only keep it for
> navsuggest results.

I chatted with tbreisacher@google.com about this yesterday.  When I looked at
the list of non-dropped result types in
OptionsUI::ProcessAutocompleteSuggestions() and then looked at which providers
seemed capable of generating them, it ended up being all of them except for the
extension/app provider.  I'd prefer to leave this code as-is, at least within
the scope of this refactoring.

[Mark P, 2012-09-08 22:31:07]

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider.cc (right):

http://codereview.chromium.org/10909130/diff/6001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider.cc:66:
AsOmniboxEventProviderType() const {
On 2012/09/08 16:37:36, Daniel Erat wrote:
> On 2012/09/07 23:23:02, Peter Kasting wrote:
> > Nit: Can this function at least move to the metrics code?
> > 
> > You can ask mpearson if there's any way to eliminate this entirely and just
> use
> > the AutocompleteProvider type enum directly.  (Or maybe the metrics code can
> > just cast one to the other and then we can use comments or assertions or
both
> to
> > try and ensure the two stay in sync.)
> 
> SearchProvider::AddProviderInfo() is calling it, too.  Mind if I go with a
TODO
> for now?

I don't think there's simplification we can do to this at the moment.  We can
change the proto type because it's going in logs and switching logging formats
is a headache (which we've already recently done).  When finally delete the old
XML logging format from the chrome source tree, we should be able to eliminate
AutocompleteProvider::Type entirely and use the proto enum everywhere.

[Peter Kasting, 2012-09-09 01:33:48]

On 2012/09/08 22:31:07, Mark P wrote:
> I don't think there's simplification we can do to this at the moment.  We can
> change the proto type because it's going in logs and switching logging formats
> is a headache (which we've already recently done).  When finally delete the
old
> XML logging format from the chrome source tree, we should be able to eliminate
> AutocompleteProvider::Type entirely and use the proto enum everywhere.

Er, what?  Why would we want to use this random metrics proto type everywhere? 
The whole point of my question was to try and kill the metrics type or else lock
the values in the same order so people can cast across and no one has to refer
to that ugly type anywhere.  Making it widespread would be the opposite of
progress.

[Mark P, 2012-09-09 17:02:09]

On 2012/09/09 01:33:48, Peter Kasting wrote:
> On 2012/09/08 22:31:07, Mark P wrote:
> > I don't think there's simplification we can do to this at the moment.  We
can
> > change the proto type because it's going in logs and switching logging
formats
> > is a headache (which we've already recently done).  When finally delete the
> old
> > XML logging format from the chrome source tree, we should be able to
eliminate
> > AutocompleteProvider::Type entirely and use the proto enum everywhere.
> 
> Er, what?  Why would we want to use this random metrics proto type everywhere?

> The whole point of my question was to try and kill the metrics type or else
lock
> the values in the same order so people can cast across and no one has to refer
> to that ugly type anywhere.  Making it widespread would be the opposite of
> progress.

I agree with you entirely.  It's stupid to have two nearly equivalent enums.  We
should have only one, and it should be defined in AutocompleteProvider.  Don't
blame me about the current situation; I wasn't the one who added the separate
second enum in the metrics code.  But now that it's there we have a problem.  We
cannot change the numbering of the enum in the metrics code, as those enum
values are written into logs and changing those will cause a real headache for
anyone who looks at logs.

If you want, you can make the AutocompleteProvider::Type enum numerically
identical to the metrics one so you can typecast between them.  I'm not sure how
to enforce that.

You can take things one step further and, after making the Autocomplete enum
numerically equivalent to the metrics one, you can eliminate one of the two
duplicate definitions.  If you do this, you have to put very strong comments by
the definition asserting that you must not change the numerical interpretation
of any of the values in the enum.  This is easier to enforce if the enum
definition lives in the metrics code, as that means a metrics person has to look
over and approve changelists that touch it.  (People sadly read big warning
comments less often than they should.)  Obviously, on the other hand, in terms
of a design perspective, the enum best belongs with the rest of the autocomplete
code.

I hope I've sufficiently explained the situation; do with it what you will.

[Peter Kasting, 2012-09-10 17:54:19]

It sounds to me like the right thing to do is to ensure the two are equivalent,
then nuke the one in the metrics code entirely and simply use the values from
the AutocompleteProvider enum directly in the metrics code.  We can add a
comment at that enum definition noting that people must not change any existing
values, we do that at various other places already.

Daniel, it seems like that doesn't need to be done in this change if you want to
keep things simpler.  I could probably make this simplification myself if you
want.  Up to you.

[Daniel Erat, 2012-09-10 18:33:54]

On 2012/09/10 17:54:19, Peter Kasting wrote:
> It sounds to me like the right thing to do is to ensure the two are
equivalent,
> then nuke the one in the metrics code entirely and simply use the values from
> the AutocompleteProvider enum directly in the metrics code.  We can add a
> comment at that enum definition noting that people must not change any
existing
> values, we do that at various other places already.
> 
> Daniel, it seems like that doesn't need to be done in this change if you want
to
> keep things simpler.  I could probably make this simplification myself if you
> want.  Up to you.

Thanks, I'd prefer to save straightening out the metrics for another change. 
Let me know if I should file a bug to track doing this.

Any other comments on this change?

[Peter Kasting, 2012-09-10 18:34:54]

No bug needed if you want to do it yourself.  If you want me to, file a bug
against me.

I'll take another look back through everything here.

[Peter Kasting, 2012-09-11 00:05:39]

LGTM

For any new TODOs you're not going to be handling, go ahead and file bugs.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.cc:28:
kDefaultOmniboxProviders)),
Nit: You have room to indent this even with "profile," so I probably would

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.h (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.h:23: //
AutocompleteController's c'tor.
Tiny nit: "AutocompleteController()" is shorter and avoids less-readable
contractions!

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:89: if (provider_types &
AutocompleteProvider::TYPE_SEARCH) {
Nit: I'd put all these in the same order as the enum.  Added bonus: This also
puts them in the same order as the members (where applicable).

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:62: // .cc file.
Nit: Comment not really necessary

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:63:
AutocompleteController(const ACProviders& providers, Profile* profile)
So what things are left using this?  Can we TODO to kill this constructor
entirely or is it unlikely we can ever manage that?

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:336:
AutocompleteController* controller = new AutocompleteController(&profile_,
Nit: Even shorter:

  controller_.reset(new ...);
  providers_.push_back(controller_->keyword_provider());

[Daniel Erat, 2012-09-11 00:42:19]

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.cc:28:
kDefaultOmniboxProviders)),
On 2012/09/11 00:05:39, Peter Kasting wrote:
> Nit: You have room to indent this even with "profile," so I probably would

Done.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_classifier.h (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_classifier.h:23: //
AutocompleteController's c'tor.
On 2012/09/11 00:05:39, Peter Kasting wrote:
> Tiny nit: "AutocompleteController()" is shorter and avoids less-readable
> contractions!

Good point.  Done.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:89: if (provider_types &
AutocompleteProvider::TYPE_SEARCH) {
On 2012/09/11 00:05:39, Peter Kasting wrote:
> Nit: I'd put all these in the same order as the enum.  Added bonus: This also
> puts them in the same order as the members (where applicable).

Done (had to do a bit of shuffling around since HistoryContentsProvider needs to
know if we're using HistoryQuickProvider).

But now that I remembered that there's a test that claims to be dependent on the
order in which providers are added (using the test version of this constructor),
making this part of the change scares me.  I'm going to revert back to the
non-reordered version, but I'm happy to try to land the reordering (you can see
it in patch set 7) in a separate change if you think it should be safe.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:62: // .cc file.
On 2012/09/11 00:05:39, Peter Kasting wrote:
> Nit: Comment not really necessary

Done.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:63:
AutocompleteController(const ACProviders& providers, Profile* profile)
On 2012/09/11 00:05:39, Peter Kasting wrote:
> So what things are left using this?  Can we TODO to kill this constructor
> entirely or is it unlikely we can ever manage that?

chrome/browser/autocomplete/autocomplete_provider_unittest.cc uses it to pass in
test implementations of AutocompleteProvider, and also to add providers in a
particular order (keyword, then search).

I think that that's it, at this point.

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/8002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:336:
AutocompleteController* controller = new AutocompleteController(&profile_,
On 2012/09/11 00:05:39, Peter Kasting wrote:
> Nit: Even shorter:
> 
>   controller_.reset(new ...);
>   providers_.push_back(controller_->keyword_provider());

Done.

[Daniel Erat, 2012-09-11 01:33:08]

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:71: for
(ACProviders::iterator it = providers_.begin();
It looks like I can remove this loop now, too.

[Peter Kasting, 2012-09-11 01:38:42]

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:93: // History quick
provider can be used on all platforms other than Android.
Nit: I'd move this comment to just below the other comments.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:94: if (provider_types &
AutocompleteProvider::TYPE_HISTORY_QUICK)
Nit: Simpler:

  use_hqp = (provider_types & AutocompleteProvider::TYPE_HISTORY_QUICK);

(May force you to add !! to avoid compiler warning)

You may find it clearer to init the variable to this and then force to false
inside a subsequent OS_ANDROID block, I don't know.  Six of one, half a dozen of
the other.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:104: #if
!defined(OS_ANDROID)
Nit: No need for this #ifdef (handled above)

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:112: // Search
provider/"tab to search" can be used on all platforms other than
Nit: I'd move this comment to just above the #ifdef.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:71: for
(ACProviders::iterator it = providers_.begin();
On 2012/09/11 01:33:08, Daniel Erat wrote:
> It looks like I can remove this loop now, too.

Yep, but I think you can kill this constructor entirely -- see
autocompelte_provider_unittest.cc.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:261: new
AutocompleteController(providers, &profile_);
Wait a minute.  AutocompleteController declares AutocompleteProviderTest as a
friend class (which is how you managed to access its providers directly for
GetProvider()).  So why can't we kill the UNIT_TEST constructor entirely and do
the following here:

  controller_.reset(new AutocompleteController(&profile_, NULL, 0);
  controller_->providers_.swap(providers);

We shouldn't even need to set the |search_provider_| or |keyword_provider_|
members of the controller -- the former because no one will care about it, the
latter because TestProvider is of type SEARCH and thus this was always NULL
before.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:409:
EXPECT_EQ(GetProvider(allow_exact_keyword_match ?
Nit: Simpler, because you can kill GetProvider() entirely:

  EXPECT_EQ(allow_exact_keyword_match ?
      AutocompleteProvider::TYPE_KEYWORD : AutocompleteProvider::TYPE_SEARCH,
      controller_->result().default_match()->provider->type());

[Daniel Erat, 2012-09-11 02:09:22]

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:93: // History quick
provider can be used on all platforms other than Android.
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Nit: I'd move this comment to just below the other comments.

Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:94: if (provider_types &
AutocompleteProvider::TYPE_HISTORY_QUICK)
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Nit: Simpler:
> 
>   use_hqp = (provider_types & AutocompleteProvider::TYPE_HISTORY_QUICK);
> 
> (May force you to add !! to avoid compiler warning)
> 
> You may find it clearer to init the variable to this and then force to false
> inside a subsequent OS_ANDROID block, I don't know.  Six of one, half a dozen
of
> the other.

Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:104: #if
!defined(OS_ANDROID)
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Nit: No need for this #ifdef (handled above)

Ah.  I was thinking that we might not be compiling the class on Android, but
that doesn't make sense; we're including the header above.  Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.cc:112: // Search
provider/"tab to search" can be used on all platforms other than
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Nit: I'd move this comment to just above the #ifdef.

Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_controller.h:71: for
(ACProviders::iterator it = providers_.begin();
On 2012/09/11 01:38:42, Peter Kasting wrote:
> On 2012/09/11 01:33:08, Daniel Erat wrote:
> > It looks like I can remove this loop now, too.
> 
> Yep, but I think you can kill this constructor entirely -- see
> autocompelte_provider_unittest.cc.

Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:261: new
AutocompleteController(providers, &profile_);
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Wait a minute.  AutocompleteController declares AutocompleteProviderTest as a
> friend class (which is how you managed to access its providers directly for
> GetProvider()).  So why can't we kill the UNIT_TEST constructor entirely and
do
> the following here:
> 
>   controller_.reset(new AutocompleteController(&profile_, NULL, 0);
>   controller_->providers_.swap(providers);
> 
> We shouldn't even need to set the |search_provider_| or |keyword_provider_|
> members of the controller -- the former because no one will care about it, the
> latter because TestProvider is of type SEARCH and thus this was always NULL
> before.

Nice!  Done.

http://codereview.chromium.org/10909130/diff/1031/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:409:
EXPECT_EQ(GetProvider(allow_exact_keyword_match ?
On 2012/09/11 01:38:42, Peter Kasting wrote:
> Nit: Simpler, because you can kill GetProvider() entirely:
> 
>   EXPECT_EQ(allow_exact_keyword_match ?
>       AutocompleteProvider::TYPE_KEYWORD : AutocompleteProvider::TYPE_SEARCH,
>       controller_->result().default_match()->provider->type());

Duh.  Done.

[commit-bot: I haz the power, 2012-09-11 04:28:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10909130/6037

[commit-bot: I haz the power, 2012-09-11 04:28:37]

Presubmit check for 10909130-6037 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/extensions/api/omnibox
    chrome/browser/instant
    chrome/browser/metrics

Presubmit checks took 2.6s to calculate.

[Daniel Erat, 2012-09-11 04:37:15]

TBR sky for miscellaneous chrome/browser updates

[commit-bot: I haz the power, 2012-09-11 04:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10909130/6037

[commit-bot: I haz the power, 2012-09-11 06:02:23]

Try job failure for 10909130-6037 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-11 12:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10909130/6037

[commit-bot: I haz the power, 2012-09-11 13:17:02]

Change committed as 156004