Remove moveCursorToSelectionEnd()

Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses the 
selection to the selection end, and renames the functions to CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter TabsTest.testTabSwitcherCollapseSelection

Review-Url: https://codereview.chromium.org/2734943005
Cr-Commit-Position: refs/heads/master@{#455963}
Committed: https://chromium.googlesource.com/chromium/src/+/351e7eca8d363d99a4847df4e750daa07a428440

[yabinh, 2017-03-08 06:35:29]

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

[commit-bot: I haz the power, 2017-03-08 06:35:49]

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-03-08 06:59:03]

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

[commit-bot: I haz the power, 2017-03-08 06:59:04]

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

[yabinh, 2017-03-08 08:09:50]

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

[commit-bot: I haz the power, 2017-03-08 08:10:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-08 08:27:28]

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

[commit-bot: I haz the power, 2017-03-08 08:28:03]

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-03-08 09:18:29]

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

[commit-bot: I haz the power, 2017-03-08 09:18:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yabinh, 2017-03-08 10:30:18]

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

[commit-bot: I haz the power, 2017-03-08 10:30:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-08 10:34:29]

Description was changed from

==========
Remove moveCursorToSelectionEnd()

BUG=
==========

to

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replace with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the related functions to 
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

[yabinh, 2017-03-08 10:38:59]

Description was changed from

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replace with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the related functions to 
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

to

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the functions to be
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

[yabinh, 2017-03-08 10:39:24]

Description was changed from

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the functions to be
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

to

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the functions to
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

[yabinh, 2017-03-08 10:57:42]

Description was changed from

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses 
the selection to the selection end, and renames the functions to
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

to

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses the 
selection to the selection end, and renames the functions to
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

[yabinh, 2017-03-08 11:44:04]

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

[commit-bot: I haz the power, 2017-03-08 11:44:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-08 12:21:08]

yabinh@chromium.org changed reviewers:
+ aelias@chromium.org, changwan@chromium.org

[yabinh, 2017-03-08 12:21:09]

PTAL, thanks!

[commit-bot: I haz the power, 2017-03-08 13:02:27]

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

[commit-bot: I haz the power, 2017-03-08 13:02:28]

Dry run: This issue passed the CQ dry run.

[yabinh, 2017-03-08 14:58:23]

https://codereview.chromium.org/2734943005/diff/80001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
(right):

https://codereview.chromium.org/2734943005/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:402:
assertWaitForKeyboardStatus(true);
On a second thought, assertWaitForSelectedText("world") seems better. I'll
replace it in the next patch.

[aelias_OOO_until_Jul13, 2017-03-08 21:55:22]

aelias@chromium.org changed reviewers:
+ avi@chromium.org, kenrb@chromium.org

[aelias_OOO_until_Jul13, 2017-03-08 21:55:23]

lgtm, adding more OWNERS for trivial rename:

kenrb@ for input_messages.h
avi@ for content/public/browser/web_contents.h

https://codereview.chromium.org/2734943005/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2734943005/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2795:
RecordAction(base::UserMetricsAction("CollapseSelection"));
I don't think there's any interest in this metric, please just delete it instead
of renaming it.

[Avi (use Gerrit), 2017-03-08 22:00:39]

lgtm

ok

[yabinh, 2017-03-09 02:05:46]

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

[commit-bot: I haz the power, 2017-03-09 02:06:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-09 02:12:55]

yabinh@chromium.org changed reviewers:
+ dtrainor@chromium.org, isherman@chromium.org

[yabinh, 2017-03-09 02:12:56]

Thanks!

Adding dtrainor@ for chrome/android/javatests, and isherman@ for
tools/metrics/actions/actions.xml

https://codereview.chromium.org/2734943005/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2734943005/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2795:
RecordAction(base::UserMetricsAction("CollapseSelection"));
On 2017/03/08 21:55:23, aelias wrote:
> I don't think there's any interest in this metric, please just delete it
instead
> of renaming it.

Done.

[commit-bot: I haz the power, 2017-03-09 02:57:51]

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

[commit-bot: I haz the power, 2017-03-09 02:57:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yabinh, 2017-03-09 08:05:56]

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

[commit-bot: I haz the power, 2017-03-09 08:06:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-09 08:17:00]

In general, I would recommend a bit of caution w.r.t. renaming actions.  If an
action is used, then renaming it will break the history.  If it's unused, then
it's better to remove it than to rename it.

https://codereview.chromium.org/2734943005/diff/120001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2734943005/diff/120001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:3218: <action name="CollapseSelection">
This action name is fairly vague.  Could you please rename it to be clearer what
the context is?

https://codereview.chromium.org/2734943005/diff/120001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:3219: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
^^^

https://codereview.chromium.org/2734943005/diff/120001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:3220: <description>Please enter the
description of the metric.</description>
^^^

[yabinh, 2017-03-09 08:23:04]

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

[commit-bot: I haz the power, 2017-03-09 08:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-09 08:23:45]

On 2017/03/09 08:17:00, Ilya Sherman wrote:
> In general, I would recommend a bit of caution w.r.t. renaming actions.  If an
> action is used, then renaming it will break the history.  If it's unused, then
> it's better to remove it than to rename it.

Removed. PTAL, thanks!

[Ilya Sherman, 2017-03-09 08:30:19]

https://codereview.chromium.org/2734943005/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/2734943005/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:15898: <action name="Unselect">
Sorry, I should have clarified: Please mark the action as <obsolete> rather than
removing its metadata entirely.

[yabinh, 2017-03-09 09:00:10]

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

[commit-bot: I haz the power, 2017-03-09 09:00:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-09 09:00:39]

https://codereview.chromium.org/2734943005/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/2734943005/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:15898: <action name="Unselect">
On 2017/03/09 08:30:18, Ilya Sherman wrote:
> Sorry, I should have clarified: Please mark the action as <obsolete> rather
than
> removing its metadata entirely.

Done.

[commit-bot: I haz the power, 2017-03-09 10:02:20]

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

[commit-bot: I haz the power, 2017-03-09 10:02:22]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-03-09 14:41:44]

ipc lgtm

[David Trainor- moved to gerrit, 2017-03-09 17:48:04]

chrome/android/javatests lgtm.

https://codereview.chromium.org/2734943005/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/2734943005/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1398:
assertTrue(mSelectionController.getSelectedText().equals(""));
TextUtils.isEmpty() to handle both null and ""?  Or do you want to guarantee
it's not null here?

[Ilya Sherman, 2017-03-09 18:32:32]

actions.xml lgtm, thanks

[yabinh, 2017-03-10 01:28:27]

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

[commit-bot: I haz the power, 2017-03-10 01:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yabinh, 2017-03-10 01:35:36]

https://codereview.chromium.org/2734943005/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/2734943005/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1398:
assertTrue(mSelectionController.getSelectedText().equals(""));
On 2017/03/09 17:48:04, David Trainor-ping if over 24h wrote:
> TextUtils.isEmpty() to handle both null and ""?  Or do you want to guarantee
> it's not null here?

Yes, both null and "" should be OK as the test name "ClearsSelection" suggests.

[yabinh, 2017-03-10 01:35:43]

The CQ bit was unchecked by yabinh@chromium.org

[yabinh, 2017-03-10 01:35:43]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2017-03-10 01:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
avi@chromium.org, kenrb@chromium.org, isherman@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2734943005/#ps180001
(title: "For dtrainor@'s review")

[commit-bot: I haz the power, 2017-03-10 01:36:15]

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-03-10 02:43:34]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489109743843430,
"parent_rev": "1fd0ec64195b3be25a369b6b27cc8eb630f7b672", "commit_rev":
"351e7eca8d363d99a4847df4e750daa07a428440"}

[commit-bot: I haz the power, 2017-03-10 02:44:27]

Description was changed from

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses the 
selection to the selection end, and renames the functions to
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection
==========

to

==========
Remove moveCursorToSelectionEnd()

moveCursorToSelectionEnd()  blocks the IME thread unnecessarily, which causes
back navigation to fail. So this CL removes moveCursorToSelectionEnd() and 
replaces with mWebContents.unselect().

In RenderFrameImpl::OnUnselect(), the cursor is deleted entirely, which causes 
text boxes to be unfocused as a side effect. To fix that, this CL collapses the 
selection to the selection end, and renames the functions to
CollapseSelection().

BUG=697756
TEST=run_chrome_public_test_apk --test-filter
TabsTest.testTabSwitcherCollapseSelection

Review-Url: https://codereview.chromium.org/2734943005
Cr-Commit-Position: refs/heads/master@{#455963}
Committed:
https://chromium.googlesource.com/chromium/src/+/351e7eca8d363d99a4847df4e750...
==========

[commit-bot: I haz the power, 2017-03-10 02:44:35]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/351e7eca8d363d99a4847df4e750...