[Mac] Cmd-Return in the omnibox should revert the omnibox text back to its or...

[Mac] Cmd-Return in the omnibox should revert the omnibox text back to its original state.

BUG=23514
TEST=Type a URL into the omnibox, then Cmd-Return.  The omnibox text should revert to its original state.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29743

[rohitrao (ping after 24h), 2009-10-05 02:22:23]

An alternative would be to add a reset_on_accept_ member to
AutocompleteEditViewMac and explicitly set it to false in the middle-click case,
but I don't know if that's really any better than what I have here...

[Scott Hess - ex-Googler, 2009-10-06 18:25:29]

http://codereview.chromium.org/260008/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/260008/diff/1/2#newcode812
Line 812: edit_view_->RevertAll();
Are you refering to how the GTK code makes the check and revert in OpenURL()?

It sounds like the odd man out is our wiring of
AutocompletePopupViewMac::AcceptInput() to use edit_view_ AcceptInput().  Maybe
the right thing to do would be to modify OpenURL() and also modify our popup
code to do more like others?

[rohitrao (ping after 24h), 2009-10-07 13:43:47]

> It sounds like the odd man out is our wiring of
> AutocompletePopupViewMac::AcceptInput() to use edit_view_ AcceptInput(). 
Maybe
> the right thing to do would be to modify OpenURL() and also modify our popup
> code to do more like others?

Let me play around with this a bit.  This can and maybe should be changed, but I
think the odder man out is that Cmd+Return doesn't exist on the other platforms.
 I think even if we make this change, we may still have to change OpenURL() or
trigger the RevertAll() in another way.

[Scott Hess - ex-Googler, 2009-10-08 17:58:23]

On 2009/10/07 13:43:47, rohitrao wrote:
> > It sounds like the odd man out is our wiring of
> > AutocompletePopupViewMac::AcceptInput() to use edit_view_ AcceptInput(). 
> Maybe
> > the right thing to do would be to modify OpenURL() and also modify our popup
> > code to do more like others?
> 
> Let me play around with this a bit.  This can and maybe should be changed, but
I
> think the odder man out is that Cmd+Return doesn't exist on the other
platforms.
>  I think even if we make this change, we may still have to change OpenURL() or
> trigger the RevertAll() in another way.

Should probably prefer like-other-platforms as possible, and Cmd+Return being
the special-to-Mac version.  Then if anyone gets a bug in their bonnet about
refactoring, they aren't going to be as lost.

[rohitrao (ping after 24h), 2009-10-16 18:49:26]

> Should probably prefer like-other-platforms as possible, and Cmd+Return being
> the special-to-Mac version.  Then if anyone gets a bug in their bonnet about
> refactoring, they aren't going to be as lost.

Ok, I *think* I understand what's going on here now.  Let me know if any of this
sounds wrong:

There are three main methods by which we can open URLs in the omnibox.

1) Urls that are typed into the text field -- we call AcceptInput() on the
model, which does some bookkeeping and eventually opens the url.
2) Popup entries selected using the up/down arrow keys -- we also call
AcceptInput() when the user presses enter, because this is no different from #1.
3) Popup entries selected using the mouse -- see below.

Right now for #3, we treat left-click and middle-clicks differently.  For
middle-clicks, we pick out the hovered row and call OpenURL directly, because
the model has no idea which entry is currently highlighted.  For left-clicks, we
end up telling the model that the highlighted row was selected (same as if we
had selected it with the arrow keys), and then we call AcceptInput() to act upon
this newly-selected row.

GTK/Windows call OpenURL() directly, without going through the formality of
selecting a row in the model.  Sounds like you want us to do the same, which
makes sense.

[rohitrao (ping after 24h), 2009-10-16 19:29:22]

Assuming my previous comments were correct, this is the resulting code.  The
popup changes are standalone and unrelated to the bug, so I can move them out
into a separate CL if you'd like.

[Scott Hess - ex-Googler, 2009-10-21 20:58:16]

I think this is clearer than before.  LGTM, but check my comment around -1.

http://codereview.chromium.org/260008/diff/7001/8002
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/260008/diff/7001/8002#newcode473
Line 473: OpenURLForRow([[popup_ contentView] selectedRow], false);
I think one reason for the previous code's SetSelectedLine was to visually
select that line as distinct from hover.  But now I see that that isn't working
right anyhow, on click the selected line goes away and you're left with ONLY the
hover.

So mousing over with not clicking should show a hover line distinct from the
selected line.  But clicking without releasing should let you move around the
selected line.  If you release while on a selected line, you go there.  If you
drag off the popup entirely and release, you go back to what was previously
live.

IOW, I think your change has no practical downside on this front :-).

http://codereview.chromium.org/260008/diff/7001/8002#newcode481
Line 481: if (row == -1) {
Make certain that you are handling the case mentioned in the previous -1 check's
comment.  Clicking and dragging off the popup should leave you in some sensible
visual state.

[rohitrao (ping after 24h), 2009-10-21 21:13:14]

Hm, now when I click and drag off, I lose the selected row blue highlight
entirely.  Investigating...

On 2009/10/21 20:58:16, shess wrote:
> I think this is clearer than before.  LGTM, but check my comment around -1.
> 
> http://codereview.chromium.org/260008/diff/7001/8002
> File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):
> 
> http://codereview.chromium.org/260008/diff/7001/8002#newcode473
> Line 473: OpenURLForRow([[popup_ contentView] selectedRow], false);
> I think one reason for the previous code's SetSelectedLine was to visually
> select that line as distinct from hover.  But now I see that that isn't
working
> right anyhow, on click the selected line goes away and you're left with ONLY
the
> hover.
> 
> So mousing over with not clicking should show a hover line distinct from the
> selected line.  But clicking without releasing should let you move around the
> selected line.  If you release while on a selected line, you go there.  If you
> drag off the popup entirely and release, you go back to what was previously
> live.
> 
> IOW, I think your change has no practical downside on this front :-).
> 
> http://codereview.chromium.org/260008/diff/7001/8002#newcode481
> Line 481: if (row == -1) {
> Make certain that you are handling the case mentioned in the previous -1
check's
> comment.  Clicking and dragging off the popup should leave you in some
sensible
> visual state.

[rohitrao (ping after 24h), 2009-10-21 21:22:27]

On 2009/10/21 21:13:14, rohitrao wrote:
> Hm, now when I click and drag off, I lose the selected row blue highlight
> entirely.  Investigating...

Ah, I accidentally took out an important line (reset the selection if
selectedRow == -1).  New diffs up, and this now works as you said it should.

[Scott Hess - ex-Googler, 2009-10-21 23:13:18]

lgtm with nits.

http://codereview.chromium.org/260008/diff/12004/12007
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/260008/diff/12004/12007#newcode112
Line 112: // Opens the URL corresponding to the given |row|.
Document force_background, especially the part about otherwise using
currentEvent to determine where to open.

http://codereview.chromium.org/260008/diff/12004/12006
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/260008/diff/12004/12006#newcode458
Line 458: int selectedRow = [[popup_ contentView] selectedRow];
Suggest bringing this line all the way back to the original form, now.

[rohitrao (ping after 24h), 2009-10-22 00:33:03]

http://codereview.chromium.org/260008/diff/12004/12007
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/260008/diff/12004/12007#newcode112
Line 112: // Opens the URL corresponding to the given |row|.
On 2009/10/21 23:13:18, shess wrote:
> Document force_background, especially the part about otherwise using
> currentEvent to determine where to open.

Done.

http://codereview.chromium.org/260008/diff/12004/12006
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/260008/diff/12004/12006#newcode458
Line 458: int selectedRow = [[popup_ contentView] selectedRow];
On 2009/10/21 23:13:18, shess wrote:
> Suggest bringing this line all the way back to the original form, now.

Done.

[Scott Hess - ex-Googler, 2009-10-22 04:30:36]

Just in case, LGTM.

Am happy with your changes, they aren't huge but the previous code was seeming a
bit cargo-cult, this feels more like we know what's going on.