InstantExtended(gtk): Hide top match if told to so.

InstantExtended(gtk): Hide top match if told to so.

CL 17114003 and 22410004 added a way for Mac and views to hide the top
omnibox match when it is a verbatim match we do not want to show. This
implements that for GTK too so it'll work on all desktop platforms.

BUG=252823
TEST=Manually. Run with no flags and verify that when you type 'faceb'
in the omnibox, the popup shows a search suggestion for 'faceb'. Run
again with the flags
--enable-instant-extended-api
--force-fieldtrials='InstantExtended/Group1 hide_verbatim:1/'
and verify that you do not see this suggestion. Also check that with the
flags set, hovering on the top suggestion works and that there are no
off-by-one problems when clicking on suggestions.

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

[Jered, 2013-08-08 13:45:59]

Please review.

[Evan Stade, 2013-08-12 15:47:36]

seems like this would be a lot more robust and cross-platform if you made it
part of the model, i.e. add a function call which returns only the visible
results.

[Jered, 2013-08-12 16:24:13]

On 2013/08/12 15:47:36, Evan Stade wrote:
> seems like this would be a lot more robust and cross-platform if you made it
> part of the model, i.e. add a function call which returns only the visible
> results.

The view code has to know about the hidden match because we want to not select
the top match in the dropdown when there's a hidden top match.

[Evan Stade, 2013-08-12 16:33:33]

On 2013/08/12 16:24:13, Jered wrote:
> On 2013/08/12 15:47:36, Evan Stade wrote:
> > seems like this would be a lot more robust and cross-platform if you made it
> > part of the model, i.e. add a function call which returns only the visible
> > results.
> 
> The view code has to know about the hidden match because we want to not select
> the top match in the dropdown when there's a hidden top match.

I don't follow. Why does the view have to distinguish whether there are 10
matches, or 11 matches but the first one is hidden?

[Jered, 2013-08-12 18:01:09]

On 2013/08/12 16:33:33, Evan Stade wrote:
> On 2013/08/12 16:24:13, Jered wrote:
> > On 2013/08/12 15:47:36, Evan Stade wrote:
> > > seems like this would be a lot more robust and cross-platform if you made
it
> > > part of the model, i.e. add a function call which returns only the visible
> > > results.
> > 
> > The view code has to know about the hidden match because we want to not
select
> > the top match in the dropdown when there's a hidden top match.
> 
> I don't follow. Why does the view have to distinguish whether there are 10
> matches, or 11 matches but the first one is hidden?

When the first match is hidden, nothing is selected in the popup. For
example, the second match should not be selected, because pressing enter
will still pick the (hidden) first match.

[Evan Stade, 2013-08-12 18:15:49]

Which line is selected is tracked in the model. I still think you could drop all
changes to this view, and just change the model (selected_line, SetSelectedLine,
etc. would have to take hidden matches into account)

[Jered, 2013-08-14 16:47:07]

On 2013/08/12 18:15:49, Evan Stade wrote:
> Which line is selected is tracked in the model. I still think you could drop
all
> changes to this view, and just change the model (selected_line,
SetSelectedLine,
> etc. would have to take hidden matches into account)

I spent about an hour looking at this idea, but it just seems way more
complex to do this by changing the models around. +Peter for his opinion
here since he knows all this code really well.

The biggest issue is what to do in AutocompleteResult.

AutocompleteResult is used both in the views and in the edit model. It
keeps a default_match_ iterator pointing into matches_, so the hidden
match must be in matches_. So I think we'd need to add separate accessors
to AutocompleteResult for iterating over visible matches, and change all
the views to use those, at which point we're changing the views already.

But then views can call into the edit model with an index, so the edit
model would have to take into account that this index is actually an index
into visible matches and adjust it, which seems error prone.

The code for handling "up" on match 0 would have to be explicitly changed
to select the invisible top match etc. whereas it just works now...

[Jered, 2013-08-14 16:47:58]

Hey Peter can you chime in on the discussion about whether and how to hide the
top match in the omnibox models instead of views?

[Peter Kasting, 2013-08-14 17:59:44]

I could see this being implemented both ways.

To me it makes sense to implement this in the view side because in a semantic
sense, the first match is actually selected.  From the point of view of the code
that needs to understand how the default match is chosen, how selection behaves,
and what happens when you hit enter, nothing changes from before this patch. 
There's just a presentational tweak, which is reasonable to do view-side.

I don't think it would necessarily be wrong to implement this in the model side,
but I'd be surprised if it could be done anywhere near as cheaply or easily. 
And, especially if we have a more complex model-side implementation, I don't
necessarily think it buys us much in the way of robustness.  It might be nice to
do it in one spot instead of in three platform-specific spots -- but even that
advantage will be lessened when Linux switches to Aura.

In the end, I could probably be convinced by seeing a simple, clean model-side
implementation, but I probably wouldn't go out of my way to try and produce one.
 This may not placate Evan though :)

[Evan Stade, 2013-08-14 19:32:47]

I will defer to those who know the code better, but I strongly believe that

a) code should not be duplicated across views
b) views should be as dumb as possible, whereas models/controllers can be smart

In lieu of following these guidelines, I request unit tests.

https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:103: gfx::Rect
GetRectForLine(size_t line, int width) {
make this a member function so you can account for hidden matches within it.

[Jered, 2013-08-14 21:16:12]

On 2013/08/14 19:32:47, Evan Stade wrote:
> I will defer to those who know the code better, but I strongly believe that
> 
> a) code should not be duplicated across views
> b) views should be as dumb as possible, whereas models/controllers can be
smart
> 
> In lieu of following these guidelines, I request unit tests.
> 
>
https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
> File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):
> 
>
https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
> chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:103: gfx::Rect
> GetRectForLine(size_t line, int width) {
> make this a member function so you can account for hidden matches within it.

PTAL, added tests.

[Jered, 2013-08-14 21:16:21]

https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/1/chrome/browser/ui/gtk/omnibox...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:103: gfx::Rect
GetRectForLine(size_t line, int width) {
On 2013/08/14 19:32:48, Evan Stade wrote:
> make this a member function so you can account for hidden matches within it.

Done.

[Evan Stade, 2013-08-15 01:05:11]

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:558: const
AutocompleteResult& OmniboxPopupViewGtk::GetResult() const {
make the definition order match the declaration order

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.h (right):

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.h:111: virtual const
AutocompleteResult& GetResult() const;
should be protected rather than private

[Jered, 2013-08-15 16:08:49]

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:558: const
AutocompleteResult& OmniboxPopupViewGtk::GetResult() const {
On 2013/08/15 01:05:12, Evan Stade wrote:
> make the definition order match the declaration order

Done.

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.h (right):

https://codereview.chromium.org/22679003/diff/18001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.h:111: virtual const
AutocompleteResult& GetResult() const;
On 2013/08/15 01:05:12, Evan Stade wrote:
> should be protected rather than private

Done.

[Evan Stade, 2013-08-15 18:57:29]

https://codereview.chromium.org/22679003/diff/24001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/24001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:187:
signal_registrar_.reset();
This should not be necessary. I realize you're just moving this code, but it
still doesn't make sense. I've just created a patch to fix this:

https://codereview.chromium.org/23257002/

[Jered, 2013-08-19 22:58:15]

On 2013/08/15 18:57:29, Evan Stade wrote:
>
https://codereview.chromium.org/22679003/diff/24001/chrome/browser/ui/gtk/omn...
> File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):
> 
>
https://codereview.chromium.org/22679003/diff/24001/chrome/browser/ui/gtk/omn...
> chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:187:
> signal_registrar_.reset();
> This should not be necessary. I realize you're just moving this code, but it
> still doesn't make sense. I've just created a patch to fix this:
> 
> https://codereview.chromium.org/23257002/

Friendly ping

[Evan Stade, 2013-08-26 23:31:13]

I landed the other patch. Can you sync this one now? thanks, sorry for the
trouble.

[Jered, 2013-09-04 23:08:30]

Rebased. (Just returned from vacation...)

[Evan Stade, 2013-09-04 23:33:46]

lgtm

https://codereview.chromium.org/22679003/diff/33001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/33001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:247: void
OmniboxPopupViewGtk::InvalidateLine(size_t line) {
perhaps

DCHECK_LE(GetHiddenLineCount(), line);

[Jered, 2013-09-09 16:26:25]

https://codereview.chromium.org/22679003/diff/33001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right):

https://codereview.chromium.org/22679003/diff/33001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:247: void
OmniboxPopupViewGtk::InvalidateLine(size_t line) {
On 2013/09/04 23:33:47, Evan Stade wrote:
> perhaps
> 
> DCHECK_LE(GetHiddenLineCount(), line);

I added an if here to return early (this actually happens when arrowing up from
the hidden top match).

[commit-bot: I haz the power, 2013-09-09 16:27:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/22679003/35001

[commit-bot: I haz the power, 2013-09-09 16:42:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Jered, 2013-09-09 16:45:52]

+isherman -> metrics_log.cc

[Ilya Sherman, 2013-09-09 20:55:59]

metrics_log.cc lgtm

[commit-bot: I haz the power, 2013-09-09 20:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/22679003/35001

[commit-bot: I haz the power, 2013-09-09 22:18:04]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-09 22:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/22679003/54001

[commit-bot: I haz the power, 2013-09-10 02:16:18]

Change committed as 222186

[Mark P, 2013-09-14 21:58:44]

One concern about logging.

https://chromiumcodereview.appspot.com/22679003/diff/54001/chrome/browser/met...
File chrome/browser/metrics/metrics_log.cc (left):

https://chromiumcodereview.appspot.com/22679003/diff/54001/chrome/browser/met...
chrome/browser/metrics/metrics_log.cc:866: #if defined(OS_MACOSX) ||
defined(TOOLKIT_VIEWS)
Is this necessarily true?  We've implemented it for Mac, Views, and GTK.  Does
this mean now it works on Android and iOS?  I don't know how their omnibox is
rendered, but I thought, for instance, that Android has special views code, so
recording ShouldHideTopMatch on Android (when you're not actually hiding the top
match) will be misleading.