[Android] Select text when stylus first button is pressed.

[Android] Select text when stylus first button is pressed.

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

[jdduke (slow), 2014-06-19 15:10:11]

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
Ah, yeah, so we would definitely need to move this check *before* the call to
|if (!gesture_provider_.OnTouchEvent(event))|, and use a separate gesture
detector altogether.

[jdduke (slow), 2014-06-20 02:14:27]

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
We'll probably want logic along the lines of:

if (event.GetAction() == ACTION_DOWN && ShouldConvertToMouseEvent(event))
  convert_touch_sequence_to_mouse_ = true;

if (convert_touch_sequence_to_mouse_) {
  SendMouseEvent(CreateWebMouseEventFromMotionEvent(event));
  gesture_provider_.OnTouchEventAck(false);
  if (event.GetAction() == ACTION_UP || event.GetAction() == ACTION_CANCEL)
    convert_touch_sequence_to_mouse_ = false;
  return true;
}

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1293:
SendGestureEvent(CreateWebGestureEventFromGestureEventData(gesture));
Then you can add a check here to drop all gestures except for long press:

if (convert_touch_sequence_to_mouse_ && gesture.type() != ET_GESTURE_LONG_PRESS)
  return;
SendGestureEvent(...);

[jdduke (slow), 2014-06-20 02:23:39]

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
On 2014/06/20 02:14:27, jdduke wrote:
> We'll probably want logic along the lines of:
> 
> if (event.GetAction() == ACTION_DOWN && ShouldConvertToMouseEvent(event))
>   convert_touch_sequence_to_mouse_ = true;

Actually this should probably be:

if (event.GetAction() == ACTION_DOWN)
  convert_touch_sequence_to_mouse_ = ShouldConvertToMouseEvent(event);

[Changwan Ryu, 2014-06-20 08:19:54]

PTAL

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
On 2014/06/19 15:10:11, jdduke wrote:
> Ah, yeah, so we would definitely need to move this check *before* the call to
> |if (!gesture_provider_.OnTouchEvent(event))|, and use a separate gesture
> detector altogether.
Moved it up and added a TODO.

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
On 2014/06/20 02:23:39, jdduke wrote:
> On 2014/06/20 02:14:27, jdduke wrote:
> > We'll probably want logic along the lines of:
> > 
> > if (event.GetAction() == ACTION_DOWN && ShouldConvertToMouseEvent(event))
> >   convert_touch_sequence_to_mouse_ = true;
> 
> Actually this should probably be:
> 
> if (event.GetAction() == ACTION_DOWN)
>   convert_touch_sequence_to_mouse_ = ShouldConvertToMouseEvent(event);

Done.

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:559: if
(ShouldConvertToMouseEvent(event)) {
On 2014/06/20 02:23:39, jdduke wrote:
> On 2014/06/20 02:14:27, jdduke wrote:
> > We'll probably want logic along the lines of:
> > 
> > if (event.GetAction() == ACTION_DOWN && ShouldConvertToMouseEvent(event))
> >   convert_touch_sequence_to_mouse_ = true;
> 
> Actually this should probably be:
> 
> if (event.GetAction() == ACTION_DOWN)
>   convert_touch_sequence_to_mouse_ = ShouldConvertToMouseEvent(event);

Done.

https://codereview.chromium.org/342633003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1293:
SendGestureEvent(CreateWebGestureEventFromGestureEventData(gesture));
On 2014/06/20 02:14:27, jdduke wrote:
> Then you can add a check here to drop all gestures except for long press:
> 
> if (convert_touch_sequence_to_mouse_ && gesture.type() !=
ET_GESTURE_LONG_PRESS)
>   return;
> SendGestureEvent(...);
I don't have access to gesture variable. Let me handle this in the next CL.

[Changwan Ryu, 2014-06-20 08:32:06]

FYI, this is based off of https://codereview.chromium.org/348813002/, and you
can check the diff by comparing a patchset with patchset #4. Thanks.

[jdduke (slow), 2014-06-20 16:52:49]

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

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:552:
convert_touch_sequence_to_mouse_ =
Sorry, my previous comment was confusing.

You'll want to move this added block of code down below "if
(!gesture_provider_.OnTouchEvent(event))", which will allow us to use the same
gesture detector for all the events.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:554: //
TODO(changwan): add a gesture detector to handle the long press event.
If you move this block below as in the previous comment, you won't need the
extra detector here.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:560: if
(event.GetAction() == ui::MotionEvent::ACTION_UP ||
I lied, we actually don't want this line to reset
|convert_touch_sequence_to_mouse_|, as then we might get a delayed GESTURE_TAP
event after the timeout.  It should be sufficient to reset the bit only with
ACTION_DOWN.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1298:
SendGestureEvent(CreateWebGestureEventFromGestureEventData(gesture));
By using the same gesture detector, we can filter out undesired gestures here
depending on the |convert_touch_sequence_to_mouse_| flag.

[jdduke (slow), 2014-06-20 21:57:59]

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

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1298:
SendGestureEvent(CreateWebGestureEventFromGestureEventData(gesture));
On 2014/06/20 16:52:49, jdduke wrote:
> By using the same gesture detector, we can filter out undesired gestures here
> depending on the |convert_touch_sequence_to_mouse_| flag.

Let's chat about this early next week.  I'm thinking that, instead of sending
WebMouseEvents (per Rick's email and offline discussion with Grace), we suppress
all gestures here (while |convert_touch_sequence_to_mouse_| is true, probably
need to rename that field), except for TAP/LONG_PRESS (it's not clear to me
which gesture they want to select a word), and then we use the
SCROLL_BEGIN/UPDATE/END gestures to dictate the |selectBetweenCoordinates|
commands.

[Changwan Ryu, 2014-06-23 06:39:22]

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

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:552:
convert_touch_sequence_to_mouse_ =
On 2014/06/20 16:52:49, jdduke wrote:
> Sorry, my previous comment was confusing.
> 
> You'll want to move this added block of code down below "if
> (!gesture_provider_.OnTouchEvent(event))", which will allow us to use the same
> gesture detector for all the events.

Done.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:554: //
TODO(changwan): add a gesture detector to handle the long press event.
On 2014/06/20 16:52:49, jdduke wrote:
> If you move this block below as in the previous comment, you won't need the
> extra detector here.

Done.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:560: if
(event.GetAction() == ui::MotionEvent::ACTION_UP ||
On 2014/06/20 16:52:49, jdduke wrote:
> I lied, we actually don't want this line to reset
> |convert_touch_sequence_to_mouse_|, as then we might get a delayed GESTURE_TAP
> event after the timeout.  It should be sufficient to reset the bit only with
> ACTION_DOWN.

Done.

https://codereview.chromium.org/342633003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1298:
SendGestureEvent(CreateWebGestureEventFromGestureEventData(gesture));
I have uploaded a version that filters out gestures when
gesture_text_selection_triggered_ is false, except for LONG PRESS event. This
kind of works. :)

[jdduke (slow), 2014-06-23 15:09:59]

My biggest question here is testing.  We should definitely have tests for this
logic, unfortunately RWHVAndroid isn't setup for well for unit tests.  Probably
the simplest thing to do would be to wrap this logic in a very simple class,
maybe called "GestureTextSelector" or "TextSelectionEmulator", that would live
in RWHVAndroid, and it would have methods:

bool WillHandleTouchEvent(const ui::MotionEvent&);
bool WillHandleGestureEvent(const ui::GestureEventData&);

where returning true means the RWHVAndroid should cease forwarding the
touch/gesture.  It would also take a client interface with methods:

void SelectBetween(...);
void SelectWord(...); 
void Unselect(...);

Then we can easily mock out the client and test the different cases. What do you
think?

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

https://codereview.chromium.org/342633003/diff/160001/content/browser/android...
content/browser/android/content_view_core_impl.cc:989: mouse_drag_start_x_ =
pos_x_0;
All of this logic should live close to where we determine the selection state
(based on the ACTION_DOWN state of the stylus).  Otherwise it makes it very
difficult to reason about event flow.  My suggestion would be to expose
|Select()| and |SelectBetweenCoordinates()| on ContentViewCoreImpl, and then we
can hook into those methods from RWHVAndroid and move this code there.  The
reason I suggested using GestureScrollBegin/Update is that there is built-in
slop with detecting scrolls, i.e., it allows you to tap/press OR scroll, but not
both, so you have a clean separation between the "SelectWord" and
"SelectBetweenCoordinates" paths.  If you use ACTION_MOVE, you could be sending
(multiple) SelectBetweenCoordinates message(s) and then a SelectWord message
when you get the long press event.

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

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:561: if
(event.GetPointerCount() == 0 ||
Nit: GetPointerCount() should never be 0, so I would make this a DCHECK.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:564: const bool
is_button_pressed =
Maybe rename to |is_secondary_button_pressed|?

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:566: const bool
is_action_up = event.GetAction() == ui::MotionEvent::ACTION_UP;
Hmm, this is a little confusing, why do we check for ACTION_UP when this
function is only called in the if (ACTION_DOWN) case? I think we can remove this
check and just check the tool type and the secondary button state, and make this
function static (or rather, just move it to the anonymous namespace).

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:591: if
(event.GetAction() == ui::MotionEvent::ACTION_UP ||
As I said, we can't reset the state using ACTION_UP and ACTION_CANCEL, because
there are timeout events like GESTURE_TAP that might get triggered after an
ACTION_UP (because of the 300ms tap delay on desktop sites).  It should be
sufficient to reset the flag only on ACTION_DOWN.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1329:
gesture_text_selection_triggered_ = false;
Hmm, I don't think we want to reset this here. Can't we just send a SelectWord
message or something to that effect?

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1331: return;
I thought Samsung wanted to be able to highlight a word using either TAP or
LONG_PRESS? Is that not the case?

[jdduke (slow), 2014-06-23 15:24:50]

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

https://codereview.chromium.org/342633003/diff/160001/content/browser/android...
content/browser/android/content_view_core_impl.cc:989: mouse_drag_start_x_ =
pos_x_0;
On 2014/06/23 15:09:59, jdduke wrote:
> All of this logic should live close to where we determine the selection state
> (based on the ACTION_DOWN state of the stylus).  Otherwise it makes it very
> difficult to reason about event flow.  My suggestion would be to expose
> |Select()| and |SelectBetweenCoordinates()| on ContentViewCoreImpl, and then
we
> can hook into those methods from RWHVAndroid and move this code there.  The
> reason I suggested using GestureScrollBegin/Update is that there is built-in
> slop with detecting scrolls, i.e., it allows you to tap/press OR scroll, but
not
> both, so you have a clean separation between the "SelectWord" and
> "SelectBetweenCoordinates" paths.  If you use ACTION_MOVE, you could be
sending
> (multiple) SelectBetweenCoordinates message(s) and then a SelectWord message
> when you get the long press event.

Hmm, it appears |GetWebContents()| is a proper public method on ContentViewCore,
so we could probably just use it directly in RWHVAndroid instead of adding
delegating methods.

[Changwan Ryu, 2014-06-24 09:29:48]

PTAL.

As you suggested, I've refactored the code into a class, and added 2 unit tests.
I think it is in a much better shape now.

BTW I couldn't figure out how to test ET_GESTURE_SCROLL events yet, though, so
there may be missing pieces there.

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

https://codereview.chromium.org/342633003/diff/160001/content/browser/android...
content/browser/android/content_view_core_impl.cc:989: mouse_drag_start_x_ =
pos_x_0;
Done

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

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:561: if
(event.GetPointerCount() == 0 ||
On 2014/06/23 15:09:59, jdduke wrote:
> Nit: GetPointerCount() should never be 0, so I would make this a DCHECK.

Done.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:564: const bool
is_button_pressed =
On 2014/06/23 15:09:59, jdduke wrote:
> Maybe rename to |is_secondary_button_pressed|?

Done.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:566: const bool
is_action_up = event.GetAction() == ui::MotionEvent::ACTION_UP;
On 2014/06/23 15:09:59, jdduke wrote:
> Hmm, this is a little confusing, why do we check for ACTION_UP when this
> function is only called in the if (ACTION_DOWN) case? I think we can remove
this
> check and just check the tool type and the secondary button state, and make
this
> function static (or rather, just move it to the anonymous namespace).

Done.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:591: if
(event.GetAction() == ui::MotionEvent::ACTION_UP ||
Removed.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1329:
gesture_text_selection_triggered_ = false;
On 2014/06/23 15:09:59, jdduke wrote:
> Hmm, I don't think we want to reset this here. Can't we just send a SelectWord
> message or something to that effect?

Fixed to call SelectWord instead.
But reset is still needed because action_move events will most likely happen
after a long press, and those events will reselect or unselect text.

https://codereview.chromium.org/342633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1331: return;
Samsung confirmed that it's only for LONG PRESS.

[benm (inactive), 2014-06-24 15:15:23]

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.h (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:14: class
GestureEventData;
s/class/struct/

[jdduke (slow), 2014-06-24 15:38:37]

Thanks, this is a very nice refactoring.

tdresser@: Do you have any opinions about adding GestureTextSelector to
ui/events/gesture_detection? It doesn't look appear to have any content-specific
dependencies, and that would avoid the need to export MockMotionEvent.  I think
down the road I may have GestureTextSelector use a separate GestureProvider
instance, but that's not necessary right now.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:27: if
(text_selection_triggered_) {
Nit: To avoid nesting invert the condition and early return, i.e.,

if (!text_selection_triggered_)
  return false;

...

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:28:
client_->ShowSelectionHandlesAutomatically();
We should probably only call this upon ACTION_DOWN.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:38:
client_->SelectRange(anchor_x_, anchor_y_,
Hmm, we shouldn't use SelectRange with both ACTION_DOWN and SCROLL_UPDATE, it
should only be the latter (with SCROLL_UPDATE).

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:42:
text_selection_triggered_ = false;
ACTION_CANCEL shouldn't reset the state, as the cancel event may trigger some
"cleanup" gestures that would then get forwarded.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:54: if
(text_selection_triggered_) {
Nit:

if (!text_selection_triggered_)
  return false;

if (drop_remaining_sequence_)
  return true;

...

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:61:
text_selection_triggered_ = false;
The problem with assigning this to false is that then we'll start allowing
touches through to the renderer, when it didn't get the first part of the
sequence.

You'll probably need another condition, |drop_remaining_sequence_| or
|long_press_triggered_|, in which case we drop all remaining gestures and
touches until ACTION_DOWN (resetting the condition).  It might be easier to
formulate this as a 3-state enum, but either way I think you need another bit
here.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:70: case
ui::ET_GESTURE_SCROLL_END: {
No need to listen to SCROLL_END, SCROLL_UPDATE should be sufficient.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:86:
DCHECK(event.GetPointerCount() > 0);
Nit: DCHECK_GT(event.GetPointerCount(), 0);

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.h (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:40: // Returns
whether it will consume the event.
Maybe put a note here that this should be called *after* gesture detection but
before associated gestures have been dispatched.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector_unittest.cc
(right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector_unittest.cc:87: {  //
Touched with a stylus, with two buttons pressed.
Do you know what the SPen does in this case? Will pressing both buttons prevent
selection?

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector_unittest.cc:148: // 4.
LONG_PRESS (This can occur before or after ACTION_UP.)
Hmm, LONG_PRESS should only ever occur before ACTION_UP, are you seeing
otherwise? I would just start a new ACTION_DOWN sequence to simulate LONG_PRESS
here

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

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1296: if
(gesture_text_selector_->OnGestureEvent(gesture)) {
Nit: No braces necessary (just add a line break).

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1454:
static_cast<WebContentsImpl*>(content_view_core_->GetWebContents())->
We'll want to ask sievers@ or aelias@ about WebContentsImpl usage here, I had
thought we used WebContents directly in RWHVAndroid, but it looks like this is
the first such usage.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:373:
scoped_ptr<GestureTextSelector> gesture_text_selector_;
I don't think we need to heap allocate this, I would just make it a regular
value member variable and move it below the |gesture_provider_| declaration.

https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.cc (right):

https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.cc:158: DCHECK_GT(pointer_count,
pointer_index);
Nit: For consistency make this "DCHECK_LT(pointer_index, pointer_count)", also
below in |SetToolType()|.

[benm (inactive), 2014-06-24 15:57:11]

Changwan, can you share how you've been testing this? I tried applying it into a
build and installing on a Note3, but can't get selection to work using the
stylus. It works with a long press on a word (but that also seems to work
without your patch), but I can't hold the putton and drag to select.

[Changwan Ryu, 2014-06-24 23:52:12]

On 2014/06/24 15:57:11, benm wrote:
> Changwan, can you share how you've been testing this? I tried applying it into
a
> build and installing on a Note3, but can't get selection to work using the
> stylus. It works with a long press on a word (but that also seems to work
> without your patch), but I can't hold the putton and drag to select.

Ben, you cannot test this with Note 3 since Note 3 does not tag MotionEvent with
BUTTON_SECONDARY flag.
Could you try commenting out the check in
GestureTextSelector::ShouldStartTextSelection()?

[Changwan Ryu, 2014-06-25 07:26:21]

PTAL

Please note that long press handling has been removed for now as our partner
hasn't decided whether they want the feature or not. If necessary, I'll upload a
separate CL.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:27: if
(text_selection_triggered_) {
On 2014/06/24 15:38:36, jdduke wrote:
> Nit: To avoid nesting invert the condition and early return, i.e.,
> 
> if (!text_selection_triggered_)
>   return false;
> 
> ...

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:28:
client_->ShowSelectionHandlesAutomatically();
On 2014/06/24 15:38:36, jdduke wrote:
> We should probably only call this upon ACTION_DOWN.

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:38:
client_->SelectRange(anchor_x_, anchor_y_,
On 2014/06/24 15:38:36, jdduke wrote:
> Hmm, we shouldn't use SelectRange with both ACTION_DOWN and SCROLL_UPDATE, it
> should only be the latter (with SCROLL_UPDATE).

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:42:
text_selection_triggered_ = false;
On 2014/06/24 15:38:37, jdduke wrote:
> ACTION_CANCEL shouldn't reset the state, as the cancel event may trigger some
> "cleanup" gestures that would then get forwarded. 

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:54: if
(text_selection_triggered_) {
On 2014/06/24 15:38:37, jdduke wrote:
> Nit:
> 
> if (!text_selection_triggered_)
>   return false;
> 
> if (drop_remaining_sequence_)
>   return true;
> 
> ...

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:61:
text_selection_triggered_ = false;
Added drop_remaining_sequence_

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:70: case
ui::ET_GESTURE_SCROLL_END: {
On 2014/06/24 15:38:36, jdduke wrote:
> No need to listen to SCROLL_END, SCROLL_UPDATE should be sufficient.

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:86:
DCHECK(event.GetPointerCount() > 0);
On 2014/06/24 15:38:37, jdduke wrote:
> Nit: DCHECK_GT(event.GetPointerCount(), 0);

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.h (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:14: class
GestureEventData;
On 2014/06/24 15:15:23, benm wrote:
> s/class/struct/

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:40: // Returns
whether it will consume the event.
On 2014/06/24 15:38:37, jdduke wrote:
> Maybe put a note here that this should be called *after* gesture detection but
> before associated gestures have been dispatched.

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector_unittest.cc
(right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector_unittest.cc:87: {  //
Touched with a stylus, with two buttons pressed.
On 2014/06/24 15:38:37, jdduke wrote:
> Do you know what the SPen does in this case? Will pressing both buttons
prevent
> selection?
SPen has only one button. We may want to assign some other behavior to two
button press, so I blocked triggering for now.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector_unittest.cc:148: // 4.
LONG_PRESS (This can occur before or after ACTION_UP.)
On 2014/06/24 15:38:37, jdduke wrote:
> Hmm, LONG_PRESS should only ever occur before ACTION_UP, are you seeing
> otherwise? I would just start a new ACTION_DOWN sequence to simulate
LONG_PRESS
> here

Removed as LONG_PRESS requirement is still in talks.

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

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1296: if
(gesture_text_selector_->OnGestureEvent(gesture)) {
On 2014/06/24 15:38:37, jdduke wrote:
> Nit: No braces necessary (just add a line break).

Done.

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1454:
static_cast<WebContentsImpl*>(content_view_core_->GetWebContents())->
On 2014/06/24 15:38:37, jdduke wrote:
> We'll want to ask sievers@ or aelias@ about WebContentsImpl usage here, I had
> thought we used WebContents directly in RWHVAndroid, but it looks like this is
> the first such usage.  

Added a TODO

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:373:
scoped_ptr<GestureTextSelector> gesture_text_selector_;
On 2014/06/24 15:38:37, jdduke wrote:
> I don't think we need to heap allocate this, I would just make it a regular
> value member variable and move it below the |gesture_provider_| declaration.

Done.

https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.cc (right):

https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.cc:158: DCHECK_GT(pointer_count,
pointer_index);
On 2014/06/24 15:38:37, jdduke wrote:
> Nit: For consistency make this "DCHECK_LT(pointer_index, pointer_count)", also
> below in |SetToolType()|.

Done.

[tdresser, 2014-06-25 12:25:41]

On 2014/06/25 07:26:21, Changwan Ryu wrote:
> PTAL
> 
> Please note that long press handling has been removed for now as our partner
> hasn't decided whether they want the feature or not. If necessary, I'll upload
a
> separate CL.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/input/gesture_text_selector.cc (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:27: if
> (text_selection_triggered_) {
> On 2014/06/24 15:38:36, jdduke wrote:
> > Nit: To avoid nesting invert the condition and early return, i.e.,
> > 
> > if (!text_selection_triggered_)
> >   return false;
> > 
> > ...
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:28:
> client_->ShowSelectionHandlesAutomatically();
> On 2014/06/24 15:38:36, jdduke wrote:
> > We should probably only call this upon ACTION_DOWN.
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:38:
> client_->SelectRange(anchor_x_, anchor_y_,
> On 2014/06/24 15:38:36, jdduke wrote:
> > Hmm, we shouldn't use SelectRange with both ACTION_DOWN and SCROLL_UPDATE,
it
> > should only be the latter (with SCROLL_UPDATE).
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:42:
> text_selection_triggered_ = false;
> On 2014/06/24 15:38:37, jdduke wrote:
> > ACTION_CANCEL shouldn't reset the state, as the cancel event may trigger
some
> > "cleanup" gestures that would then get forwarded. 
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:54: if
> (text_selection_triggered_) {
> On 2014/06/24 15:38:37, jdduke wrote:
> > Nit:
> > 
> > if (!text_selection_triggered_)
> >   return false;
> > 
> > if (drop_remaining_sequence_)
> >   return true;
> > 
> > ...
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:61:
> text_selection_triggered_ = false;
> Added drop_remaining_sequence_
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:70: case
> ui::ET_GESTURE_SCROLL_END: {
> On 2014/06/24 15:38:36, jdduke wrote:
> > No need to listen to SCROLL_END, SCROLL_UPDATE should be sufficient.
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.cc:86:
> DCHECK(event.GetPointerCount() > 0);
> On 2014/06/24 15:38:37, jdduke wrote:
> > Nit: DCHECK_GT(event.GetPointerCount(), 0);
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/input/gesture_text_selector.h (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.h:14: class
> GestureEventData;
> On 2014/06/24 15:15:23, benm wrote:
> > s/class/struct/
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector.h:40: // Returns
> whether it will consume the event.
> On 2014/06/24 15:38:37, jdduke wrote:
> > Maybe put a note here that this should be called *after* gesture detection
but
> > before associated gestures have been dispatched.
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/input/gesture_text_selector_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector_unittest.cc:87: { 
//
> Touched with a stylus, with two buttons pressed.
> On 2014/06/24 15:38:37, jdduke wrote:
> > Do you know what the SPen does in this case? Will pressing both buttons
> prevent
> > selection?
> SPen has only one button. We may want to assign some other behavior to two
> button press, so I blocked triggering for now.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/input/gesture_text_selector_unittest.cc:148: //
4.
> LONG_PRESS (This can occur before or after ACTION_UP.)
> On 2014/06/24 15:38:37, jdduke wrote:
> > Hmm, LONG_PRESS should only ever occur before ACTION_UP, are you seeing
> > otherwise? I would just start a new ACTION_DOWN sequence to simulate
> LONG_PRESS
> > here
> 
> Removed as LONG_PRESS requirement is still in talks.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.cc:1296: if
> (gesture_text_selector_->OnGestureEvent(gesture)) {
> On 2014/06/24 15:38:37, jdduke wrote:
> > Nit: No braces necessary (just add a line break).
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.cc:1454:
> static_cast<WebContentsImpl*>(content_view_core_->GetWebContents())->
> On 2014/06/24 15:38:37, jdduke wrote:
> > We'll want to ask sievers@ or aelias@ about WebContentsImpl usage here, I
had
> > thought we used WebContents directly in RWHVAndroid, but it looks like this
is
> > the first such usage.  
> 
> Added a TODO
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_android.h (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.h:373:
> scoped_ptr<GestureTextSelector> gesture_text_selector_;
> On 2014/06/24 15:38:37, jdduke wrote:
> > I don't think we need to heap allocate this, I would just make it a regular
> > value member variable and move it below the |gesture_provider_| declaration.
> 
> Done.
> 
>
https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
> File ui/events/gesture_detection/mock_motion_event.cc (right):
> 
>
https://codereview.chromium.org/342633003/diff/220001/ui/events/gesture_detec...
> ui/events/gesture_detection/mock_motion_event.cc:158: DCHECK_GT(pointer_count,
> pointer_index);
> On 2014/06/24 15:38:37, jdduke wrote:
> > Nit: For consistency make this "DCHECK_LT(pointer_index, pointer_count)",
also
> > below in |SetToolType()|.
> 
> Done.

I'd prefer we leave GestureTextSelector where it is. I'm worried that if we move
it to ui/events/gesture_detection, we'll start a trend and end up with a bunch
of code in ui/events/gesture_detection which is unrelated to the detection of
gestures.

[jdduke (slow), 2014-06-25 14:54:19]

Hmm, that's a shame about tap/long press, as that's where most of the complexity
is here (i.e., if we don't need to detect long press or tap, we could easily
just consume the MotionEvent stream *before* it goes through gesture detection
). We can always make that simplification later.

Adding aelias@ as render_widget_host_view_android.{cc,h} OWNER.

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:25:
text_selection_triggered_ = ShouldStartTextSelection(event);
Maybe add a comment here along the lines of "// Only trigger selection on
ACTION_DOWN to prevent partial touch or gesture sequences from being forwarded."

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.h (right):

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:10: #include
"base/memory/scoped_ptr.h"
Don't think we need scoped_ptr.h anymore.

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:42: // Returns
whether it will consume the event.
Nit: Let's add a line break between the two methods.

https://codereview.chromium.org/342633003/diff/240001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/342633003/diff/240001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:272: sources = [
I think if we move mock_motion_event to ui/events/test, we shouldn't need to do
this.

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp#ne...
ui/events/events.gyp:275: 'gesture_detection/mock_motion_event.cc',
What if we move |mock_motion_event.{h,cc}| to ui/events/test, and put it in the
|events_test_support| target? Then both events and content tests will pull it in
automatically, and this could be useful for other targets as well.

[tdresser, 2014-06-25 15:20:54]

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp#ne...
ui/events/events.gyp:275: 'gesture_detection/mock_motion_event.cc',
On 2014/06/25 14:54:19, jdduke wrote:
> What if we move |mock_motion_event.{h,cc}| to ui/events/test, and put it in
the
> |events_test_support| target? Then both events and content tests will pull it
in
> automatically, and this could be useful for other targets as well. 

That sounds like a good plan to me.

[Changwan Ryu, 2014-06-26 00:05:27]

Adding sadrul@ for ui/events/BUILD.gn approval

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:25:
text_selection_triggered_ = ShouldStartTextSelection(event);
On 2014/06/25 14:54:19, jdduke wrote:
> Maybe add a comment here along the lines of "// Only trigger selection on
> ACTION_DOWN to prevent partial touch or gesture sequences from being
forwarded."

Done.

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.h (right):

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:10: #include
"base/memory/scoped_ptr.h"
On 2014/06/25 14:54:19, jdduke wrote:
> Don't think we need scoped_ptr.h anymore.
Removed.

https://codereview.chromium.org/342633003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.h:42: // Returns
whether it will consume the event.
On 2014/06/25 14:54:19, jdduke wrote:
> Nit: Let's add a line break between the two methods.

Done.

https://codereview.chromium.org/342633003/diff/240001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/342633003/diff/240001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:272: sources = [
On 2014/06/25 14:54:19, jdduke wrote:
> I think if we move mock_motion_event to ui/events/test, we shouldn't need to
do
> this.

Done.

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/342633003/diff/240001/ui/events/events.gyp#ne...
ui/events/events.gyp:275: 'gesture_detection/mock_motion_event.cc',
On 2014/06/25 15:20:54, tdresser wrote:
> On 2014/06/25 14:54:19, jdduke wrote:
> > What if we move |mock_motion_event.{h,cc}| to ui/events/test, and put it in
> the
> > |events_test_support| target? Then both events and content tests will pull
it
> in
> > automatically, and this could be useful for other targets as well. 
> 
> That sounds like a good plan to me.

Done.

[jdduke (slow), 2014-06-26 00:33:21]

content/browser/android, content/browser/renderer_host/input and
ui/events/gesture_detection lgtm with one question.

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:48:
client_->ShowSelectionHandlesAutomatically();
I'm curious how the handles look while you're dragging the selection? Do they
pop in and out or interfere with the stylus? If they look OK, then let's move
the |client_->ShowSelectionHandlesAutomatically();| call to
|ET_GESTURE_SCROLL_BEGIN|.  Otherwise, we may need to figure out another way to
show them, perhaps by hooking into ET_GESTURE_SCROLL_END?

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:54: class
GestureTextSelector;
Nit: No need for forward decl when we include the header.

https://codereview.chromium.org/342633003/diff/260001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/342633003/diff/260001/content/content_tests.g...
content/content_tests.gypi:364: '../ui/events/events.gyp:events_test_support',
Hmm, do we need this? I see events_test_support in test_support_content, which
is already included?

[aelias_OOO_until_Jul13, 2014-06-26 01:12:39]

lgtm for render_widget_host_view_android

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

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1453: //
TODO(changwan): check if we'd want to use WebContentsImpl directly without
It doesn't matter, they'll be merged eventually anyway.  You can remove this
TODO.

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:238: virtual
void ShowSelectionHandlesAutomatically() OVERRIDE;
nit: move these up next to the other virtual methods

[Changwan Ryu, 2014-06-26 02:54:11]

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_text_selector.cc (right):

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/input/gesture_text_selector.cc:48:
client_->ShowSelectionHandlesAutomatically();
On 2014/06/26 00:33:21, jdduke wrote:
> I'm curious how the handles look while you're dragging the selection? Do they
> pop in and out or interfere with the stylus? If they look OK, then let's move
> the |client_->ShowSelectionHandlesAutomatically();| call to
> |ET_GESTURE_SCROLL_BEGIN|.  Otherwise, we may need to figure out another way
to
> show them, perhaps by hooking into ET_GESTURE_SCROLL_END?

Moved ShowSelectionHandlesAutomatically to GESTURE_SCROLL_BEGIN.
But the current implementation of ShowSelectionHandlesAutomatically() only shows
handles when it's called before we change the selection, so I couldn't do it for
now (added a TODO). Please consider this use case when you work on the
composited selection handles.

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

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1453: //
TODO(changwan): check if we'd want to use WebContentsImpl directly without
On 2014/06/26 01:12:39, aelias wrote:
> It doesn't matter, they'll be merged eventually anyway.  You can remove this
> TODO.

Removed.

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:54: class
GestureTextSelector;
On 2014/06/26 00:33:21, jdduke wrote:
> Nit: No need for forward decl when we include the header.

Done.

https://codereview.chromium.org/342633003/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:238: virtual
void ShowSelectionHandlesAutomatically() OVERRIDE;
On 2014/06/26 01:12:39, aelias wrote:
> nit: move these up next to the other virtual methods

Done.

https://codereview.chromium.org/342633003/diff/260001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/342633003/diff/260001/content/content_tests.g...
content/content_tests.gypi:364: '../ui/events/events.gyp:events_test_support',
On 2014/06/26 00:33:21, jdduke wrote:
> Hmm, do we need this? I see events_test_support in test_support_content, which
> is already included?

Removed

[Changwan Ryu, 2014-06-26 02:54:30]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2014-06-26 02:54:32]

The CQ bit was unchecked by changwan@chromium.org

[Changwan Ryu, 2014-06-26 04:19:36]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-26 04:20:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/342633003/280001

[Changwan Ryu, 2014-06-26 04:44:06]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-26 04:45:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/342633003/320001

[commit-bot: I haz the power, 2014-06-26 05:33:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/21781)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/25948)

[Changwan Ryu, 2014-06-26 05:34:47]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-26 05:35:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/342633003/340001

[Changwan Ryu, 2014-06-26 06:07:15]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-26 06:10:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/342633003/360001

[Changwan Ryu, 2014-06-26 06:43:48]

Sadrul, could you approve the changes around mock_motion_event (build and
moving)?

[sadrul, 2014-06-26 09:26:29]

lgtm

[commit-bot: I haz the power, 2014-06-26 13:04:15]

Change committed as 279991