Omnibox: Combine Two Input Type Enums into One

Omnibox: Combine Two Input Type Enums into One

There are two input type enums:
* one in the autocomplete code
* one in the metrics code that's used for UMA logging;
  this one is meant to remain stable

As part of fixing the linked bug, I created
https://codereview.chromium.org/314773002/
whch needed an input type enum, one which would remain stable.
It didn't necessarily have to be the metrics enum.

After discussion with the UMA folks (who deal with how to handle
stable enum problems all the time), we reached the conclusion
that we should have one stable enum and use it everywhere.  That's
the cleanest answer.

This single enum has to live in the metrics directory because metrics
is a component.

This change combines the two existing enums into one, putting the
new enum in a new file in the metrics directory.  The reason for
a new file is so we can include it without include the whole
OmniboxEventProto or any other metrics code.

The main files to review are autocomplete_input.h and *.proto.
All other changes are a mechanical result of the changes in
those three files.

The internal proto change has been submitted.

TBR=stevenjb
for the trivial change to chrome/browser/ui/app_list/search/omnibox_provider.cc
that removes an unnecessary include

BUG=284781
NOTRY=True

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

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

[Mark P, 2014-06-06 00:35:46]

Guys, please take a look.  Thanks!  --mark

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
File components/metrics/proto/omnibox_input_type.proto (right):

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
components/metrics/proto/omnibox_input_type.proto:14: // Note that the type
below may be misleading.  For example, "http:/" alone
This "Note" came from AutocompleteInput::Type.  The rest of the comment (the
first sentence and the interior variables and their comments) came from
OmniboxEventProto::InputType.

[Ilya Sherman, 2014-06-06 05:01:02]

Thanks, Mark.  I'm not super convinced that this is so much better than what we
had before, but I'm ok with it if you and Peter both want this.  (My personal
vote on that thread would have been to just have separate enums for separate
contexts.  I don't personally find the conversion functions so unreasonable,
though I agree that they're a little silly.)

Note: There is actually a difference between the two enums that you are merging:
one has a deprecated item in the middle of the list.  Therefore, you couldn't
have just directly logged the Chrome enum to the metrics server without creating
an incompatibility between metrics uploaded by older and newer versions of
Chrome.  Just sayin' :P

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:124: return std::string();
I'd prefer that you remove the default case, so that the compiler can remind you
if this ever needs to be updated.

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.h:122:
metrics::OmniboxInputType::Type type, base::string16* text);
Optional nit: Within the scope of the class, you could "typedef
metrics::OmniboxInputType::Type Type;" if you wanted to.

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1002: return 0;
I'd prefer that you omit the default case here, so that the compiler can remind
you if there are any new cases to consider.

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:142:
metrics::OmniboxInputType::Type type, bool prefer_keyword);
nit: In header files, params should always be one per line if the entire method
signature does not fit on a single line.  You might want to run "git cl format"
over your CL (and double check that its output is reasonable).

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
File components/metrics/proto/omnibox_input_type.proto (right):

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
components/metrics/proto/omnibox_input_type.proto:23: // desired_tld
Hmm, it's a little awkward to expose this deprecated value to Chrome code. 
Perhaps we should at least rename it to DEPRECATED_REQUESTED_URL?

[Peter Kasting, 2014-06-06 18:13:07]

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.h:122:
metrics::OmniboxInputType::Type type, base::string16* text);
On 2014/06/06 05:01:01, Ilya Sherman wrote:
> Optional nit: Within the scope of the class, you could "typedef
> metrics::OmniboxInputType::Type Type;" if you wanted to.

I think this would be a good idea.  It would eliminate most of the changes in
this file and reduce the impact of the changes in the .cc file.

This would also let you change other files that current do
"AutocompleteInput::FOO" to "AutocompleteInput::Type::FOO" instead of referring
to metrics::OmniboxInputType or pulling in any additional headers, which I think
would also be clearer.

[Mark P, 2014-06-06 20:22:51]

Please review my comments / take another look.

Also,
>>>
Note: There is actually a difference between the two enums that you are merging:
one has a deprecated item in the middle of the list.  Therefore, you couldn't
have just directly logged the Chrome enum to the metrics server without creating
an incompatibility between metrics uploaded by older and newer versions of
Chrome.  Just sayin' :P
>>>
Yes, I noticed.  :-)  That's why I used the metrics enum as the canonical.

--mark

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:124: return std::string();
On 2014/06/06 05:01:01, Ilya Sherman wrote:
> I'd prefer that you remove the default case, so that the compiler can remind
you
> if this ever needs to be updated.

Done.  Like in the other place, note that I add to add DEPRECATED_REQUESTED_URL
and INVALID to the case list here.  Without the default block, I also needed to
add a return at the end of the function (outside the switch).  With these
changes, I'm not sure this is an improvement.  What do you think?

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.h:122:
metrics::OmniboxInputType::Type type, base::string16* text);
On 2014/06/06 18:13:07, Peter Kasting wrote:
> On 2014/06/06 05:01:01, Ilya Sherman wrote:
> > Optional nit: Within the scope of the class, you could "typedef
> > metrics::OmniboxInputType::Type Type;" if you wanted to.
> 
> I think this would be a good idea.  It would eliminate most of the changes in
> this file and reduce the impact of the changes in the .cc file.

I hoped I would get such a sweet suggestion.  Thanks Ilya!  Done.  This
shortened a lot of code everywhere.

> This would also let you change other files that current do
> "AutocompleteInput::FOO" to "AutocompleteInput::Type::FOO" instead of
referring
> to metrics::OmniboxInputType or pulling in any additional headers, which I
think
> would also be clearer.

This change doesn't let me AutocompleteInput::Type::FOO.
"../../chrome/browser/autocomplete/search_provider.cc:180:38: error:
‘AutocompleteInput::Type’ is not a class or namespace"

Nor does it work without the Type:: blip.
../../chrome/browser/autocomplete/search_provider.cc:180:19: error: ‘QUERY’ is
not a member of ‘AutocompleteInput’

Know of a way to get the enum values to be visible without AutocompleteInput's
scope?

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1002: return 0;
On 2014/06/06 05:01:01, Ilya Sherman wrote:
> I'd prefer that you omit the default case here, so that the compiler can
remind
> you if there are any new cases to consider.

Done.  Note that I add to add DEPRECATED_REQUESTED_URL and INVALID to the case
list here.  Without the default block, I also needed to add a return at the end
of the function (outside the switch).  With these changes, I'm not sure this is
an improvement.  What do you think?

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:142:
metrics::OmniboxInputType::Type type, bool prefer_keyword);
On 2014/06/06 05:01:01, Ilya Sherman wrote:
> nit: In header files, params should always be one per line if the entire
method
> signature does not fit on a single line.

Done.

> You might want to run "git cl format"
> over your CL (and double check that its output is reasonable).

I ran it.  It didn't do much.  I accepted a suggestion or two.  Mostly it
disagrees with current code about how to format ternary operators.

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
File components/metrics/proto/omnibox_input_type.proto (right):

https://codereview.chromium.org/319523005/diff/40001/components/metrics/proto...
components/metrics/proto/omnibox_input_type.proto:23: // desired_tld
On 2014/06/06 05:01:02, Ilya Sherman wrote:
> Hmm, it's a little awkward to expose this deprecated value to Chrome code. 
> Perhaps we should at least rename it to DEPRECATED_REQUESTED_URL?

Done.

Because this longer enum name made it ugly to vertically align comments, I
reformatted the comments to align left and go above the relevant enum name.

[Ilya Sherman, 2014-06-06 20:29:24]

LGTM % nits.  Thanks.

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:124: return std::string();
On 2014/06/06 20:22:51, Mark P wrote:
> On 2014/06/06 05:01:01, Ilya Sherman wrote:
> > I'd prefer that you remove the default case, so that the compiler can remind
> you
> > if this ever needs to be updated.
> 
> Done.  Like in the other place, note that I add to add
DEPRECATED_REQUESTED_URL
> and INVALID to the case list here.  Without the default block, I also needed
to
> add a return at the end of the function (outside the switch).  With these
> changes, I'm not sure this is an improvement.  What do you think?

Yes, I think it's still an improvement.  The code is slightly longer, but not in
a way that much harms readability; and you get the benefit that the compiler
will help you avoid bugs in the future.  IMO that's a great tradeoff.

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:123: return "";
nit: s/""/std::string()

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1002: ;
nit: I'd have a break or a return.  Just a semicolon looks pretty odd.

[Peter Kasting, 2014-06-06 20:37:08]

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.h:122:
metrics::OmniboxInputType::Type type, base::string16* text);
On 2014/06/06 20:22:51, Mark P wrote:
> This change doesn't let me AutocompleteInput::Type::FOO.
> "../../chrome/browser/autocomplete/search_provider.cc:180:38: error:
> ‘AutocompleteInput::Type’ is not a class or namespace"

That's strange.  The following code compiles for me in MSVC 2013:

class A {
public:
  enum Type { ONE, TWO };
};

class B {
public:
  typedef A::Type Foo;
};

void foo() {
  B::Foo foo = B::Foo::ONE;
}

I wonder if this is non-standard, or C++11-only, or something?

I don't have any other ideas of how to avoid using the original, underlying
enum's name.  I found this page:
http://stackoverflow.com/questions/902095/c-typedef-another-classs-enum which
may not help much.

[Peter Kasting, 2014-06-06 20:42:33]

LGTM modulo comment

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:138: input.type() !=
metrics::OmniboxInputType::FORCED_QUERY;
Nit: Put parens on both subexprs if you're going to put them on one.

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1001: case
metrics::OmniboxInputType::DEPRECATED_REQUESTED_URL:
I'm actually going to push back and say that I disagree with listing all the
enum elements in this switch, and think the old code was better than either
this, or this + break/return.

The other place in this CL you did this was fine, but in this one, I'd prefer
how it was done before.

[Mark P, 2014-06-06 21:24:52]

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/319523005/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.h:122:
metrics::OmniboxInputType::Type type, base::string16* text);
On 2014/06/06 20:37:08, Peter Kasting wrote:
> On 2014/06/06 20:22:51, Mark P wrote:
> > This change doesn't let me AutocompleteInput::Type::FOO.
> > "../../chrome/browser/autocomplete/search_provider.cc:180:38: error:
> > ‘AutocompleteInput::Type’ is not a class or namespace"
> 
> That's strange.  The following code compiles for me in MSVC 2013:
> 
> class A {
> public:
>   enum Type { ONE, TWO };
> };
> 
> class B {
> public:
>   typedef A::Type Foo;
> };
> 
> void foo() {
>   B::Foo foo = B::Foo::ONE;
> }
> 
> I wonder if this is non-standard, or C++11-only, or something?
> 
> I don't have any other ideas of how to avoid using the original, underlying
> enum's name.  I found this page:
> http://stackoverflow.com/questions/902095/c-typedef-another-classs-enum which
> may not help much.

Judging by the stackoverflow question, it sounds like what we're trying to do is
non-standard.  And because we have our compiler warnings turned all the way up,
it's getting reported as an error.  I think we're stuck with the icky
metrics::OmniboxInputType:: everywhere.

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_input.cc:123: return "";
On 2014/06/06 20:29:24, Ilya Sherman wrote:
> nit: s/""/std::string()

Done.

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:138: input.type() !=
metrics::OmniboxInputType::FORCED_QUERY;
On 2014/06/06 20:42:33, Peter Kasting wrote:
> Nit: Put parens on both subexprs if you're going to put them on one.

Done.  (put parens on both)

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1001: case
metrics::OmniboxInputType::DEPRECATED_REQUESTED_URL:
On 2014/06/06 20:42:33, Peter Kasting wrote:
> I'm actually going to push back and say that I disagree with listing all the
> enum elements in this switch, and think the old code was better than either
> this, or this + break/return.
> 
> The other place in this CL you did this was fine, but in this one, I'd prefer
> how it was done before.

I went back to the old way.

The reason the old way isn't so bad is that if one adds a type without adding
something to this case, things will crash the instant the user types a key into
the omnibox.  (This function gets run all the time.)  In the other function is
called much more rarely (I think only in debug code), which makes forgetting to
add a value there much less likely to be discovered.

https://codereview.chromium.org/319523005/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1002: ;
On 2014/06/06 20:29:24, Ilya Sherman wrote:
> nit: I'd have a break or a return.  Just a semicolon looks pretty odd.

Took Peter's advice; see comment there.

[Mark P, 2014-06-07 16:12:44]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-06-07 16:13:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpearson@chromium.org/319523005/140001

[commit-bot: I haz the power, 2014-06-07 20:34:30]

Change committed as 275696

[dcheng, 2014-06-07 23:17:49]

A revert of this CL has been created in
https://codereview.chromium.org/320713002/ by dcheng@chromium.org.

The reason for reverting is: Broke the build on
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac.

[Vitaly Buka (NO REVIEWS), 2014-06-08 00:00:58]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-08 00:01:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpearson@chromium.org/319523005/140001

[commit-bot: I haz the power, 2014-06-08 00:02:24]

Change committed as 275713

[Vitaly Buka (NO REVIEWS), 2014-06-08 00:04:19]

On 2014/06/08 00:02:24, I haz the power (commit-bot) wrote:
> Change committed as 275713

Should compile with r275707
Re-applied with CQ. For some reason drover fails to revert r275706.