Context Menu Refactor

Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles being shown in ContentViewCore.java. This context menu is then populated according to ContentViewCore's current state of the text (i.e. don't show "Copy" or "Cut" options if text is a password). I believe this state is mostly kept up to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is converted to a right click event which triggers blink to show a context menu. Eventually this gets delegated to either "AwWebContentsViewDelegate" or "ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView doesn't support Modal context menus (discussed in #3), but it is also duplicates code.

3) The Modal context menu has the same path as the Paste menu (#2) up until ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in blink as a context menu. This would make it so a ContextMenuParams argument is passed during the creation meaning that creating the menu wouldn’t have to rely on state from updateImeAdapter. This also has the benefit that it unifies the first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not only select the text but also send a right click event. Then to avoid WebView and Chrome code duplication I handle the context menu in WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add an extra parameter to this method and hence change many files). WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that communicates to its java counterpart when to show/hide/move/close the menu. 

BUG=598880, 623783

[amaralp, 2016-06-22 00:27:04]

Description was changed from

==========
Context Menu Refactor

Refactoring Android select action mode context menu
==========

to

==========
Context Menu Refactor

Refactoring Android select action mode context menu
==========

[amaralp, 2016-06-22 00:38:42]

amaralp@chromium.org changed reviewers:
+ sievers@chromium.org

[amaralp, 2016-06-22 01:18:34]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org

[amaralp, 2016-06-22 01:28:08]

On 2016/06/22 at 01:18:34, amaralp wrote:
> amaralp@chromium.org changed reviewers:
> + aelias@chromium.org

This is a rough draft of the context menu refactor. The relevant
files are:

components/web_contents_delegate_android/web_contents_delegate_android.cc
content/browser/renderer_host/render_widget_host_view_android.cc
content/browser/renderer_host/select_action_mode.cc
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java

Let me know what you think.

[aelias_OOO_until_Jul13, 2016-06-22 02:35:10]

Could you summarize the scope and goal of this refactor in the patch
description?

[amaralp, 2016-06-22 19:12:13]

Description was changed from

==========
Context Menu Refactor

Refactoring Android select action mode context menu
==========

to

==========
Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles
being shown in ContentViewCore.java. This context menu is then populated
according to ContentViewCore's current state of the text (i.e. don't show "Copy"
or "Cut" options if text is a password). I believe this state is mostly kept up
to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is
converted to a right click event which triggers blink to show a context menu.
Eventually this gets delegated to either "AwWebContentsViewDelegate" or
"ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView
doesn't support Modal context menus (discussed in #3), but it is also duplicates
code.

3) The Modal context menu has the same path as the Paste menu (#2) up until
ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the
ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in
blink as a context menu. This would make it so a ContextMenuParams argument is
passed during the creation meaning that creating the menu wouldn’t have to rely
on state from updateImeAdapter. This also has the benefit that it unifies the
first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not
only select the text but also send a right click event. Then to avoid WebView
and Chrome code duplication I handle the context menu in
WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add
an extra parameter to this method and hence change many files).
WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the
menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that
communicates to its java counterpart when to show/hide/move/close the menu.
==========

[amaralp, 2016-06-22 19:12:38]

On 2016/06/22 at 02:35:10, aelias wrote:
> Could you summarize the scope and goal of this refactor in the patch
description?

Done. Let me know if you need more info.

[no sievers, 2016-06-23 20:26:42]

Description was changed from

==========
Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles
being shown in ContentViewCore.java. This context menu is then populated
according to ContentViewCore's current state of the text (i.e. don't show "Copy"
or "Cut" options if text is a password). I believe this state is mostly kept up
to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is
converted to a right click event which triggers blink to show a context menu.
Eventually this gets delegated to either "AwWebContentsViewDelegate" or
"ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView
doesn't support Modal context menus (discussed in #3), but it is also duplicates
code.

3) The Modal context menu has the same path as the Paste menu (#2) up until
ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the
ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in
blink as a context menu. This would make it so a ContextMenuParams argument is
passed during the creation meaning that creating the menu wouldn’t have to rely
on state from updateImeAdapter. This also has the benefit that it unifies the
first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not
only select the text but also send a right click event. Then to avoid WebView
and Chrome code duplication I handle the context menu in
WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add
an extra parameter to this method and hence change many files).
WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the
menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that
communicates to its java counterpart when to show/hide/move/close the menu.
==========

to

==========
Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles
being shown in ContentViewCore.java. This context menu is then populated
according to ContentViewCore's current state of the text (i.e. don't show "Copy"
or "Cut" options if text is a password). I believe this state is mostly kept up
to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is
converted to a right click event which triggers blink to show a context menu.
Eventually this gets delegated to either "AwWebContentsViewDelegate" or
"ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView
doesn't support Modal context menus (discussed in #3), but it is also duplicates
code.

3) The Modal context menu has the same path as the Paste menu (#2) up until
ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the
ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in
blink as a context menu. This would make it so a ContextMenuParams argument is
passed during the creation meaning that creating the menu wouldn’t have to rely
on state from updateImeAdapter. This also has the benefit that it unifies the
first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not
only select the text but also send a right click event. Then to avoid WebView
and Chrome code duplication I handle the context menu in
WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add
an extra parameter to this method and hence change many files).
WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the
menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that
communicates to its java counterpart when to show/hide/move/close the menu. 

BUG=598880
==========

[no sievers, 2016-06-23 23:16:41]

very nice cleanup... and ContentViewCore.java -513 lines, yay!

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1402:
contentViewCore.clearSelection();
just wondering... why do we need to call this here at all?
Why is it ok to always call clearSelection() now?

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(left):

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:526: if
(viewCore != null) viewCore.hideSelectActionMode();
How does the select action mode get hidden now when you focus the omnibox?

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:400: // Do not show the
context menu.
nit: I'd remove this silly comment which doesn't explain why we do things
differently in debug builds here.

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:34:
// the same context menu code.
Isn't this TODO addressed now or what does WebView still do differently?

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/DEPS (right):

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
components/web_contents_delegate_android/DEPS:2:
"+content/browser/renderer_host/render_widget_host_view_android.h",
Can we avoid the non-public dependency here somehow?

I'm wondering:

What if SelectActionMode moved to ui/android (also see ui/touch_selection)?

Instead of RWHV then ViewAndroid could own the SelectActionMode instance and you
could get to it here through RWHV::GetNativeView() (which is public).

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1233:
select_action_mode_->close();
why does this need a call to close() first while below we just clobber it with
reset()?

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1251: // TODO
can you put a bug number that also explains what's missing/broken?

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1262: }
What about other events we listened to in ContentViewCore.java before your patch
that caused the popups to disappear?

In particular I'm wondering about rotation and then the loss of focus. I'm
guessing that we might have specifically handled the case where the activity
gets paused and some other activity shows on top and partially obscures the web
contents.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/select_action_mode.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/select_action_mode.cc:12: #include
"content/public/browser/render_view_host.h"
render_view_host.h is unused

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/select_action_mode.h (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/select_action_mode.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2668:
if (mWebContents == null || resultCode != Activity.RESULT_OK // Fix this
So why don't we check before we process the text whether it's editable?

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
nit: 2016

[amaralp, 2016-06-24 00:45:50]

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1402:
contentViewCore.clearSelection();
On 2016/06/23 at 23:16:41, sievers wrote:
> just wondering... why do we need to call this here at all?
> Why is it ok to always call clearSelection() now?

The clearSelection() is necessary because if a select action
bar is showing then hitting back will clear the selection thus
causing the bar to also clear (it would also simultaneously 
the contextual search bar). I haven't thought too hard about
many of the subleties of my changes because I wanted to get an
opinion before getting bogged down on the details. But you are
right, this change would break back pressing.

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(left):

https://codereview.chromium.org/2089933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:526: if
(viewCore != null) viewCore.hideSelectActionMode();
On 2016/06/23 at 23:16:41, sievers wrote:
> How does the select action mode get hidden now when you focus the omnibox?

To be honest, I don't understand why this code was ever necessary.
Focusing on the omnibox clears selection which in turn hides the
select action mode. There might be some edge case that I'm not 
considering though.

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:400: // Do not show the
context menu.
On 2016/06/23 at 23:16:41, sievers wrote:
> nit: I'd remove this silly comment which doesn't explain why we do things
differently in debug builds here.

Done.

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:34:
// the same context menu code.
On 2016/06/23 at 23:16:41, sievers wrote:
> Isn't this TODO addressed now or what does WebView still do differently?

I don't understand what this TODO means. Previously WebView just omitted 
the else statement. My understanding is that WebView doesn't ever create
this type of menu. I don't know where the Populator/delegate would be used.

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/DEPS (right):

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
components/web_contents_delegate_android/DEPS:2:
"+content/browser/renderer_host/render_widget_host_view_android.h",
On 2016/06/23 at 23:16:41, sievers wrote:
> Can we avoid the non-public dependency here somehow?
> 
> I'm wondering:
> 
> What if SelectActionMode moved to ui/android (also see ui/touch_selection)?
> 
> Instead of RWHV then ViewAndroid could own the SelectActionMode instance and
you could get to it here through RWHV::GetNativeView() (which is public).

I also didn't like the non-public dependency, but I'm not sure I understand your
suggestion. Isn't ViewAndroid an abstract class? What subclass would
implement it once ContentViewCore is gone?

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1233:
select_action_mode_->close();
On 2016/06/23 at 23:16:41, sievers wrote:
> why does this need a call to close() first while below we just clobber it with
reset()?

Good point. I previously had this implemented in Java so I had to close and set
it to
null, but now I could use the destructor.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1251: // TODO
On 2016/06/23 at 23:16:41, sievers wrote:
> can you put a bug number that also explains what's missing/broken?

This is just something for myself so that I don't forget. I don't
plan on uploading this without the insertion handle tapped logic.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1262: }
On 2016/06/23 at 23:16:41, sievers wrote:
> What about other events we listened to in ContentViewCore.java before your
patch that caused the popups to disappear?
> 
> In particular I'm wondering about rotation and then the loss of focus. I'm
guessing that we might have specifically handled the case where the activity
gets paused and some other activity shows on top and partially obscures the web
contents.

You are right. I'll have to take a look at those more carefully.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/select_action_mode.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/select_action_mode.cc:12: #include
"content/public/browser/render_view_host.h"
On 2016/06/23 at 23:16:41, sievers wrote:
> render_view_host.h is unused

Nice catch.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/select_action_mode.h (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/select_action_mode.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2016/06/23 at 23:16:41, sievers wrote:
> nit: 2016

Done

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2668:
if (mWebContents == null || resultCode != Activity.RESULT_OK // Fix this
On 2016/06/23 at 23:16:41, sievers wrote:
> So why don't we check before we process the text whether it's editable?

Yeah we probably should. I removed as much as possible from ContentViewCore
and probably broke a lot of stuff. I plan on going back to check in more detail.

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java
(right):

https://codereview.chromium.org/2089933002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2016/06/23 at 23:16:41, sievers wrote:
> nit: 2016

Done.

[aelias_OOO_until_Jul13, 2016-06-24 21:37:29]

https://codereview.chromium.org/2089933002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java
(right):

https://codereview.chromium.org/2089933002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectActionMode.java:24:
private void show() {
So I assume this patch only compiles but doesn't work yet because methods like
this have empty implementations, and I don't see the Blink change mentioned in
the description?

At a glance, I have no particular problem to any part of this patch, but I
prefer to do detailed code review only on working changes, so please ping again
when you have something that doesn't break any behavior (to your knowledge). 
Also, in the future, if you have a nonworking WIP, please specify that it
doesn't work in the patch description since that's different from the default
assumption (more commonly, WIPs work but are factored ugly -- this is the
reverse :).

[no sievers, 2016-06-24 22:17:25]

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://codereview.chromium.org/2089933002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:34:
// the same context menu code.
On 2016/06/24 00:45:49, amaralp wrote:
> On 2016/06/23 at 23:16:41, sievers wrote:
> > Isn't this TODO addressed now or what does WebView still do differently?
> 
> I don't understand what this TODO means. Previously WebView just omitted 
> the else statement. My understanding is that WebView doesn't ever create
> this type of menu. I don't know where the Populator/delegate would be used.

Ok I'd just remove the comment then. It seems like the code would be unified
enough now.

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
File components/web_contents_delegate_android/DEPS (right):

https://codereview.chromium.org/2089933002/diff/20001/components/web_contents...
components/web_contents_delegate_android/DEPS:2:
"+content/browser/renderer_host/render_widget_host_view_android.h",
On 2016/06/24 00:45:49, amaralp wrote:
> On 2016/06/23 at 23:16:41, sievers wrote:
> > Can we avoid the non-public dependency here somehow?
> > 
> > I'm wondering:
> > 
> > What if SelectActionMode moved to ui/android (also see ui/touch_selection)?
> > 
> > Instead of RWHV then ViewAndroid could own the SelectActionMode instance and
> you could get to it here through RWHV::GetNativeView() (which is public).
> 
> I also didn't like the non-public dependency, but I'm not sure I understand
your
> suggestion. Isn't ViewAndroid an abstract class? What subclass would
> implement it once ContentViewCore is gone?

Feel free to add view_android.cc. I was going to do that anyways with
crbug.com/622847.

Otherwise you could also override the implementation through CVC in the
meantime.
You could even delegate the implementation from ViewAndroid to its delegate
(ViewAndroidDelagate?) which should be then implemented by RWHVAndroid. I was
going to add that delegate interface also for aforementioned bug.

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2089933002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1262: }
On 2016/06/24 00:45:49, amaralp wrote:
> On 2016/06/23 at 23:16:41, sievers wrote:
> > What about other events we listened to in ContentViewCore.java before your
> patch that caused the popups to disappear?
> > 
> > In particular I'm wondering about rotation and then the loss of focus. I'm
> guessing that we might have specifically handled the case where the activity
> gets paused and some other activity shows on top and partially obscures the
web
> contents.
> 
> You are right. I'll have to take a look at those more carefully.

Note that we know about visibility (host_->is_hidden()) and focus here
(Focus()/Blur()). And one way to detect orientation changes could be from
WasResized() and then looking at GetOrientationTypeForMobile().

We can similarly make available what's needed if you end up moving some stuff to
ViewAndroid as in crbug.com/622847 there.

[no sievers, 2016-06-28 00:33:49]

I've started a bug for how to configure the select action mode over here:
crbug.com/623783.

We can see which parts of that refactor help make this patch simpler (since like
you say in your description also: you have a better hook now at creation time to
configure things based on the content and can even let the embedder override
behavior there too) or what we could do as a follow up instead.
But it really seems convoluted and confusing of how this all came together over
time (see bug, took a bit to figure out what's even going on here).

[no sievers, 2016-06-28 00:34:18]

Description was changed from

==========
Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles
being shown in ContentViewCore.java. This context menu is then populated
according to ContentViewCore's current state of the text (i.e. don't show "Copy"
or "Cut" options if text is a password). I believe this state is mostly kept up
to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is
converted to a right click event which triggers blink to show a context menu.
Eventually this gets delegated to either "AwWebContentsViewDelegate" or
"ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView
doesn't support Modal context menus (discussed in #3), but it is also duplicates
code.

3) The Modal context menu has the same path as the Paste menu (#2) up until
ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the
ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in
blink as a context menu. This would make it so a ContextMenuParams argument is
passed during the creation meaning that creating the menu wouldn’t have to rely
on state from updateImeAdapter. This also has the benefit that it unifies the
first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not
only select the text but also send a right click event. Then to avoid WebView
and Chrome code duplication I handle the context menu in
WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add
an extra parameter to this method and hence change many files).
WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the
menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that
communicates to its java counterpart when to show/hide/move/close the menu. 

BUG=598880
==========

to

==========
Context Menu Refactor

Currently there are 3 different ways a context menu is generated:

1) The "Copy, Cut, Paste, ..." context menu is triggered by selection handles
being shown in ContentViewCore.java. This context menu is then populated
according to ContentViewCore's current state of the text (i.e. don't show "Copy"
or "Cut" options if text is a password). I believe this state is mostly kept up
to date through the ContentViewCore's updateImeAdapter method.

2) The "Paste" menu is triggered on long press. The long press event is
converted to a right click event which triggers blink to show a context menu.
Eventually this gets delegated to either "AwWebContentsViewDelegate" or
"ChromeWebContentsViewDelegateAndroid." I believe this is done because WebView
doesn't support Modal context menus (discussed in #3), but it is also duplicates
code.

3) The Modal context menu has the same path as the Paste menu (#2) up until
ChromeWebContentsViewDelegateAndroid. There ShowContextMenu checks the
ContextMenuParams to see if a Modal or Paste Menu is appropriate.

The goal of the CL is to make the first type of context menu be triggered in
blink as a context menu. This would make it so a ContextMenuParams argument is
passed during the creation meaning that creating the menu wouldn’t have to rely
on state from updateImeAdapter. This also has the benefit that it unifies the
first and second context menu types.

My plan to accomplish this is to have long pressing or double tapping text not
only select the text but also send a right click event. Then to avoid WebView
and Chrome code duplication I handle the context menu in
WebContentsDelegateAndroid::HandleContextMenu (to get this to work I had to add
an extra parameter to this method and hence change many files).
WebContentsDelegateAndroid then notifies RenderWidgetHostViewAndroid to show the
menu. RenderWidgetHostViewAndroid creates a SelectActionMode object that
communicates to its java counterpart when to show/hide/move/close the menu. 

BUG=598880,623783
==========