Modify Entity Suggestion support and Add Profile Suggest support

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 7373008ae41c28178d92cd5c1eb82d4ecb533100..e04c7ce619c866f1942a6c4c3fc99a61269bf875 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -101,42 +101,30 @@ bool HasMultipleWords(const string16& text) {
 // the given |AutocompleteMatch|.
 void SetAndClassifyMatchContents(const string16& query_string,
                                  const string16& input_text,
-                                 const string16& match_contents,
-                                 const string16& annotation,
+                                 const string16& title,
+                                 const ACMatchClassifications& title_class,
                                  AutocompleteMatch* match) {
-  size_t match_contents_start = 0;
-  size_t annotation_start = match_contents.size();
-  // Append annotation if present.
-  if (annotation.empty()) {
-    match->contents = match_contents;
+  if (title.empty()) {
+    match->contents = query_string;
   } else {
-    std::vector<size_t> positions;
-    match->contents = l10n_util::GetStringFUTF16(
-        IDS_ANNOTATED_SUGGESTION, match_contents, annotation, &positions);
-    match_contents_start = positions[0];
-    annotation_start = positions[1];
+    match->contents = title;
+    if (!title_class.empty()) {
+      match->contents_class = title_class;
+      return;
+    }
   }
-  size_t match_contents_end = match_contents_start + match_contents.size();
-
-  if (!annotation.empty() && (annotation_start < match_contents_start))
-    match->contents_class.push_back(ACMatchClassification(
-        annotation_start, ACMatchClassification::DIM));
-
   // We do intra-string highlighting for suggestions - the suggested segment
   // will be highlighted, e.g. for input_text = "you" the suggestion may be
   // "youtube", so we'll bold the "tube" section: you*tube*.
-  if (input_text != match_contents) {
-    size_t input_position = match->contents.substr(
-        match_contents_start, match_contents.length()).find(input_text);
+  if (input_text != title) {
+    size_t input_position = match->contents.find(input_text);
     if (input_position == string16::npos) {
       // The input text is not a substring of the query string, e.g. input
       // text is "slasdot" and the query string is "slashdot", so we bold the
       // whole thing.
       match->contents_class.push_back(ACMatchClassification(
-          match_contents_start, ACMatchClassification::MATCH));
+          0, ACMatchClassification::MATCH));
     } else {
-      input_position += match_contents_start;
-
       // TODO(beng): ACMatchClassification::MATCH now seems to just mean
       //             "bold" this. Consider modifying the terminology.
       // We don't iterate over the string here annotating all matches because
@@ -144,9 +132,9 @@ void SetAndClassifyMatchContents(const string16& query_string,
       // short as a single character highlighted in a query suggestion result,
       // e.g. for input text "s" and query string "southwest airlines", it
       // looks odd if both the first and last s are highlighted.
-      if (input_position != match_contents_start) {
+      if (input_position != 0) {
         match->contents_class.push_back(ACMatchClassification(
-            match_contents_start, ACMatchClassification::MATCH));
+            0, ACMatchClassification::MATCH));
       }
       match->contents_class.push_back(
           ACMatchClassification(input_position, ACMatchClassification::NONE));
@@ -160,12 +148,22 @@ void SetAndClassifyMatchContents(const string16& query_string,
     // Otherwise, |match| is a verbatim (what-you-typed) match, either for the
     // default provider or a keyword search provider.
     match->contents_class.push_back(ACMatchClassification(
-        match_contents_start, ACMatchClassification::NONE));
+        0, ACMatchClassification::NONE));
   }
+}
 
-  if (!annotation.empty() && (annotation_start >= match_contents_start))
-    match->contents_class.push_back(ACMatchClassification(
-        match_contents_end, ACMatchClassification::DIM));
+AutocompleteMatchType::Type GetAutocompleteMatchType(const std::string& type) {
+  if (type == "ENTITY")
+    return AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  if (type == "INFINITE")
+    return AutocompleteMatchType::SEARCH_SUGGEST_INFINITE;
+  return AutocompleteMatchType::SEARCH_SUGGEST;
+}
+
+// Whether the given suggestion types supports suggestion detail.

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

Nit: types -> type

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

Method removed.

+bool TypeSupportsSuggestionDetail(const std::string& type) {
+  return (type == "ENTITY") || (type == "INFINITE") || (type == "PROFILE") ||
+      (type == "PERSONALIZED_QUERY");
 }
 
 }  // namespace
@@ -206,8 +204,11 @@ SearchProvider::Result::~Result() {
 
 SearchProvider::SuggestResult::SuggestResult(
     const string16& suggestion,
-    const string16& match_contents,
+    AutocompleteMatchType::Type type,
+    const string16& title,
+    ACMatchClassifications title_class,
     const string16& annotation,
+    ACMatchClassifications annotation_class,
     const std::string& suggest_query_params,
     bool from_keyword_provider,
     int relevance,
@@ -215,8 +216,11 @@ SearchProvider::SuggestResult::SuggestResult(
     bool should_prefetch)
     : Result(from_keyword_provider, relevance, relevance_from_server),
       suggestion_(suggestion),
-      match_contents_(match_contents),
+      type_(type),
+      title_(title),
+      title_class_(title_class),
       annotation_(annotation),
+      annotation_class_(annotation_class),
       suggest_query_params_(suggest_query_params),
       should_prefetch_(should_prefetch) {
 }
@@ -347,8 +351,10 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
     int relevance,
     AutocompleteMatch::Type type,
     bool is_keyword,
-    const string16& match_contents,
+    const string16& title,
+    const ACMatchClassifications& title_class,

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

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.

[Anuj, 2013/11/06 00:18:28]

This change was in accordance with our in person discussion last week. I will
revert this pending other questions.

     const string16& annotation,
+    const ACMatchClassifications& annotation_class,

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

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?

[Anuj, 2013/11/06 00:18:28]

Please elaborate.
What do you mean by "one caller"?

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

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

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

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

Classification parameters removed

     const TemplateURL* template_url,
     const string16& query_string,
     const std::string& suggest_query_params,
@@ -362,9 +368,15 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
   match.keyword = template_url->keyword();
 
   SetAndClassifyMatchContents(
-      query_string, input_text, match_contents, annotation, &match);
+      query_string, input_text, title, title_class, &match);
 
-  match.allowed_to_be_default_match = (input_text == match_contents);
+  if (!annotation.empty()) {
+    match.description = annotation;
+    if (!annotation_class.empty())
+      match.description_class = annotation_class;
+  }
+
+  match.allowed_to_be_default_match = (input_text == title);
 
   // When the user forced a query, we need to make sure all the fill_into_edit
   // values preserve that property.  Otherwise, if the user starts editing a
@@ -1036,25 +1048,42 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
             *this, url, title, is_keyword, relevance, true));
       }
     } else {
+      AutocompleteMatchType::Type match_type = GetAutocompleteMatchType(type);
       bool should_prefetch = static_cast<int>(index) == prefetch_index;
       DictionaryValue* suggestion_detail = NULL;
-      string16 match_contents = suggestion;
-      string16 disambiguating_query;
+      string16 title;
+      ACMatchClassifications title_class;
       string16 annotation;
+      ACMatchClassifications annotation_class;
+      std::string classifications;
       std::string suggest_query_params;
-      if (suggestion_details && (type == "ENTITY") &&
+      if (suggestion_details && TypeSupportsSuggestionDetail(type) &&
           suggestion_details->GetDictionary(index, &suggestion_detail) &&
           suggestion_detail) {
-        suggestion_detail->GetString("a", &annotation);
-        if (suggestion_detail->GetString("dq", &disambiguating_query) &&
-            !disambiguating_query.empty())
-          suggestion = disambiguating_query;
-        suggestion_detail->GetString("q", &suggest_query_params);
+        suggestion_detail->GetString("title", &title) ||
+            suggestion_detail->GetString("t", &title);
+        suggestion_detail->GetString("title_class", &classifications) ||
+            suggestion_detail->GetString("tc", &classifications);
+        title_class =
+            AutocompleteMatch::ClassificationsFromString(classifications);

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

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.

[Anuj, 2013/11/06 00:18:28]

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.

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

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.
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.

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

Classifications removed.

+        classifications.clear();
+
+        suggestion_detail->GetString("annotation", &annotation) ||
+            suggestion_detail->GetString("a", &annotation);
+        suggestion_detail->GetString("annotation_class", &classifications) ||
+            suggestion_detail->GetString("ac", &classifications);
+        annotation_class =
+            AutocompleteMatch::ClassificationsFromString(classifications);
+        classifications.clear();
+
+        suggestion_detail->GetString("query_params", &suggest_query_params) ||
+            suggestion_detail->GetString("q", &suggest_query_params);
       }
       // TODO(kochi): Improve calculator suggestion presentation.
       results->suggest_results.push_back(SuggestResult(
-          suggestion, match_contents, annotation, suggest_query_params,
-          is_keyword, relevance, true, should_prefetch));
+          suggestion, match_type, title,  title_class, annotation,

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

Nit: Two spaces

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

Done.

+          annotation_class, suggest_query_params, is_keyword, relevance, true,
+          should_prefetch));
     }
   }
 
@@ -1100,7 +1129,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                   false,
                   input_.text(),
+                  ACMatchClassifications(),
                   string16(),
+                  ACMatchClassifications(),
                   input_.text(),
                   did_not_accept_default_suggestion,
                   std::string(),
@@ -1128,7 +1159,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                       AutocompleteMatchType::SEARCH_OTHER_ENGINE,
                       true,
                       keyword_input_.text(),
+                      ACMatchClassifications(),
                       string16(),
+                      ACMatchClassifications(),
                       keyword_input_.text(),
                       did_not_accept_keyword_suggestion,
                       std::string(),
@@ -1376,7 +1409,9 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                   AutocompleteMatchType::SEARCH_HISTORY,
                   is_keyword,
                   i->suggestion(),
+                  ACMatchClassifications(),
                   string16(),
+                  ACMatchClassifications(),
                   i->suggestion(),
                   did_not_accept_suggestion,
                   std::string(),
@@ -1428,9 +1463,10 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
     int relevance = CalculateRelevanceForHistory(
         i->time, is_keyword, !prevent_inline_autocomplete,
         prevent_search_history_inlining);
-    scored_results.push_back(
-        SuggestResult(i->term, string16(), string16(), std::string(),
-                      is_keyword, relevance, false, false));
+    scored_results.push_back(SuggestResult(
+        i->term, AutocompleteMatchType::SEARCH_HISTORY, string16(),
+        ACMatchClassifications(), string16(), ACMatchClassifications(),
+        std::string(), is_keyword, relevance, false, false));
   }
 
   // History returns results sorted for us.  However, we may have docked some
@@ -1462,10 +1498,12 @@ void SearchProvider::AddSuggestResultsToMap(const SuggestResults& results,
                   results[i].relevance_from_server(),
                   results[i].should_prefetch(),
                   metadata,
-                  AutocompleteMatchType::SEARCH_SUGGEST,
+                  results[i].type(),
                   is_keyword,
-                  results[i].match_contents(),
+                  results[i].title(),
+                  results[i].title_class(),
                   results[i].annotation(),
+                  results[i].annotation_class(),
                   results[i].suggestion(),
                   i,
                   results[i].suggest_query_params(),
@@ -1581,19 +1619,22 @@ int SearchProvider::CalculateRelevanceForHistory(
   return std::max(0, base_score - score_discount);
 }
 
-void SearchProvider::AddMatchToMap(const string16& input_text,
-                                   int relevance,
-                                   bool relevance_from_server,
-                                   bool should_prefetch,
-                                   const std::string& metadata,
-                                   AutocompleteMatch::Type type,
-                                   bool is_keyword,
-                                   const string16& match_contents,
-                                   const string16& annotation,
-                                   const string16& query_string,
-                                   int accepted_suggestion,
-                                   const std::string& suggest_query_params,
-                                   MatchMap* map) {
+void SearchProvider::AddMatchToMap(
+    const string16& input_text,
+    int relevance,
+    bool relevance_from_server,
+    bool should_prefetch,
+    const std::string& metadata,
+    AutocompleteMatch::Type type,
+    bool is_keyword,
+    const string16& title,
+    const ACMatchClassifications& title_class,
+    const string16& annotation,
+    const ACMatchClassifications& annotation_class,
+    const string16& query_string,
+    int accepted_suggestion,
+    const std::string& suggest_query_params,
+    MatchMap* map) {
   // On non-mobile, ask the instant controller for the appropriate start margin.
   // On mobile the start margin is unused, so leave the value as default there.
   int omnibox_start_margin = chrome::kDisableStartMargin;
@@ -1612,9 +1653,9 @@ void SearchProvider::AddMatchToMap(const string16& input_text,
   const TemplateURL* template_url = is_keyword ?
       providers_.GetKeywordProviderURL() : providers_.GetDefaultProviderURL();
   AutocompleteMatch match = CreateSearchSuggestion(
-      this, input_, input_text, relevance, type, is_keyword, match_contents,
-      annotation, template_url, query_string, suggest_query_params,
-      accepted_suggestion, omnibox_start_margin,
+      this, input_, input_text, relevance, type, is_keyword, title, title_class,
+      annotation, annotation_class, template_url, query_string,
+      suggest_query_params, accepted_suggestion, omnibox_start_margin,
       !is_keyword || providers_.default_provider().empty());
   if (!match.destination_url.is_valid())
     return;