removing ShouldHideTopMatch

Remove ShouldHideTopMatch

BUG=492452
Committed: https://crrev.com/1c28401c698ce3240dc83010b58021fd1ef5f3e6
Cr-Commit-Position: refs/heads/master@{#333772}

[dschuyler, 2015-05-27 00:51:35]

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

[dschuyler, 2015-05-27 19:02:48]

Hi Peter,  I'm making some guesses about what else to remove related to the
ShouldHideTopMatch removal.  Does this CL look like it's going in the right
direction?

[dschuyler, 2015-05-27 19:03:33]

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

[groby-ooo-7-16, 2015-05-27 19:25:49]

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

[groby-ooo-7-16, 2015-05-27 19:25:51]

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_controller.cc:64: if
((result.ShouldHideTopMatch() ||
I suppose the question here is, does the RHS of the || still make sense. (See
other comments on Chrome Instant)

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_controller.cc:56: const
AutocompleteResult::const_iterator default_match(
While we're here - is there any reason to copy-construct the iterator? It's not
like we're changing it?

https://codereview.chromium.org/1154063003/diff/80001/components/metrics/prot...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/80001/components/metrics/prot...
components/metrics/proto/omnibox_event.proto:44: optional bool
is_top_result_hidden_in_dropdown = 14;
mark with [deprecate = true];
see https://developers.google.com/protocol-buffers/docs/proto#options

Also, remember the matching file for UMA server.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.cc (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.cc:298: return
chrome::ShouldHideTopVerbatimMatch() &&
If ShouldHideTopMatch goes, ShouldHideTopVerbatimMatch can be killed as well.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
According to https://codereview.chromium.org/69703002, this is part of Chrome
Instant. Instant v1 and v2 are deprecated, but I don't know if that's true for
v3?

[groby-ooo-7-16, 2015-05-27 20:38:35]

@kmadhusu: Would you mind taking a quick look at this CL, to see if the removal
is OK?

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
On 2015/05/27 19:25:51, groby wrote:
> According to https://codereview.chromium.org/69703002, this is part of Chrome
> Instant. Instant v1 and v2 are deprecated, but I don't know if that's true for
> v3?

Just chatted with kmadhusu, the owner of the CL in question - instant V3 still
exists. I'll ping her so she can speak to if it's needed.

[dschuyler, 2015-05-27 20:39:56]

On 2015/05/27 20:38:35, groby wrote:
> @kmadhusu: Would you mind taking a quick look at this CL, to see if the
removal
> is OK?
> 
>
https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
> File components/omnibox/autocomplete_result.h (left):
> 
>
https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
> components/omnibox/autocomplete_result.h:131: bool
> TopMatchIsStandaloneVerbatimMatch() const;
> On 2015/05/27 19:25:51, groby wrote:
> > According to https://codereview.chromium.org/69703002, this is part of
Chrome
> > Instant. Instant v1 and v2 are deprecated, but I don't know if that's true
for
> > v3?
> 
> Just chatted with kmadhusu, the owner of the CL in question - instant V3 still
> exists. I'll ping her so she can speak to if it's needed.

Awesome, thanks!

[kmadhusu, 2015-05-28 01:55:08]

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

[kmadhusu, 2015-05-28 01:55:09]

Thanks for checking.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
On 2015/05/27 20:38:35, groby wrote:
> On 2015/05/27 19:25:51, groby wrote:
> > According to https://codereview.chromium.org/69703002, this is part of
Chrome
> > Instant. Instant v1 and v2 are deprecated, but I don't know if that's true
for
> > v3?
> 
> Just chatted with kmadhusu, the owner of the CL in question - instant V3 still
> exists. I'll ping her so she can speak to if it's needed.

You can remove this. This was a part of our previous experiment. We didn't
launch that.

https://codereview.chromium.org/1154063003/diff/100001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/100001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:43: // dropdown (which can
happen for non-default matches only if we're hiding a top
Please update this comment.

[dschuyler, 2015-05-28 18:58:06]

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/80001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_controller.cc:64: if
((result.ShouldHideTopMatch() ||
On 2015/05/27 19:25:51, groby wrote:
> I suppose the question here is, does the RHS of the || still make sense. (See
> other comments on Chrome Instant)

It sounds like it's not used in instant.

https://codereview.chromium.org/1154063003/diff/80001/components/metrics/prot...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/80001/components/metrics/prot...
components/metrics/proto/omnibox_event.proto:44: optional bool
is_top_result_hidden_in_dropdown = 14;
On 2015/05/27 19:25:51, groby wrote:
> mark with [deprecate = true];
> see https://developers.google.com/protocol-buffers/docs/proto#options
> 
> Also, remember the matching file for UMA server.

Done.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.cc (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.cc:298: return
chrome::ShouldHideTopVerbatimMatch() &&
On 2015/05/27 19:25:51, groby wrote:
> If ShouldHideTopMatch goes, ShouldHideTopVerbatimMatch can be killed as well. 

Thanks!

Done.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (left):

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
On 2015/05/28 01:55:09, kmadhusu wrote:
> On 2015/05/27 20:38:35, groby wrote:
> > On 2015/05/27 19:25:51, groby wrote:
> > > According to https://codereview.chromium.org/69703002, this is part of
> Chrome
> > > Instant. Instant v1 and v2 are deprecated, but I don't know if that's true
> for
> > > v3?
> > 
> > Just chatted with kmadhusu, the owner of the CL in question - instant V3
still
> > exists. I'll ping her so she can speak to if it's needed.
> 
> You can remove this. This was a part of our previous experiment. We didn't
> launch that. 
> 

Done.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
On 2015/05/27 19:25:51, groby wrote:
> According to https://codereview.chromium.org/69703002, this is part of Chrome
> Instant. Instant v1 and v2 are deprecated, but I don't know if that's true for
> v3?

Acknowledged.

https://codereview.chromium.org/1154063003/diff/80001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:131: bool
TopMatchIsStandaloneVerbatimMatch() const;
On 2015/05/27 20:38:35, groby wrote:
> On 2015/05/27 19:25:51, groby wrote:
> > According to https://codereview.chromium.org/69703002, this is part of
Chrome
> > Instant. Instant v1 and v2 are deprecated, but I don't know if that's true
for
> > v3?
> 
> Just chatted with kmadhusu, the owner of the CL in question - instant V3 still
> exists. I'll ping her so she can speak to if it's needed.

Acknowledged.

https://codereview.chromium.org/1154063003/diff/100001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/100001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:43: // dropdown (which can
happen for non-default matches only if we're hiding a top
On 2015/05/28 01:55:09, kmadhusu wrote:
> Please update this comment.

Done.

[Peter Kasting, 2015-05-28 23:06:24]

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/omnibox_metrics_provider.cc:147: const bool
consider_popup_open = log.is_popup_open && !log.is_paste_and_go;
Nit: Inline this into the next statement

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:54: result.default_match();
Nit: Not sure why the change of initializer format... but if you're mucking with
this line anyway I might change to use "const auto" here.

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

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:152: if
(model_->result().size() <= 0 ||
< 0?  How about: if (model_->result.empty() ||

https://codereview.chromium.org/1154063003/diff/120001/components/omnibox/aut...
File components/omnibox/autocomplete_result.cc (left):

https://codereview.chromium.org/1154063003/diff/120001/components/omnibox/aut...
components/omnibox/autocomplete_result.cc:303: if (empty() ||
!match_at(0).IsVerbatimType())
These are the only uses of IsVerbatimType(), so you can remove that function as
well.

[dschuyler, 2015-05-29 20:38:40]

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/omnibox_metrics_provider.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/omnibox_metrics_provider.cc:147: const bool
consider_popup_open = log.is_popup_open && !log.is_paste_and_go;
On 2015/05/28 23:06:24, Peter Kasting wrote:
> Nit: Inline this into the next statement

Done.

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:54: result.default_match();
On 2015/05/28 23:06:24, Peter Kasting wrote:
> Nit: Not sure why the change of initializer format... 
Groby requested (above in this CL) that it be changed.

> ...but if you're mucking with
> this line anyway I might change to use "const auto" here.

I went ahead and changed it.  I am wondering why auto would be preferred though.
 Can you tell me more about the <auto vs explicit type> preference or direct me
to something to read to help me understand this nit?

Done.

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

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:152: if
(model_->result().size() <= 0 ||
On 2015/05/28 23:06:24, Peter Kasting wrote:
> < 0?  How about: if (model_->result.empty() ||

Done.

https://codereview.chromium.org/1154063003/diff/120001/components/omnibox/aut...
File components/omnibox/autocomplete_result.cc (left):

https://codereview.chromium.org/1154063003/diff/120001/components/omnibox/aut...
components/omnibox/autocomplete_result.cc:303: if (empty() ||
!match_at(0).IsVerbatimType())
On 2015/05/28 23:06:24, Peter Kasting wrote:
> These are the only uses of IsVerbatimType(), so you can remove that function
as
> well.

Done.

[groby-ooo-7-16, 2015-05-29 20:46:05]

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:54: result.default_match();
On 2015/05/28 23:06:24, Peter Kasting wrote:
> Nit: Not sure why the change of initializer format... but if you're mucking
with
> this line anyway I might change to use "const auto" here.

Presumably because I asked why we use the copy-ctor here :)

What I meant by that is, why do we make a second instance at all? Can't we just
use result.default_match() *instead* of default_match()?

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:107: for (size_t ii =
0; ii < rows; ++ii) {
Personal nit: I think a range-based loop would be slightly clearer (plus, death
to ii! ;)
  NSInteger row = 0;
  for (auto const AutocompleteMatch& match : result) {
    OmniboxPopupCell* cell = [matrix_ cellAtRow:row++ column:0];
    [cell setImage:ImageForMatch(match)];
... and so on.

If you don't use that, please do replace GetResult() below with |result|

[Peter Kasting, 2015-05-29 21:10:12]

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:54: result.default_match();
On 2015/05/29 20:46:04, groby wrote:
> On 2015/05/28 23:06:24, Peter Kasting wrote:
> > Nit: Not sure why the change of initializer format... but if you're mucking
> with
> > this line anyway I might change to use "const auto" here.
> 
> Presumably because I asked why we use the copy-ctor here :)
> 
> What I meant by that is, why do we make a second instance at all? Can't we
just
> use result.default_match() *instead* of default_match()?

We totally can.  This was just intended to be a shorter alias.

I'm fine with removing this entirely and writing result.default_match()
explicitly below if it's not too cumbersome.

On 2015/05/29 20:38:39, dschuyler wrote:
> I am wondering why auto would be preferred though.
>  Can you tell me more about the <auto vs explicit type> preference or direct
me
> to something to read to help me understand this nit?

Check out the internal style guide's section on "auto" for more, but basically,
use auto when the real type name is verbose and doesn't really add clarity for
the reader (either the type is obvious or it's not important to know the precise
type).

[groby-ooo-7-16, 2015-05-29 21:11:22]

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/1154063003/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:54: result.default_match();
On 2015/05/29 20:38:39, dschuyler wrote:
> On 2015/05/28 23:06:24, Peter Kasting wrote:
> > Nit: Not sure why the change of initializer format... 
> Groby requested (above in this CL) that it be changed.
> 
> > ...but if you're mucking with
> > this line anyway I might change to use "const auto" here.
> 
> I went ahead and changed it.  I am wondering why auto would be preferred
though.
>  Can you tell me more about the <auto vs explicit type> preference or direct
me
> to something to read to help me understand this nit?
> 
> Done.

auto vs. explicit in a nutshell: Use auto if the specific type doesn't add
clarity. (It doesn't here - it's some kind of iterator, and we pass through it's
value. The specific kind is irrelevant).

In general, discussion of C++11 features in Chromium is at
https://chromium-cpp.appspot.com/
For all features, there's a link to a long discussion on chromium-dev why/how to
use this.

[Peter Kasting, 2015-05-29 23:44:39]

LGTM in any case

[Mark P, 2015-06-01 19:23:03]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
I don't understand why you're removing this block.

https://codereview.chromium.org/1154063003/diff/140001/components/metrics/pro...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/140001/components/metrics/pro...
components/metrics/proto/omnibox_event.proto:42: // DEPRECATED. Whether or not
the top match was hidden in the omnibox
This proto change looks good to me.  Please make this change internally first,
submit it, and then I'll approve this external changelist.

[Peter Kasting, 2015-06-01 20:18:02]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
On 2015/06/01 19:23:02, Mark P (out sick) wrote:
> I don't understand why you're removing this block.

kmadhusu said the TopMatchIsStandaloneVerbatimMatch() stuff should be removed.

[kmadhusu, 2015-06-01 20:24:32]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
On 2015/06/01 20:18:01, Peter Kasting wrote:
> On 2015/06/01 19:23:02, Mark P (out sick) wrote:
> > I don't understand why you're removing this block.
> 
> kmadhusu said the TopMatchIsStandaloneVerbatimMatch() stuff should be removed.

pkasting@, I am assuming default match is always the top match. Is my assumption
correct?

[Peter Kasting, 2015-06-01 20:26:58]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
On 2015/06/01 20:24:32, kmadhusu wrote:
> On 2015/06/01 20:18:01, Peter Kasting wrote:
> > On 2015/06/01 19:23:02, Mark P (out sick) wrote:
> > > I don't understand why you're removing this block.
> > 
> > kmadhusu said the TopMatchIsStandaloneVerbatimMatch() stuff should be
removed.
> 
> pkasting@, I am assuming default match is always the top match. Is my
assumption
> correct?

Default match is always top match.

What removing this block entirely will do is, previously if the default match
was the verbatim input and the next match was a prefetchable non-verbatim match,
this would prefetch it.  By removing it we'll no longer do that.  Are you sure
you want this gone?  You said it was part of an unlaunched experiment but I
didn't see anything about this that looked experiment-gated.

[kmadhusu, 2015-06-01 20:37:21]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
On 2015/06/01 20:26:57, Peter Kasting wrote:
> On 2015/06/01 20:24:32, kmadhusu wrote:
> > On 2015/06/01 20:18:01, Peter Kasting wrote:
> > > On 2015/06/01 19:23:02, Mark P (out sick) wrote:
> > > > I don't understand why you're removing this block.
> > > 
> > > kmadhusu said the TopMatchIsStandaloneVerbatimMatch() stuff should be
> removed.
> > 
> > pkasting@, I am assuming default match is always the top match. Is my
> assumption
> > correct?
> 
> Default match is always top match.
> 
> What removing this block entirely will do is, previously if the default match
> was the verbatim input and the next match was a prefetchable non-verbatim
match,
> this would prefetch it.  By removing it we'll no longer do that.  Are you sure
> you want this gone?  You said it was part of an unlaunched experiment but I
> didn't see anything about this that looked experiment-gated.

I see. Thanks for the details. I think we need this block.

Use case: 
(1) User typed "south" in the omnibox.
(2) Omnibox suggestions:
   south
   southwest
   www.southwest.com
   www.southwest.com/xyz
   www.southwest.com/abc

In this case, we want to check if "southwest" is marked for prefetching.

[dschuyler, 2015-06-05 20:33:54]

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/140001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:62: // Otherwise, if the top
match is a verbatim match and the very next match
On 2015/06/01 20:37:21, kmadhusu wrote:
> On 2015/06/01 20:26:57, Peter Kasting wrote:
> > On 2015/06/01 20:24:32, kmadhusu wrote:
> > > On 2015/06/01 20:18:01, Peter Kasting wrote:
> > > > On 2015/06/01 19:23:02, Mark P (out sick) wrote:
> > > > > I don't understand why you're removing this block.
> > > > 
> > > > kmadhusu said the TopMatchIsStandaloneVerbatimMatch() stuff should be
> > removed.
> > > 
> > > pkasting@, I am assuming default match is always the top match. Is my
> > assumption
> > > correct?
> > 
> > Default match is always top match.
> > 
> > What removing this block entirely will do is, previously if the default
match
> > was the verbatim input and the next match was a prefetchable non-verbatim
> match,
> > this would prefetch it.  By removing it we'll no longer do that.  Are you
sure
> > you want this gone?  You said it was part of an unlaunched experiment but I
> > didn't see anything about this that looked experiment-gated.
> 
> I see. Thanks for the details. I think we need this block.
> 
> Use case: 
> (1) User typed "south" in the omnibox.
> (2) Omnibox suggestions:
>    south
>    southwest
>    http://www.southwest.com
>    http://www.southwest.com/xyz
>    http://www.southwest.com/abc
> 
> In this case, we want to check if "southwest" is marked for prefetching. 
>     

Thanks for going over that!

Done.

[kmadhusu, 2015-06-08 18:45:16]

https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:44: // verbatim match) or the
second entry in the dropdown (which can happen for
Please add the following comment in your latest version: "or the second entry in
the dropdown (which can happen for non-default matches when a top verbatim match
is shown);"

[dschuyler, 2015-06-08 18:57:37]

https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_controller.cc:44: // verbatim match) or the
second entry in the dropdown (which can happen for
On 2015/06/08 18:45:16, kmadhusu wrote:
> Please add the following comment in your latest version: "or the second entry
in
> the dropdown (which can happen for non-default matches when a top verbatim
match
> is shown);"

Done.

[kmadhusu, 2015-06-09 02:09:40]

On 2015/06/08 18:57:37, dschuyler wrote:
>
https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
> File chrome/browser/ui/omnibox/omnibox_controller.cc (left):
> 
>
https://codereview.chromium.org/1154063003/diff/220001/chrome/browser/ui/omni...
> chrome/browser/ui/omnibox/omnibox_controller.cc:44: // verbatim match) or the
> second entry in the dropdown (which can happen for
> On 2015/06/08 18:45:16, kmadhusu wrote:
> > Please add the following comment in your latest version: "or the second
entry
> in
> > the dropdown (which can happen for non-default matches when a top verbatim
> match
> > is shown);"
> 
> Done.

Thanks. LGTM.

[groby-ooo-7-16, 2015-06-09 23:01:12]

c/b/ui/cocoa LGTM

[dschuyler, 2015-06-09 23:01:45]

https://codereview.chromium.org/1154063003/diff/140001/components/metrics/pro...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/140001/components/metrics/pro...
components/metrics/proto/omnibox_event.proto:42: // DEPRECATED. Whether or not
the top match was hidden in the omnibox
On 2015/06/01 19:23:03, Mark P wrote:
> This proto change looks good to me.  Please make this change internally first,
> submit it, and then I'll approve this external changelist.

I'm hoping I did the right internal change here:
https://critique.corp.google.com/#review/94695891

Done.

[Mark P, 2015-06-10 04:26:19]

https://codereview.chromium.org/1154063003/diff/240001/components/metrics/pro...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/240001/components/metrics/pro...
components/metrics/proto/omnibox_event.proto:44: optional bool
is_top_result_hidden_in_dropdown = 14 [deprecated = true];
Given that changed the name of the field internally, you should change the name
of the field here too.  We want the internal and external files to remain in
sync.

[dschuyler, 2015-06-10 18:29:34]

https://codereview.chromium.org/1154063003/diff/240001/components/metrics/pro...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/1154063003/diff/240001/components/metrics/pro...
components/metrics/proto/omnibox_event.proto:44: optional bool
is_top_result_hidden_in_dropdown = 14 [deprecated = true];
On 2015/06/10 04:26:18, Mark P wrote:
> Given that changed the name of the field internally, you should change the
name
> of the field here too.  We want the internal and external files to remain in
> sync.

Done.

[Mark P, 2015-06-10 18:36:13]

lgtm

[dschuyler, 2015-06-10 18:36:53]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-06-10 18:36:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, groby@chromium.org, kmadhusu@chromium.org
Link to the patchset: https://codereview.chromium.org/1154063003/#ps260001
(title: "changed deprecated var name")

[commit-bot: I haz the power, 2015-06-10 18:37:44]

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

[commit-bot: I haz the power, 2015-06-10 19:22:33]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-06-10 19:23:22]

Patchset 14 (id:??) landed as
https://crrev.com/1c28401c698ce3240dc83010b58021fd1ef5f3e6
Cr-Commit-Position: refs/heads/master@{#333772}