[omnibox] Constrain 'IsSelectAll()' to be more accurate

[omnibox] Constrain 'IsSelectAll()' to be more accurate

Currently, 'IsSelectAll()' returns true for a range of (0,0) but the
text is empty.  This isn't correct, and a range with identical
endpoints actually represents a cursor/caret position with nothing
selected.  Specifically, (0,0) is what the Omnibox code requests to
'home' the cursor.  Returning true is causing a problem for
'OmniboxViewViews::Update()' which then thinks it should select all of
the new text.

BUG=729415
Review-Url: https://codereview.chromium.org/2942013002
Cr-Commit-Position: refs/heads/master@{#480450}
Committed: https://chromium.googlesource.com/chromium/src/+/4033a5a4297ace7d7bf93cdc9b8ea35db4fa35da

[Kevin Bailey, 2017-06-15 14:32:33]

Description was changed from

==========
[omnibox] Constrain 'IsSelectAll()' to be more accurate

Currently, 'IsSelectAll()' returns true for a range of (0,0) but the
text is empty.  This isn't correct, and a range with identical
endpoints actually represents a cursor/caret position with nothing
selected.  Specifically, (0,0) is what the Omnibox code requests to
'home' the cursor.  Returning true is causing a problem for
'OmniboxViewViews::Update()' which then thinks it should select all of
the new text.

BUG=729415
==========

to

==========
[omnibox] Constrain 'IsSelectAll()' to be more accurate

Currently, 'IsSelectAll()' returns true for a range of (0,0) but the
text is empty.  This isn't correct, and a range with identical
endpoints actually represents a cursor/caret position with nothing
selected.  Specifically, (0,0) is what the Omnibox code requests to
'home' the cursor.  Returning true is causing a problem for
'OmniboxViewViews::Update()' which then thinks it should select all of
the new text.

BUG=729415
==========

[Kevin Bailey, 2017-06-15 14:32:33]

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

[Kevin Bailey, 2017-06-15 16:01:40]

Hi Peter,

I tracked this inadvertent select-all down to this questionable 'IsSelectAll()'.
During the course of resolving the URL and loading the page, there is an instant
(in 'OmniboxViewViews::Update()') where the cursor is homed and the text
actually empty, which was causing 'IsSelectAll()' to return true. Then later in 
'OmniboxViewViews::Update()', it would put in the eventual URL and select all of
it. Later, it gets corrected (since 'text()' is no longer empty) causing this
"flash".

Another way to fix it would be to have 'Update()' constrain 'was_select_all'
better, and leave 'IsSelectAll()' alone. I see that OmniboxViewTest::
BasicTextOperations (line 858) explicitly expects 'IsSelectAll()' to return true
in this case. So it would appear that 'IsSelectAll()' is "intentionally
inaccurate" (which is fine, as long as the issue is fixed.)

My only problem with this approach is that we'd be digging into the Textfield to
get the answer (to see if 'text()' is empty or not). If someone changed the
implementation within Textfield, this would break. It would be safer to simply
have the Textfield report the truth.

If you like this approach, I'll fix up the browser test. It only needs a single
line changed, and this policy doesn't appear to be some fundamental dependency
of the Omnibox.

[Peter Kasting, 2017-06-15 21:32:00]

It's not obvious to me whether IsSelectAll() should be always-true or
always-false when the text is empty, but I think it's at least reasonable to
claim it should always be false (as opposed to the current "vacuously true"). 
So I'm fine with this approach.

LGTM

[Kevin Bailey, 2017-06-16 13:16:35]

The CQ bit was checked by krb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 13:16:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 14:04:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 14:04:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin Bailey, 2017-06-16 14:13:56]

The CQ bit was checked by krb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 14:14:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kevin Bailey, 2017-06-16 15:32:15]

krb@chromium.org changed reviewers:
+ rohitrao@chromium.org

[Kevin Bailey, 2017-06-16 15:33:17]

Hi Rohit,

Could you bless the Cocoa side of this?

thx,
krb

[commit-bot: I haz the power, 2017-06-16 16:45:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 16:45:13]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-06-18 13:22:25]

mac/ lgtm

[Kevin Bailey, 2017-06-19 14:27:35]

The CQ bit was checked by krb@chromium.org

[Kevin Bailey, 2017-06-19 14:27:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2942013002/#ps40001
(title: "Missed a fix")

[commit-bot: I haz the power, 2017-06-19 14:27:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 15:34:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497882455783140,
"parent_rev": "9f6fd4b8c8792db8ee6ab1abc8de0b4490e5ff9e", "commit_rev":
"4033a5a4297ace7d7bf93cdc9b8ea35db4fa35da"}

[commit-bot: I haz the power, 2017-06-19 15:34:51]

Description was changed from

==========
[omnibox] Constrain 'IsSelectAll()' to be more accurate

Currently, 'IsSelectAll()' returns true for a range of (0,0) but the
text is empty.  This isn't correct, and a range with identical
endpoints actually represents a cursor/caret position with nothing
selected.  Specifically, (0,0) is what the Omnibox code requests to
'home' the cursor.  Returning true is causing a problem for
'OmniboxViewViews::Update()' which then thinks it should select all of
the new text.

BUG=729415
==========

to

==========
[omnibox] Constrain 'IsSelectAll()' to be more accurate

Currently, 'IsSelectAll()' returns true for a range of (0,0) but the
text is empty.  This isn't correct, and a range with identical
endpoints actually represents a cursor/caret position with nothing
selected.  Specifically, (0,0) is what the Omnibox code requests to
'home' the cursor.  Returning true is causing a problem for
'OmniboxViewViews::Update()' which then thinks it should select all of
the new text.

BUG=729415

Review-Url: https://codereview.chromium.org/2942013002
Cr-Commit-Position: refs/heads/master@{#480450}
Committed:
https://chromium.googlesource.com/chromium/src/+/4033a5a4297ace7d7bf93cdc9b8e...
==========

[commit-bot: I haz the power, 2017-06-19 15:34:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4033a5a4297ace7d7bf93cdc9b8e...