[Android] Composited selection handle rendering

[Android] Composited selection handle rendering

Previously, selection handles on Android were rendered using a PopupWindow
construct.  This is problematic for a number of reasons, the chief being poor
performance and responsiveness. As an alternative, support a native, composited
layer rendering pipeline for selection handles using the browser compositor.
Also port selection handle-related logic from Java to C++, using a
PopupWindow rendering fallback for WebView, lacking a proper browser compositor.

This change depends directly on selection update and visibility routing through
the compositor: https://codereview.chromium.org/300323005/

BUG=380781, 135959, 279489, 393948
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284491

[jdduke (slow), 2014-06-30 18:55:52]

Chris, I'd like to start initial review of this change.  I realize it's rather
large, so I'm looking more for high level feedback on the conversion of
InsertionHandleController/SelectionHandleController and HandleView to
TouchSelectionController and TouchHandle. 

For now, feel free to ignore everything but
content/browser/renderer_host/input/touch_handle.{cc,h} and
content/browser/renderer_host/input/touch_selection_controller.{cc,h} and how
they're wired up to RenderWidgetHostViewAndroid (and from there into
ContentViewCore). The rest of the changes are fairly straightforward and I can
get other reviewers to offset the review burden there. 

I still haven't figured out how I'd like to wire up the paste popup showing via
long press.  We have a number of places that trigger this when the context menu
is unhandled, but I'm wondering if there's a simpler way. I also debated porting
the PastePopup code to use a cc layer, but there's really no need as it is
always hidden as soon as the content or insertion point shifts. 

Also of note, I've preserved PopupWindow rendering support for WebView, as
WebView doesn't have a proper browser compositor and there are cases where we
probably want/need the handles to spill out of the view bounds. 

In any case, thanks for (initial =/) review.

[jdduke (slow), 2014-07-07 18:26:57]

Chris, would you like me to find another reviewer?

[cjhopman, 2014-07-07 18:51:08]

On 2014/07/07 18:26:57, jdduke wrote:
> Chris, would you like me to find another reviewer?

No, I'll send my comments soon.

[jdduke (slow), 2014-07-07 19:00:38]

On 2014/07/07 18:51:08, cjhopman wrote:
> On 2014/07/07 18:26:57, jdduke wrote:
> > Chris, would you like me to find another reviewer?
> 
> No, I'll send my comments soon.

Great, thank you sir (and again, no rush, particularly as this is holiday
season).

[cjhopman, 2014-07-07 22:13:41]

High level: this looks great.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:183:
drawable_->SetVisible(is_visible_);
Should this call drawable_->SetFocus(position_)? When fading out position
updates aren't set to the drawable and so these would be out of sync at this
point (though it probably doesn't really matter since it isn't visible).

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.h (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:85: TouchHandleOrientation
deferred_orientation_;
A comment somewhere that orientation changes while dragging are deferred until
dragging finishes would be helpful. Either here or on SetOrientation().

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:90: base::TimeTicks
touch_timer_;
Maybe make a note about what things might get out of sync between this and the
drawable during a fade. These would sort of be the things that then need to be
updated in CancelFade(). It looks like this would be is_visible_ and (sometimes)
position_.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:59: if
(anchor.x() != focus.x() || anchor.y() != focus.y() ||
anchor != focus

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:60:
(start_selection_handle_ && start_selection_handle_->is_dragging()) ||
Can this just use is_selection_active_ rather than checking that the handles are
not null? It's not exactly the same as the handle can exist when not active, but
they shouldn't ever be dragging if not active.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:64: if
(anchor_orientation_ == TOUCH_HANDLE_CENTER && start_selection_handle_)
AIUI, if anchor_orientation_ == TOUCH_HANDLE_CENTER, then
start_selection_handle_ must not be null since one of the selection handles is
being dragged. This block might be clearer if you checked for selection dragging
+ fixed up orientation outside this condition. Something like:

if (check_for_selection_dragging) {
  is_selection_dragging = true
  fixup orientation
}

if (anchor != focus || is_selection_dragging) {
  OnSelectionChanged();
  return;
}

idk if that's any better.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.h:41: void
OnSelectionBoundsChanged(const gfx::RectF& anchor_rect,
based on the naming (all the way into WebView::selectionBounds), this returns
the anchor/focus, but it looks like it actually returns the start/end...

This really confused me, because you were using it (correctly) as though it is
the start/end. The names for these parameters should probably be fixed to
start/end all the way to WebView::selectionBounds (I think it's probably easier
to deal with start/end than anchor/focus).

One thing to note about that, though, is that during a selection drag, the fixed
handle can switch between being the start and being the end (whereas it is
always the 'anchor').

This implementation (and the java one), then have the unintuitive behavior that
the handle receiving move events may not be the handle underneath the finger
(when the end handle is dragged past the start handle, the start handle begins
moving and the end handle stops, even though the end handle is the one handling
the move events). "Fixing" this requires swapping the orientation of the handles
during the drag and I could never get that to actually look good with the
PopupWindows (and I don't know that "fixing" it would be any better).

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

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1207:
selection_controller_->AllowAutomaticInsertionShowing();
It appears that this is the main bit of logic about when to show
insertion/selection handles that lives outside of the
touch_selection_controller. How would you feel about moving this there?

https://codereview.chromium.org/335943002/diff/160001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java
(right):

https://codereview.chromium.org/335943002/diff/160001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:92:
if (!mAllowAutomaticShowing) return;
I don't like that showAt() sometimes doesn't and I think I would prefer that
this logic were more explicit in ContentViewCore. The current way makes it
harder for me to figure out when we actually show a paste popup.

[jdduke (slow), 2014-07-08 00:54:45]

Thanks for review!

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:183:
drawable_->SetVisible(is_visible_);
On 2014/07/07 22:13:40, cjhopman wrote:
> Should this call drawable_->SetFocus(position_)? When fading out position
> updates aren't set to the drawable and so these would be out of sync at this
> point (though it probably doesn't really matter since it isn't visible).

Yeah, may not matter but probably best for consistency.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.h (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:85: TouchHandleOrientation
deferred_orientation_;
On 2014/07/07 22:13:41, cjhopman wrote:
> A comment somewhere that orientation changes while dragging are deferred until
> dragging finishes would be helpful. Either here or on SetOrientation().

Good call, added some basic documentation to each of the public methods.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:90: base::TimeTicks
touch_timer_;
On 2014/07/07 22:13:41, cjhopman wrote:
> Maybe make a note about what things might get out of sync between this and the
> drawable during a fade. These would sort of be the things that then need to be
> updated in CancelFade(). It looks like this would be is_visible_ and
(sometimes)
> position_.

Done.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:59: if
(anchor.x() != focus.x() || anchor.y() != focus.y() ||
On 2014/07/07 22:13:41, cjhopman wrote:
> anchor != focus

Done.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:60:
(start_selection_handle_ && start_selection_handle_->is_dragging()) ||
On 2014/07/07 22:13:41, cjhopman wrote:
> Can this just use is_selection_active_ rather than checking that the handles
are
> not null? It's not exactly the same as the handle can exist when not active,
but
> they shouldn't ever be dragging if not active.

Done.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:64: if
(anchor_orientation_ == TOUCH_HANDLE_CENTER && start_selection_handle_)
On 2014/07/07 22:13:41, cjhopman wrote:
> AIUI, if anchor_orientation_ == TOUCH_HANDLE_CENTER, then
> start_selection_handle_ must not be null since one of the selection handles is
> being dragged. This block might be clearer if you checked for selection
dragging
> + fixed up orientation outside this condition. Something like:
> 
> if (check_for_selection_dragging) {
>   is_selection_dragging = true
>   fixup orientation
> }
> 
> if (anchor != focus || is_selection_dragging) {
>   OnSelectionChanged();
>   return;
> }
> 
> idk if that's any better.

It's definitely better, good call.

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.h:41: void
OnSelectionBoundsChanged(const gfx::RectF& anchor_rect,
On 2014/07/07 22:13:41, cjhopman wrote:
> based on the naming (all the way into WebView::selectionBounds), this returns
> the anchor/focus, but it looks like it actually returns the start/end...
> 
> This really confused me, because you were using it (correctly) as though it is
> the start/end. The names for these parameters should probably be fixed to
> start/end all the way to WebView::selectionBounds (I think it's probably
easier
> to deal with start/end than anchor/focus).
> 
> One thing to note about that, though, is that during a selection drag, the
fixed
> handle can switch between being the start and being the end (whereas it is
> always the 'anchor').
> 
> This implementation (and the java one), then have the unintuitive behavior
that
> the handle receiving move events may not be the handle underneath the finger
> (when the end handle is dragged past the start handle, the start handle begins
> moving and the end handle stops, even though the end handle is the one
handling
> the move events). "Fixing" this requires swapping the orientation of the
handles
> during the drag and I could never get that to actually look good with the
> PopupWindows (and I don't know that "fixing" it would be any better).

Sigh, yeah, it took me a while to figure out what was going on, and by then I
already had most of the naming wired up.  I'll go back and change everything to
use start/end for consistency (https://codereview.chromium.org/379473002/).

I played around with doing some swapping when the relative handle positioning
changes , but I never really arrived at anything better than the current
approach other than allowing the "fixed" handle orientation to be updated while
dragging.

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

https://codereview.chromium.org/335943002/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1207:
selection_controller_->AllowAutomaticInsertionShowing();
On 2014/07/07 22:13:41, cjhopman wrote:
> It appears that this is the main bit of logic about when to show
> insertion/selection handles that lives outside of the
> touch_selection_controller. How would you feel about moving this there?

I actually had it there originally.  I moved it out in speculation that we might
be moving TouchSelectionController/TouchHandle to ui/ for use with Aura
(preventing the dependency on blink::WebGestureEvent), but perhaps that was
premature. I'd move it back in, but it looks like we still need a hook to allow
showing the selection handles automatically for SPen selection (without
preceding gestures).

[jdduke (slow), 2014-07-08 01:35:42]

https://codereview.chromium.org/335943002/diff/160001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java
(right):

https://codereview.chromium.org/335943002/diff/160001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:92:
if (!mAllowAutomaticShowing) return;
On 2014/07/07 22:13:41, cjhopman wrote:
> I don't like that showAt() sometimes doesn't and I think I would prefer that
> this logic were more explicit in ContentViewCore. The current way makes it
> harder for me to figure out when we actually show a paste popup.
> 

Done.

[cjhopman, 2014-07-09 22:29:12]

https://codereview.chromium.org/335943002/diff/210001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/android...
content/browser/android/content_view_core_impl.cc:805: if (anchor.x() ==
focus.x() && anchor.y() == focus.y())
nit: if (anchor == focus) {

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:150: bool
should_recycle)
So this constructor makes like ~16 jni calls (and was private to make you use
the one that doesn't require any jni calls).

Passing around ~20 arguments (especially ones that can be so easily swapped
without compile failure) is incredibly ugly, though.

I don't know if we should be making this public and using it.

One possible way to avoid all the jni calls would be to call up to a java method
passing the event that then calls back down to native unpacking all the event
fields into args.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:31:
drawable_->SetOrientation(orientation_);
Should these initial values be arguments to client->CreateDrawable()? I noticed
that the java popup drawable defaults to different values. I guess it doesn't
really matter.

Another note that probably doesn't matter: at this point TouchHandle::position_
might not be the same as the drawable's position.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_unittest.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_unittest.cc:32: explicit
MockTouchHandleDrawable(MockDrawableData& data) : data_(data) {}
This should be a pointer, not a reference

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:63: // is
being dragged, incorrectly reporting a CENTER orientation.
Is it possible that I could have selection and then drag one handle to the other
and then release such that the handle is no longer dragging when we actually get
the updated selection bounds?

(I actually just tried playing around a bit with this with the current
implementation and this can definitely happen and is easy to trigger. Even
better, is that you can get the end drag while there are actually multiple
updates still in flight such that you get one updated selection bounds where
start == end and orientation is center followed by another where start != end.)

I don't know if there is a simple way to handle that case.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.h:45: // To be
called when the selection bounds have chnaged.
s/chnaged/changed

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller_unittest.cc
(right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_unittest.cc:211:
TEST_F(TouchSelectionControllerTest, InsertionClearedWhenLongerEditable) {
s/WhenLonger/WhenNoLonger

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

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2359:
private void showPastePopup(int xDip, int yDip) {
Is this called by native? Should it show the paste popup?

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:53:
private long mTouchTimer;
Tap is now detected by native, so touchtimer isn't needed.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:54:
private boolean mIsInsertionHandle = false;
This seems unused.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:55:
private float mAlpha;
Fading is handled by native, now, right? So all the stuff in here for it can be
removed.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:184:
if (!isPositionVisible()) {
This probably shouldn't be showing/hiding itself and should just do what it's
told by the native touch handle.

[jdduke (slow), 2014-07-10 02:08:40]

Thanks again for review.  I need to do some deeper investigation of the WebView
animation path; I've been hearing some different reports as to whether the
current animation system will work, so I just need to bite the bullet and do a
full WebView Android build (I've tested the PopupTouchHandleDrawable path with
clank and it works just fine, and I've also tested WebViewShell, but I'll
probably want a more complicated example and particularly one that uses the
WebView hardware rendering path to make sure I'm not doing anything
pathological).

https://codereview.chromium.org/335943002/diff/210001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/android...
content/browser/android/content_view_core_impl.cc:805: if (anchor.x() ==
focus.x() && anchor.y() == focus.y())
On 2014/07/09 22:29:12, cjhopman wrote:
> nit: if (anchor == focus) {

Done.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:150: bool
should_recycle)
On 2014/07/09 22:29:12, cjhopman wrote:
> So this constructor makes like ~16 jni calls (and was private to make you use
> the one that doesn't require any jni calls).
> 
> Passing around ~20 arguments (especially ones that can be so easily swapped
> without compile failure) is incredibly ugly, though.
> 
> I don't know if we should be making this public and using it.
> 
> One possible way to avoid all the jni calls would be to call up to a java
method
> passing the event that then calls back down to native unpacking all the event
> fields into args.

Agreed, I'll take a closer look at this.  I think there's a way to avoid this
additional public path that's probably for the best.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:31:
drawable_->SetOrientation(orientation_);
On 2014/07/09 22:29:12, cjhopman wrote:
> Should these initial values be arguments to client->CreateDrawable()? I
noticed
> that the java popup drawable defaults to different values. I guess it doesn't
> really matter.
> 
> Another note that probably doesn't matter: at this point
TouchHandle::position_
> might not be the same as the drawable's position.

I'll push through the position_ for completeness.  I'm tempted to have the
orientation be an argument to |client_->CreateDrawable()|, but I'm OK leaving
as-is for now.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_unittest.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_unittest.cc:32: explicit
MockTouchHandleDrawable(MockDrawableData& data) : data_(data) {}
On 2014/07/09 22:29:12, cjhopman wrote:
> This should be a pointer, not a reference

Done.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:63: // is
being dragged, incorrectly reporting a CENTER orientation.
On 2014/07/09 22:29:12, cjhopman wrote:
> Is it possible that I could have selection and then drag one handle to the
other
> and then release such that the handle is no longer dragging when we actually
get
> the updated selection bounds?
> 
> (I actually just tried playing around a bit with this with the current
> implementation and this can definitely happen and is easy to trigger. Even
> better, is that you can get the end drag while there are actually multiple
> updates still in flight such that you get one updated selection bounds where
> start == end and orientation is center followed by another where start !=
end.)
> 
> I don't know if there is a simple way to handle that case.

Hmm, you're quite right... perhaps we require that, once a selection is active,
it can only be deactivated if the orientation becomes undefined or it's
explicitly hidden.  Maybe only enforce that if a gesture has occurred? I'll
probably do that as a follow-up, keeping the current behavior but I'll file a
bug.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.h:45: // To be
called when the selection bounds have chnaged.
On 2014/07/09 22:29:12, cjhopman wrote:
> s/chnaged/changed

Done.

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller_unittest.cc
(right):

https://codereview.chromium.org/335943002/diff/210001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller_unittest.cc:211:
TEST_F(TouchSelectionControllerTest, InsertionClearedWhenLongerEditable) {
On 2014/07/09 22:29:12, cjhopman wrote:
> s/WhenLonger/WhenNoLonger

Done.

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

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2359:
private void showPastePopup(int xDip, int yDip) {
On 2014/07/09 22:29:12, cjhopman wrote:
> Is this called by native? Should it show the paste popup?

Yeah, I was debating what to do here.  I'd like to rip it out eventually, as I'm
not sure we need it and it forces every shell to implement the wiring
(content/chrome/clank/webview).  I'll hook it up for now.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:53:
private long mTouchTimer;
On 2014/07/09 22:29:12, cjhopman wrote:
> Tap is now detected by native, so touchtimer isn't needed.

Done.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:54:
private boolean mIsInsertionHandle = false;
On 2014/07/09 22:29:12, cjhopman wrote:
> This seems unused.

Done.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:55:
private float mAlpha;
On 2014/07/09 22:29:12, cjhopman wrote:
> Fading is handled by native, now, right? So all the stuff in here for it can
be
> removed.

Done.

https://codereview.chromium.org/335943002/diff/210001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:184:
if (!isPositionVisible()) {
On 2014/07/09 22:29:12, cjhopman wrote:
> This probably shouldn't be showing/hiding itself and should just do what it's
> told by the native touch handle.

Agreed, though I think I'll need to add a general "setEnabled()" call to the
TouchHandleDrawable and TouchHandle so that we can dismiss the PopupWindow after
it's no longer in use (as opposed to simply changing its visibility).

[jdduke (slow), 2014-07-10 21:11:28]

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

https://codereview.chromium.org/335943002/diff/290001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2340:
return new PopupTouchHandleDrawable(
So, I think the one major remaining issue is WebView and garbage collection. 
There is no guarantee that |destroy()| will be called on ContentViewCore, which
means the PopupTouchHandleDrawable owned by native code (in RWHVA) that we
create will hold a persistent reference to ContentViewCore, preventing garbage
collection.

aurimas@ and yfriedman@ discussed a solution in which there's a
RenderWidgetHostViewAndroid.java object that doesn't hold a strong reference to
ContentViewCore. That could work, but in the meantime I think I'll try using a
WeakReference object provided to PopupTouchHandleDrawable for dealing with the
outside world.  Then, in PopupTouchHandleDrawable.hide(), we can remove any
strong references (e.g., to the position observer), allowing GC. Hide should be
called whenever the selection is cleared, which should happen whenever the
WebView loses focus.

[cjhopman, 2014-07-14 18:23:52]

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:48: EndFade();
I think that, since EndDrag() can trigger a fade, this should do EndDrag()
first, then EndFade(). In fact, it looks like this should hit the DCHECK in
BeginFade() if this is disabled while dragging (you have tests for disabling
while dragging/fading so maybe I'm wrong about the DCHECK).

I think the SetEnabled() should probably come after ending the drag/fade.

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:72: // the drawable when the
handle regains visibility (see |{Begin|End}Fade|).
For handling the deferred position while invisible, would it be easier to see
that it is correct if you updated the position in SetVisible instead of
Begin/EndFade? Do we really need to defer the position updates while not
visible?

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:73: if (is_visible_)
This allows changing the position while fading in (which seems to conflict with
the comment).

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_unittest.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_unittest.cc:276: // Diabling
mid-drag shoudl cancel the drag.
s/Diabling/Disabling
s/shoudl/should

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:207:
insertion_handle_->SetVisible(start_visible_, GetAnimationStyle());
This now fades in when first activated?

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:219: const
TouchHandle::AnimationStyle animation = GetAnimationStyle();
This also now fades in?

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

https://codereview.chromium.org/335943002/diff/290001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2340:
return new PopupTouchHandleDrawable(
On 2014/07/10 21:11:28, jdduke wrote:
> So, I think the one major remaining issue is WebView and garbage collection. 
> There is no guarantee that |destroy()| will be called on ContentViewCore,
which
> means the PopupTouchHandleDrawable owned by native code (in RWHVA) that we
> create will hold a persistent reference to ContentViewCore, preventing garbage
> collection.
> 
> aurimas@ and yfriedman@ discussed a solution in which there's a
> RenderWidgetHostViewAndroid.java object that doesn't hold a strong reference
to
> ContentViewCore. That could work, but in the meantime I think I'll try using a
> WeakReference object provided to PopupTouchHandleDrawable for dealing with the
> outside world.  Then, in PopupTouchHandleDrawable.hide(), we can remove any
> strong references (e.g., to the position observer), allowing GC. Hide should
be
> called whenever the selection is cleared, which should happen whenever the
> WebView loses focus.

Yeah, this whole structure (not this change specifically) seems fragile and
likely to break in the future.

[jdduke (slow), 2014-07-15 15:44:25]

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:48: EndFade();
On 2014/07/14 18:23:52, cjhopman wrote:
> I think that, since EndDrag() can trigger a fade, this should do EndDrag()
> first, then EndFade(). In fact, it looks like this should hit the DCHECK in
> BeginFade() if this is disabled while dragging (you have tests for disabling
> while dragging/fading so maybe I'm wrong about the DCHECK).
> 
> I think the SetEnabled() should probably come after ending the drag/fade.

You're absolutely right, I've been running tests locally in release mode =/

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:72: // the drawable when the
handle regains visibility (see |{Begin|End}Fade|).
On 2014/07/14 18:23:52, cjhopman wrote:
> For handling the deferred position while invisible, would it be easier to see
> that it is correct if you updated the position in SetVisible instead of
> Begin/EndFade? Do we really need to defer the position updates while not
> visible?

That would be a better place, but we still need the deferral when the handle is
fading out.  The problem is most evident when you're scrolling a subdiv, and you
scroll the handles such that they become occluded by another layer.  If you
continue updating the positions while it's fading out, you get this bothersome
ghosting where the handle remains visible while the content to which it's
attached is invisible.  Deferring the reposition masks our inability to
perfectly clip the handle and leads to a nicer experience.

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:73: if (is_visible_)
On 2014/07/14 18:23:52, cjhopman wrote:
> This allows changing the position while fading in (which seems to conflict
with
> the comment).

Hmm, yeah we only want to defer while fading *out*, I'll update the comment.

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_unittest.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_unittest.cc:276: // Diabling
mid-drag shoudl cancel the drag.
On 2014/07/14 18:23:52, cjhopman wrote:
> s/Diabling/Disabling
> s/shoudl/should

Done.

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:207:
insertion_handle_->SetVisible(start_visible_, GetAnimationStyle());
On 2014/07/14 18:23:52, cjhopman wrote:
> This now fades in when first activated?

Bah, yeah, I played with this locally and thought it was fading, but per offline
discussion if it's fading it's too fast to really notice. I'll change to the
behavior I had.

https://codereview.chromium.org/335943002/diff/290001/content/browser/rendere...
content/browser/renderer_host/input/touch_selection_controller.cc:219: const
TouchHandle::AnimationStyle animation = GetAnimationStyle();
On 2014/07/14 18:23:52, cjhopman wrote:
> This also now fades in?

Yeah but I'll revert.

[cjhopman, 2014-07-15 18:40:28]

Mostly just some notes about weird edge cases.

I didn't really look into the states where the handle is disabled (but I expect
they should all just work the same as when enabled). Still, I'd just say that
you aren't allowed to change things while disabled (if we don't already violate
that).

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:93: deferred_orientation_ =
TOUCH_HANDLE_ORIENTATION_UNDEFINED;
If you reset deferred_orientation in whatever calls SetOrientation() for that
case instead of here, you can assert here that there is no deferred orientation.
I don't know if that's easier to reason about or not.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:116: BeginDrag();
The handle could be fading in at this point. It's probably easiest to reason
about stuff if that fade is ended when dragging starts (so that this is never
dragging and fading at the same time).

If we don't make that change, then if the drag ends before the animate, the
EndFade() call in EndDrag() will end the fade early. If while fading and
dragging, we do another setvisible something bad happens (I think it would
continue fading to the new visibility).

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:127: if (delay <
base::TimeDelta::FromMilliseconds(180))
Should we delay dragging during this period? Delay dragging within some distance
during this period? (I don't really know what Android does for that case) This
can probably be done in a followup.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:191: if (target_alpha ==
alpha_)
If the handle is visible and I do fade out immediately followed by fade in, the
handle will go to 0 alpha and fade in. It looks like otherwise switching between
fade in and out works.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:197: kFadeDurationMs *
std::abs(target_alpha - alpha_));
If i fade out a handle, then reposition it, then fade it in (before the fade out
finishes), it will start the fade at whatever alpha it was at during the fade
out. Due to the repositioning, it feels like maybe should fade from 0? I'm not
really sure about what should happen in this case (and don't think it really
matters).

[jdduke (slow), 2014-07-15 21:51:57]

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:93: deferred_orientation_ =
TOUCH_HANDLE_ORIENTATION_UNDEFINED;
On 2014/07/15 18:40:28, cjhopman wrote:
> If you reset deferred_orientation in whatever calls SetOrientation() for that
> case instead of here, you can assert here that there is no deferred
orientation.
> I don't know if that's easier to reason about or not.

Done.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:116: BeginDrag();
On 2014/07/15 18:40:28, cjhopman wrote:
> The handle could be fading in at this point. It's probably easiest to reason
> about stuff if that fade is ended when dragging starts (so that this is never
> dragging and fading at the same time).
> 
> If we don't make that change, then if the drag ends before the animate, the
> EndFade() call in EndDrag() will end the fade early. If while fading and
> dragging, we do another setvisible something bad happens (I think it would
> continue fading to the new visibility).

Done.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:127: if (delay <
base::TimeDelta::FromMilliseconds(180))
On 2014/07/15 18:40:28, cjhopman wrote:
> Should we delay dragging during this period? Delay dragging within some
distance
> during this period? (I don't really know what Android does for that case) This
> can probably be done in a followup.

Well, we really ought to be using a proper touch slop suppression distance. I've
considered adding a GestureDetector instance here, although I think that's
overkill. What I had planned was pulling in the platform slop distance and tap
duration, and if an ACTION_MOVE exceeds that distance we prevent a tap from
being triggered.  I'll do that as a follow-up, and make a note of it.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:191: if (target_alpha ==
alpha_)
On 2014/07/15 18:40:28, cjhopman wrote:
> If the handle is visible and I do fade out immediately followed by fade in,
the
> handle will go to 0 alpha and fade in. It looks like otherwise switching
between
> fade in and out works.

I'll terminate the fade here if (target_alpha == alpha_) for good measure.

https://codereview.chromium.org/335943002/diff/350001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:197: kFadeDurationMs *
std::abs(target_alpha - alpha_));
On 2014/07/15 18:40:28, cjhopman wrote:
> If i fade out a handle, then reposition it, then fade it in (before the fade
out
> finishes), it will start the fade at whatever alpha it was at during the fade
> out. Due to the repositioning, it feels like maybe should fade from 0? I'm not
> really sure about what should happen in this case (and don't think it really
> matters). 

Hmm, preserving the fade alpha is somewhat desirable if the handle is along a
visibility boundary and transitioning back and forth but moving slightly (e.g.,
scroll subdiv up, hiding the handle, then immediately back down). I think the
duration is short enough that most other scenarios should be OK.

[cjhopman, 2014-07-17 00:48:38]

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:178: void
TouchHandle::EndDrag() {
This can DCHECK(enabled_) and then the condition below can assume it's true

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:195: EndFade();
the else part is no longer needed

(and after reading more) oh, this is needed because endfade actually applies
is_visible_ to the underlying drawable.

Hm, it'd probably be easier to understand how visibility is deferred if it
followed the pattern for orientation. That is, if SetVisible is called while
dragging, you store the arguments without changing is_visible_ and then in
EndDrag() you call SetVisible with the deferred values.

That being said, I would be happy with just a comment somewhere noting that
EndFade() applies is_visible_ to the underlying drawable (either here or by
EndFade's definition, probably).

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:216: void
TouchHandle::EndFade() {
This can also DCHECK(enabled_) and so can SetAlpha(), but those are simple
enough that it doesn't really help.

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.h (right):

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:63: // and further
manipulation is *explicitly not supported* and may result in
I can re-enable it and then use it, right?

[jdduke (slow), 2014-07-17 15:43:04]

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:178: void
TouchHandle::EndDrag() {
On 2014/07/17 00:48:37, cjhopman wrote:
> This can DCHECK(enabled_) and then the condition below can assume it's true

Done.

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:195: EndFade();
On 2014/07/17 00:48:37, cjhopman wrote:
> the else part is no longer needed
> 
> (and after reading more) oh, this is needed because endfade actually applies
> is_visible_ to the underlying drawable.
> 
> Hm, it'd probably be easier to understand how visibility is deferred if it
> followed the pattern for orientation. That is, if SetVisible is called while
> dragging, you store the arguments without changing is_visible_ and then in
> EndDrag() you call SetVisible with the deferred values.
> 
> That being said, I would be happy with just a comment somewhere noting that
> EndFade() applies is_visible_ to the underlying drawable (either here or by
> EndFade's definition, probably).

Done.

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:216: void
TouchHandle::EndFade() {
On 2014/07/17 00:48:37, cjhopman wrote:
> This can also DCHECK(enabled_) and so can SetAlpha(), but those are simple
> enough that it doesn't really help.

Added to EndFade().

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.h (right):

https://codereview.chromium.org/335943002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.h:63: // and further
manipulation is *explicitly not supported* and may result in
On 2014/07/17 00:48:37, cjhopman wrote:
> I can re-enable it and then use it, right?

Yeah I'll make a note.

[cjhopman, 2014-07-17 16:04:31]

lgtm

https://codereview.chromium.org/335943002/diff/410001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/410001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:192: if (enabled_ &&
animate_deferred_fade_) {
enabled_ is always true

[jdduke (slow), 2014-07-18 18:49:27]

https://codereview.chromium.org/335943002/diff/410001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle.cc (right):

https://codereview.chromium.org/335943002/diff/410001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle.cc:192: if (enabled_ &&
animate_deferred_fade_) {
On 2014/07/17 16:04:31, cjhopman wrote:
> enabled_ is always true

Of course, fixed.

[jdduke (slow), 2014-07-18 18:56:39]

aelias@: PTAL for owner review of
content/browser/renderer_host/render_widget_host_view_android.{cc,h}.  

I'm somewhat inclined to land this using the existing handle pipeline (selection
updates sent from the main thread), as there's a good amount of code here and
letting it rot is quite a burden (need to resolve some Blink positioning issues
first, see crbug.com/395153).  It should just work with the old pipeline, and
once the Blink enabling patch lands we can flip a switch and toggle the place
where we insert the handle visibility/positions. Thoughts?

[aelias_OOO_until_Jul13, 2014-07-18 19:06:40]

lgtm for RWHVA

I'm okay with landing this before the Blink side, it does seem orthogonal.

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

https://codereview.chromium.org/335943002/diff/450001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1572:
selection_controller_->AllowAutomaticSelectionShowing();
nit: can this be renamed to be consistent with the method name through the rest
of the stack?  (ShowSelectionHandlesAutomatically)

[jdduke (slow), 2014-07-18 22:58:06]

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

https://codereview.chromium.org/335943002/diff/450001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1572:
selection_controller_->AllowAutomaticSelectionShowing();
On 2014/07/18 19:06:40, aelias wrote:
> nit: can this be renamed to be consistent with the method name through the
rest
> of the stack?  (ShowSelectionHandlesAutomatically)

Done.

[jdduke (slow), 2014-07-18 23:05:50]

brettw@: Owner review for content/content.gyp? Thanks.

[jdduke (slow), 2014-07-18 23:09:04]

boliu@: Review for android_webview/android_webview.gyp? Thanks.

[boliu, 2014-07-18 23:13:32]

https://codereview.chromium.org/335943002/diff/530001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/335943002/diff/530001/android_webview/android...
android_webview/android_webview.gyp:25: # android_webview_java in
android_webview/Android.mk.
Need to update Android.mk file. Just copy the same pattern as the existing ones
and it should work...

[jdduke (slow), 2014-07-18 23:21:33]

https://codereview.chromium.org/335943002/diff/530001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/335943002/diff/530001/android_webview/android...
android_webview/android_webview.gyp:25: # android_webview_java in
android_webview/Android.mk.
On 2014/07/18 23:13:31, boliu wrote:
> Need to update Android.mk file. Just copy the same pattern as the existing
ones
> and it should work...

Ah, yeah I was confused, looks like it moved to java_library_common.mk. Updated
the comments appropriately.

[boliu, 2014-07-18 23:24:59]

lgtm

Thanks!

[jdduke (slow), 2014-07-21 16:14:26]

jam@: Looks like brettw@ may be out, would you mind reviewing the changes to
content/content.gyp? Thanks.

[jam, 2014-07-21 17:11:34]

lgtm

[jdduke (slow), 2014-07-21 17:14:33]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-07-21 17:15:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/335943002/620001

[commit-bot: I haz the power, 2014-07-21 19:58:49]

Change committed as 284491