Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=

Committed: https://crrev.com/2b530deb82adb676d49e39e4ff7b24e206bbbf05
Cr-Commit-Position: refs/heads/master@{#416302}

[mattreynolds, 2016-08-22 19:01:04]

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

[mattreynolds, 2016-08-22 19:01:05]

Hi Mark, I came across this one while testing the PhysicalWebProvider changes.
When the omnibox is tapped it fills the input field with the current page URL.
If the scheme of the current page doesn't match the scheme of the default match,
the DCHECK fails. The fix is to not check the scheme if the input was filled as
a result of focusing the omnibox. This better matches the behavior described in
the comment: "If the user explicitly typed a scheme, ..."

[Mark P, 2016-08-22 19:18:46]

On focus, the default match should be current URL so the user can tap and press
go to reload the page.

If there is no current URL, then no match should be the default.  See comments
14-17 on this thread:
https://codereview.chromium.org/2124563002/

While it's nice that the Go button is disabled for empty boxes on iOS, this
change is much more heavy-handed and affects all platforms and could hide
regressions in the future.

The default match is supposed to be what happens when the user presses enter /
Go.  If enter/Go doesn't do anything, I think it's appropriate that you don't
have any match marked as the default match.  i.e., for empty omniboxes with
disabled buttons, make sure no match is marked as the default.

I suggest making sure the physical web suggestions aren't allowed to be default.
 (You might hit a different dcheck somewhere with no default--I'm not sure--but
that will likely be a cleaner place to do the cleanup code than here.)

--mark

[mattreynolds, 2016-08-22 21:34:18]

On 2016/08/22 19:18:46, Mark P wrote:
> On focus, the default match should be current URL so the user can tap and
press
> go to reload the page.
> 
> If there is no current URL, then no match should be the default.  See comments
> 14-17 on this thread:
> https://codereview.chromium.org/2124563002/
> 
> While it's nice that the Go button is disabled for empty boxes on iOS, this
> change is much more heavy-handed and affects all platforms and could hide
> regressions in the future.
> 
> The default match is supposed to be what happens when the user presses enter /
> Go.  If enter/Go doesn't do anything, I think it's appropriate that you don't
> have any match marked as the default match.  i.e., for empty omniboxes with
> disabled buttons, make sure no match is marked as the default.
> 
> I suggest making sure the physical web suggestions aren't allowed to be
default.
>  (You might hit a different dcheck somewhere with no default--I'm not
sure--but
> that will likely be a cleaner place to do the cleanup code than here.)
> 
> --mark

Yeah, if there's no default match it hits an earlier DCHECK in the same
SortAndCull function.

It looks like ClipboardURLProvider handles this by inserting a "verbatim match"
containing the current URL. In cases where the verbatim match is invalid (eg, no
current URL) then it allows the clipboard match to be the default. Does it seem
reasonable to do the same in the Physical Web provider?

[Mark P, 2016-08-22 22:08:33]

On 2016/08/22 21:34:18, mattreynolds wrote:
> On 2016/08/22 19:18:46, Mark P wrote:
> > On focus, the default match should be current URL so the user can tap and
> press
> > go to reload the page.
> > 
> > If there is no current URL, then no match should be the default.  See
comments
> > 14-17 on this thread:
> > https://codereview.chromium.org/2124563002/
> > 
> > While it's nice that the Go button is disabled for empty boxes on iOS, this
> > change is much more heavy-handed and affects all platforms and could hide
> > regressions in the future.
> > 
> > The default match is supposed to be what happens when the user presses enter
/
> > Go.  If enter/Go doesn't do anything, I think it's appropriate that you
don't
> > have any match marked as the default match.  i.e., for empty omniboxes with
> > disabled buttons, make sure no match is marked as the default.
> > 
> > I suggest making sure the physical web suggestions aren't allowed to be
> default.
> >  (You might hit a different dcheck somewhere with no default--I'm not
> sure--but
> > that will likely be a cleaner place to do the cleanup code than here.)
> > 
> > --mark
> 
> Yeah, if there's no default match it hits an earlier DCHECK in the same
> SortAndCull function.
> 
> It looks like ClipboardURLProvider handles this by inserting a "verbatim
match"
> containing the current URL. In cases where the verbatim match is invalid (eg,
no
> current URL) then it allows the clipboard match to be the default. Does it
seem
> reasonable to do the same in the Physical Web provider?

No, as you may see from the thread I linked to, when pkasting@ and I noticed
this
odd behavior of Clipboard provider, we asked for it to be fixed.  Instead, the
code
author punted with an ("emergency") workaround.  I see this change of yours as
another workaround that's trying to avoid fixing/clarifying the underlying
situation.

If the problem is the "has default match" dcheck earlier, I'd rather see that
DCHECK
be something like
if
  there is an underlying permanent URL
  OR
  this is from omnibox focus AND the user hasn't typed any text (this last part
should probably be redundant but can't hurt)
then
  dcheck there should be a default match.

And you'll need to revise these two iOS providers to make this behavior work.

--mark

[mattreynolds, 2016-08-23 21:57:01]

> If the problem is the "has default match" dcheck earlier, I'd rather see that
> DCHECK
> be something like
> if
>   there is an underlying permanent URL
>   OR
>   this is from omnibox focus AND the user hasn't typed any text (this last
part
> should probably be redundant but can't hurt)
> then
>   dcheck there should be a default match.
> 
> And you'll need to revise these two iOS providers to make this behavior work.
> 
> --mark

Okay, Physical Web (and Clipboard) suggestions should never be the default
match.  The fix is:

1. If we expect the omnibox text to be empty (eg, chrome://newtab on iOS), don't
require a default match
2. If the omnibox text is not empty, the provider will create a verbatim match
with the current URL and set it as the default match.

"there is an underlying permanent URL": In what situation would you expect there
to not be a permanent URL?  I think there is always a permanent URL, for the NTP
it's chrome://newtab.

I think what we really want is ToolbarModel::ShouldDisplayURL.  The ToolbarModel
decides which URLs shouldn't be displayed in the omnibox, and varies by
platform.  I don't see any easy way to make this accessible from
AutocompleteInput, but we can guess the return value by comparing input.text()
and input.current_url().  If input.text() is empty but input.current_url() is
valid, and if we're sure the user hasn't modified the input text (ie,
input.from_omnibox_focus()), then we're probably on a "Shouldn't Display URL"
page.

if (input.from_omnibox_focus() && input.text().empty() &&
input.current_url().is_valid()) {
  // This is a page that shouldn't display its URL, probably chrome://newtab. No
default match required.
} else {
  // There should be a valid default match.
}

WDYT?

[Mark P, 2016-08-23 22:01:52]

On Tue, Aug 23, 2016 at 2:57 PM, <mattreynolds@chromium.org> wrote:

> > If the problem is the "has default match" dcheck earlier, I'd rather see
> that
> > DCHECK
> > be something like
> > if
> > there is an underlying permanent URL
> > OR
> > this is from omnibox focus AND the user hasn't typed any text (this last
> part
> > should probably be redundant but can't hurt)
> > then
> > dcheck there should be a default match.
> >
> > And you'll need to revise these two iOS providers to make this behavior
> work.
> >
> > --mark
>
> Okay, Physical Web (and Clipboard) suggestions should never be the default
> match. The fix is:
>
> 1. If we expect the omnibox text to be empty (eg, chrome://newtab on iOS),
> don't
> require a default match
> 2. If the omnibox text is not empty, the provider will create a verbatim
> match
> with the current URL and set it as the default match.
>
> "there is an underlying permanent URL": In what situation would you expect
> there
> to not be a permanent URL? I think there is always a permanent URL, for
> the NTP
> it's chrome://newtab.
>
> I think what we really want is ToolbarModel::ShouldDisplayURL. The
> ToolbarModel
> decides which URLs shouldn't be displayed in the omnibox, and varies by
> platform. I don't see any easy way to make this accessible from
> AutocompleteInput, but we can guess the return value by comparing
> input.text()
> and input.current_url(). If input.text() is empty but input.current_url()
> is
> valid, and if we're sure the user hasn't modified the input text (ie,
> input.from_omnibox_focus()), then we're probably on a "Shouldn't Display
> URL"
> page.
>
> if (input.from_omnibox_focus() && input.text().empty() &&
> input.current_url().is_valid()) {
> // This is a page that shouldn't display its URL, probably
> chrome://newtab. No
> default match required.
> } else {
> // There should be a valid default match.
> }
>
> WDYT?
>

Now that you've walked me through this logic, I wonder if there would be
anything wrong with:
if (input.text().empty()) {
  // There should not be a default match.
} else {
  // There should be a valid default match.
}

I feel like a formulation like this might help us catch other bugs too
(e.g., overly aggressive caching of previous matches).

--mark



> https://codereview.chromium.org/2266083002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattreynolds, 2016-08-30 21:24:41]

Hi Mark, PTAL.  This change prevents non-verbatim matches from being marked as
the default for the Clipboard URL and Physical Web providers, and asserts that
there should be no default match when the omnibox input is empty.

I ran into a different zero-suggest-related bug while testing this, filed it
here:

https://bugs.chromium.org/p/chromium/issues/detail?id=642506

[Mark P, 2016-08-30 22:26:20]

lgtm, with one minor comment below, plus assuming all the tests still pass

--mark

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

https://codereview.chromium.org/2266083002/diff/20001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.h:12: 
nit: forward declare HistoryURLProvider

[Mark P, 2016-08-30 22:52:27]

P.S. If you have an Android test device, can you try it with your change?  I
wonder if Android's ZeroSuggest plays well with the new DCHECK.

[mattreynolds, 2016-08-30 23:08:05]

On 2016/08/30 22:52:27, Mark P wrote:
> P.S. If you have an Android test device, can you try it with your change?  I
> wonder if Android's ZeroSuggest plays well with the new DCHECK.

I'll try it on an Android device, but I've never seen any ZeroSuggestProvider
results on Android.  I have "Search and URL suggestions" enabled, is there
anything more I need?

[Mark P, 2016-08-30 23:18:32]

On 2016/08/30 23:08:05, mattreynolds wrote:
> On 2016/08/30 22:52:27, Mark P wrote:
> > P.S. If you have an Android test device, can you try it with your change?  I
> > wonder if Android's ZeroSuggest plays well with the new DCHECK.
> 
> I'll try it on an Android device, but I've never seen any ZeroSuggestProvider
> results on Android.  I have "Search and URL suggestions" enabled, is there
> anything more I need?

Zero-suggest on android should be your most visited URLs.  I don't think you
need any special boxes checked for it.

--mark

[mattreynolds, 2016-08-31 22:06:48]

I tested on a Pixel C and it seems fine, it didn't fail the DCHECK and I was
able to see ZSP results.

Some unit tests in autocomplete_result_unittest.cc needed to have a non-empty
input text to avoid failing the new DCHECK.  I also updated the
PhysicalWebProvider unit tests to check both the empty and non-empty input
cases.

Could you take another look?

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

https://codereview.chromium.org/2266083002/diff/20001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.h:12: 
On 2016/08/30 22:26:20, Mark P wrote:
> nit: forward declare HistoryURLProvider

Done.

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

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:73: if (!matches_.empty() &&
!input.text().empty()) {
bugfix: we should only add the verbatim match if we already added at least one
match

[Mark P, 2016-08-31 22:39:33]

still lgtm, minor comments below

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

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:71: // If the omnibox is not
empty, add a default verbatim match.
Perhaps revise the first sentence to something like
// If we're going to show matches and the omnibox is non-empty, add the default
current as the default match.
(and maybe drop the second sentence)

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:73: if (!matches_.empty() &&
!input.text().empty()) {
On 2016/08/31 22:06:48, mattreynolds wrote:
> bugfix: we should only add the verbatim match if we already added at least one
> match

Acknowledged.  Good catch.

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/physical_web_provider_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:124: std::string(),
GURL(), metrics::OmniboxEventProto::OTHER,
better: INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:125: true, false,
true, true, true, TestSchemeClassifier());
nit: change the first "true" to "false"; that's more typical
ditto below

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:197:
EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB, match.type);
would it be easy (and correct) to make a copy of metadata_match around line 186
and simply check that this match is the same as that one?  (Hopefully
AutocompleteMatch equality is defined.)

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:240:
EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW, match.type);
Analogously, if AutocompletMatch equality is defined, perhaps you can declare an
overflow match object and check it for equality with the current match both here
and in 269.
If this turns out complicated, then skip it.  This is just a nicety in a test.

[mattreynolds, 2016-09-01 01:24:11]

Thanks Mark!

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/physical_web_provider_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:124: std::string(),
GURL(), metrics::OmniboxEventProto::OTHER,
On 2016/08/31 22:39:33, Mark P wrote:
> better: INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS

Done.

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:125: true, false,
true, true, true, TestSchemeClassifier());
On 2016/08/31 22:39:33, Mark P wrote:
> nit: change the first "true" to "false"; that's more typical
> ditto below

Done.

https://codereview.chromium.org/2266083002/diff/60001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider_unittest.cc:197:
EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB, match.type);
On 2016/08/31 22:39:33, Mark P wrote:
> would it be easy (and correct) to make a copy of metadata_match around line
186
> and simply check that this match is the same as that one?  (Hopefully
> AutocompleteMatch equality is defined.)
> 

Unfortunately equality is not implemented for AutocompleteMatch. I think it
would be nice to factor out the comparisons for an individual match so it's
clearer that we're checking the same things in both cases. This test will need
to be updated when we change the strings for the match item, so I'll do the
refactoring then.

[Mark P, 2016-09-01 04:21:06]

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

https://codereview.chromium.org/2266083002/diff/80001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:71: // If the omnibox is not
empty, add a default verbatim match.
On 2016/08/31 22:39:33, Mark P wrote:
> Perhaps revise the first sentence to something like
> // If we're going to show matches and the omnibox is non-empty, add the
default
> current as the default match.
> (and maybe drop the second sentence)

Did you miss this comment?

[mattreynolds, 2016-09-01 17:09:22]

Thanks Mark!

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

https://codereview.chromium.org/2266083002/diff/80001/components/omnibox/brow...
components/omnibox/browser/physical_web_provider.cc:71: // If the omnibox is not
empty, add a default verbatim match.
On 2016/09/01 04:21:06, Mark P wrote:
> On 2016/08/31 22:39:33, Mark P wrote:
> > Perhaps revise the first sentence to something like
> > // If we're going to show matches and the omnibox is non-empty, add the
> default
> > current as the default match.
> > (and maybe drop the second sentence)
> 
> Did you miss this comment?

Oops, I did.

[mattreynolds, 2016-09-01 17:09:29]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-09-01 17:09:30]

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/2266083002/#ps100001
(title: "revise comment")

[commit-bot: I haz the power, 2016-09-01 17:09:47]

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-09-01 17:49:15]

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

[commit-bot: I haz the power, 2016-09-01 17:49:16]

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

[mattreynolds, 2016-09-01 21:34:14]

mattreynolds@chromium.org changed reviewers:
+ jochen@chromium.org

[mattreynolds, 2016-09-01 21:34:15]

Hi Jochen, could you take a look at the change in
omnibox_view_views_browsertest.cc?

The test fails with this CL because we are enforcing stricter standards on when
default matches should be present in the autocomplete match list. Previously, we
required a default match to always be present. Now we will require that a
default match is NOT present when the user is on a page that doesn't auto-fill
the current page URL in the omnibox input field. Currently this only occurs for
a handful of chrome:// pages, including NTP.

In the context of unit tests, it means that omnibox tests should be rigorous
about ensuring the AutocompleteInput's text field is empty if and only if none
of the AutocompleteMatch items have the allowed_to_be_default_match flag set.
This change replaces the default-constructed AutocompleteInput() currently used
in the test with an equivalent AutocompleteInput that sets text to a non-empty
value.

[Mark P, 2016-09-01 21:37:19]

Description was changed from

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=
==========

to

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=
==========

[Mark P, 2016-09-01 21:37:19]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org
- jochen@chromium.org

[Mark P, 2016-09-01 21:38:14]

[jochen to CC]

pkasting, can you please rubberstamp this changelist?  I've reviewed it all, but
apparently I'm not an OWNER of
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc

thanks,
mark

[Peter Kasting, 2016-09-01 22:06:36]

LGTM with nits

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:321: const
AutocompleteInput& input(base::ASCIIToUTF16("a"), base::string16::npos,
Nit: Omit &

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:322:
std::string(), GURL(), metrics::OmniboxEventProto::INVALID_SPEC,
Nit: All lines of args must be aligned (try running git cl format)

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_result.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/autocomplete_result.cc:176: if (input.text().empty())
{
Nit: Less nesting and code duplication.  (I rewrote the comments a bit as well,
especially to provide more detail on when we get into different scenarios and
why we do or don't want default matches.)

    // We should only get here with an empty omnibox for automatic suggestions
    // on focus on the NTP; in these cases hitting enter should do nothing, so
    // there should be no default match.  Otherwise, we're doing automatic
    // suggestions for the currently visible URL (and hitting enter should
    // reload it), or the user is typing; in either of these cases, there should
    // be a default match.
    DCHECK_NE(input.text().empty(), default_match_->allowed_to_be_default_match)
        << debug_info;

    // For navigable default matches, make sure the destination type is what the
    // user would expect given the input.
    if (default_match_->allowed_to_be_default_match &&
        default_match_->destination_url.is_valid()) {
      ...

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/autocomplete_result_unittest.cc:615:
TestSchemeClassifier());
Nit: Consider pulling this temp out to where |data| is defined and using it in
both blocks.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:72: // current URL as the
default match.
Nit: Explain why

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:76:
matches_.push_back(verbatim_match);
Nit: Just inline:

    matches_.push_back(VerbatimMatchForURL(client_, input, input.current_url(),
                                           history_url_provider_, -1));

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.h (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.h:46: // Used for efficiency
when creating the verbatim match.  Can be NULL.
Nit: NULL -> null

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.h:48: };
Nit: DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:94: provider_ =
PhysicalWebProvider::Create(client_.get(), NULL);
Nit: nullptr

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:124: std::string(),
GURL(),
Nit: All lines of args must be aligned (2 places)

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:196: const
AutocompleteMatch& match = provider_->matches()[i];
Nit: Use range-based for:

  for (const auto& match : provider_->matches()) {
    ...

(3 places)

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:198:
EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB, match.type);
Nit: This EXPECT_EQ can never fail because you just checked this in the
conditional.  Remove.  (3 places)

[mattreynolds, 2016-09-02 00:29:08]

Thanks Peter!

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:321: const
AutocompleteInput& input(base::ASCIIToUTF16("a"), base::string16::npos,
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: Omit &

Done.

https://codereview.chromium.org/2266083002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:322:
std::string(), GURL(), metrics::OmniboxEventProto::INVALID_SPEC,
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: All lines of args must be aligned (try running git cl format)

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_result.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/autocomplete_result.cc:176: if (input.text().empty())
{
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: Less nesting and code duplication.  (I rewrote the comments a bit as
well,
> especially to provide more detail on when we get into different scenarios and
> why we do or don't want default matches.)
> 
>     // We should only get here with an empty omnibox for automatic suggestions
>     // on focus on the NTP; in these cases hitting enter should do nothing, so
>     // there should be no default match.  Otherwise, we're doing automatic
>     // suggestions for the currently visible URL (and hitting enter should
>     // reload it), or the user is typing; in either of these cases, there
should
>     // be a default match.
>     DCHECK_NE(input.text().empty(),
default_match_->allowed_to_be_default_match)
>         << debug_info;
> 
>     // For navigable default matches, make sure the destination type is what
the
>     // user would expect given the input.
>     if (default_match_->allowed_to_be_default_match &&
>         default_match_->destination_url.is_valid()) {
>       ...

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/autocomplete_result_unittest.cc:615:
TestSchemeClassifier());
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: Consider pulling this temp out to where |data| is defined and using it in
> both blocks.

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:72: // current URL as the
default match.
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: Explain why

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:76:
matches_.push_back(verbatim_match);
On 2016/09/01 22:06:35, Peter Kasting wrote:
> Nit: Just inline:
> 
>     matches_.push_back(VerbatimMatchForURL(client_, input,
input.current_url(),
>                                            history_url_provider_, -1));

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.h (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.h:46: // Used for efficiency
when creating the verbatim match.  Can be NULL.
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: NULL -> null

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.h:48: };
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider_unittest.cc (right):

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:94: provider_ =
PhysicalWebProvider::Create(client_.get(), NULL);
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:124: std::string(),
GURL(),
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: All lines of args must be aligned (2 places)

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:196: const
AutocompleteMatch& match = provider_->matches()[i];
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: Use range-based for:
> 
>   for (const auto& match : provider_->matches()) {
>     ...
> 
> (3 places)

Done.

https://codereview.chromium.org/2266083002/diff/120001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider_unittest.cc:198:
EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB, match.type);
On 2016/09/01 22:06:36, Peter Kasting wrote:
> Nit: This EXPECT_EQ can never fail because you just checked this in the
> conditional.  Remove.  (3 places)

Removed.

[mattreynolds, 2016-09-02 17:10:21]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-09-02 17:10:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2266083002/#ps140001
(title: "changes for pkasting@")

[commit-bot: I haz the power, 2016-09-02 17:11:02]

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-09-02 18:20:49]

Description was changed from

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=
==========

to

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=
==========

[commit-bot: I haz the power, 2016-09-02 18:20:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-02 18:27:41]

Description was changed from

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=
==========

to

==========
Avoid DCHECK failure for chrome:// URLs in autocomplete suggestions

If the URL in the omnibox input field was not entered by the user, then
avoid checking that the scheme of the default autocomplete suggestion
is equivalent to the scheme of the input URL.

This check causes a failure in the typical case for the Physical Web
provider, which only offers suggestions when the user has not entered
text in the omnibox and can suggest chrome://physical-web as the default
suggestion.

BUG=

Committed: https://crrev.com/2b530deb82adb676d49e39e4ff7b24e206bbbf05
Cr-Commit-Position: refs/heads/master@{#416302}
==========

[commit-bot: I haz the power, 2016-09-02 18:27:42]

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