Modify Entity Suggestion support and Add Profile Suggest support

Modify Entity Suggestion support and Add Profile Suggest support

- Added new AutocompleteMatchType for the two, and additionally for personalized and infinite suggestion.
- Added logging for the new suggestion types.
- Annotation for entities is now used as description in the match. This allows multiline rendering in mobile easily.
- Profile suggestions also look like entity suggestion, and use title, annotation and query fields in response.

BUG=

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

[Anuj, 2013-11-05 02:07:30]

PTAL.

I haven't modified tests yet, but please take a look before I spend more time.

I have separate patches for
- Infinite Suggest UX on Mac.
- Entity Suggest on Clank.

Both the above are functional, and dependent on this patch.

[Peter Kasting, 2013-11-05 23:36:58]

In short, I veto allowing the server to provide the classification data.  Do it
how you were doing it before.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:163: // Whether the given
suggestion types supports suggestion detail.
Nit: types -> type

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:355: const
ACMatchClassifications& title_class,
I'm not terribly keen on changing the param names here to be |title| and
|title_class|, because this function doesn't know or care that |title| is
actually a Title, and for many calls (that are standard search suggestions) it
won't be.  It seems like the old |match_contents| name was better here and in
SetAndClassifyMatchContents(), and maybe in AddMatchToMap() also.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:357: const
ACMatchClassifications& annotation_class,
Do we have to pass |annotation| and |annotation_class| in at all?  Why can't the
one caller who needs something special w.r.t. them set the match description +
classifications directly?

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1068:
AutocompleteMatch::ClassificationsFromString(classifications);
The more I think about this the less OK I am with the idea that the server is
going to pass us data that we're going to feed directly to
ClassificationsFromString(), for several reasons.

We should pass down data that Chrome decides how to mark up, as we were doing
before.  Even if we wanted to pass explicit markup control from the server we'd
need to do it as a separate XML/JSON set of directives that we then parsed, but
I think even that is a bad idea.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1084: suggestion, match_type,
title,  title_class, annotation,
Nit: Two spaces

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
File chrome/common/autocomplete_match_type.h (right):

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:14: URL_WHAT_YOU_TYPED = 0,      // The
input as a URL.
Why indent all these comments more?

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:33: SEARCH_SUGGEST_INFINITE,     // A
suggested search to complete the tail of
Nit: Place these two after SEARCH_SUGGEST.  I am pretty sure this enum's values
don't need to be fixed.

However, you should also muck with omnibox_event.proto, and the order/values may
matter there.  Consult mpearson about this issue in general as he knows the
rules better than I do.

[Anuj, 2013-11-06 00:18:27]

Please explain your veto! I am really not a fan of implementing text layouts in
C++.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:355: const
ACMatchClassifications& title_class,
This change was in accordance with our in person discussion last week. I will
revert this pending other questions.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:357: const
ACMatchClassifications& annotation_class,
Please elaborate.
What do you mean by "one caller"?

Currently I plan to use it for Entity and Profile suggestions.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1068:
AutocompleteMatch::ClassificationsFromString(classifications);
I failed to identify "several reasons" in your comment.

We discussed and agreed that server can provide rendering hints using the
AutocompleteMatchType. I fail to understand why is this a bad idea now. This
scheme allows server to exercise variations within the bounds specified by the
Chrome UX.

If the serialization/deserialization is concerned, I think that is a trivial
matter. ClassificationsFromString is as good as any other option as long as
server is going to be aware of the general structure of ACMatchClassifications.

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
File chrome/common/autocomplete_match_type.h (right):

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:14: URL_WHAT_YOU_TYPED = 0,      // The
input as a URL.
I was about to add SEARCH_SUGGEST_PERSONALIZED too :)

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:33: SEARCH_SUGGEST_INFINITE,     // A
suggested search to complete the tail of
My understanding of protos and logs is that you don't mess with the enum values.
(Always add new properties, never remove old, only mark obsolete.)

Also, these values also have a mapping in Clank codebase.

[Peter Kasting, 2013-11-06 00:34:26]

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:357: const
ACMatchClassifications& annotation_class,
On 2013/11/06 00:18:28, Anuj wrote:
> Please elaborate.
> What do you mean by "one caller"?

Only AddMatchToMap() needs to do anything with this field.  So the relevant code
below can just be moved there.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1068:
AutocompleteMatch::ClassificationsFromString(classifications);
On 2013/11/06 00:18:28, Anuj wrote:
> I failed to identify "several reasons" in your comment.

That's because I didn't give them.  I'd really rather not write out things and
debate.  But the short summary is that I don't want the server to control the
layout at this low level in principle; I don't want to expose the low-level
details of the classification values and the stringified forms to anyone outside
the Chrome codebase; and I don't want to pass server data directly to conversion
functions without making them robust against attacks (e.g. passing in very long
strings or invalid offsets or something).

These reasons haven't changed from why I had you pass down the annotation and
disambiguating query, and add IDS_ANNOTATED_SUGGESTION and code to mark things
up using it, in the first place.  I'm not OK with reversing that course.

> We discussed and agreed that server can provide rendering hints using the
> AutocompleteMatchType.

The server can tell you what kind of a suggestion you're getting, sure.  That's
far from telling you precisely how to format some string of text.

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
File chrome/common/autocomplete_match_type.h (right):

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:14: URL_WHAT_YOU_TYPED = 0,      // The
input as a URL.
On 2013/11/06 00:18:28, Anuj wrote:
> I was about to add SEARCH_SUGGEST_PERSONALIZED too :)

OK, assuming you do so in this CL.

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:33: SEARCH_SUGGEST_INFINITE,     // A
suggested search to complete the tail of
On 2013/11/06 00:18:28, Anuj wrote:
> My understanding of protos and logs is that you don't mess with the enum
values.
> (Always add new properties, never remove old, only mark obsolete.)

Which is why I said that the .proto file may have different rules.

However, this enum is not used directly in protobufs, but is translated in the
proto file, which is why regardless of the rules there, this enum should be
reordered as we add new values, to keep them in the ideal order.

Mark and I discussed this in the past and agreed (as far as I recall) that this
enum doesn't need to stay in the same order as the .proto file in principle.

I don't know what you mean by "mapping in Clank codebase".

[Anuj, 2013-12-09 08:24:56]

Adding sky@ for logging.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:163: // Whether the given
suggestion types supports suggestion detail.
Method removed.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:357: const
ACMatchClassifications& annotation_class,
Classification parameters removed

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1068:
AutocompleteMatch::ClassificationsFromString(classifications);
Classifications removed.

https://codereview.chromium.org/58003005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:1084: suggestion, match_type,
title,  title_class, annotation,
On 2013/11/05 23:36:58, Peter Kasting wrote:
> Nit: Two spaces

Done.

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
File chrome/common/autocomplete_match_type.h (right):

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:14: URL_WHAT_YOU_TYPED = 0,      // The
input as a URL.
On 2013/11/06 00:34:26, Peter Kasting wrote:
> On 2013/11/06 00:18:28, Anuj wrote:
> > I was about to add SEARCH_SUGGEST_PERSONALIZED too :)
> 
> OK, assuming you do so in this CL.

Done.

https://codereview.chromium.org/58003005/diff/1/chrome/common/autocomplete_ma...
chrome/common/autocomplete_match_type.h:33: SEARCH_SUGGEST_INFINITE,     // A
suggested search to complete the tail of
After consulting with clank team, it is okay to modify this enum. Done.

[Anuj, 2013-12-10 00:54:38]

Bumping up in your queue

[sky, 2013-12-10 01:24:21]

By logging I think you mean metrics. There are better local owners for that.
sky->isherman

[Ilya Sherman, 2013-12-10 01:28:23]

Please first commit the .proto changes in the google3 repository, to satisfy the
log team's request to review all .proto changes before they go live.

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
What does SEARCH_SUGGEST now entail?  Is that bucket deprecated?  Please
document within this proto file.

[Anuj, 2013-12-10 01:39:17]

Sent the google3 CL.

[Anuj, 2013-12-10 01:39:45]

I will submit this change after the google3 CL is submitted. PTAL.

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
Meaning of SEARCH_SUGGEST doesn't change. Although I can rename it to
SEARCH_SUGGEST_QUERY if you want. These suggestion types are newly being added
to Chrome omnibox.

[Peter Kasting, 2013-12-10 03:52:30]

LGTM

https://codereview.chromium.org/58003005/diff/80001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/58003005/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:120: IDS_ANNOTATED_SUGGESTION,
match_contents, annotation, &positions);
If we're removing the only reference to IDS_ANNOTATED_SUGGESTION, it needs to be
removed from the .grd file.

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
On 2013/12/10 01:39:45, Anuj wrote:
> Meaning of SEARCH_SUGGEST doesn't change. Although I can rename it to
> SEARCH_SUGGEST_QUERY if you want. These suggestion types are newly being added
> to Chrome omnibox.

Don't rename SEARCH_SUGGEST, but do update the comments here and in
autocomplete_match_type.h to add "...that doesn't fall into one of the more
specific suggestion categories below" to its description.

[Mark P, 2013-12-10 15:10:18]

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
On 2013/12/10 03:52:30, Peter Kasting wrote:
> On 2013/12/10 01:39:45, Anuj wrote:
> > Meaning of SEARCH_SUGGEST doesn't change. Although I can rename it to
> > SEARCH_SUGGEST_QUERY if you want. These suggestion types are newly being
added
> > to Chrome omnibox.
> 
> Don't rename SEARCH_SUGGEST, but do update the comments here and in
> autocomplete_match_type.h to add "...that doesn't fall into one of the more
> specific suggestion categories below" to its description.

+1 to this suggestion.

And, when this is resolved, please make sure the internal google file is
identical (i.e., same comment, formatting, etc.)

And also please try to keep the autocomplete_match_type comments in sync with
this as well.  (formatting not important, but content should be equivalent)

[Anuj, 2013-12-10 23:08:08]

I will sync the comments in google3 proto file shortly after submitting this CL.

Ilya,
Please approve for the logging code.

https://codereview.chromium.org/58003005/diff/80001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/58003005/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:120: IDS_ANNOTATED_SUGGESTION,
match_contents, annotation, &positions);
On 2013/12/10 03:52:30, Peter Kasting wrote:
> If we're removing the only reference to IDS_ANNOTATED_SUGGESTION, it needs to
be
> removed from the .grd file.

Done.

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
On 2013/12/10 03:52:30, Peter Kasting wrote:
> On 2013/12/10 01:39:45, Anuj wrote:
> > Meaning of SEARCH_SUGGEST doesn't change. Although I can rename it to
> > SEARCH_SUGGEST_QUERY if you want. These suggestion types are newly being
added
> > to Chrome omnibox.
> 
> Don't rename SEARCH_SUGGEST, but do update the comments here and in
> autocomplete_match_type.h to add "...that doesn't fall into one of the more
> specific suggestion categories below" to its description.

Done.

https://codereview.chromium.org/58003005/diff/80001/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:178: // Google+ profile.
On 2013/12/10 15:10:18, Mark P wrote:
> On 2013/12/10 03:52:30, Peter Kasting wrote:
> > On 2013/12/10 01:39:45, Anuj wrote:
> > > Meaning of SEARCH_SUGGEST doesn't change. Although I can rename it to
> > > SEARCH_SUGGEST_QUERY if you want. These suggestion types are newly being
> added
> > > to Chrome omnibox.
> > 
> > Don't rename SEARCH_SUGGEST, but do update the comments here and in
> > autocomplete_match_type.h to add "...that doesn't fall into one of the more
> > specific suggestion categories below" to its description.
> 
> +1 to this suggestion.
> 
> And, when this is resolved, please make sure the internal google file is
> identical (i.e., same comment, formatting, etc.)
> 
> And also please try to keep the autocomplete_match_type comments in sync with
> this as well.  (formatting not important, but content should be equivalent)

Done.

[Ilya Sherman, 2013-12-10 23:11:07]

metrics lgtm

[commit-bot: I haz the power, 2013-12-10 23:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/58003005/120001

[commit-bot: I haz the power, 2013-12-11 00:12:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Anuj, 2013-12-11 00:46:42]

Adding back sky for autocomplete_match_type.{h,cc}.

Peter, Why don't you own those files?!!!!

[sky, 2013-12-11 14:06:47]

autocomplete_match.* LGTM

[commit-bot: I haz the power, 2013-12-11 18:08:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/58003005/120001

[commit-bot: I haz the power, 2013-12-11 20:45:06]

Change committed as 240174

[H Fung, 2014-07-01 17:39:03]

Do you want to delete patch set 5?  I think it's unrelated to this CL.  (I'm
guessing patch set 4 was committed).