android_webview: Let AwContents manage TouchHandleDrawable

android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977
Committed: https://crrev.com/5356a1399fdc2e940db8553788ed2c540e5abe68
Cr-Commit-Position: refs/heads/master@{#415542}

[Jinsuk Kim, 2016-08-22 07:00:49]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org, sievers@chromium.org

[Jinsuk Kim, 2016-08-22 07:00:49]

Bo/Daniel I haven't fully tested this yet, but could I have your initial review
on how the new way TouchHandleDrawble gets notified of container view switching
in this CL without using ContainerViewObserver through ContentViewCore?

[boliu, 2016-08-22 18:10:47]

Feels like PopupTouchHandleDrawable should be moved to android_webview instead.
Looks should be pretty easy?

If you don't want to go through WebContentsImpl/WebContentsObserver, another
option is go through SynchronousCompositorClient. I don't care much either way
though.

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

https://codereview.chromium.org/2263043002/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents.h:346: void
DidCreateTouchHandle(ui::TouchHandleDrawable* drawable) override;
no blank line above

[Jinsuk Kim, 2016-08-23 06:39:02]

On 2016/08/22 18:10:47, boliu wrote:
> Feels like PopupTouchHandleDrawable should be moved to android_webview
instead.
> Looks should be pretty easy?

PopupTouchHandleDrawable::Create() is now called inside RWHVA at
|CreateDrawable()|.
For PopupTouchHandleDrawable to be moved to android_webview, the creation
should be delegated to aw_contents. I haven't figured out what would be the best
way to do that. Adding a new delegate doesn't seem like a good choice... 

An idea I have now is for aw_contents, when notified via WebContentsObserver,
to create a PopupTouchHandleDrawable instance and pass it to RWHVA.

[Jinsuk Kim, 2016-08-23 06:41:33]

Thanks for the feedback.

https://codereview.chromium.org/2263043002/diff/1/content/public/browser/andr...
File content/public/browser/android/popup_touch_handle_drawable.h (right):

https://codereview.chromium.org/2263043002/diff/1/content/public/browser/andr...
content/public/browser/android/popup_touch_handle_drawable.h:38: const
base::android::ScopedJavaLocalRef<jobject>
I guess I should return JavaObjectWeakGlobalRef, right?

[Jinsuk Kim, 2016-08-23 07:02:52]

On 2016/08/23 06:39:02, Jinsuk wrote:
> On 2016/08/22 18:10:47, boliu wrote:
> > Feels like PopupTouchHandleDrawable should be moved to android_webview
> instead.
> > Looks should be pretty easy?
> 
> PopupTouchHandleDrawable::Create() is now called inside RWHVA at
> |CreateDrawable()|.
> For PopupTouchHandleDrawable to be moved to android_webview, the creation
> should be delegated to aw_contents. I haven't figured out what would be the
best
> way to do that. Adding a new delegate doesn't seem like a good choice... 
> 
> An idea I have now is for aw_contents, when notified via WebContentsObserver,
> to create a PopupTouchHandleDrawable instance and pass it to RWHVA.

Actually this wouldn't work :/
Now I'm thinking along the line of having an independent instance of
ui::TouchSelectionControllerClient so that it can be more accessible to
aw_contents.

[boliu, 2016-08-23 15:02:29]

On 2016/08/23 07:02:52, Jinsuk wrote:
> On 2016/08/23 06:39:02, Jinsuk wrote:
> > On 2016/08/22 18:10:47, boliu wrote:
> > > Feels like PopupTouchHandleDrawable should be moved to android_webview
> > instead.
> > > Looks should be pretty easy?
> > 
> > PopupTouchHandleDrawable::Create() is now called inside RWHVA at
> > |CreateDrawable()|.
> > For PopupTouchHandleDrawable to be moved to android_webview, the creation
> > should be delegated to aw_contents. I haven't figured out what would be the
> best
> > way to do that. Adding a new delegate doesn't seem like a good choice... 

I already suggested SynchronousCompositorClient

> > 
> > An idea I have now is for aw_contents, when notified via
WebContentsObserver,
> > to create a PopupTouchHandleDrawable instance and pass it to RWHVA.
> 
> Actually this wouldn't work :/

Why not?

> Now I'm thinking along the line of having an independent instance of
> ui::TouchSelectionControllerClient so that it can be more accessible to
> aw_contents.

[boliu, 2016-08-23 15:03:26]

https://codereview.chromium.org/2263043002/diff/1/content/public/browser/andr...
File content/public/browser/android/popup_touch_handle_drawable.h (right):

https://codereview.chromium.org/2263043002/diff/1/content/public/browser/andr...
content/public/browser/android/popup_touch_handle_drawable.h:38: const
base::android::ScopedJavaLocalRef<jobject>
On 2016/08/23 06:41:33, Jinsuk wrote:
> I guess I should return JavaObjectWeakGlobalRef, right?

Yeah

[Jinsuk Kim, 2016-08-24 01:48:06]

On 2016/08/23 15:02:29, boliu wrote:
> On 2016/08/23 07:02:52, Jinsuk wrote:
> > On 2016/08/23 06:39:02, Jinsuk wrote:
> > > On 2016/08/22 18:10:47, boliu wrote:
> > > > Feels like PopupTouchHandleDrawable should be moved to android_webview
> > > instead.
> > > > Looks should be pretty easy?
> > > 
> > > PopupTouchHandleDrawable::Create() is now called inside RWHVA at
> > > |CreateDrawable()|.
> > > For PopupTouchHandleDrawable to be moved to android_webview, the creation
> > > should be delegated to aw_contents. I haven't figured out what would be
the
> > best
> > > way to do that. Adding a new delegate doesn't seem like a good choice... 
> 
> I already suggested SynchronousCompositorClient
> 
> > > 
> > > An idea I have now is for aw_contents, when notified via
> WebContentsObserver,
> > > to create a PopupTouchHandleDrawable instance and pass it to RWHVA.
> > 
> > Actually this wouldn't work :/
> 
> Why not?

It needs to return the created drawable synchronously but with
WebContentsObserver in between, it becomes tricky.

I checked out SynchronousCompositorClient and it seems like a good
alternative delegating the creation and returning the drawable. 
Let me go this way. Thanks for the suggestion.

> 
> > Now I'm thinking along the line of having an independent instance of
> > ui::TouchSelectionControllerClient so that it can be more accessible to
> > aw_contents.

[Jinsuk Kim, 2016-08-24 06:34:28]

Description was changed from

==========
Notify TouchHandleDrawable via WebContentsObserver

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in |WebContentsObserver|
to let AwContents manage the handles directly, and notify them
of the event of the container view switching.

BUG=624977
==========

to

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in |WebContentsObserver|
to let AwContents manage the handles directly, and notify them
of the event of the container view switching.

BUG=624977
==========

[Jinsuk Kim, 2016-08-24 06:35:11]

> I checked out SynchronousCompositorClient and it seems like a good
> alternative delegating the creation and returning the drawable. 
> Let me go this way. Thanks for the suggestion.
> 
PTAL

[Jinsuk Kim, 2016-08-24 06:36:20]

Description was changed from

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in |WebContentsObserver|
to let AwContents manage the handles directly, and notify them
of the event of the container view switching.

BUG=624977
==========

to

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in 
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977
==========

[boliu, 2016-08-24 23:35:31]

I think it's easier now to create PopupTouchHandleDrawable from Java AwContents,
then java PopupTouchHandleDrawable can create the native side, then there is no
risk of gc issues, and avoids a few jni calls I think.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:704: CHECK_GT(dip_scale_, 0.f);
hmm, curious, does this really need to be a release check?

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:26: #include
"ui/touch_selection/touch_handle.h"
include should not be needed in header, since it must have been forward declared
by the parent already

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:12: #include
"ui/touch_selection/touch_handle.h"
forward declare should be enough here

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2693:
mTouchHandleDrawables.add(drawable);
when are things ever removed from the list?

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
curious, did presubmit complain about updating the year?

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1066: auto obj = java_ref_.get(env);
don't use auto, jni types are scary, but not that long to type

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1069: auto cvc =
ContentViewCore::FromWebContents(web_contents_.get());
ditto here and below, imo auto is making things harder to read, not easier

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1071: auto drawable =
PopupTouchHandleDrawable::Create(cvc, dip_scale);
if gc runs between this line and the line below, then this would break

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

https://codereview.chromium.org/2263043002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1087: return
std::unique_ptr<ui::TouchHandleDrawable>(
don't go through web_contents here. you already have sync_compositor_ var,
either directly get the sync_comopsitor_client, or go through sync_compositor
instead

https://codereview.chromium.org/2263043002/diff/20001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2263043002/diff/20001/content/content_browser...
content/content_browser.gypi:1: 
blank line

[Jinsuk Kim, 2016-08-25 07:32:21]

I don't have a concrete idea how to create PopupTouchHandlDrawable from java and
go down native. Can I have your thought?

I'm inclined to switching return type of call chain |CreateDrawable| from
pointer to std::unique_ptr as it's done in the lower chain. wdyt?

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:704: CHECK_GT(dip_scale_, 0.f);
On 2016/08/24 23:35:30, boliu wrote:
> hmm, curious, does this really need to be a release check?

Nope, meant to use DCHECK_GT like in other places in this class.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:26: #include
"ui/touch_selection/touch_handle.h"
On 2016/08/24 23:35:30, boliu wrote:
> include should not be needed in header, since it must have been forward
declared
> by the parent already

Done.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:12: #include
"ui/touch_selection/touch_handle.h"
On 2016/08/24 23:35:30, boliu wrote:
> forward declare should be enough here

Done.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2693:
mTouchHandleDrawables.add(drawable);
On 2016/08/24 23:35:30, boliu wrote:
> when are things ever removed from the list?

Good catch. Drawable's is ultimately owned by RWHVA through
TouchSelectionController. I may be wrong but I think it is safe to clear the
list when AwContent is destroyed since the elements will be gone by then anyway.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/08/24 23:35:30, boliu wrote:
> curious, did presubmit complain about updating the year?

No presubmit doesn't. Let me know if the year doesn't need to be updated for
files moved over. I have updated it since they are 'new' to the repository.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1066: auto obj = java_ref_.get(env);
On 2016/08/24 23:35:30, boliu wrote:
> don't use auto, jni types are scary, but not that long to type

Done.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1069: auto cvc =
ContentViewCore::FromWebContents(web_contents_.get());
On 2016/08/24 23:35:30, boliu wrote:
> ditto here and below, imo auto is making things harder to read, not easier

Done.

https://codereview.chromium.org/2263043002/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1071: auto drawable =
PopupTouchHandleDrawable::Create(cvc, dip_scale);
On 2016/08/24 23:35:30, boliu wrote:
> if gc runs between this line and the line below, then this would break

Oops did it again.... Done.

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

https://codereview.chromium.org/2263043002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1087: return
std::unique_ptr<ui::TouchHandleDrawable>(
On 2016/08/24 23:35:30, boliu wrote:
> don't go through web_contents here. you already have sync_compositor_ var,
> either directly get the sync_comopsitor_client, or go through sync_compositor
> instead

Done.

https://codereview.chromium.org/2263043002/diff/20001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2263043002/diff/20001/content/content_browser...
content/content_browser.gypi:1: 
On 2016/08/24 23:35:31, boliu wrote:
> blank line

Done.

[boliu, 2016-08-25 20:51:29]

haven't reviewed new changes yet...

On 2016/08/25 07:32:21, Jinsuk wrote:
> I don't have a concrete idea how to create PopupTouchHandlDrawable from java
and
> go down native. Can I have your thought?

native AwContents calls java AwContents to get the native Drawable pointer, java
AwContents creates the java Drawable and at the same time set up the container
view observer thing, java Drawable creates native Drawable, then java AwContents
can return the native Drawable pointer

> 
> I'm inclined to switching return type of call chain |CreateDrawable| from
> pointer to std::unique_ptr as it's done in the lower chain. wdyt?

sounds ok, but maybe in follow up CL, is there a  compelling reason why it
should be in this one?

[Jinsuk Kim, 2016-08-26 01:07:29]

On 2016/08/25 20:51:29, boliu wrote:
> haven't reviewed new changes yet...
> 
> On 2016/08/25 07:32:21, Jinsuk wrote:
> > I don't have a concrete idea how to create PopupTouchHandlDrawable from java
> and
> > go down native. Can I have your thought?
> 
> native AwContents calls java AwContents to get the native Drawable pointer,
java
> AwContents creates the java Drawable and at the same time set up the container
> view observer thing, java Drawable creates native Drawable, then java
AwContents
> can return the native Drawable pointer
> 
Thanks. Updated.

> > 
> > I'm inclined to switching return type of call chain |CreateDrawable| from
> > pointer to std::unique_ptr as it's done in the lower chain. wdyt?
> 
> sounds ok, but maybe in follow up CL, is there a  compelling reason why it
> should be in this one?

Because PopupTouchHandleDrawable::Create was already returning std::unique_ptr,
I thought it not necessary to release the ptr in the middle. But with the latest
patch this situation changed. So not valid anymore. Feel free to ignore.

[boliu, 2016-08-26 04:15:45]

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2263043002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2693:
mTouchHandleDrawables.add(drawable);
On 2016/08/25 07:32:21, Jinsuk wrote:
> On 2016/08/24 23:35:30, boliu wrote:
> > when are things ever removed from the list?
> 
> Good catch. Drawable's is ultimately owned by RWHVA through
> TouchSelectionController. I may be wrong but I think it is safe to clear the
> list when AwContent is destroyed since the elements will be gone by then
anyway.

That's not good enough. There can be an unbounded number of selection handles
created in the lifetime of AwContents.

Sounds like it should be removed when RWHVA destroys the native side?

https://codereview.chromium.org/2263043002/diff/60001/android_webview/browser...
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/browser...
android_webview/browser/DEPS:47: "+ui/touch_selection/touch_handle.h",
I don't think this is needed anymore

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:1066: return
reinterpret_cast<ui::TouchHandleDrawable *>(
no space before *

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
File android_webview/native/popup_touch_handle_drawable.cc (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/popup_touch_handle_drawable.cc:96: if
(!content_view_core.obj())
can we move this check to java side? which means java side should use a static
create method rather than a constructor, so it can return null as well

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/popup_touch_handle_drawable.cc:101: }
blank line after

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

https://codereview.chromium.org/2263043002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1087:
sync_compositor_->client()->CreateDrawable());
need a null check for sync_compositor_ here and return null, ie don't try to
create CompositedTouchHandleDrawable

[Jinsuk Kim, 2016-08-26 09:17:20]

Patchset #5 (id:80001) has been deleted

[Jinsuk Kim, 2016-08-26 09:21:19]

PTAL

https://codereview.chromium.org/2263043002/diff/60001/android_webview/browser...
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/browser...
android_webview/browser/DEPS:47: "+ui/touch_selection/touch_handle.h",
On 2016/08/26 04:15:44, boliu wrote:
> I don't think this is needed anymore

Better. done.

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:1066: return
reinterpret_cast<ui::TouchHandleDrawable *>(
On 2016/08/26 04:15:45, boliu wrote:
> no space before *

Done.

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
File android_webview/native/popup_touch_handle_drawable.cc (right):

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/popup_touch_handle_drawable.cc:96: if
(!content_view_core.obj())
On 2016/08/26 04:15:45, boliu wrote:
> can we move this check to java side? which means java side should use a static
> create method rather than a constructor, so it can return null as well

Done.

https://codereview.chromium.org/2263043002/diff/60001/android_webview/native/...
android_webview/native/popup_touch_handle_drawable.cc:101: }
On 2016/08/26 04:15:45, boliu wrote:
> blank line after

Done.

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

https://codereview.chromium.org/2263043002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1087:
sync_compositor_->client()->CreateDrawable());
On 2016/08/26 04:15:45, boliu wrote:
> need a null check for sync_compositor_ here and return null, ie don't try to
> create CompositedTouchHandleDrawable

Done.

[boliu, 2016-08-26 20:28:02]

https://codereview.chromium.org/2263043002/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:164:
if (contentViewCore == null) return null;
umm, after this is moved to java, it should be clear that this will never happen

https://codereview.chromium.org/2263043002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2263043002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:325:
sync_compositor_->client()->DestroyAllDrawables();
this is equivalent to awcontents.destroy (in most cases)

why can't you just call up from native Drawable destructor to tell java to
remove itself from the observer list?

[Jinsuk Kim, 2016-08-26 22:50:50]

https://codereview.chromium.org/2263043002/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:164:
if (contentViewCore == null) return null;
On 2016/08/26 20:28:02, boliu wrote:
> umm, after this is moved to java, it should be clear that this will never
happen

Done.

https://codereview.chromium.org/2263043002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2263043002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:325:
sync_compositor_->client()->DestroyAllDrawables();
On 2016/08/26 20:28:02, boliu wrote:
> this is equivalent to awcontents.destroy (in most cases)
> 
> why can't you just call up from native Drawable destructor to tell java to
> remove itself from the observer list?

Done. Passed the list to popup handle for it to remove itself on destroy().

[boliu, 2016-08-26 23:00:24]

I meant the list in AwContents..

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2698: if
(drawable == null) return 0L;
you can remove this too

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:100:
private List<PopupTouchHandleDrawable> mDrawableObserver;
wah?

[Jinsuk Kim, 2016-08-26 23:52:44]

PTAL

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2698: if
(drawable == null) return 0L;
On 2016/08/26 23:00:24, boliu wrote:
> you can remove this too

Done.

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2263043002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:100:
private List<PopupTouchHandleDrawable> mDrawableObserver;
On 2016/08/26 23:00:24, boliu wrote:
> wah?

Chatted offline to use ObserverList in place of a plan List to deal with
add/remove while iterating.

[boliu, 2016-08-26 23:56:08]

lgtm

[no sievers, 2016-08-29 23:59:52]

content lgtm!

[Jinsuk Kim, 2016-08-30 00:57:04]

jinsukkim@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[Jinsuk Kim, 2016-08-30 00:57:05]

mfomitchev@ could you review changes in android_webview/native/DEPS? I'd like an
approval for depend-on path added in it.

[mfomitchev, 2016-08-30 15:33:48]

rubber stamp LGTM for android_webview/native/DEPS

[Jinsuk Kim, 2016-08-30 22:50:23]

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

[commit-bot: I haz the power, 2016-08-30 22:51:15]

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, 2016-08-30 23:15:24]

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

[commit-bot: I haz the power, 2016-08-30 23:15:27]

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

[Jinsuk Kim, 2016-08-31 00:00:53]

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

[commit-bot: I haz the power, 2016-08-31 00:01:43]

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, 2016-08-31 00:35:13]

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

[commit-bot: I haz the power, 2016-08-31 00:35:15]

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

[Jinsuk Kim, 2016-08-31 01:15:27]

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

[Jinsuk Kim, 2016-08-31 01:15:33]

Patchset #8 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-08-31 01:15:55]

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, 2016-08-31 02:17:35]

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

[commit-bot: I haz the power, 2016-08-31 02:17:36]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2016-08-31 02:21:23]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-08-31 02:21:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, boliu@chromium.org, mfomitchev@chromium.org
Link to the patchset: https://codereview.chromium.org/2263043002/#ps180001
(title: "fix build")

[commit-bot: I haz the power, 2016-08-31 02:21:47]

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, 2016-08-31 02:26:47]

Description was changed from

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in 
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977
==========

to

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in 
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977
==========

[commit-bot: I haz the power, 2016-08-31 02:26:49]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-08-31 02:28:31]

Description was changed from

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in 
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977
==========

to

==========
android_webview: Let AwContents manage TouchHandleDrawable

WebView's text selection handles(TouchHandleDrawables) should be
notified when their container view gets swapped out to be
repositioned accordingly. This is now being done through
|ContainerViewObserver| in |ContentViewCore| which is slated for
removal as a part of content layer refactoring.

This CL makes use of a new interface method in
|SynchronousCompositorClent| to let AwContents manage the handles
directly, and notify them of the event of the container view switching.

BUG=624977

Committed: https://crrev.com/5356a1399fdc2e940db8553788ed2c540e5abe68
Cr-Commit-Position: refs/heads/master@{#415542}
==========

[commit-bot: I haz the power, 2016-08-31 02:28:33]

Patchset 8 (id:??) landed as
https://crrev.com/5356a1399fdc2e940db8553788ed2c540e5abe68
Cr-Commit-Position: refs/heads/master@{#415542}