Selection Action mode triggered like a context menu

Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make range touch selections trigger contextmenu events. This means 
that a long-press or double tap that results in a range selection sends a context 
menu event. Because of Android O's smart select we also need to know if the 
context menu was triggered by moving touch handles or select all so we plumb that
information through to ContextMenuParams.

BUG=627234
Review-Url: https://codereview.chromium.org/2785853002
Cr-Commit-Position: refs/heads/master@{#476355}
Committed: https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e486658183456c

[amaralp, 2017-03-29 23:59:14]

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

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

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-30 00:11:26]

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

[commit-bot: I haz the power, 2017-03-30 00:11:26]

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

[amaralp, 2017-03-30 00:20:19]

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

[commit-bot: I haz the power, 2017-03-30 00:21: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-30 01:01:54]

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

[commit-bot: I haz the power, 2017-03-30 01:01:55]

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_...)

[amaralp, 2017-03-30 02:23:39]

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

[commit-bot: I haz the power, 2017-03-30 02:24:28]

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-30 02:36:23]

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

[commit-bot: I haz the power, 2017-03-30 02:36:24]

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

[amaralp, 2017-04-04 21:06:24]

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

[commit-bot: I haz the power, 2017-04-04 21:07:32]

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-04-04 21:50:21]

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

[commit-bot: I haz the power, 2017-04-04 21:50:22]

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

[amaralp, 2017-04-05 00:32:23]

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

[commit-bot: I haz the power, 2017-04-05 00:33:51]

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-04-05 01:13:49]

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

[commit-bot: I haz the power, 2017-04-05 01:13:50]

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_...)

[amaralp, 2017-04-05 01:19:23]

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

[commit-bot: I haz the power, 2017-04-05 01:19:59]

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-04-05 03:09:28]

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

[commit-bot: I haz the power, 2017-04-05 03:09:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[amaralp, 2017-04-07 07:28:47]

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

[commit-bot: I haz the power, 2017-04-07 07:29:10]

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-04-07 07:49:07]

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

[commit-bot: I haz the power, 2017-04-07 07:49:08]

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

[amaralp, 2017-04-10 18:18:07]

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

[commit-bot: I haz the power, 2017-04-10 18:19:02]

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-04-10 18:24:25]

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

[commit-bot: I haz the power, 2017-04-10 18:24:26]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[amaralp, 2017-04-10 23:09:02]

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

[commit-bot: I haz the power, 2017-04-10 23:09: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-04-10 23:23:11]

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

[commit-bot: I haz the power, 2017-04-10 23:23:12]

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

[amaralp, 2017-04-14 00:34:07]

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

[commit-bot: I haz the power, 2017-04-14 00:34:36]

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-04-14 01:40:46]

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

[commit-bot: I haz the power, 2017-04-14 01:40:47]

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_...)

[amaralp, 2017-04-14 22:40:51]

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

[commit-bot: I haz the power, 2017-04-14 22:41:25]

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

[amaralp, 2017-04-14 23:51:28]

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

[commit-bot: I haz the power, 2017-04-14 23:51:57]

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-04-14 23:56:23]

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

[commit-bot: I haz the power, 2017-04-14 23:56:24]

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_...)

[amaralp, 2017-04-15 00:01:45]

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

[commit-bot: I haz the power, 2017-04-15 00:02:23]

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-04-15 00:21:12]

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

[commit-bot: I haz the power, 2017-04-15 00:21:14]

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

[amaralp, 2017-04-18 21:19:34]

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

[commit-bot: I haz the power, 2017-04-18 21:20:09]

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

[amaralp, 2017-04-18 23:35:30]

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

[commit-bot: I haz the power, 2017-04-18 23:36:23]

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-04-18 23:41:01]

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

[commit-bot: I haz the power, 2017-04-18 23:41:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_chromeos_ozone_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_...)

[amaralp, 2017-04-18 23:46:04]

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

[commit-bot: I haz the power, 2017-04-18 23:46:39]

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-04-18 23:59:35]

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

[commit-bot: I haz the power, 2017-04-18 23:59:38]

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_...)

[amaralp, 2017-04-19 00:04:22]

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

[commit-bot: I haz the power, 2017-04-19 00:05:04]

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-04-19 01:11:30]

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

[commit-bot: I haz the power, 2017-04-19 01:11: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_...)

[amaralp, 2017-04-20 22:37:43]

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

[commit-bot: I haz the power, 2017-04-20 22:38:05]

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-04-20 22:40:53]

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

[commit-bot: I haz the power, 2017-04-20 22:40:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[amaralp, 2017-04-20 22:47:23]

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

[commit-bot: I haz the power, 2017-04-20 22:47:53]

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-04-20 22:50:12]

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

[commit-bot: I haz the power, 2017-04-20 22:50:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[amaralp, 2017-04-21 01:01:05]

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

[commit-bot: I haz the power, 2017-04-21 01:01:44]

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

[amaralp, 2017-04-21 05:13:31]

Description was changed from

==========
Selection Action mode triggered like a context menu

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. The context menu
event comes with ContextMenuParams

This patch also 
BUG=627234
==========

[amaralp, 2017-04-21 05:14:52]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. The context menu
event comes with ContextMenuParams

This patch also 
BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. The context menu
event comes with |ContextMenuParams| that tell us the relevant state of the
selection at the time we are creating the menu.

BUG=627234
==========

[amaralp, 2017-04-21 05:16:59]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. The context menu
event comes with |ContextMenuParams| that tell us the relevant state of the
selection at the time we are creating the menu.

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

BUG=627234
==========

[amaralp, 2017-04-21 05:17:12]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

BUG=627234
==========

[amaralp, 2017-04-21 05:18:41]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org, jinsukkim@chromium.org,
timav@chromium.org

[amaralp, 2017-04-21 05:18:42]

PTAL

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

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

[commit-bot: I haz the power, 2017-04-21 06:30:07]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-04-21 18:50:16]

On 2017/04/21 05:18:42, amaralp wrote:
> PTAL

For CL with many reviewers, you should specify what should each reviewer look
at.

[boliu, 2017-04-21 21:20:19]

https://codereview.chromium.org/2785853002/diff/380001/content/browser/render...
File content/browser/renderer_host/input/touch_emulator_client.h (right):

https://codereview.chromium.org/2785853002/diff/380001/content/browser/render...
content/browser/renderer_host/input/touch_emulator_client.h:25: bool from_touch
= false) = 0;
default arg on virtual function banned by styleguide:
https://google.github.io/styleguide/cppguide.html#Default_Arguments

and function overloading is generally still frowned upon even though it's no
longer outright banned

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java
(right):

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:97:
requestSurroundingText(shouldSuggest ? SUGGEST_AND_CLASSIFY : CLASSIFY);
this method right now is "does this client send selection updates", not "do
selection updates"

Probably should just reword the method. Maybe also make cancel explicit

https://codereview.chromium.org/2785853002/diff/380001/content/public/common/...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/2785853002/diff/380001/content/public/common/...
content/public/common/context_menu_params.h:170: gfx::Rect selection_rect;
comment explaining this, what coordinate space is this in, etc.

should this be in the OS_ANDROID block that also contains selection_start/end?
Actually what's the relationship between this and the selection points?

https://codereview.chromium.org/2785853002/diff/380001/content/public/common/...
content/public/common/context_menu_params.h:172: bool from_touch;
how is from_touch different from the two source types used in
ContentViewCoreImpl::ShowSelectionMenu?

[aelias_OOO_until_Jul13, 2017-04-21 21:42:19]

Nice cleanup overall.

https://codereview.chromium.org/2785853002/diff/380001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2785853002/diff/380001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:627: params.from_touch;
Is it possible to use the existing source_type enum entirely instead of
introducing the new boolean?

https://codereview.chromium.org/2785853002/diff/380001/content/browser/render...
File content/browser/renderer_host/input/touch_emulator_client.h (right):

https://codereview.chromium.org/2785853002/diff/380001/content/browser/render...
content/browser/renderer_host/input/touch_emulator_client.h:25: bool from_touch
= false) = 0;
Default arguments are disallowed by Chromium style guide (only allowed in Blink
style guide)

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
(right):

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:119:
private boolean mSelectionHandleDragStopped;
I think it would be clearer to invert this, "mDraggingSelectionHandle"

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:219:
mHasSelection = selectionText.length() != 0;
This state is redundant now, can be replaced by bool hasSelection() { return
mLastSelectedText.length() != 0; }  (different behavior with
onSelectionChanged() but that seems like probably a fix?).

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:220:
mIsInsertion = selectionText.length() == 0;
Likewise for mIsInsertion maybe?  Is there any value-added to setting it in
INSERTION_HANDLE_SHOWN?

https://codereview.chromium.org/2785853002/diff/380001/content/public/common/...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/2785853002/diff/380001/content/public/common/...
content/public/common/context_menu_params.h:163: gfx::Point selection_start;
What about this?  Looks redundant with selection_rect.  I think it's fine for
selection_rect to be cross-platform instead of Android-specific, so can this be
deleted?

https://codereview.chromium.org/2785853002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2785853002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1420: crbug.com/627234
fast/events/hit-test-counts.html [ NeedsRebaseline ]
Could you include the new expectation files as part of this patch instead of
using NeedsRebaseline?  NeedsRebaseline is generally for tests with different
image output expectations per platform.

https://codereview.chromium.org/2785853002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1924: IntPoint
global_position =
Could you refactor the code that's duplicated-ish with
SendContextMenuEventForKey() into a parametrized helper method?

[Jinsuk Kim, 2017-04-24 06:33:16]

Nice refactoring.

FYI I'm working on https://crrev.com/2826303003 not to use content_view_core for
selection events. Would you leave a TODO somewhere for a reminder later for
further refactoring?

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/SelectionClient.java
(right):

https://codereview.chromium.org/2785853002/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionClient.java:37:
boolean sendsSelectionPopupUpdates(boolean shouldSuggest);
Update javadoc.

[amaralp, 2017-05-03 22:53:57]

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

[commit-bot: I haz the power, 2017-05-03 22:54:25]

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-05-04 02:06:31]

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

[commit-bot: I haz the power, 2017-05-04 02:06:32]

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_...)

[Shimi Zhang, 2017-05-22 18:23:33]

amaralp@, when could this CL land? If it still takes a while, I think I'll need
a work around for paste as plain text.

[amaralp, 2017-05-23 03:02:57]

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

[commit-bot: I haz the power, 2017-05-23 03:03:24]

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-05-23 03:06:28]

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

[commit-bot: I haz the power, 2017-05-23 03:06:29]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[amaralp, 2017-05-23 03:57:31]

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

[commit-bot: I haz the power, 2017-05-23 03:57: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-05-23 06:05:26]

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

[commit-bot: I haz the power, 2017-05-23 06:05:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[amaralp1, 2017-05-23 07:56:27]

amaralp@google.com changed reviewers:
+ amaralp@google.com, bokan@chromium.org, sadrul@chromium.org,
tedchoc@chromium.org, yosin@chromium.org

[amaralp1, 2017-05-23 07:56:29]

PTAL

boliu@ - android_webview, content/browser/android, CVC.java/SPC.java, cvc.h,
cvci.cc/h
tedchoc@ - chrome_web_contents_view_delegate.cc,
shell_web_contents_view_delegate_android.cc
aelias@ - touch_selection_controller_client_aura.cc, content/renderer,
WebKit/public, WebKit/Source/web
yosin@ - WebKit/Source/core/editing
bokan@ - WebKit/Source/core/[events|input|loader|page]
sadrul@ - ui/base/

[boliu, 2017-05-23 17:34:13]

I'll wait for blink to be approved first.

Probably should update CL description to say what the blink changes are. I
didn't read the blink diff, and description was not very helpful with
understanding blink changes.

And test?

https://codereview.chromium.org/2785853002/diff/480001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2785853002/diff/480001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:169:
const bool from_touch = params.source_type == ui::MENU_SOURCE_LONG_PRESS ||
were you intending to change aura? I guess blink changes made this necessary?

https://codereview.chromium.org/2785853002/diff/480001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2785853002/diff/480001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4298:
GetRenderWidget()->input_handler().context_menu_source_type();
pass this into Builder, instead of having this brittle check?

[amaralp, 2017-05-23 19:45:54]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make touch selections also trigger touch events. This means that a 
long-press or double tap that results in a range selection sends a context menu
event. Because of Android O's smart select we also need to know if the context
menu was triggered by moving touch handles or select all so we plumb that
information through to ContextMenuParams.
BUG=627234
==========

[amaralp, 2017-05-23 19:46:16]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make touch selections also trigger touch events. This means that a 
long-press or double tap that results in a range selection sends a context menu
event. Because of Android O's smart select we also need to know if the context
menu was triggered by moving touch handles or select all so we plumb that
information through to ContextMenuParams.
BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make touch selections also trigger touch events. This means that a 
long-press or double tap that results in a range selection sends a context menu
event. Because of Android O's smart select we also need to know if the context
menu was triggered by moving touch handles or select all so we plumb that
information through to ContextMenuParams.

BUG=627234
==========

[amaralp, 2017-05-23 19:48:56]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make touch selections also trigger touch events. This means that a 
long-press or double tap that results in a range selection sends a context menu
event. Because of Android O's smart select we also need to know if the context
menu was triggered by moving touch handles or select all so we plumb that
information through to ContextMenuParams.

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make range touch selections trigger contextmenu events. This means 
that a long-press or double tap that results in a range selection sends a
context 
menu event. Because of Android O's smart select we also need to know if the 
context menu was triggered by moving touch handles or select all so we plumb
that
information through to ContextMenuParams.

BUG=627234
==========

[aelias_OOO_until_Jul13, 2017-05-23 20:52:05]

My OWNERS files lgtm, although I have a few comments on Source/core:

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1917: WebInputEventResult
EventHandler::SendContextMenuEventForTouchSelection(
This return value is unused by the callers.  Since this is a side effect of an
otherwise system-handled event rather than part of the gesture handling pipeline
per se, I suggest just making this return void.

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:305:
frame_->GetEventHandler().SendContextMenuEventForTouchSelection(
How about putting this behind "if (event_result ==
WebInputEventResult::kHandledSystem)"?  I think if this was kHandledApplication
for example, we don't want to show the menu.

[boliu, 2017-05-23 21:18:21]

lgtm % previous comments

[amaralp, 2017-05-23 21:36:00]

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

[commit-bot: I haz the power, 2017-05-23 21:36:38]

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-05-23 22:10:34]

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

[commit-bot: I haz the power, 2017-05-23 22:10:36]

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

[amaralp, 2017-05-24 02:56:44]

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

[commit-bot: I haz the power, 2017-05-24 02:57:55]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-05-24 04:51:23]

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:726: if
(IsHandleVisible()) {
It is OK to show context menu when JS calls |document.execCommand('selectAll')|?

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1924:
selection.ComputeVisibleSelectionInFlatTree().IsRange()))
Please change this conditional expression to:

!selection.IsAvaiable() || !selction.IsHandleVisible() || selection.IsHidden())


selection.IsHidden() returns false non-range selection, or unfocused range
selection.

[commit-bot: I haz the power, 2017-05-24 05:07:12]

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

[commit-bot: I haz the power, 2017-05-24 05:07:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[amaralp, 2017-05-24 06:41:53]

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

[commit-bot: I haz the power, 2017-05-24 06:42:29]

Dry run: CQ is trying da patch.

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

[amaralp1, 2017-05-24 08:30:35]

https://codereview.chromium.org/2785853002/diff/480001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2785853002/diff/480001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:169:
const bool from_touch = params.source_type == ui::MENU_SOURCE_LONG_PRESS ||
On 2017/05/23 at 17:34:13, boliu wrote:
> were you intending to change aura? I guess blink changes made this necessary?

Yes, the now blink sends context menu events when touch selection handles are
shown and aura is not expecting them so we need to ignore them in aura.
Eventually we should change aura's implementation to rely on the context menu
events instead of selection handle changes.

https://codereview.chromium.org/2785853002/diff/480001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2785853002/diff/480001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4298:
GetRenderWidget()->input_handler().context_menu_source_type();
On 2017/05/23 at 17:34:13, boliu wrote:
> pass this into Builder, instead of having this brittle check?

I decided to remove the SelectAll plumbing from this CL. I'll do the plumbing in
a separate CL.

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1917: WebInputEventResult
EventHandler::SendContextMenuEventForTouchSelection(
On 2017/05/23 at 20:52:05, aelias wrote:
> This return value is unused by the callers.  Since this is a side effect of an
otherwise system-handled event rather than part of the gesture handling pipeline
per se, I suggest just making this return void.

Done.

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:305:
frame_->GetEventHandler().SendContextMenuEventForTouchSelection(
On 2017/05/23 at 20:52:05, aelias wrote:
> How about putting this behind "if (event_result ==
WebInputEventResult::kHandledSystem)"?  I think if this was kHandledApplication
for example, we don't want to show the menu.

Done.

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:726: if
(IsHandleVisible()) {
On 2017/05/24 at 04:51:23, yosin_UTC9 wrote:
> It is OK to show context menu when JS calls
|document.execCommand('selectAll')|?

I think it is OK in this case. The selectAll will make a range selection with
touch handles and we want to show the context menu in that case. Also currently
selectAll will show a context menu (although without contextmenu events).

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1924:
selection.ComputeVisibleSelectionInFlatTree().IsRange()))
On 2017/05/24 at 04:51:23, yosin_UTC9 wrote:
> Please change this conditional expression to:
> 
> !selection.IsAvaiable() || !selction.IsHandleVisible() ||
selection.IsHidden())
> 
> 
> selection.IsHidden() returns false non-range selection, or unfocused range
selection.

Done.

[commit-bot: I haz the power, 2017-05-24 09:15:15]

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

[commit-bot: I haz the power, 2017-05-24 09:15:16]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-05-24 16:20:23]

On 2017/05/24 08:30:39, amaralp1 wrote:

chrome_web_contents_view_delegate - lgtm

[amaralp1, 2017-05-24 17:40:26]

amaralp@google.com changed reviewers:
- sadrul@chromium.org

[amaralp1, 2017-05-24 17:52:42]

bokan@ could you please take a look at core/input? Thanks!

[bokan, 2017-05-24 19:15:53]

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1813: WebInputEventResult
EventHandler::SendContextMenuEventForKey(
This method should be renamed as it's not just for keyboard anymore (and hasn't
been prior to this I think). Perhaps ShowNonLocatedContextMenu?

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1885: // keyboard.  This
is required for web compatibility.
keyboard or other methods.

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1923: if
(!selection.IsAvailable() || !selection.IsHandleVisible() ||
Do we need a new method here? Couldn't we just do this in the above method based
on menu_source?

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/GestureManager.cpp (right):

https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/GestureManager.cpp:305: if (event_result ==
WebInputEventResult::kHandledSystem) {
Is a synthetic mouse up/release being kHandledSystem always indicative that we
need to show a context menu? That seems a bit fragile to me, could we send the
context menu at the place where we decide it's needed (presumably somewhere in
the text selection code?)

Also, this should be |else if|

[amaralp, 2017-05-25 00:26:08]

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

[commit-bot: I haz the power, 2017-05-25 00:27:04]

Dry run: CQ is trying da patch.

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

[amaralp, 2017-05-25 00:31:15]

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

[commit-bot: I haz the power, 2017-05-25 00:32:10]

Dry run: CQ is trying da patch.

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

[amaralp, 2017-05-25 01:55:14]

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

[commit-bot: I haz the power, 2017-05-25 01:56:51]

Dry run: CQ is trying da patch.

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

[amaralp, 2017-05-25 02:49:52]

On 2017/05/24 at 19:15:53, bokan wrote:
>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1813:
WebInputEventResult EventHandler::SendContextMenuEventForKey(
> This method should be renamed as it's not just for keyboard anymore (and
hasn't been prior to this I think). Perhaps ShowNonLocatedContextMenu?
> 
Done.

>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1885: // keyboard.  This
is required for web compatibility.
> keyboard or other methods.
> 
Done.

>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1923: if
(!selection.IsAvailable() || !selection.IsHandleVisible() ||
> Do we need a new method here? Couldn't we just do this in the above method
based on menu_source?
> 
Done.

>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/GestureManager.cpp (right):
> 
>
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/GestureManager.cpp:305: if (event_result
== WebInputEventResult::kHandledSystem) {
> Is a synthetic mouse up/release being kHandledSystem always indicative that we
need to show a context menu? That seems a bit fragile to me, could we send the
context menu at the place where we decide it's needed (presumably somewhere in
the text selection code?)
> 
> Also, this should be |else if|

Moved sending of context menu to SelectionController.cpp

[commit-bot: I haz the power, 2017-05-25 04:21:09]

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

[commit-bot: I haz the power, 2017-05-25 04:21:11]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-05-25 04:47:21]

https://codereview.chromium.org/2785853002/diff/600001/content/browser/androi...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/2785853002/diff/600001/content/browser/androi...
content/browser/android/content_view_core_impl.h:59: bool
ShowSelectionMenu(const ContextMenuParams& params) override;
It is better to move renaming of |ShowPastePopup()| to
|ShowSelectionSelectionMenu()| in another patch.

https://codereview.chromium.org/2785853002/diff/600001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
(right):

https://codereview.chromium.org/2785853002/diff/600001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:927:
if (!hasSelection() || !isSelectionEditable()) return;
It is better to have another patch to replace |mHasSelection| to
|hasSelection()|.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:199:
frame_->GetEventHandler().ShowNonLocatedContextMenu(nullptr,
Since "selectionstart" event is cancelalbe, we should now show context menu if
"selectionstart" event is canceled.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:200:
kMenuSourceTouch);
Could you tell me why we use |kMenuSourceTouch| even if mouse single click?
Should we check |event.FromTouch()|?

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:822: if
(did_select && Selection().IsHandleVisible()) {
early-return style is better.

if (!did_selection)
  return false;

if (Selection().IsHandleVisible()) {
  ShwoNonLocatedCotnextMenu(...)
}

return true;

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:105: bool
SelectClosestWordFromMouseEvent(const MouseEventWithHitTestResults&);
Please add a comment for |bool| return value.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1813: WebInputEventResult
EventHandler::ShowNonLocatedContextMenu(
Can we have another patch rename |SendContextMenuEventForKey()| to
|ShowNonLocatedCotnextMenu()|?

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ContextMenuAllowedScope.h (left):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ContextMenuAllowedScope.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Could you move relocation of ContextMenuAllowedScope.{cpp,h} in another patch to
make this patch smaller?

[amaralp1, 2017-05-25 07:11:56]

https://codereview.chromium.org/2785853002/diff/600001/content/browser/androi...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/2785853002/diff/600001/content/browser/androi...
content/browser/android/content_view_core_impl.h:59: bool
ShowSelectionMenu(const ContextMenuParams& params) override;
On 2017/05/25 at 04:47:20, yosin_UTC9 wrote:
> It is better to move renaming of |ShowPastePopup()| to
|ShowSelectionSelectionMenu()| in another patch.

This new name only makes sense with this change. Renaming this function by
itself wouldn't make sense.

https://codereview.chromium.org/2785853002/diff/600001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
(right):

https://codereview.chromium.org/2785853002/diff/600001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:927:
if (!hasSelection() || !isSelectionEditable()) return;
On 2017/05/25 at 04:47:20, yosin_UTC9 wrote:
> It is better to have another patch to replace |mHasSelection| to
|hasSelection()|.

Done. https://codereview.chromium.org/2904033002

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:199:
frame_->GetEventHandler().ShowNonLocatedContextMenu(nullptr,
On 2017/05/25 at 04:47:20, yosin_UTC9 wrote:
> Since "selectionstart" event is cancelalbe, we should now show context menu if
> "selectionstart" event is canceled.

Changed to only show context menu if
|UpdateSelectionForMouseDownDispatchingSelectStart()| returns true.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:200:
kMenuSourceTouch);
On 2017/05/25 at 04:47:21, yosin_UTC9 wrote:
> Could you tell me why we use |kMenuSourceTouch| even if mouse single click?
> Should we check |event.FromTouch()|?

|event.FromTouch()| has to be true or else we would have returned in the
previous if statement:

if (!event.Event().FromTouch())
  return false;

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:822: if
(did_select && Selection().IsHandleVisible()) {
On 2017/05/25 at 04:47:20, yosin_UTC9 wrote:
> early-return style is better.
> 
> if (!did_selection)
>   return false;
> 
> if (Selection().IsHandleVisible()) {
>   ShwoNonLocatedCotnextMenu(...)
> }
> 
> return true;

Done.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.h:105: bool
SelectClosestWordFromMouseEvent(const MouseEventWithHitTestResults&);
On 2017/05/25 at 04:47:21, yosin_UTC9 wrote:
> Please add a comment for |bool| return value.

Done.

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1813: WebInputEventResult
EventHandler::ShowNonLocatedContextMenu(
On 2017/05/25 at 04:47:21, yosin_UTC9 wrote:
> Can we have another patch rename |SendContextMenuEventForKey()| to
|ShowNonLocatedCotnextMenu()|?

Done. https://codereview.chromium.org/2905783002

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ContextMenuAllowedScope.h (left):

https://codereview.chromium.org/2785853002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ContextMenuAllowedScope.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2017/05/25 at 04:47:20, yosin_UTC9 wrote:
> Could you move relocation of ContextMenuAllowedScope.{cpp,h} in another patch
to make this patch smaller?

Done. https://codereview.chromium.org/2906633002

[bokan, 2017-05-25 14:26:54]

Source/core and Source/web lgtm but please wait on yosin@ to approve overall
editing logic.

[amaralp, 2017-05-30 19:29:08]

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

[commit-bot: I haz the power, 2017-05-30 19:30:01]

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-05-30 19:36:55]

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

[commit-bot: I haz the power, 2017-05-30 19:36:57]

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

[amaralp, 2017-05-30 21:23:00]

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

[commit-bot: I haz the power, 2017-05-30 21:23:38]

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-05-30 21:35:01]

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

[commit-bot: I haz the power, 2017-05-30 21:35:03]

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

[amaralp, 2017-05-31 01:45:49]

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

[commit-bot: I haz the power, 2017-05-31 01:46:34]

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-05-31 02:00:36]

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

[commit-bot: I haz the power, 2017-05-31 02:00:37]

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

[amaralp, 2017-05-31 02:33:59]

aelias@, I found a problem with stylus selection and made some changes to stylus
code in content/browser/renderer_host/input to fix it. Could you take a look?

[aelias_OOO_until_Jul13, 2017-05-31 03:02:58]

content/browser/renderer_host/input lgtm

[amaralp1, 2017-05-31 04:40:43]

amaralp@google.com changed reviewers:
+ avi@chromium.org, sadrul@chromium.org

[amaralp1, 2017-05-31 04:40:45]

avi@ for content/child/assert_matching_enums.cc
sadrul@ for ui/base/ui_base_types.h
yosin@ could you take a look at the editing changes? I think I have addressed
all your comments.

[Avi (use Gerrit), 2017-05-31 04:43:40]

On 2017/05/31 04:40:45, amaralp1 wrote:
> avi@ for content/child/assert_matching_enums.cc
> sadrul@ for ui/base/ui_base_types.h
> yosin@ could you take a look at the editing changes? I think I have addressed
> all your comments.

matching enums match. check.

lgtm

[yosin_UTC9, 2017-05-31 07:47:28]

lgtm

Sorry for late response. I missed... m(_ _)m

[amaralp, 2017-06-01 01:02:02]

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

[commit-bot: I haz the power, 2017-06-01 01:02:57]

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-01 02:10:28]

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

[commit-bot: I haz the power, 2017-06-01 02:10:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sadrul, 2017-06-01 15:52:39]

lgtm

[amaralp, 2017-06-01 17:20:27]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2017-06-01 17:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
tedchoc@chromium.org, bokan@chromium.org, aelias@chromium.org, avi@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2785853002/#ps700001
(title: "fix rebase")

[commit-bot: I haz the power, 2017-06-01 17:20:53]

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-01 18:20:26]

CQ is committing da patch.

Bot data: {"patchset_id": 700001, "attempt_start_ts": 1496337627290760,
"parent_rev": "639c840bf93e2934fe6b3f564f90060313d5791a", "commit_rev":
"58d4d0b2d9511fe57bde3cbe82e486658183456c"}

[commit-bot: I haz the power, 2017-06-01 18:20:35]

Description was changed from

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make range touch selections trigger contextmenu events. This means 
that a long-press or double tap that results in a range selection sends a
context 
menu event. Because of Android O's smart select we also need to know if the 
context menu was triggered by moving touch handles or select all so we plumb
that
information through to ContextMenuParams.

BUG=627234
==========

to

==========
Selection Action mode triggered like a context menu

Currently the selection action mode is triggered by selection events
(SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered
by context menu events. This is beneficial for two reasons:

1) The paste popup menu is already being triggered by context menu events and
we'd like to eventually unify the selection and paste menus.

2) Currently the selection action mode uses state from the IME which can lead to
race conditions causing the menu to show stale information. Using context menu
events fixes this problem because we can pass all relevant state through
|ContextMenuParams|.

In Blink we make range touch selections trigger contextmenu events. This means 
that a long-press or double tap that results in a range selection sends a
context 
menu event. Because of Android O's smart select we also need to know if the 
context menu was triggered by moving touch handles or select all so we plumb
that
information through to ContextMenuParams.

BUG=627234

Review-Url: https://codereview.chromium.org/2785853002
Cr-Commit-Position: refs/heads/master@{#476355}
Committed:
https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e4...
==========

[commit-bot: I haz the power, 2017-06-01 18:20:39]

Committed patchset #36 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e4...