Don't try to create a verbatim match with an empty input.

Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input| (if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313
Committed: https://crrev.com/3986ea5094d68640f88501cfddb01689d89eb8ba
Cr-Commit-Position: refs/heads/master@{#405136}

[jif-google, 2016-07-04 16:13:35]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

BUG=None.
==========

to

==========
Don't try to create a verbatim match with an empty input.

BUG=625313
==========

[jif-google, 2016-07-04 16:21:29]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

BUG=625313
==========

to

==========
Don't try to create a verbatim match with an empty input.

On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty
|input| (if the omnibox was empty), which eventually triggers a DCHECK
in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

This CL makes it forbidden to pass an empty input to |VerbatimMatchForURL|,
and fixes the clipboard_url_provider.

BUG=625313
==========

[jif-google, 2016-07-04 16:26:38]

Patchset #1 (id:1) has been deleted

[jif-google, 2016-07-04 16:30:37]

jif@google.com changed reviewers:
+ mpearson@google.com, pkasting@google.com

[jif-google, 2016-07-04 16:31:39]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty
|input| (if the omnibox was empty), which eventually triggers a DCHECK
in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

This CL makes it forbidden to pass an empty input to |VerbatimMatchForURL|,
and fixes the clipboard_url_provider.

BUG=625313
==========

to

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty
|input| (if the omnibox was empty), which eventually triggers a DCHECK
in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to |VerbatimMatchForURL|,
and fixes the clipboard_url_provider.

BUG=625313
==========

[jif-google, 2016-07-04 16:43:49]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty
|input| (if the omnibox was empty), which eventually triggers a DCHECK
in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to |VerbatimMatchForURL|,
and fixes the clipboard_url_provider.

BUG=625313
==========

to

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input|
(if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313
==========

[jif, 2016-07-04 16:45:54]

Hi,
this is a somewhat urgent CL as the omnibox in Chrome iOS is pretty
much unusable at this moment: we are hitting the DCHECK_GT
whenever we press the omnibox from the NTP.

AFAIK there are two ways to solve the problem described in the description of
this CL:
1/ Never call VerbatimMatchForURL when the input is empty, which would make
sense because there's really no verbatim match to return.
2/ Early return in VerbatimMatchForURL if the input is empty. This allows
callers of this function not to worry about the content of the input, but incurs
an unnecessary (albeit admittedly probably negligible) overhead in almost all
cases.

In this CL I choose option 1/.
Let me know if you think it's not the right way, and what would be the correct
fix, whether it'd be option 2/ or something else.

[sdefresne, 2016-07-05 10:08:56]

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

[sdefresne, 2016-07-05 10:08:57]

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

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:46: // If the omnibox is
not empty, adds a default match.
nit: I would use if (!input.text().empty())

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

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/verbatim_match.cc:21: DCHECK(input.text().length());
nit: I would use DCHECK(!input.text().empty())

[Mark P, 2016-07-05 22:01:57]

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

[Mark P, 2016-07-05 22:01:57]

I'm not yet convinced this changelist is the right approach.

If you want a band-aid solution, please simply pass in a nullptr for the
HistoryURLProvider* from clipboard provider to the verbatim match code.  That
one-line  change will fix your issue.

For me to help identify what the right change is, can you explain previously
what happened on iOS when the user had a URL in the clipboard and tapped in an
omnibox that was currently empty.  What if anything was suggested?

--mark

[jif-google, 2016-07-06 08:13:23]

On 2016/07/05 22:01:57, Mark P wrote:
> I'm not yet convinced this changelist is the right approach.
> 
> If you want a band-aid solution, please simply pass in a nullptr for the
> HistoryURLProvider* from clipboard provider to the verbatim match code.  That
> one-line  change will fix your issue.

Ok, thanks.
It turns out that the we are hitting the DCHECK only if there's a URL in the the
pasteboard, so I think we can skip the band-aid solution.

> 
> For me to help identify what the right change is, can you explain previously
> what happened on iOS when the user had a URL in the clipboard and tapped in an
> omnibox that was currently empty.  What if anything was suggested?

If the omnibox was empty and there was something in the clipboard URL:
The ClipboardURLProvider got a verbatim_match with a invalid destination_url,
therefore the verbatim_match was not pushed to the matches_ (line 45 in the
original clipboard_url_provider.cc).
An AutocompleteMatch was then created with a relevance of 1299 (the
verbatim_match.relevance was 1300 even if the omnibox was empty), and added to
the matches_

> --mark

[Mark P, 2016-07-06 18:56:47]

Given your explanation, this sound looks reasonable.  Some comments below.

--mark

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

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:46: // If the omnibox is
not empty, adds a default match.
nit: adds -> add

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:51: if
(verbatim_match.destination_url.is_valid())
This if test shouldn't be necessary anymore.  (It should always be valid.)

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:55: // Add the clipboard
match. The relevance can be '1' because there can only be
side comment: once iOS enables ZeroSuggest to most visited, a la Android, this
'1' will have to change.  You'll probably want a number more like 800 (so it
beats the default other zero suggest results).  You may want to change it now in
preparation.

[Peter Kasting, 2016-07-06 20:55:21]

If the verbatim match cannot be constructed, won't we now add the clipboard
match as the only (and thus default) match?  I don't think we ever want the
clipboard match to be default.

Maybe instead we should be using the underlying URL instead of the visible URL
for the NTP, so we can always create a verbatim match.

[Mark P, 2016-07-06 21:17:27]

On 2016/07/06 20:55:21, Peter Kasting wrote:
> If the verbatim match cannot be constructed, won't we now add the clipboard
> match as the only (and thus default) match?  I don't think we ever want the
> clipboard match to be default.
> 
> Maybe instead we should be using the underlying URL instead of the visible URL
> for the NTP, so we can always create a verbatim match.

Maybe iOS omnibox handles no-legal-default-match situation gracefully?
(i.e., pressing enter/Go doesn't do anything)

--mark

[Peter Kasting, 2016-07-06 21:28:09]

On 2016/07/06 21:17:27, Mark P wrote:
> On 2016/07/06 20:55:21, Peter Kasting wrote:
> > If the verbatim match cannot be constructed, won't we now add the clipboard
> > match as the only (and thus default) match?  I don't think we ever want the
> > clipboard match to be default.
> > 
> > Maybe instead we should be using the underlying URL instead of the visible
URL
> > for the NTP, so we can always create a verbatim match.
> 
> Maybe iOS omnibox handles no-legal-default-match situation gracefully?
> (i.e., pressing enter/Go doesn't do anything)

See the last section in this file: if there's no verbatim match, we make
clipboard default.  Not sure that's the right thing to do.

[jif, 2016-07-11 15:09:18]

On 2016/07/06 21:28:09, Peter Kasting (OOO til Jul 18) wrote:
> On 2016/07/06 21:17:27, Mark P wrote:
> > On 2016/07/06 20:55:21, Peter Kasting wrote:
> > > If the verbatim match cannot be constructed, won't we now add the
clipboard
> > > match as the only (and thus default) match?  I don't think we ever want
the
> > > clipboard match to be default.
> > > 
> > > Maybe instead we should be using the underlying URL instead of the visible
> URL
> > > for the NTP, so we can always create a verbatim match.
> > 
> > Maybe iOS omnibox handles no-legal-default-match situation gracefully?
> > (i.e., pressing enter/Go doesn't do anything)
> 
> See the last section in this file: if there's no verbatim match, we make
> clipboard default.  Not sure that's the right thing to do.

We do make the clipboard suggestion be the default match when there's nothing in
the omnibox, however:
The UITextField is configured to disable the "go" key when it is empty, so even
when the clipboard suggestion is the default we can't navigate to it by pressing
"go".

[jif, 2016-07-11 15:09:36]

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

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:46: // If the omnibox is
not empty, adds a default match.
On 2016/07/05 10:08:56, sdefresne wrote:
> nit: I would use if (!input.text().empty())

Done.

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:46: // If the omnibox is
not empty, adds a default match.
On 2016/07/06 18:56:47, Mark P wrote:
> nit: adds -> add

Done.

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:51: if
(verbatim_match.destination_url.is_valid())
On 2016/07/06 18:56:47, Mark P wrote:
> This if test shouldn't be necessary anymore.  (It should always be valid.)

Good catch. Done.

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:55: // Add the clipboard
match. The relevance can be '1' because there can only be
On 2016/07/06 18:56:47, Mark P wrote:
> side comment: once iOS enables ZeroSuggest to most visited, a la Android, this
> '1' will have to change.  You'll probably want a number more like 800 (so it
> beats the default other zero suggest results).  You may want to change it now
in
> preparation.

Done.

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

https://codereview.chromium.org/2124563002/diff/20001/components/omnibox/brow...
components/omnibox/browser/verbatim_match.cc:21: DCHECK(input.text().length());
On 2016/07/05 10:08:56, sdefresne wrote:
> nit: I would use DCHECK(!input.text().empty())

Done.

[Mark P, 2016-07-11 17:04:16]

lgtm
though you may want to wait for others to re-review too

--mark

[sdefresne, 2016-07-12 08:40:31]

lgtm

[jif-google, 2016-07-12 08:41:13]

The CQ bit was checked by jif@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 08:41:35]

Dry run: 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, 2016-07-12 10:43:28]

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

[commit-bot: I haz the power, 2016-07-12 10:43:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jif, 2016-07-13 13:06:15]

On 2016/07/12 08:40:31, sdefresne wrote:
> lgtm

pkasting is OOO for the rest of the week, so I'll just land it as it is.

[jif, 2016-07-13 13:06:24]

The CQ bit was checked by jif@chromium.org

[commit-bot: I haz the power, 2016-07-13 13:06:39]

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, 2016-07-13 13:43:55]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input|
(if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313
==========

to

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input|
(if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313
==========

[commit-bot: I haz the power, 2016-07-13 13:43:59]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-13 13:44:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 13:45:25]

Description was changed from

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input|
(if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313
==========

to

==========
Don't try to create a verbatim match with an empty input.

## Description of the problem
On iOS, tapping on an empty omnibox triggers a request to the
ClipboardUrlProvider.
The ClipboardURLProvider then calls VerbatimMatchForURL with an empty |input|
(if the omnibox was empty), which eventually triggers a
DCHECK_GT in |AutocompleteMatch::ClassifyLocationInString|:

  // Classifying an empty match makes no sense and will lead to validation
  // errors later.
  DCHECK_GT(match_length, 0U);

## Description of the solution
This CL makes it forbidden to pass an empty input to
|VerbatimMatchForURL|, and fixes the clipboard_url_provider.

BUG=625313

Committed: https://crrev.com/3986ea5094d68640f88501cfddb01689d89eb8ba
Cr-Commit-Position: refs/heads/master@{#405136}
==========

[commit-bot: I haz the power, 2016-07-13 13:45:27]

Patchset 2 (id:??) landed as
https://crrev.com/3986ea5094d68640f88501cfddb01689d89eb8ba
Cr-Commit-Position: refs/heads/master@{#405136}

[Peter Kasting, 2016-07-18 22:16:14]

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

[Peter Kasting, 2016-07-18 22:16:15]

I'm back now.

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:34: // If the user started
typing, do not offer clipboard based match.
Nit: I would not add these next two comments.  They simply restate the code
without adding any useful information such as motivation why we do this, so
they're just clutter.

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:46: // If the omnibox is
not empty, add a default match.
Again, this adds detail that just restates the code.  If you think the old
comment is lacking, focus on describing why it matters that the omnibox is
non-empty, instead of just restating the code.

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:55: AutocompleteMatch
match(this, 800, false, AutocompleteMatchType::CLIPBOARD);
I don't think we should hardcode 800 here.  If there is a verbatim match, using
its relevance - 1, as before, seems correct.  If there isn't, will there ever be
ZeroSuggest results either?  If not, then something like max(1, verbatim
relevance - 1) would probably be correct.

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2124563002/diff/40001/components/omnibox/brow...
components/omnibox/browser/verbatim_match.cc:21: DCHECK(!input.text().empty());
How hard would it be to just be robust against this case (i.e. not try to
classify an empty string)?

That seems like a better answer, if it's doable.