[AiS] Merge answers into highest-scoring result

components/omnibox/base_search_provider.cc

Index: components/omnibox/base_search_provider.cc
diff --git a/components/omnibox/base_search_provider.cc b/components/omnibox/base_search_provider.cc
index dfc26a59a207d966a01a27a120d3e35e4ece90bc..1cb26781e75c1ce922ad50b23c952ef1536371f9 100644
--- a/components/omnibox/base_search_provider.cc
+++ b/components/omnibox/base_search_provider.cc
@@ -420,6 +420,16 @@ void BaseSearchProvider::AddMatchToMap(
           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;
+    }
   }
 }