[AiS] Add a new AutocompleteMatchType for Answers in Suggest.

[AiS] Add a new AutocompleteMatchType for answers in suggest.

BUG=383456
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276924

[Justin Donnelly, 2014-06-10 21:30:05]

Peter, Ted - please take a look and advise on the addition of a new suggest
type.

The primary motivation is for clearer logging and simpler logs analysis (vs.
existence checks on answer contents).  A lesser motivation is slightly clearer
UI code.

But I'm not 100% clear on the implications of a couple of the changes here so
let me know if you think this approach makes sense.

See https://chrome-internal-review.googlesource.com/165705 for associated
downstream change.

[Peter Kasting, 2014-06-10 21:49:46]

You'll want to update this CL to reflect mpearson's recent changes to eliminate
the AutocompleteMatchType enum.

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:344: type ==
AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
The effect of putting this here will be that the ShortcutsProvider, on seeing
the user hit enter on one of these matches, will instead create a shortcut entry
for a standard search that uses the search terms that triggered this match.

In other words, if I type in "foo", and I get an Answer, and I hit enter on it,
from the ShortcutsProvider perspective this will be treated like I did a search
for "foo".

I don't know whether this is what you want or not.

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:918: match_type =
AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
My only concern here would be if there's a validation issue, where if we don't
get "correct" data from the ansa/ansb parameters (or whatever), some later stage
will auto-downgrade this match to appear like a normal search result or
something.  If that could ever happen, we should ensure it happens here so we
tag the match type correctly for what the user will actually see.

[Justin Donnelly, 2014-06-11 15:51:53]

> You'll want to update this CL to reflect mpearson's recent > changes to
eliminate the AutocompleteMatchType enum.

Which changes?  It looks like you may be referring to
https://codereview.chromium.org/319523005/ but that doesn't change
AutocompleteMatchType (and is included in my baseline regardless).

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:344: type ==
AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
On 2014/06/10 21:49:46, Peter Kasting wrote:
> The effect of putting this here will be that the ShortcutsProvider, on seeing
> the user hit enter on one of these matches, will instead create a shortcut
entry
> for a standard search that uses the search terms that triggered this match.
> 
> In other words, if I type in "foo", and I get an Answer, and I hit enter on
it,
> from the ShortcutsProvider perspective this will be treated like I did a
search
> for "foo".
> 
> I don't know whether this is what you want or not.

Well, hitting enter on an Answer for "foo" will, in fact, search for "foo" so
this might be what I want.  But what's the alternative?  In other words, what's
the effect of putting the raw match into the shortcuts backend?

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:918: match_type =
AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
On 2014/06/10 21:49:46, Peter Kasting wrote:
> My only concern here would be if there's a validation issue, where if we don't
> get "correct" data from the ansa/ansb parameters (or whatever), some later
stage
> will auto-downgrade this match to appear like a normal search result or
> something.  If that could ever happen, we should ensure it happens here so we
> tag the match type correctly for what the user will actually see.

Good question, but no we don't plan to downgrade answers.  If the JSON is
missing required data or malformed, we're going to drop the answer altogether. 
(The current frontend code does, in fact downgrade but my next CL is going to
change this.)

[Peter Kasting, 2014-06-11 17:02:02]

LGTM

On 2014/06/11 15:51:53, Justin Donnelly wrote:
> > You'll want to update this CL to reflect mpearson's recent
> > changes to eliminate the AutocompleteMatchType enum.
> 
> Which changes?  It looks like you may be referring to
> https://codereview.chromium.org/319523005/ but that doesn't change
> AutocompleteMatchType (and is included in my baseline regardless).

Uh... never mind then.  :)

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/327003003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:344: type ==
AutocompleteMatchType::SEARCH_SUGGEST_ANSWER;
On 2014/06/11 15:51:53, Justin Donnelly wrote:
> On 2014/06/10 21:49:46, Peter Kasting wrote:
> > The effect of putting this here will be that the ShortcutsProvider, on
seeing
> > the user hit enter on one of these matches, will instead create a shortcut
> entry
> > for a standard search that uses the search terms that triggered this match.
> > 
> > In other words, if I type in "foo", and I get an Answer, and I hit enter on
> it,
> > from the ShortcutsProvider perspective this will be treated like I did a
> search
> > for "foo".
> > 
> > I don't know whether this is what you want or not.
> 
> Well, hitting enter on an Answer for "foo" will, in fact, search for "foo" so
> this might be what I want.  But what's the alternative?  In other words,
what's
> the effect of putting the raw match into the shortcuts backend?

The alternative is that you get back a match exactly like the one that went it. 
So, I type "foo", I get an Answer, I hit enter on it, it searches for "foo". 
Now the next time I type "f", the shortcuts system returns the "Answer for foo"
match.

With the other specialized search results we've decided this isn't really what
we want, that instead, the act of "hitting enter on this means search for foo"
should result in the shortcuts system promoting "search for foo" in the future. 
I think that's probably appropriate for you as well.

[Alexei Svitkine (slow), 2014-06-11 17:04:46]

https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:207: SEARCH_SUGGEST_ANSWER   = 22;
// A short result for a suggested search.
Please get this change committed to the google3 version of the proto before
landing this CL.

[Ted C, 2014-06-11 17:20:53]

On 2014/06/11 17:04:46, Alexei Svitkine wrote:
>
https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
> File components/metrics/proto/omnibox_event.proto (right):
> 
>
https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
> components/metrics/proto/omnibox_event.proto:207: SEARCH_SUGGEST_ANSWER   =
22;
> // A short result for a suggested search.
> Please get this change committed to the google3 version of the proto before
> landing this CL.

android - lgtm

[Justin Donnelly, 2014-06-11 17:21:39]

https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/327003003/diff/20001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:207: SEARCH_SUGGEST_ANSWER   = 22;
// A short result for a suggested search.
On 2014/06/11 17:04:45, Alexei Svitkine wrote:
> Please get this change committed to the google3 version of the proto before
> landing this CL.

Will do.  I assume you're referring to
//logs/proto/googleclient/chrome/uma/omnibox_event.proto?

[Alexei Svitkine (slow), 2014-06-11 17:22:55]

Yes, that's the one.


On Wed, Jun 11, 2014 at 10:21 AM, <jdonnelly@chromium.org> wrote:

>
> https://codereview.chromium.org/327003003/diff/20001/
> components/metrics/proto/omnibox_event.proto
> File components/metrics/proto/omnibox_event.proto (right):
>
> https://codereview.chromium.org/327003003/diff/20001/
> components/metrics/proto/omnibox_event.proto#newcode207
> components/metrics/proto/omnibox_event.proto:207: SEARCH_SUGGEST_ANSWER
>  = 22; // A short result for a suggested search.
> On 2014/06/11 17:04:45, Alexei Svitkine wrote:
>
>> Please get this change committed to the google3 version of the proto
>>
> before
>
>> landing this CL.
>>
>
> Will do.  I assume you're referring to
> //logs/proto/googleclient/chrome/uma/omnibox_event.proto?
>
> https://codereview.chromium.org/327003003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2014-06-12 20:36:29]

On 2014/06/11 17:22:55, Alexei Svitkine wrote:
> Yes, that's the one.

The change in google3 has landed.  Can you take another look, Alexei?

[Alexei Svitkine (slow), 2014-06-12 21:18:20]

LGTM for components/metrics/proto/omnibox_event.proto

I defer to others for reviews of the other files.

[Justin Donnelly, 2014-06-12 21:44:40]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-06-12 21:46:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/327003003/20001

[commit-bot: I haz the power, 2014-06-13 04:26:16]

Change committed as 276924