[AiS] Merge answers into highest-scoring result

components/omnibox/base_search_provider.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/base_search_provider.h"
 
 #include "base/i18n/case_conversion.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/metrics/proto/omnibox_event.pb.h"
@@ skipping 402 matching lines @@
         // is sufficiently relevant that the user is likely to choose it.
         // Surely setting the prefetch bit on a match of even higher relevance
         // won't violate this assumption.
         should_prefetch |= ShouldPrefetch(i.first->second);
         i.first->second.RecordAdditionalInfo(kShouldPrefetchKey,
                                              should_prefetch ? kTrue : kFalse);
         if (should_prefetch)
           i.first->second.RecordAdditionalInfo(kSuggestMetadataKey, metadata);
       }
     }
+    // Copy over answer data from lower-ranking item, if necessary.
+    // This depends on the lower-ranking item always being added last - see
+    // use of push_back above.
+    const AutocompleteMatch& less_relevant_match =
+        i.first->second.duplicate_matches.back();
+    if (!less_relevant_match.answer_type.empty()) {
+      DCHECK(i.first->second.answer_type.empty());

[Mark P, 2014/09/22 23:54:06]

We try to make Chrome work well even when a suggest server replies in unexpected
ways.  Call it future-proofing, defensive programming, guarding against
non-Google people implementing the same protocol.  Regardless, I'd rather the
DCHECK wasn't there.  (Think about what would happen: suddenly a server push
could cause all Chromes to start crashing when people typed certainly input.)

This would be different if the protocol only allows one answer to be returned. 
But, as I recall, I think any/all suggestions can come with answer data.

A reasonable, defensive way to program this is to only grab the lower-scoring
answer if the top-scoring match doesn't already have an answer.

If you think the situation (Chrome thinks there are multiple answers for the
same query) is important to detect (and detect on the client, not on the suggest
server side), we can talk more.  There might be a better way to detect this than
a DCHECK.

[groby-ooo-7-16, 2014/09/23 03:34:00]

Since it's a DCHECK. It'll never trigger outside of debug builds. This is to
document an invariant we assume, and to alert people using debug builds (i.e.
devs) should that ever be violated.

I'm torn - the invariant is not a Chrome-side invariant, so we can technically
not make that guarantee, you're right. OTOH, it's assumed in a lot of places.

Your suggestions (only copy answers if the higher-scoring match doesn't have
one) results in more robust code even in the face of server side changes, so
I'll go with that. 

I'd still appreciate ideas on how to ensure this would still catch attention if
it happened. (Outside this CL, please :)

[Mark P, 2014/09/23 16:26:41]

Here are three ideas:
(1) server side monitoring
(2) UMA histogram (check it regularly, or possibly attach it to an alerting
system)
(3) the code might handle multiple answers fine right now; make your alerting
system a user filing a bug that something is wrong

+      i.first->second.answer_type = less_relevant_match.answer_type;
+      i.first->second.answer_contents = less_relevant_match.answer_contents;
+    }
   }
 }
 
 bool BaseSearchProvider::ParseSuggestResults(
     const base::Value& root_val,
     int default_result_relevance,
     bool is_keyword_result,
     SearchSuggestionParser::Results* results) {
   if (!SearchSuggestionParser::ParseSuggestResults(
       root_val, GetInput(is_keyword_result),
@@ skipping 27 matching lines @@
 }
 
 void BaseSearchProvider::OnDeletionComplete(
     bool success, SuggestionDeletionHandler* handler) {
   RecordDeletionResult(success);
   SuggestionDeletionHandlers::iterator it = std::find(
       deletion_handlers_.begin(), deletion_handlers_.end(), handler);
   DCHECK(it != deletion_handlers_.end());
   deletion_handlers_.erase(it);
 }