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

components/omnibox/browser/search_suggestion_parser.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/omnibox/browser/search_suggestion_parser.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <utility>
 
 #include "base/i18n/icu_string_conversions.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #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"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_fetcher.h"
 #include "url/url_constants.h"
 
 namespace {
 
 AutocompleteMatchType::Type GetAutocompleteMatchType(const std::string& type) {
@@ skipping 189 matching lines @@
 
 SearchSuggestionParser::NavigationResult::NavigationResult(
     const AutocompleteSchemeClassifier& scheme_classifier,
     const GURL& url,
     AutocompleteMatchType::Type type,
     const base::string16& description,
     const std::string& deletion_url,
     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,
              type,
              deletion_url),
       url_(url),
       formatted_url_(AutocompleteInput::FormattedStringWithEquivalentMeaning(
           url,
           url_formatter::FormatUrl(url,
                                    url_formatter::kFormatUrlOmitAll &
                                        ~url_formatter::kFormatUrlOmitHTTP,
                                    net::UnescapeRule::SPACES,
                                    nullptr,
                                    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
 SearchSuggestionParser::NavigationResult::CalculateAndClassifyMatchContents(
     const bool allow_bolding_nothing,
     const base::string16& input_text) {
   if (input_text.empty()) {
     // In case of zero-suggest results, do not highlight matches.
     match_contents_class_.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
@@ skipping 176 matching lines @@
     if (extras->GetList("google:suggestdetail", &suggestion_details) &&
         suggestion_details->GetSize() != results_list->GetSize())
       suggestion_details = NULL;
 
     // Store the metadata that came with the response in case we need to pass it
     // along with the prefetch query to Instant.
     JSONStringValueSerializer json_serializer(&results->metadata);
     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();
   results->answers_image_urls.clear();
 
   base::string16 suggestion;
   std::string type;
   int relevance = default_result_relevance;
   const base::string16& trimmed_input =
       base::CollapseWhitespace(input.text(), false);
@@ skipping 19 matching lines @@
 
     if ((match_type == AutocompleteMatchType::NAVSUGGEST) ||
         (match_type == AutocompleteMatchType::NAVSUGGEST_PERSONALIZED)) {
       // Do not blindly trust the URL coming from the server to be valid.
       GURL url(url_formatter::FixupURL(base::UTF16ToUTF8(suggestion),
                                        std::string()));
       if (url.is_valid()) {
         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
       // then this may be removed.
       if ((match_type == AutocompleteMatchType::CALCULATOR) &&
           !suggestion.compare(0, 2, base::UTF8ToUTF16("= ")))
         suggestion.erase(0, 2);
 
       base::string16 match_contents = suggestion;
       base::string16 match_contents_prefix;
@@ skipping 47 matching lines @@
           base::CollapseWhitespace(match_contents, false),
           match_contents_prefix, annotation, answer_contents, answer_type_str,
           std::move(answer), suggest_query_params, deletion_url,
           is_keyword_result, relevance, relevances != NULL, should_prefetch,
           trimmed_input));
     }
   }
   results->relevances_from_server = relevances != NULL;
   return true;
 }