Omnibox: Send Input Type to Suggest Server

Omnibox: Send Input Type to Suggest Server

This has been approved by the privacy team.

I also tested this interactively (by sniffing the network traffic for
various omnibox inputs).

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

[Mark P, 2014-06-03 19:25:31]

This is straightforward.  Please take a look.

--mark

https://codereview.chromium.org/314773002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/314773002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url.h:268: GOOGLE_FORCE_INSTANT_RESULTS,
I noticed this was out of alphabetical order.

[Bart N., 2014-06-03 20:38:26]

https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_input.h:31: };
I came across this inconsistency on the server side (OmniboxEventProto):

  // What kind of input the user provided.
  enum InputType {
    INVALID = 0;        // Empty input (should not reach here)
    UNKNOWN = 1;        // Valid input whose type cannot be determined
    REQUESTED_URL = 2;  // DEPRECATED. Input autodetected as UNKNOWN, which the
                        // user wants to treat as an URL by specifying a
                        // desired_tld
    URL = 3;            // Input autodetected as a URL
    QUERY = 4;          // Input autodetected as a query
    FORCED_QUERY = 5;   // Input forced to be a query by an initial '?'
  }
  optional InputType input_type = 8;

can you verify if this is OK (note the IDs off by 1 after presumably deleted
REQUESTED_URL value?).

[Mark P, 2014-06-03 20:53:34]

https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_input.h:31: };
On 2014/06/03 20:38:26, Bart N. wrote:
> I came across this inconsistency on the server side (OmniboxEventProto):
> 
>   // What kind of input the user provided.
>   enum InputType {
>     INVALID = 0;        // Empty input (should not reach here)
>     UNKNOWN = 1;        // Valid input whose type cannot be determined
>     REQUESTED_URL = 2;  // DEPRECATED. Input autodetected as UNKNOWN, which
the
>                         // user wants to treat as an URL by specifying a
>                         // desired_tld
>     URL = 3;            // Input autodetected as a URL
>     QUERY = 4;          // Input autodetected as a query
>     FORCED_QUERY = 5;   // Input forced to be a query by an initial '?'
>   }
>   optional InputType input_type = 8;
> 
> can you verify if this is OK (note the IDs off by 1 after presumably deleted
> REQUESTED_URL value?).

Good point.

The discrepancy is on the client side too.
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

Probably I should be uploading OmniboxEventProto::InputType rather than
AutocompleteInput::Type.  These things as you note are not identical.  However,
that would require including the metrics proto here, which I'm not sure if the
best idea.

Do you have an opinion?

Also, Peter what are your thoughts?

[Bart N., 2014-06-03 20:57:24]

On 2014/06/03 20:38:26, Bart N. wrote:
>
https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/autocomplete_input.h (right):
> 
>
https://codereview.chromium.org/314773002/diff/1/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/autocomplete_input.h:31: };
> I came across this inconsistency on the server side (OmniboxEventProto):
> 
>   // What kind of input the user provided.
>   enum InputType {
>     INVALID = 0;        // Empty input (should not reach here)
>     UNKNOWN = 1;        // Valid input whose type cannot be determined
>     REQUESTED_URL = 2;  // DEPRECATED. Input autodetected as UNKNOWN, which
the
>                         // user wants to treat as an URL by specifying a
>                         // desired_tld
>     URL = 3;            // Input autodetected as a URL
>     QUERY = 4;          // Input autodetected as a query
>     FORCED_QUERY = 5;   // Input forced to be a query by an initial '?'
>   }
>   optional InputType input_type = 8;
> 
> can you verify if this is OK (note the IDs off by 1 after presumably deleted
> REQUESTED_URL value?).
My preference is to use this enum, since this is the original source of data. If
you want to keep it consistent with omniboxeventlog proto, perhaps you can
update the IDs to skip ID=2? I'll update my enum on the server side, too.

[Peter Kasting, 2014-06-03 21:04:15]

I think the core question here is what the purpose of different enums is.

If all we ever needed to do was have some semantic listing of the different
input types, we'd only need one, up-to-date enum.  Instead we have two: one that
represents the current recognized input types, and a different one that exists
to map those to some set of more stable, consistent values for use by a
particular server component.

Now we're adding another, different server component that wants the same
information, albeit at a different time.  The paragraph above suggests to me
that the right way to do this is by sending up values from the enum that exists
to provide a stable, consistent set of values.  So I wouldn't upload the
AutocompleteInput::Type values.

If we really wanted to make AutocompleteInput::Type sendable to the server
directly, I would then suggest removing the separate "proto" enum, and using
these values everywhere.  There's no reason to have two enums if their values
always have to stay in lockstep.

This sort of opinion applies everywhere we have separate "internal" and "proto"
enums, and reflects at the core my general unhappiness that we tend to have such
pairs of enums so extensively in the first place.

[Bart N., 2014-06-03 23:30:07]

On 2014/06/03 21:04:15, Peter Kasting wrote:
> I think the core question here is what the purpose of different enums is.
> 
> If all we ever needed to do was have some semantic listing of the different
> input types, we'd only need one, up-to-date enum.  Instead we have two: one
that
> represents the current recognized input types, and a different one that exists
> to map those to some set of more stable, consistent values for use by a
> particular server component.
> 
> Now we're adding another, different server component that wants the same
> information, albeit at a different time.  The paragraph above suggests to me
> that the right way to do this is by sending up values from the enum that
exists
> to provide a stable, consistent set of values.  So I wouldn't upload the
> AutocompleteInput::Type values.
> 
> If we really wanted to make AutocompleteInput::Type sendable to the server
> directly, I would then suggest removing the separate "proto" enum, and using
> these values everywhere.  There's no reason to have two enums if their values
> always have to stay in lockstep.
> 
> This sort of opinion applies everywhere we have separate "internal" and
"proto"
> enums, and reflects at the core my general unhappiness that we tend to have
such
> pairs of enums so extensively in the first place.
Peter, do you anticipate adding new input types in near future?

Also, I see your point. Perhaps we could do the following: have an enum 3
different values (URL/QUERY/UNKNOWN) on the server side and do the mapping in
Chrome. Server only cares about 3 possible values.

[Mark P, 2014-06-03 23:45:24]

On Tue, Jun 3, 2014 at 4:30 PM, <bartn@chromium.org> wrote:

> On 2014/06/03 21:04:15, Peter Kasting wrote:
>
>> I think the core question here is what the purpose of different enums is.
>>
>
>  If all we ever needed to do was have some semantic listing of the
>> different
>> input types, we'd only need one, up-to-date enum.  Instead we have two:
>> one
>>
> that
>
>> represents the current recognized input types, and a different one that
>> exists
>> to map those to some set of more stable, consistent values for use by a
>> particular server component.
>>
>
>  Now we're adding another, different server component that wants the same
>> information, albeit at a different time.  The paragraph above suggests to
>> me
>> that the right way to do this is by sending up values from the enum that
>>
> exists
>
>> to provide a stable, consistent set of values.  So I wouldn't upload the
>> AutocompleteInput::Type values.
>>
>
>  If we really wanted to make AutocompleteInput::Type sendable to the server
>> directly, I would then suggest removing the separate "proto" enum, and
>> using
>> these values everywhere.  There's no reason to have two enums if their
>> values
>> always have to stay in lockstep.
>>
>
>  This sort of opinion applies everywhere we have separate "internal" and
>>
> "proto"
>
>> enums, and reflects at the core my general unhappiness that we tend to
>> have
>>
> such
>
>> pairs of enums so extensively in the first place.
>>
> Peter, do you anticipate adding new input types in near future?
>
> Also, I see your point. Perhaps we could do the following: have an enum 3
> different values (URL/QUERY/UNKNOWN) on the server side and do the mapping
> in
> Chrome. Server only cares about 3 possible values.
>

The server kind of cares about all four currently-used values.
 (FORCED_QUERY mode would be nice to pass; see crbug.com/313229)


>
> https://codereview.chromium.org/314773002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-06-03 23:57:19]

On 2014/06/03 23:30:07, Bart N. wrote:
> Peter, do you anticipate adding new input types in near future?

No.

[Mark P, 2014-06-04 00:13:46]

I'm going to start and internal thread about the logging folks about the
best strategy to use here.


On Tue, Jun 3, 2014 at 4:57 PM, <pkasting@chromium.org> wrote:

> On 2014/06/03 23:30:07, Bart N. wrote:
>
>> Peter, do you anticipate adding new input types in near future?
>>
>
> No.
>
> https://codereview.chromium.org/314773002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-06-05 04:33:30]

Per the internal thread, I'm going with the following solution:
* make one (stable) enum that gets used in all the three places (used in
autocomplete code, sent to suggest server, put in UMA logs)
* that enum will have to live in metrics code because metrics is a component and
none of the other systems are
* the enum will be in a standalone file so it's easy to include without
including a ton of actual executable code from metrics

If you have an objections, please voice them before I begin the migration.  I'll
do it in another changelist, then revise this change on top of that.

--mark

[Mark P, 2014-06-08 00:06:57]

Now that there's only one enum (per https://codereview.chromium.org/319523005/),
this change makes a bit more sense.  Not that it didn't make sense before, but
it was odd with three different use cases of what's qualitatively the same enum
running around.

Please take another look.

thanks,
mark

[Peter Kasting, 2014-06-09 00:16:49]

LGTM

[Mark P, 2014-06-09 18:48:52]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-06-09 18:50:14]

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

[Mark P, 2014-06-09 21:50:42]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2014-06-09 22:16:57]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-06-09 22:20:21]

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

[commit-bot: I haz the power, 2014-06-10 12:15:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-10 13:34:47]

Change committed as 276008