Make GetInfoForCurrentText() return the correct match when we've selected an

Make GetInfoForCurrentText() return the correct match when we've selected an
associated keyword in the dropdown.

I can't think of any concrete bugs this would have caused offhand, but there
might be some, and it's certainly more correct.

BUG=none
TEST=none

Committed: https://crrev.com/19b75b7e5ac685cb3e4006e6ff38531abfc2a8b6
Cr-Commit-Position: refs/heads/master@{#326151}

[Peter Kasting, 2015-04-18 01:24:00]

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

[Mark P, 2015-04-21 18:23:42]

This sounds a bit related to the two bug reported on:
https://code.google.com/p/chromium/issues/detail?id=313643

Can you perhaps fix that?  I think if you fix those (which also involves
deciding how pressing enter immediately after entering keyword mode should
behave) then this fix would might be different or not even necessary.  Perhaps
the default match you get after you immediately enter keyword mode will be fine
to use--you won't need to fallback to the associated_keyword match.

--mark

[Peter Kasting, 2015-04-21 19:02:42]

On 2015/04/21 18:23:42, Mark P wrote:
> This sounds a bit related to the two bug reported on:
> https://code.google.com/p/chromium/issues/detail?id=313643
> 
> Can you perhaps fix that?  I think if you fix those (which also involves
> deciding how pressing enter immediately after entering keyword mode should
> behave) then this fix would might be different or not even necessary.

This fix is correct regardless of that bug, and shouldn't depend on fixing that.
 So please review this now.

> Perhaps
> the default match you get after you immediately enter keyword mode will be
fine
> to use--you won't need to fallback to the associated_keyword match.

It's not just about what the destination_url_ of the match in question is, it's
about the other state of the AutocompleteMatch, and so yes, I should be
returning the match we've actually selected.

[Mark P, 2015-04-21 19:10:09]

On 2015/04/21 19:02:42, Peter Kasting wrote:
> On 2015/04/21 18:23:42, Mark P wrote:
> > This sounds a bit related to the two bug reported on:
> > https://code.google.com/p/chromium/issues/detail?id=313643
> > 
> > Can you perhaps fix that?  I think if you fix those (which also involves
> > deciding how pressing enter immediately after entering keyword mode should
> > behave) then this fix would might be different or not even necessary.
> 
> This fix is correct regardless of that bug, and shouldn't depend on fixing
that.
>  So please review this now.

I'm afraid I don't understand this fix.  What's wrong with using the
selected_match even if the user just entered keyword mode?  Why does it matter
how the default match got there?

--mark


> 
> > Perhaps
> > the default match you get after you immediately enter keyword mode will be
> fine
> > to use--you won't need to fallback to the associated_keyword match.
> 
> It's not just about what the destination_url_ of the match in question is,
it's
> about the other state of the AutocompleteMatch, and so yes, I should be
> returning the match we've actually selected.

[Peter Kasting, 2015-04-21 19:12:33]

On 2015/04/21 19:10:09, Mark P wrote:
> On 2015/04/21 19:02:42, Peter Kasting wrote:
> > On 2015/04/21 18:23:42, Mark P wrote:
> > > This sounds a bit related to the two bug reported on:
> > > https://code.google.com/p/chromium/issues/detail?id=313643
> > > 
> > > Can you perhaps fix that?  I think if you fix those (which also involves
> > > deciding how pressing enter immediately after entering keyword mode should
> > > behave) then this fix would might be different or not even necessary.
> > 
> > This fix is correct regardless of that bug, and shouldn't depend on fixing
> that.
> >  So please review this now.
> 
> I'm afraid I don't understand this fix.  What's wrong with using the
> selected_match even if the user just entered keyword mode?  Why does it matter
> how the default match got there?

When you're in keyword mode the selected match is the associated keyword match. 
It isn't the "selected match".  The two have different contents, descriptions,
types, etc.  If someone wants to know what the currently-visible match is, they
need to look at the actual visible match.

We just never updated this code when we added the concept of associated keyword
matches.

[Mark P, 2015-04-21 20:26:27]

On 2015/04/21 19:12:33, Peter Kasting wrote:
> On 2015/04/21 19:10:09, Mark P wrote:
> > On 2015/04/21 19:02:42, Peter Kasting wrote:
> > > On 2015/04/21 18:23:42, Mark P wrote:
> > > > This sounds a bit related to the two bug reported on:
> > > > https://code.google.com/p/chromium/issues/detail?id=313643
> > > > 
> > > > Can you perhaps fix that?  I think if you fix those (which also involves
> > > > deciding how pressing enter immediately after entering keyword mode
should
> > > > behave) then this fix would might be different or not even necessary.
> > > 
> > > This fix is correct regardless of that bug, and shouldn't depend on fixing
> > that.
> > >  So please review this now.
> > 
> > I'm afraid I don't understand this fix.  What's wrong with using the
> > selected_match even if the user just entered keyword mode?  Why does it
matter
> > how the default match got there?
> 
> When you're in keyword mode the selected match is the associated keyword
match. 
> It isn't the "selected match".  The two have different contents, descriptions,
> types, etc.

Then what is the "selected match" at this stage?

Can you please update the comment in
chrome/browser/ui/omnibox/omnibox_popup_model.h
?
  // If the selected line has both a normal match and a keyword match, this
  // determines whether the normal match (if NORMAL) or the keyword match
  // (if KEYWORD) is selected.
I'm trying to understand what you're doing, but I don't understand how a
selected line can have both a normal match and a keyword match, and none of the
comments in that file about how selected lines work gives me enough of a clue.

--mark


> If someone wants to know what the currently-visible match is, they
> need to look at the actual visible match.
> 
> We just never updated this code when we added the concept of associated
keyword
> matches.

[Peter Kasting, 2015-04-21 20:53:04]

On 2015/04/21 20:26:27, Mark P wrote:
> On 2015/04/21 19:12:33, Peter Kasting wrote:
> > When you're in keyword mode the selected match is the associated keyword
> match. 
> > It isn't the "selected match".  The two have different contents,
descriptions,
> > types, etc.
> 
> Then what is the "selected match" at this stage?
> 
> Can you please update the comment in
> chrome/browser/ui/omnibox/omnibox_popup_model.h
> ?
>   // If the selected line has both a normal match and a keyword match, this
>   // determines whether the normal match (if NORMAL) or the keyword match
>   // (if KEYWORD) is selected.
> I'm trying to understand what you're doing, but I don't understand how a
> selected line can have both a normal match and a keyword match, and none of
the
> comments in that file about how selected lines work gives me enough of a clue.

The comment you refer to is correct.  The fix I'm making makes
GetInfoForCurrentText() comply with that comment.

All matches in the dropdown may have an associated keyword match.  If they do,
then on views (not implemented on Mac AFAIK), we'll draw a ">" icon at the right
edge of each line.  When on that line, tab/shift-tab toggle into and out of the
associated (keyword) match without disturbing the actual dropdown state or
rerunning autocomplete.  (Note that toggling into that match draws glitchily,
which is a different bug I'm also fixing.)

This means that every line of the dropdown may potentially have two different
matches on it, one of which is showing (and considered selected) at any time.

When the associated keyword match is the selected one, and code asks "what is
the selected match", we need to reply that the keyword match is selected.

[Mark P, 2015-04-21 21:24:33]

On Tue, Apr 21, 2015 at 1:53 PM, <pkasting@chromium.org> wrote:

> On 2015/04/21 20:26:27, Mark P wrote:
>
>> On 2015/04/21 19:12:33, Peter Kasting wrote:
>> > When you're in keyword mode the selected match is the associated keyword
>> match.
>> > It isn't the "selected match".  The two have different contents,
>>
> descriptions,
>
>> > types, etc.
>>
>
>  Then what is the "selected match" at this stage?
>>
>
>  Can you please update the comment in
>> chrome/browser/ui/omnibox/omnibox_popup_model.h
>> ?
>>    // If the selected line has both a normal match and a keyword match,
>> this
>>    // determines whether the normal match (if NORMAL) or the keyword match
>>    // (if KEYWORD) is selected.
>> I'm trying to understand what you're doing, but I don't understand how a
>> selected line can have both a normal match and a keyword match, and none
>> of
>>
> the
>
>> comments in that file about how selected lines work gives me enough of a
>> clue.
>>
>
> The comment you refer to is correct.  The fix I'm making makes
> GetInfoForCurrentText() comply with that comment.
>
> All matches in the dropdown may have an associated keyword match.  If they
> do,
> then on views (not implemented on Mac AFAIK), we'll draw a ">" icon at the
> right
> edge of each line.  When on that line, tab/shift-tab toggle into and out
> of the
> associated (keyword) match without disturbing the actual dropdown state or
> rerunning autocomplete.  (Note that toggling into that match draws
> glitchily,
> which is a different bug I'm also fixing.)
>
> This means that every line of the dropdown may potentially have two
> different
> matches on it, one of which is showing (and considered selected) at any
> time.
>

Thanks for the explanation.  I never knew about shift-tab, and I simply
always assumed that we re-ran the autocomplete suggestion system when
entering or exiting keyword mode.  e.g., tab doesn't simply change to
associated_keyword and vice versa, it sets prefer_keyword and re-ran
autocomplete.

Can you put part of your explanation in the comment I pointed to?  Here's a
possibility (changes from previous comment in bold).
If the selected line has both a normal match and an *associated_*keyword
match, this *indicates* whether the normal match (if NORMAL) or the keyword
match (if KEYWORD) is selected.*  This allows the autocomplete system to
support tab/shift-tab toggle into and out of the associated keyword match
state without disturbing the actual dropdown state or rerunning
autocomplete.*

--mark


> When the associated keyword match is the selected one, and code asks "what
> is
> the selected match", we need to reply that the keyword match is selected.
>
> https://codereview.chromium.org/1072163003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2015-04-21 21:29:22]

With your explanation, this code is clearly lgtm.  But please don't submit it
until you put in clarifying comments in
chrome/browser/ui/omnibox/omnibox_popup_model.h
as I requested.

--mark

[Peter Kasting, 2015-04-21 21:41:33]

On 2015/04/21 21:29:22, Mark P wrote:
> With your explanation, this code is clearly lgtm.  But please don't submit it
> until you put in clarifying comments in
> chrome/browser/ui/omnibox/omnibox_popup_model.h
> as I requested.

I added comments in a couple places I thought were appropriate.

[Peter Kasting, 2015-04-21 21:41:37]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2015-04-21 21:41:37]

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/1072163003/#ps20001
(title: "Add comments")

[commit-bot: I haz the power, 2015-04-21 21:42:19]

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

[Mark P, 2015-04-21 21:54:08]

https://codereview.chromium.org/1072163003/diff/20001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_popup_model.h (right):

https://codereview.chromium.org/1072163003/diff/20001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_popup_model.h:89: // If the selected line has
both a normal match and a keyword match, this can
FYI: I would love if this is said associated_keyword instead of keyword.  That
makes it clear the keyword match is actually part of the normal match
suggestion.

[Peter Kasting, 2015-04-21 21:58:00]

https://codereview.chromium.org/1072163003/diff/20001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_popup_model.h (right):

https://codereview.chromium.org/1072163003/diff/20001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_popup_model.h:89: // If the selected line has
both a normal match and a keyword match, this can
On 2015/04/21 21:54:08, Mark P wrote:
> FYI: I would love if this is said associated_keyword instead of keyword.  That
> makes it clear the keyword match is actually part of the normal match
> suggestion.

I think that's a slight layering violation, comment-wise -- it exposes the
implementation detail of how AutocompleteMatch tracks these two matches.  From
the perspective of the caller here, they don't care whether the matches are
independent or one is stored on the other.

[commit-bot: I haz the power, 2015-04-21 22:42:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-21 22:44:32]

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