[Android WebView] Convert context menu callback to long press

[Android WebView] Convert context menu callback to long press

A GestureLongPress is converted to a context menu callback inside WebKit. For Android WebView, use this callback to trigger the long click logic which by default triggers context menu.

BUG=
Android only change. Ran through android trybots.
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172457

[boliu, 2012-12-07 22:11:44]

Uploaded for initial code review, not ready to submit yet.

Still needs to check that this is a good idea. And there is a DEPS violation.

[joth, 2012-12-08 01:29:12]

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
File android_webview/browser/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
android_webview/browser/aw_web_contents_view_delegate.cc:30: // press.
also may want to do the same content_view_core->ShowPastePopup() code here as
ChromeWebContentsViewDelegateAndroid

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
android_webview/browser/aw_web_contents_view_delegate.cc:34: return;
it feels like this class could be implemented in the aw/native layer. Then just
need a factory function to create an instance of this, that is visible to
AwContentBrowserClient

https://codereview.chromium.org/11471040/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11471040/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:835:
mContentsClient.onLongPress();
mContainerView.performLongClick should do you here

https://codereview.chromium.org/11471040/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11471040/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.h:67: void
ConvertContextMenuCallbackToLongPress();
I'd prefer the name to either reflect when it gets called
("handleShowContextMenu") or describe what happens when you call it
("SendPerformLongClickToView"), rather than try and capture both. Otherwise a
change in the policy needs propagating around to lots of folks who don't
actually care.

If we plumbed mContainerView through to AwWebContentsViewDelegate c'tor (which
thinking about it, seems a very logical relationship to hold) then we'd not need
to send this method call via here at all.

[boliu, 2012-12-10 20:19:28]

Still needs comments in general and still haven't checked webkit logic yet.

Adding a Java AwWebContentsViewDelegate that simply holds a mContainerView. Java
object owned by native.

The java object cannot be created in native object constructor because it is
called before AwContents::SetWebContents so cannot retrieve the AwContents from
WebContents. So doing a lazy construction when needed. Also it has to go through
AwContents since ContainerView is private.

And all this to plumb one call through...

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
File android_webview/browser/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
android_webview/browser/aw_web_contents_view_delegate.cc:30: // press.
On 2012/12/08 01:29:12, joth wrote:
> also may want to do the same content_view_core->ShowPastePopup() code here as
> ChromeWebContentsViewDelegateAndroid

Done.

https://codereview.chromium.org/11471040/diff/1/android_webview/browser/aw_we...
android_webview/browser/aw_web_contents_view_delegate.cc:34: return;
On 2012/12/08 01:29:12, joth wrote:
> it feels like this class could be implemented in the aw/native layer. Then
just
> need a factory function to create an instance of this, that is visible to
> AwContentBrowserClient

Done.

https://codereview.chromium.org/11471040/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11471040/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:835:
mContentsClient.onLongPress();
On 2012/12/08 01:29:12, joth wrote:
> mContainerView.performLongClick should do you here

Done.

https://codereview.chromium.org/11471040/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11471040/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.h:67: void
ConvertContextMenuCallbackToLongPress();
On 2012/12/08 01:29:12, joth wrote:
> I'd prefer the name to either reflect when it gets called
> ("handleShowContextMenu") or describe what happens when you call it
> ("SendPerformLongClickToView"), rather than try and capture both. Otherwise a
> change in the policy needs propagating around to lots of folks who don't
> actually care.
> 
> If we plumbed mContainerView through to AwWebContentsViewDelegate c'tor (which
> thinking about it, seems a very logical relationship to hold) then we'd not
need
> to send this method call via here at all.

Done.

[boliu, 2012-12-10 21:00:06]

webkit logic seems pretty straight forward:

If it hits a scroll bar, let scrollbar handle it
If it hits a node with touch handler, let node handle it
If it's inside a text edit, select word
Lastly, context menu logic (which we convert back to long press)

The logic for filtering other types of inappropriate events that could trigger
context menu happens at beginning of
ContextMenuClientImpl::getCustomMenuFromDefaultItems. So in fact, only user
input events can trigger the context menu logic back to chromium code.

So currently it's safe use this logic, but not sure how stable this is.

[boliu, 2012-12-10 23:56:06]

Ready for review

[joth, 2012-12-11 01:22:11]

lg, but the global ref may be defeating GC.

https://codereview.chromium.org/11471040/diff/11001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsViewDelegate.java
(right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsViewDelegate.java:14:
* context menu callback currently.
nit: delete second sentence. (too easy to go out of date)
what would be useful is to add a note on ownership model. native counterpart is
owned by the underlying WebContents, this java side is retained so long as the
associated AwContents instance is valid.
(?)

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
android_webview/native/aw_web_contents_view_delegate.cc:50: }
this code is identical to ChromeWebContentsViewDelegateAndroid::ShowContextMenu
(which, incidentally, also has obsolete comments about being in webview mode and
handling tab == NULL which we could remove at the same time)

it totally feels like this code should be factored out somehow, perhaps even
into the caller (WebContentsImpl::ShowContextMenu) but I don't see any sensible
way to do that so suggest you add a counter-part comment here that this is
duplicating that other method and if the logic here grows we should think about
some way of sharing it.

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
File android_webview/native/aw_web_contents_view_delegate.h (right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
android_webview/native/aw_web_contents_view_delegate.h:38:
base::android::ScopedJavaGlobalRef<jobject> web_contents_view_delegate_;
ah. hmm. this maybe a problem: this will stop the java side getting GC'd so long
as the native object exists. Snag is the java side object holds a ref to the
ViewGroup instance, which will stop the WebView being GC'ed, and hence the
WebContents would never get deleted. leak.

Solution: use a weak ref here, and have a java-side object (AwContents, if no
one else) hold a ref to the AwWebContentsViewDelegate java instance to keep in
valid in memory, which the GC can then correctly spot unreachable cycles
through.

(Generalized rule: always use a weak ref from c++ to java unless you know for
sure you shouldn't)

https://codereview.chromium.org/11471040/diff/13003/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11471040/diff/13003/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:69: return
CreateAwWebContentsViewDelegate(web_contents);
this works ok.. but rather than call straight out to a static function here, it
would be better to have a factory {interface|callback} passed into the
AwContentBrowserClient constructor (as aw/lib can see all layers).
(AwContentBrowserClient could be a straight member of AwMainDelegate now, no
need for the lazy instance in there)

callback or interface: either have it as an abstract base class, or typedef for
a Callback<WebContentsViewDelegate*(WebContents*)>. either seems ok to me.  you
can just declare the abstract interface/callback in in
aw_content_browser_client.h, no need for the factory header.

There's a risk that such an injected factory will just grow to replicate most of
the AwContentBrowserClient interface at which point we'll conclude 
AwContentBrowserClient should just have been defined in the native layer
anyway...
keeping it as a focused WebContentsViewDelegateFactory would reduce chance of
that.

[boliu, 2012-12-11 02:25:33]

https://codereview.chromium.org/11471040/diff/11001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsViewDelegate.java
(right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsViewDelegate.java:14:
* context menu callback currently.
On 2012/12/11 01:22:11, joth wrote:
> nit: delete second sentence. (too easy to go out of date)
> what would be useful is to add a note on ownership model. native counterpart
is
> owned by the underlying WebContents, this java side is retained so long as the
> associated AwContents instance is valid.
> (?)

Done.

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
android_webview/native/aw_web_contents_view_delegate.cc:50: }
On 2012/12/11 01:22:11, joth wrote:
> this code is identical to
ChromeWebContentsViewDelegateAndroid::ShowContextMenu
> (which, incidentally, also has obsolete comments about being in webview mode
and
> handling tab == NULL which we could remove at the same time)
> 
> it totally feels like this code should be factored out somehow, perhaps even
> into the caller (WebContentsImpl::ShowContextMenu) but I don't see any
sensible
> way to do that so suggest you add a counter-part comment here that this is
> duplicating that other method and if the logic here grows we should think
about
> some way of sharing it.

Added a TODO

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
File android_webview/native/aw_web_contents_view_delegate.h (right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
android_webview/native/aw_web_contents_view_delegate.h:38:
base::android::ScopedJavaGlobalRef<jobject> web_contents_view_delegate_;
That dependency graph feels weird...does this work?:

Have java AwWebContentsViewDelegate hold a WeakReference<ViewGroup> instead?
Then we still have the native owns java, but don't have the dependency that
prevents gc?

https://codereview.chromium.org/11471040/diff/13003/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11471040/diff/13003/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:69: return
CreateAwWebContentsViewDelegate(web_contents);
On 2012/12/11 01:22:11, joth wrote:
> this works ok.. but rather than call straight out to a static function here,
it
> would be better to have a factory {interface|callback} passed into the
> AwContentBrowserClient constructor (as aw/lib can see all layers).
> (AwContentBrowserClient could be a straight member of AwMainDelegate now, no
> need for the lazy instance in there)
> 
> callback or interface: either have it as an abstract base class, or typedef
for
> a Callback<WebContentsViewDelegate*(WebContents*)>. either seems ok to me. 
you
> can just declare the abstract interface/callback in in
> aw_content_browser_client.h, no need for the factory header.
> 
> There's a risk that such an injected factory will just grow to replicate most
of
> the AwContentBrowserClient interface at which point we'll conclude 
> AwContentBrowserClient should just have been defined in the native layer
> anyway...
> keeping it as a focused WebContentsViewDelegateFactory would reduce chance of
> that. 

Choosing callback since it's easier.

One snag in making AwContentBrowserClient an straight instance variable of
AwMainDelegate is that it causes the error:

F/chromium(20632): [1210/181208:FATAL:jni_android.cc(133)] Check failed:
!g_application_context.Get().is_null().

ie GetApplicationContext is called before InitApplicationContext. No useful
stack trace from master. Solution for now is to use scoped_ptr and create it
lazily...

[joth, 2012-12-11 02:37:39]

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
File android_webview/native/aw_web_contents_view_delegate.h (right):

https://codereview.chromium.org/11471040/diff/11001/android_webview/native/aw...
android_webview/native/aw_web_contents_view_delegate.h:38:
base::android::ScopedJavaGlobalRef<jobject> web_contents_view_delegate_;
On 2012/12/11 02:25:33, boliu wrote:
> That dependency graph feels weird...does this work?:
> 
yeah, it feels weird -- par for the course in mixing GCed and non-GCed languages
it seems...

do you have evidence it won't work?

> Have java AwWebContentsViewDelegate hold a WeakReference<ViewGroup> instead?
> Then we still have the native owns java, but don't have the dependency that
> prevents gc?

Experience is, this is a loosing battle: you'll need a and a Bad-Ass comment on
AwWebContentsViewDelegate.java explaining why it's only holding WeakRefs and why
new references must be too, but still some new weird case will come up (some one
subclasses it? Someone makes an anon inner class bound to both the outer class
and to something else?) that thwarts the GC anyway.

so, yeah, we're moving to a point where all the native objects are owned by
WebContents, all java objects held valid (indirectly) by the WebView instance,
and there's a single point that has finalizer to cleanup the WebContents (and
it's tree of objects) when the java-side web disappear.

https://codereview.chromium.org/11471040/diff/8034/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/11471040/diff/8034/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.h:17: typedef
content::WebContentsViewDelegate* ViewDelegateFactory(
suggest ViewDelegateFactoryFunction (or sometimes I've seen/used FactoryFn)

.. my patterns brain always thinks of a FooFactory as a class type.

[boliu, 2012-12-11 19:34:08]

Per offline discussion, reverted to old path (through aw_contents in patch set
1) to avoid creating an additional java class and weird ownership/gc issues.

https://codereview.chromium.org/11471040/diff/8034/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/11471040/diff/8034/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.h:17: typedef
content::WebContentsViewDelegate* ViewDelegateFactory(
On 2012/12/11 02:37:39, joth wrote:
> suggest ViewDelegateFactoryFunction (or sometimes I've seen/used FactoryFn)
> 
> .. my patterns brain always thinks of a FooFactory as a class type.

FactoryFn.

[joth, 2012-12-11 21:11:47]

lgtm % nits. (and what's that commented out line?)

https://codereview.chromium.org/11471040/diff/18001/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:70: return NULL; 
//(*view_delegate_factory_)(web_contents);
??

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:67: if (!content_browser_client_) {
I *think* this check is not needed. (you can DCHECK instead if you like).

see the version in  ash/shell/content_client/shell_main_delegate.cc

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:77: return
&g_webview_content_renderer_client.Get();
patch creep: wanna switch this one to be a member too while you're at it?

https://codereview.chromium.org/11471040/diff/18001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/native/aw...
android_webview/native/aw_contents.h:35: class AwWebContentsViewDelegate;
unused? remove?

[boliu, 2012-12-11 22:13:09]

https://codereview.chromium.org/11471040/diff/18001/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:70: return NULL; 
//(*view_delegate_factory_)(web_contents);
On 2012/12/11 21:11:47, joth wrote:
> ??

Oops...the commented code does indeed work though :)

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:67: if (!content_browser_client_) {
On 2012/12/11 21:11:47, joth wrote:
> I *think* this check is not needed. (you can DCHECK instead if you like).
> 
> see the version in  ash/shell/content_client/shell_main_delegate.cc

does the ash shell support multiple web_contents? Anyway, I guess it should be
safe. Let's see what the trybots say.

https://codereview.chromium.org/11471040/diff/18001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:77: return
&g_webview_content_renderer_client.Get();
On 2012/12/11 21:11:47, joth wrote:
> patch creep: wanna switch this one to be a member too while you're at it?

Done.

https://codereview.chromium.org/11471040/diff/18001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11471040/diff/18001/android_webview/native/aw...
android_webview/native/aw_contents.h:35: class AwWebContentsViewDelegate;
On 2012/12/11 21:11:47, joth wrote:
> unused? remove? 

Done.

[joth, 2012-12-11 22:15:30]

lgtm

https://codereview.chromium.org/11471040/diff/27001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.h (right):

https://codereview.chromium.org/11471040/diff/27001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.h:10: #include
"android_webview/renderer/aw_content_renderer_client.h"
nit: fwd declare AwContentBrowserClient & RenderClient rather than include
headers in headers

[boliu, 2012-12-11 22:36:46]

https://codereview.chromium.org/11471040/diff/27001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.h (right):

https://codereview.chromium.org/11471040/diff/27001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.h:10: #include
"android_webview/renderer/aw_content_renderer_client.h"
On 2012/12/11 22:15:31, joth wrote:
> nit: fwd declare AwContentBrowserClient & RenderClient rather than include
> headers in headers

Done. Left over from first using straight up instance variable, and then not
thinking the second time.

[commit-bot: I haz the power, 2012-12-11 23:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11471040/37001

[commit-bot: I haz the power, 2012-12-11 23:42:24]

Change committed as 172457