Mac: Omnibox: Retain the "SelectAll" state after a navigation.

Mac: Omnibox: Retain the "SelectAll" state after a navigation.

When a navigation occurs and the omnibox is focused and fully selected,
the replacement URL should also be fully selected. OmnboxViewMac
currently does not do this.

Use the same logic from void OmniboxViewViews::Update().

This makes Mac consistent with other platforms. Adds a cross-platform
test to verify.

BUG=471635
Committed: https://crrev.com/59495481b1ff51a626d93bc8f4895cd8c675c893
Cr-Commit-Position: refs/heads/master@{#330683}

[tapted, 2015-05-19 06:05:27]

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

[tapted, 2015-05-19 06:05:28]

Hi Peter, please take a look.

[Peter Kasting, 2015-05-19 07:04:01]

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:228: // we're getting the
selection and popup right.
Nit: Remove TODO; copy comment from views code

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:230: SelectAll(false /*
reversed: ignored on mac */);
Reversed state shouldn't be ignored.  You should save and restore it correctly.

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:352: // even know if that
makes sense for Mac.
Surely Mac can distinguish between a left-to-right and a right-to-left
selection?

[tapted, 2015-05-19 10:56:19]

Patchset #3 (id:40001) has been deleted

[tapted, 2015-05-19 11:28:39]

Thanks Peter! PTAL

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:228: // we're getting the
selection and popup right.
On 2015/05/19 07:04:01, Peter Kasting wrote:
> Nit: Remove TODO; copy comment from views code

Done.

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:230: SelectAll(false /*
reversed: ignored on mac */);
On 2015/05/19 07:04:00, Peter Kasting wrote:
> Reversed state shouldn't be ignored.  You should save and restore it
correctly.

Done.

https://codereview.chromium.org/1144853003/diff/1/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:352: // even know if that
makes sense for Mac.
On 2015/05/19 07:04:00, Peter Kasting wrote:
> Surely Mac can distinguish between a left-to-right and a right-to-left
> selection?

Done+tested.

[Peter Kasting, 2015-05-19 21:32:55]

LGTM

Interesting that Mac tracks selection and affinity separately.  This makes me
worry about all the code that uses SetSelectedRange() (or equivalent) -- I
wonder if everything that gets/sets selection ranges, and isn't also preserving
affinity, can get this wrong.

Also, does Mac not autoscroll the end of the selection into view?  The reason
the direction is important is because the code assumes that whatever the "end"
of the selection is -- the back normally, the front if reversed -- will be
scrolled into view.  But some testing today shows that maybe Mac doesn't do
this?  Does it just never scroll at all on programmatically setting a selection?
 If so, I wonder if various other bits of omnibox code are going to be buggy...

https://codereview.chromium.org/1144853003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1144853003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:361: if
(!model()->has_focus())
This line isn't in views -- are you sure it's correct?  I'm wondering when it
will actually trigger.

[tapted, 2015-05-20 00:52:22]

On 2015/05/19 21:32:55, Peter Kasting wrote:
> Interesting that Mac tracks selection and affinity separately.  This makes me
> worry about all the code that uses SetSelectedRange() (or equivalent) -- I
> wonder if everything that gets/sets selection ranges, and isn't also
preserving
> affinity, can get this wrong.

Yeah.. NSRange is offset+length and uses unsigned ints for both, so no easy way
to merge them. I looked at the calls in OmniboxViewMac that do SetSelectedRange
and it seems internally consistent at least.

> Also, does Mac not autoscroll the end of the selection into view?  The reason
> the direction is important is because the code assumes that whatever the "end"
> of the selection is -- the back normally, the front if reversed -- will be
> scrolled into view.  But some testing today shows that maybe Mac doesn't do
> this?  Does it just never scroll at all on programmatically setting a
selection?

That's correct. E.g. pressing Escape will call SelectAll(true). This CL will
make it reliably move the insertion point to the start of the textbox, but it
still won't make it scroll :/. I haven't found a nice way to do that apart from
(effectively) simulating a "Home" keypress on the textfield before setting the
selection. But even selecting manually with (e.g.) Shift+End won't scroll the
text to the end on Mac, so this may be "expected" Cocoa behavior (or possibly
just an oversight/bug in Cocoa itself).

>  If so, I wonder if various other bits of omnibox code are going to be
buggy...

ToolkitViews is getting pretty usable on Mac.... there's a long-tail of broken
tests and lots of look+feel to resolve. But mac_views_browser=1 in GYP_DEFINES
has been building a usable and reasonably robust Chrome (minus all the Cocoa UI)
for a while.

https://codereview.chromium.org/1144853003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1144853003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:361: if
(!model()->has_focus())
On 2015/05/19 21:32:55, Peter Kasting wrote:
> This line isn't in views -- are you sure it's correct?  I'm wondering when it
> will actually trigger.

Yeah - this was to keep consistent with the behavior of the SetSelectedRange
call that was here previously. There's a few comments about focus stealing in
here (e.g.  OmniboxViewMac::OnTabChanged), so I thought it best to keep it. (If
it were false, text_view below would be `nil` and the last line would just no-op
in Obj-C's annoying way, but that's a bit too subtle).

I think the difference on Mac is that (for things that aren't WebContents, which
don't use Cocoa's FieldEditor), it's not possible to have text selected without
focus. i.e. selection can only be shown in a FieldEditor, and there is only one
field editor per window (which moves around with focus). Whereas on other
platforms (and inside WebContents), selection is done by RenderText.

I checked all the callers of SelectAll -- they're preceeded by a SetFocus call,
verify that |user_input_in_progress_|, or are
`OmniboxEditModel::OnEscapeKeyPressed()`, so I think focus is generally ensured
here, but on Mac any window control has an opportunity to "refuse" a focus
change away from it, so I'm not sure it can be guaranteed.

[tapted, 2015-05-20 02:34:15]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-20 02:34:42]

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

[commit-bot: I haz the power, 2015-05-20 02:38:25]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-20 02:39:12]

Patchset 3 (id:??) landed as
https://crrev.com/59495481b1ff51a626d93bc8f4895cd8c675c893
Cr-Commit-Position: refs/heads/master@{#330683}