Add a new entry to omnibox_event.proto to log specific type of contextual suggestions

components/omnibox/browser/search_suggestion_parser.cc

Index: components/omnibox/browser/search_suggestion_parser.cc
diff --git a/components/omnibox/browser/search_suggestion_parser.cc b/components/omnibox/browser/search_suggestion_parser.cc
index 04e10a02ba461d6d5b8f60b09e06c229b207673a..b4be6c9a6fd46e2b759d245bfa23e5b9964f7832 100644
--- a/components/omnibox/browser/search_suggestion_parser.cc
+++ b/components/omnibox/browser/search_suggestion_parser.cc
@@ -20,6 +20,7 @@
 #include "base/values.h"
 #include "components/omnibox/browser/autocomplete_i18n.h"
 #include "components/omnibox/browser/autocomplete_input.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/url_prefix.h"
 #include "components/url_formatter/url_fixer.h"
 #include "components/url_formatter/url_formatter.h"
@@ -229,7 +230,8 @@ SearchSuggestionParser::NavigationResult::NavigationResult(
     bool from_keyword_provider,
     int relevance,
     bool relevance_from_server,
-    const base::string16& input_text)
+    const base::string16& input_text,
+    const std::string& zero_suggest_specific_type)
     : Result(from_keyword_provider,
              relevance,
              relevance_from_server,
@@ -246,11 +248,14 @@ SearchSuggestionParser::NavigationResult::NavigationResult(
                                    nullptr,
                                    nullptr),
           scheme_classifier)),
-      description_(description) {
+      description_(description),
+      zero_suggest_specific_type_(zero_suggest_specific_type) {
   DCHECK(url_.is_valid());
   CalculateAndClassifyMatchContents(true, input_text);
 }
 
+SearchSuggestionParser::NavigationResult::NavigationResult(
+    const NavigationResult& nav) = default;
 SearchSuggestionParser::NavigationResult::~NavigationResult() {}
 
 void
@@ -447,6 +452,13 @@ bool SearchSuggestionParser::ParseSuggestResults(
     json_serializer.Serialize(*extras);
   }
 
+  // Get zero suggest subtypes.
+  const base::ListValue* zero_suggest_subtypes = NULL;

[Mark P, 2017/03/14 20:17:50]

If we're getting stuff from the server, it should be as part of
// 5th element: Optional key-value pairs from the Suggest server.
not at yet another element.

[gcomanici, 2017/03/14 20:42:55]

I agree. Although the server side puts this values in the 6th elements for now,
this will change in the near future to your proposal. I will create a server
side CL to fix this and change this code accordingly.

+  if (root_list->GetList(5, &zero_suggest_subtypes) &&
+      zero_suggest_subtypes->GetSize() != results_list->GetSize()) {
+    zero_suggest_subtypes = NULL;
+  }
+
   // Clear the previous results now that new results are available.
   results->suggest_results.clear();
   results->navigation_results.clear();
@@ -486,9 +498,15 @@ bool SearchSuggestionParser::ParseSuggestResults(
         base::string16 title;
         if (descriptions != NULL)
           descriptions->GetString(index, &title);
+        // add zero suggest subtype
+        std::string zero_suggest_specific_type;
+        if (zero_suggest_subtypes) {
+          zero_suggest_subtypes->GetString(index, &zero_suggest_specific_type);

[Mark P, 2017/03/14 20:17:49]

I'm curious: why do you have the server send down human-readable strings as
opposed simply an int / enum value?  The strings will make it harder to change
the logging in the future.  I suppose there's something to be said for
readability (both in the response and if you're displaying them within Chrome
somewhere as a debug page), but the lack of extensibility worries me.

[gcomanici, 2017/03/14 20:42:55]

The short answer is that it makes it easy to use the exact same output for other
analyses that are independent of Chrome code and where the human-readable
strings are useful. But I do agree with you. I will take your advice to make a
server side CL that addresses this issue.

[Mark P, 2017/03/14 21:04:09]

The elephant in the room here is that regardless of int or string, once this is
set, it cannot be easily changed because the UMA values are written to logs.

Having the server send strings means that you cannot add new types without a
chrome change.

Having the server send ints means that the server needs to keep these ints
constant for all eternity.  For example, if the news int = 5, it must always be
= 5, and if you remove the news type from the server, you cannot reuse that enum
value.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

[gcomanici, 2017/03/15 01:35:56]

I would go with the latter proposal (i.e. use ints). I think it will be easy to
comply to these restrictions on the server side.

+        }
         results->navigation_results.push_back(NavigationResult(
             scheme_classifier, url, match_type, title, deletion_url,
-            is_keyword_result, relevance, relevances != NULL, input.text()));
+            is_keyword_result, relevance, relevances != NULL, input.text(),
+            zero_suggest_specific_type));
       }
     } else {
       // TODO(dschuyler) If the "= " is no longer sent from the back-end