Show Physical Web zero-suggest omnibox suggestions after query is deleted

Show Physical Web zero-suggest omnibox suggestions after query is deleted

The Physical Web omnibox provider provides suggestions when the user first
focuses the omnibox (zero-suggest) as well as after the user begins typing
a query (query suggest). This CL will allow PW zero-suggest suggestions to
be shown when the user has begun typing a query and then deleted it.

BUG=680505
Review-Url: https://codereview.chromium.org/2657473003
Cr-Commit-Position: refs/heads/master@{#446193}
Committed: https://chromium.googlesource.com/chromium/src/+/cac0db7f354b8187b4932f1131d3701c5197d71b

[mattreynolds, 2017-01-24 01:36:45]

mattreynolds@chromium.org changed reviewers:
+ iankc@google.com, mpearson@chromium.org

[mattreynolds, 2017-01-24 01:36:46]

Hi Mark and Ian, PTAL

Previously we would only take the zero-suggest branch if
input.from_omnibox_focus() is true, which only occurs when the omnibox is first
focused. This change also allows us to take the branch if the omnibox input is
empty, which can happen when the user types a query and then deletes it.

[Mark P, 2017-01-24 04:52:46]

I think displaying a suggestion for an after-typing empty-omnibox should only be
done as part of the after typing experiment.  That's just my instinct but it
feels like it makes sense to me.  (If I type and delete characters to get
something empty and would see a physical web URL suggestion then, I'd think that
if on the way to getting there that I had text in the omnibox that matched the
URL, it would display the URL.)

I'm open to being convinced otherwise.

--mark

[mattreynolds, 2017-01-24 18:49:09]

On 2017/01/24 04:52:46, Mark P wrote:
> I think displaying a suggestion for an after-typing empty-omnibox should only
be
> done as part of the after typing experiment.  That's just my instinct but it
> feels like it makes sense to me.  (If I type and delete characters to get
> something empty and would see a physical web URL suggestion then, I'd think
that
> if on the way to getting there that I had text in the omnibox that matched the
> URL, it would display the URL.)
> 
> I'm open to being convinced otherwise.
> 
> --mark

I see where you're coming from, maybe it should require both AfterTyping and
ZeroSuggest. But, I think this issue should be considered a bug (ie, ZeroSuggest
should apply any time the omnibox is empty, regardless of how you got there). If
we only fix it for the AfterTyping case, or for AfterTyping && ZeroSuggest, then
that still leaves buggy behavior for users with only ZeroSuggest enabled.

On Android, this is already the default behavior -- PhysicalWebProvider will
show ZeroSuggest results after clearing the input, even without this CL.

For metrics purposes I think it will be more useful for us to group after-typing
empty-omnibox suggestions with the zero-suggest suggestions, since they display
the same content.

[Mark P, 2017-01-24 19:04:06]

On 2017/01/24 18:49:09, mattreynolds wrote:
> On 2017/01/24 04:52:46, Mark P wrote:
> > I think displaying a suggestion for an after-typing empty-omnibox should
only
> be
> > done as part of the after typing experiment.  That's just my instinct but it
> > feels like it makes sense to me.  (If I type and delete characters to get
> > something empty and would see a physical web URL suggestion then, I'd think
> that
> > if on the way to getting there that I had text in the omnibox that matched
the
> > URL, it would display the URL.)
> > 
> > I'm open to being convinced otherwise.
> > 
> > --mark
> 
> I see where you're coming from, maybe it should require both AfterTyping and
> ZeroSuggest. But, I think this issue should be considered a bug (ie,
ZeroSuggest
> should apply any time the omnibox is empty, regardless of how you got there).
If
> we only fix it for the AfterTyping case, or for AfterTyping && ZeroSuggest,
then
> that still leaves buggy behavior for users with only ZeroSuggest enabled.

I'm still not convinced.  Here's a different aspect of my reasoning:

One reason we separated the zero suggest and after typing behavior is because
it's possible that, by typing, the user has rejected the zero suggest
suggestions
they're previously seen and doesn't want to see them again.  Bringing them
back after this rejection seems odd, which is why I think it probably should be
grouped in the after-typing bucket.

> On Android, this is already the default behavior -- PhysicalWebProvider will
> show ZeroSuggest results after clearing the input, even without this CL.
> 
> For metrics purposes I think it will be more useful for us to group
after-typing
> empty-omnibox suggestions with the zero-suggest suggestions, since they
display
> the same content.

Yes, but after-typing empty-omnibox suggestions could be qualitatively different
than
before-typing empty-omnibox suggestions.  (See above.)

Also,
if we launch them with after-typing, we'd be able to estimate the effect by
looking at
the difference between physical web clickthrough in the zero suggest experiment
(implicitly all input_length==0) and physical web clickthrough on
input_length==0
inputs in the joint zero suggest && after typing experiment.  In other words, if
we
care to measure them, I think we can do so only if launched as part of the after
typing experiment

Still willing to be convinced, yet also trying to convince you. :-)

--mark

[Mark P, 2017-01-24 19:06:12]

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:95: if (!matches_.empty() &&
!input.text().empty()) {
I don't think this test is exactly right anymore.
On a non-NTP, if I delete all the input text, I still see the current page as
the top suggestion.
I guess some other provider is adding the current page URL as a suggestion, so
in some sense the fact that we don't isn't an issue.  Yet, if we're relying on
some other provider to add it in this case (user modified a non-NTP URL to an
empty string), we should at least comment on this reliance here.  Or revise the
code so we don't rely on it. :-)

[mattreynolds, 2017-01-24 22:41:54]

> One reason we separated the zero suggest and after typing behavior is because
> it's possible that, by typing, the user has rejected the zero suggest
> suggestions
> they're previously seen and doesn't want to see them again.  Bringing them
> back after this rejection seems odd, which is why I think it probably should
be
> grouped in the after-typing bucket.

Why is that odd? Other providers don't consider it a signal of rejection.

I think treating user input as rejection is assuming too much. What if the user
deleted their query because they wanted to see zero suggest results again?

> Also,
> if we launch them with after-typing, we'd be able to estimate the effect by
> looking at
> the difference between physical web clickthrough in the zero suggest
experiment
> (implicitly all input_length==0) and physical web clickthrough on
> input_length==0
> inputs in the joint zero suggest && after typing experiment.  In other words,
if
> we
> care to measure them, I think we can do so only if launched as part of the
after
> typing experiment

Better metrics is good enough justification for me, let's put it in the joint
experiment.

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:95: if (!matches_.empty() &&
!input.text().empty()) {
I think that's an Android bug, we should never display only the verbatim match.
On iOS, no suggestions are shown when you clear the input, and if I try to call
VerbatimMatchForURL with an empty input I hit a DCHECK.

Is a default match actually required in this case? IIUC, a default is added to
support focus->enter for reloading the page. If the user has already modified
the input, do we care about adding the verbatim match?

[Mark P, 2017-01-25 20:19:47]

lgtm, though see the two comments below

Can you confirm that the newly added test fails before this code change?

thanks,
mark

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:95: if (!matches_.empty() &&
!input.text().empty()) {
On 2017/01/24 22:41:54, mattreynolds wrote:
> I think that's an Android bug,
Filed crbug.com/685332.  It seems straightforward to fix.  Can I assign it to
you, as this is the area you're working in right now.

> we should never display only the verbatim match.
> On iOS, no suggestions are shown when you clear the input, and if I try to
call
> VerbatimMatchForURL with an empty input I hit a DCHECK.
> 
> Is a default match actually required in this case? IIUC, a default is added to
> support focus->enter for reloading the page. If the user has already modified
> the input, do we care about adding the verbatim match?

Good point.

https://codereview.chromium.org/2657473003/diff/20001/components/omnibox/brow...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/20001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:78: bool input_from_focus =
input.from_omnibox_focus();
nit: both const

[mattreynolds, 2017-01-25 22:01:52]

Yep, it fails without this change.  Thanks Mark!

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:95: if (!matches_.empty() &&
!input.text().empty()) {
On 2017/01/25 20:19:47, Mark P wrote:
> On 2017/01/24 22:41:54, mattreynolds wrote:
> > I think that's an Android bug,
> Filed crbug.com/685332.  It seems straightforward to fix.  Can I assign it to
> you, as this is the area you're working in right now.

Thanks, I'll look into it.

https://codereview.chromium.org/2657473003/diff/20001/components/omnibox/brow...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2657473003/diff/20001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:78: bool input_from_focus =
input.from_omnibox_focus();
On 2017/01/25 20:19:47, Mark P wrote:
> nit: both const

Done.

[mattreynolds, 2017-01-25 22:02:42]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2017-01-25 22:02:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2657473003/#ps40001
(title: "const")

[commit-bot: I haz the power, 2017-01-25 22:03:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 01:27:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485381762870460,
"parent_rev": "f79ede40658ecab4a6c72b6e51c456d68dab2ad0", "commit_rev":
"cac0db7f354b8187b4932f1131d3701c5197d71b"}

[commit-bot: I haz the power, 2017-01-26 01:28:11]

Description was changed from

==========
Show Physical Web zero-suggest omnibox suggestions after query is deleted

The Physical Web omnibox provider provides suggestions when the user first
focuses the omnibox (zero-suggest) as well as after the user begins typing
a query (query suggest). This CL will allow PW zero-suggest suggestions to
be shown when the user has begun typing a query and then deleted it.

BUG=680505
==========

to

==========
Show Physical Web zero-suggest omnibox suggestions after query is deleted

The Physical Web omnibox provider provides suggestions when the user first
focuses the omnibox (zero-suggest) as well as after the user begins typing
a query (query suggest). This CL will allow PW zero-suggest suggestions to
be shown when the user has begun typing a query and then deleted it.

BUG=680505

Review-Url: https://codereview.chromium.org/2657473003
Cr-Commit-Position: refs/heads/master@{#446193}
Committed:
https://chromium.googlesource.com/chromium/src/+/cac0db7f354b8187b4932f1131d3...
==========

[commit-bot: I haz the power, 2017-01-26 01:28:12]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cac0db7f354b8187b4932f1131d3...