[AiS] Merge answers into highest-scoring result

[AiS] Merge answers into highest-scoring result

Answer contents and type must always be copied to the highest-scoring
suggestion, to ensure answers are not lost due to suggestion
deduplication.

BUG=415836
R=mpearson

Committed: https://crrev.com/978a4bf4a3be714950ed265660bd9566f7abd922
Cr-Commit-Position: refs/heads/master@{#296520}

[groby-ooo-7-16, 2014-09-19 01:24:16]

Please take a look (this is the end-result of our discussions around merging
answers content into other queries to prevent accidental suppression)

[Mark P, 2014-09-19 18:56:37]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2014-09-19 18:56:38]

I think this ought to have a test as well.

--mark

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
File components/omnibox/base_search_provider.cc (right):

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
components/omnibox/base_search_provider.cc:425:
i.first->second.duplicate_matches.back();
This makes me a little nervous because you're depending that duplicate-tracking
code above always appends the latest duplicate to the end of the list.  I'm not
wedded to this idea (so if you think this behavior is obvious, you don't have to
do anything), but I think you should either:
- comment somewhere about relying on this property
- simply add another variable, MatchMap::const_iterator latest_duplicate, that
you set appropriately in either branch above, then use here.

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
components/omnibox/base_search_provider.cc:427:
DCHECK(i.first->second.answer_type.empty());
Why is this true?  Why can't multiple equivalent suggestions have answers
(probably the same answers)?

[groby-ooo-7-16, 2014-09-22 05:49:26]

PTAL - and if I may say so, setting this up for a test without chrome/
dependencies is a PITA :)

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
File components/omnibox/base_search_provider.cc (right):

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
components/omnibox/base_search_provider.cc:425:
i.first->second.duplicate_matches.back();
On 2014/09/19 18:56:38, Mark P wrote:
> This makes me a little nervous because you're depending that
duplicate-tracking
> code above always appends the latest duplicate to the end of the list.  I'm
not
> wedded to this idea (so if you think this behavior is obvious, you don't have
to
> do anything), but I think you should either:
> - comment somewhere about relying on this property
> - simply add another variable, MatchMap::const_iterator latest_duplicate, that
> you set appropriately in either branch above, then use here.

Comment added. I think it's fairly clear, but calling it out doesn't hurt. Not
sure the iterator would make it clearer.

https://codereview.chromium.org/584653004/diff/1/components/omnibox/base_sear...
components/omnibox/base_search_provider.cc:427:
DCHECK(i.first->second.answer_type.empty());
On 2014/09/19 18:56:38, Mark P wrote:
> Why is this true?  Why can't multiple equivalent suggestions have answers
> (probably the same answers)?

Because suggestions are only provided by the search engine, not Chrome - and it
will only deliver one answer. The DCHECK is here to flag that necessity -
merging answers when there are two of them will be an interesting problem :)

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

nice work on the test by the way!

--mark

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
File components/omnibox/base_search_provider.cc (right):

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
components/omnibox/base_search_provider.cc:429:
DCHECK(i.first->second.answer_type.empty());
On 2014/09/22 05:49:25, groby wrote:
> On 2014/09/19 18:56:38, Mark P wrote:
> > Why is this true?  Why can't multiple equivalent suggestions have answers
> > (probably the same answers)?
> 
> Because suggestions are only provided by the search engine, not Chrome - and
it
> will only deliver one answer. The DCHECK is here to flag that necessity -
> merging answers when there are two of them will be an interesting problem :)

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.

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
File components/omnibox/base_search_provider_unittest.cc (right):

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
components/omnibox/base_search_provider_unittest.cc:104:
scoped_ptr<TemplateURLServiceClient>(),
Not sure about style considerations, but if this is () is the same as passing in
NULL, please do it explicitly.

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

PTAL

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
File components/omnibox/base_search_provider.cc (right):

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
components/omnibox/base_search_provider.cc:429:
DCHECK(i.first->second.answer_type.empty());
On 2014/09/22 23:54:06, Mark P wrote:
> On 2014/09/22 05:49:25, groby wrote:
> > On 2014/09/19 18:56:38, Mark P wrote:
> > > Why is this true?  Why can't multiple equivalent suggestions have answers
> > > (probably the same answers)?
> > 
> > Because suggestions are only provided by the search engine, not Chrome - and
> it
> > will only deliver one answer. The DCHECK is here to flag that necessity -
> > merging answers when there are two of them will be an interesting problem :)
> 
> 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.)

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 :)

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

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
File components/omnibox/base_search_provider_unittest.cc (right):

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
components/omnibox/base_search_provider_unittest.cc:104:
scoped_ptr<TemplateURLServiceClient>(),
On 2014/09/22 23:54:06, Mark P wrote:
> Not sure about style considerations, but if this is () is the same as passing
in
> NULL, please do it explicitly.

It is not, unfortunately - the ctor wants a scoped_ptr, not NULL. And you can't
pass NULL to a scoped_ptr without casting, which looks somewhat ridiculous:
 
scoped_ptr<TemplateURLServiceClient>(reinterpret_cast<TemplateURLServiceClient>(NULL));

std::nullptr can't come soon enough :)

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

lgtm

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
File components/omnibox/base_search_provider.cc (right):

https://codereview.chromium.org/584653004/diff/40001/components/omnibox/base_...
components/omnibox/base_search_provider.cc:429:
DCHECK(i.first->second.answer_type.empty());
On 2014/09/23 03:34:00, groby wrote:
> On 2014/09/22 23:54:06, Mark P wrote:
> > On 2014/09/22 05:49:25, groby wrote:
> > > On 2014/09/19 18:56:38, Mark P wrote:
> > > > Why is this true?  Why can't multiple equivalent suggestions have
answers
> > > > (probably the same answers)?
> > > 
> > > Because suggestions are only provided by the search engine, not Chrome -
and
> > it
> > > will only deliver one answer. The DCHECK is here to flag that necessity -
> > > merging answers when there are two of them will be an interesting problem
:)
> > 
> > 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.)
> 
> 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 :)

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
> 
> > 
> > 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 20:31:26]

The CQ bit was checked by groby@chromium.org

[groby-ooo-7-16, 2014-09-23 20:32:17]

There's a new CL coming up that enforces a single answer, so let's see how that
goes :)

[commit-bot: I haz the power, 2014-09-23 20:36:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584653004/80001

[commit-bot: I haz the power, 2014-09-23 21:18:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:18:34]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2014-09-24 20:36:35]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-09-24 20:37:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584653004/100001

[commit-bot: I haz the power, 2014-09-24 21:17:41]

Committed patchset #6 (id:100001) as 6c43c0a8556854ef02bf6ea65257da4e26ccad2d

[commit-bot: I haz the power, 2014-09-24 21:18:25]

Patchset 6 (id:??) landed as
https://crrev.com/978a4bf4a3be714950ed265660bd9566f7abd922
Cr-Commit-Position: refs/heads/master@{#296520}

[newt (away), 2014-09-24 23:00:57]

newt@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2014-09-24 23:00:58]

The Linux ASan LSan Tests bot is failing when running
BaseSearchProviderTest.PreserveAnswersWhenDeduplicating

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

[jiayl, 2014-09-24 23:04:03]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/598253002/ by jiayl@chromium.org.

The reason for reverting is: The new tests are flaky:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....