Support longpress drag selection

Support longpress drag selection

Allow users to extend a longpress-triggered selection by dragging their
finger continuously after the longpress. Enabled using the
"--enable-longpress-drag-selection" flag on Android.

BUG=466749
Committed: https://crrev.com/3f2a3abd4acdd336e4c76ce963663507bd05e7bd
Cr-Commit-Position: refs/heads/master@{#332439}

[mfomitchev, 2015-04-18 16:09:08]

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

[mfomitchev, 2015-04-18 16:09:08]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:166:
((touch_sequence_start_position_ - position).LengthSquared() <
I must be missing something, but why do we need to confirm here that we are
close to touch_sequence_start_position_? If this is resolved as a long press by
the platform, then doesn't that already mean that the current position is close
to the DOWN position (by whatever metrics are used by the platform)? Why do we
need to impose our own custom long press qualifications on top of the already
existing ones?

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:500:
SetTemporarilyHiddenForLongPressDrag(false);
It would be nice to do UMA logging for this usecase. Ideally I think we would
want to be able to distinguish between handle drag and long press drag from UMA
events.

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:169: // TODO(jdduke): Factor out
longpress drag-specific logic into a more isolated
I wonder if we can reuse some of the TouchHandle code for this. The 'long press
logic unit' seems somewhat similar to the TouchHandle in the way it interacts
with TSC. Perhaps we can factor out the common functionality in a superclass
(TouchSelectionEventProcessor)? 
- The long press logic unit would either extend this superclass or use it
directly. 
- TouchHandle would extend it. 
- TSC would interact with the handles and the long press logic unit through this
superclass's interfact (i.e. the instance of start_selection_handle_ would be
TouchSelectionEventProcessor). 
- TouchHandleClient would become TouchSelectionEventProcessorClient

Then instead of calling WillHandleTouchEventForLongPressDrag(event), we'd just
call longpress_drag_processor_->WillHandleTouchEvent(event), same way we do for
handles. Instead of storing temporarily_hidden_for_longpress_drag_, we'd just
check longpress_drag_processor_->is_dragging(), etc. Do you think something like
this would work?

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:171: bool
temporarily_hidden_for_longpress_drag_;
handles_hidden_for_longpress_drag_? Otherwise it's not clear what's hidden.Same
with the setter.

[jdduke (slow), 2015-04-21 19:25:57]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:166:
((touch_sequence_start_position_ - position).LengthSquared() <
On 2015/04/18 16:09:08, mfomitchev wrote:
> I must be missing something, but why do we need to confirm here that we are
> close to touch_sequence_start_position_? If this is resolved as a long press
by
> the platform, then doesn't that already mean that the current position is
close
> to the DOWN position (by whatever metrics are used by the platform)? Why do we
> need to impose our own custom long press qualifications on top of the already
> existing ones?

So, the issue right now is that touch events are fed to the TSC when they arrive
in the browser, but gesture events are fed to the TSC before they are sent to
the renderer. If a touch/gesture sequence is delayed, this means there may be
intervening gestures between the touch and gesture events being observed.

It's a rather nasty issue, which is mostly a consequence of the fact that we use
the longpress/tap signals to allow showing of the selection handles (so we want
to observe them at the very last moment before they're sent to Blink). What we
should really be doing is flipping a user gesture bit on the selection (in
Blink) that propagates back to the browser to tell us whether the selection is
caused by a user gesture, and whether we should show the handles.

The possible solutions are:

1) use the heuristic I have now, which may not be exact, but should handle the
common delayed cases
2) move touch filtering down in the pipeline, so we filter touches just before
they're sent to the renderer - this has some nasty side effects, and also
doesn't handle the case that the gesture sequence itself is delayed
3) bundle the user gesture bits with the selection, as I said before, and
instead use *scroll* gesture event filtering to drive the drag selection
(instead of touch events).

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:169: // TODO(jdduke): Factor out
longpress drag-specific logic into a more isolated
On 2015/04/18 16:09:08, mfomitchev wrote:
> I wonder if we can reuse some of the TouchHandle code for this. The 'long
press
> logic unit' seems somewhat similar to the TouchHandle in the way it interacts
> with TSC. Perhaps we can factor out the common functionality in a superclass
> (TouchSelectionEventProcessor)? 
> - The long press logic unit would either extend this superclass or use it
> directly. 
> - TouchHandle would extend it. 
> - TSC would interact with the handles and the long press logic unit through
this
> superclass's interfact (i.e. the instance of start_selection_handle_ would be
> TouchSelectionEventProcessor). 
> - TouchHandleClient would become TouchSelectionEventProcessorClient
> 
> Then instead of calling WillHandleTouchEventForLongPressDrag(event), we'd just
> call longpress_drag_processor_->WillHandleTouchEvent(event), same way we do
for
> handles. Instead of storing temporarily_hidden_for_longpress_drag_, we'd just
> check longpress_drag_processor_->is_dragging(), etc. Do you think something
like
> this would work? 

I think this is very reasonable. I'd been planning to also pull the
|StylusSelectionController| into this class and have it serve a similar function
(i.e., delegate to it touch events, have it manipulate the selection as a
side-effect). I'll see if I can tease out a simple/common interface, not sure if
we need to force both the handles/drag processing through identical interface,
but I think sharing logic would be beneficial.

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:171: bool
temporarily_hidden_for_longpress_drag_;
On 2015/04/18 16:09:08, mfomitchev wrote:
> handles_hidden_for_longpress_drag_? Otherwise it's not clear what's
hidden.Same
> with the setter.

Hmm, well this is a parallel of the existing |temporarily_hidden_| flag, which I
guess is also somewhat ambiguous. I'll update.

[jdduke (slow), 2015-04-21 19:30:47]

On 2015/04/21 19:25:57, jdduke wrote:
> |StylusSelectionController| into this class and have it serve a similar
function

StylusTextSelector that is.

[mfomitchev, 2015-04-21 21:08:57]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:166:
((touch_sequence_start_position_ - position).LengthSquared() <
On 2015/04/21 19:25:57, jdduke wrote:
> On 2015/04/18 16:09:08, mfomitchev wrote:
> > I must be missing something, but why do we need to confirm here that we are
> > close to touch_sequence_start_position_? If this is resolved as a long press
> by
> > the platform, then doesn't that already mean that the current position is
> close
> > to the DOWN position (by whatever metrics are used by the platform)? Why do
we
> > need to impose our own custom long press qualifications on top of the
already
> > existing ones?
> 
> So, the issue right now is that touch events are fed to the TSC when they
arrive
> in the browser, but gesture events are fed to the TSC before they are sent to
> the renderer. If a touch/gesture sequence is delayed, this means there may be
> intervening gestures between the touch and gesture events being observed.
> 
> It's a rather nasty issue, which is mostly a consequence of the fact that we
use
> the longpress/tap signals to allow showing of the selection handles (so we
want
> to observe them at the very last moment before they're sent to Blink). What we
> should really be doing is flipping a user gesture bit on the selection (in
> Blink) that propagates back to the browser to tell us whether the selection is
> caused by a user gesture, and whether we should show the handles.
> 
> The possible solutions are:
> 
> 1) use the heuristic I have now, which may not be exact, but should handle the
> common delayed cases
> 2) move touch filtering down in the pipeline, so we filter touches just before
> they're sent to the renderer - this has some nasty side effects, and also
> doesn't handle the case that the gesture sequence itself is delayed
> 3) bundle the user gesture bits with the selection, as I said before, and
> instead use *scroll* gesture event filtering to drive the drag selection
> (instead of touch events).

If do #3, then the webpage will get all the touch events and would be able to
react to them - even if they resulted in scroll events which are only meant to
move the handles and not impact the webpage in any way. That won't be right...
And if we consume the touch events, the scroll gestures won't be generated.

I talked with tdresser briefly about this, and he didn't think the current
implementation could support generating scroll gestures while preventing touch
events from getting to the renderer (unless we host our own separate gesture
recognizer inside TSC). Do you agree with this assessment or are we missing
something? If you agree - we should probably start a discussion on how to solve
this. Incidentally we have a similar issue with GestureNav.

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:169: // TODO(jdduke): Factor out
longpress drag-specific logic into a more isolated
On 2015/04/21 19:25:57, jdduke wrote:
> On 2015/04/18 16:09:08, mfomitchev wrote:
> > I wonder if we can reuse some of the TouchHandle code for this. The 'long
> press
> > logic unit' seems somewhat similar to the TouchHandle in the way it
interacts
> > with TSC. Perhaps we can factor out the common functionality in a superclass
> > (TouchSelectionEventProcessor)? 
> > - The long press logic unit would either extend this superclass or use it
> > directly. 
> > - TouchHandle would extend it. 
> > - TSC would interact with the handles and the long press logic unit through
> this
> > superclass's interfact (i.e. the instance of start_selection_handle_ would
be
> > TouchSelectionEventProcessor). 
> > - TouchHandleClient would become TouchSelectionEventProcessorClient
> > 
> > Then instead of calling WillHandleTouchEventForLongPressDrag(event), we'd
just
> > call longpress_drag_processor_->WillHandleTouchEvent(event), same way we do
> for
> > handles. Instead of storing temporarily_hidden_for_longpress_drag_, we'd
just
> > check longpress_drag_processor_->is_dragging(), etc. Do you think something
> like
> > this would work? 
> 
> I think this is very reasonable. I'd been planning to also pull the
> |StylusSelectionController| into this class and have it serve a similar
function
> (i.e., delegate to it touch events, have it manipulate the selection as a
> side-effect). I'll see if I can tease out a simple/common interface, not sure
if
> we need to force both the handles/drag processing through identical interface,
> but I think sharing logic would be beneficial.

Do you mean StylusTextSelector?

[mfomitchev, 2015-04-21 21:09:33]

On 2015/04/21 19:30:47, jdduke wrote:
> On 2015/04/21 19:25:57, jdduke wrote:
> > |StylusSelectionController| into this class and have it serve a similar
> function
> 
> StylusTextSelector that is.

ah :)
Makes sense

[jdduke (slow), 2015-04-21 21:28:33]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:166:
((touch_sequence_start_position_ - position).LengthSquared() <
On 2015/04/21 21:08:57, mfomitchev wrote:
> On 2015/04/21 19:25:57, jdduke wrote:
> > On 2015/04/18 16:09:08, mfomitchev wrote:
> > > I must be missing something, but why do we need to confirm here that we
are
> > > close to touch_sequence_start_position_? If this is resolved as a long
press
> > by
> > > the platform, then doesn't that already mean that the current position is
> > close
> > > to the DOWN position (by whatever metrics are used by the platform)? Why
do
> we
> > > need to impose our own custom long press qualifications on top of the
> already
> > > existing ones?
> > 
> > So, the issue right now is that touch events are fed to the TSC when they
> arrive
> > in the browser, but gesture events are fed to the TSC before they are sent
to
> > the renderer. If a touch/gesture sequence is delayed, this means there may
be
> > intervening gestures between the touch and gesture events being observed.
> > 
> > It's a rather nasty issue, which is mostly a consequence of the fact that we
> use
> > the longpress/tap signals to allow showing of the selection handles (so we
> want
> > to observe them at the very last moment before they're sent to Blink). What
we
> > should really be doing is flipping a user gesture bit on the selection (in
> > Blink) that propagates back to the browser to tell us whether the selection
is
> > caused by a user gesture, and whether we should show the handles.
> > 
> > The possible solutions are:
> > 
> > 1) use the heuristic I have now, which may not be exact, but should handle
the
> > common delayed cases
> > 2) move touch filtering down in the pipeline, so we filter touches just
before
> > they're sent to the renderer - this has some nasty side effects, and also
> > doesn't handle the case that the gesture sequence itself is delayed
> > 3) bundle the user gesture bits with the selection, as I said before, and
> > instead use *scroll* gesture event filtering to drive the drag selection
> > (instead of touch events).
> 
> If do #3, then the webpage will get all the touch events and would be able to
> react to them - even if they resulted in scroll events which are only meant to
> move the handles and not impact the webpage in any way. That won't be right...
> And if we consume the touch events, the scroll gestures won't be generated.
> 

Indeed, we'd be at the mercy of the web page. This likely wouldn't be an issue
for most sites, but for JS-scrolling sites it might prevent longpress+drag
selection entirely.

> I talked with tdresser briefly about this, and he didn't think the current
> implementation could support generating scroll gestures while preventing touch
> events from getting to the renderer (unless we host our own separate gesture
> recognizer inside TSC). Do you agree with this assessment or are we missing
> something? If you agree - we should probably start a discussion on how to
solve
> this. Incidentally we have a similar issue with GestureNav.

Yup, there's no clean way to do this with the existing touch/gesture pipeline.
Note that the StylusTextSelector does in fact host its own gesture detector for
scroll/tap detection. We could do a similar thing here, but it wouldn't solve
the issue you raised with this particular code segment. We still need a way to
"hand off" control of the touch/gesture sequence.

I felt like the above heuristic was a simple compromise that should handle the
99.9% case without too much machinery/plumbing, but long term there's likely a
better fix out there.

[mfomitchev, 2015-04-21 22:20:29]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:166:
((touch_sequence_start_position_ - position).LengthSquared() <
On 2015/04/21 21:28:33, jdduke wrote:
> On 2015/04/21 21:08:57, mfomitchev wrote:
> > On 2015/04/21 19:25:57, jdduke wrote:
> > > On 2015/04/18 16:09:08, mfomitchev wrote:
> > > > I must be missing something, but why do we need to confirm here that we
> are
> > > > close to touch_sequence_start_position_? If this is resolved as a long
> press
> > > by
> > > > the platform, then doesn't that already mean that the current position
is
> > > close
> > > > to the DOWN position (by whatever metrics are used by the platform)? Why
> do
> > we
> > > > need to impose our own custom long press qualifications on top of the
> > already
> > > > existing ones?
> > > 
> > > So, the issue right now is that touch events are fed to the TSC when they
> > arrive
> > > in the browser, but gesture events are fed to the TSC before they are sent
> to
> > > the renderer. If a touch/gesture sequence is delayed, this means there may
> be
> > > intervening gestures between the touch and gesture events being observed.
> > > 
> > > It's a rather nasty issue, which is mostly a consequence of the fact that
we
> > use
> > > the longpress/tap signals to allow showing of the selection handles (so we
> > want
> > > to observe them at the very last moment before they're sent to Blink).
What
> we
> > > should really be doing is flipping a user gesture bit on the selection (in
> > > Blink) that propagates back to the browser to tell us whether the
selection
> is
> > > caused by a user gesture, and whether we should show the handles.
> > > 
> > > The possible solutions are:
> > > 
> > > 1) use the heuristic I have now, which may not be exact, but should handle
> the
> > > common delayed cases
> > > 2) move touch filtering down in the pipeline, so we filter touches just
> before
> > > they're sent to the renderer - this has some nasty side effects, and also
> > > doesn't handle the case that the gesture sequence itself is delayed
> > > 3) bundle the user gesture bits with the selection, as I said before, and
> > > instead use *scroll* gesture event filtering to drive the drag selection
> > > (instead of touch events).
> > 
> > If do #3, then the webpage will get all the touch events and would be able
to
> > react to them - even if they resulted in scroll events which are only meant
to
> > move the handles and not impact the webpage in any way. That won't be
right...
> > And if we consume the touch events, the scroll gestures won't be generated.
> > 
> 
> Indeed, we'd be at the mercy of the web page. This likely wouldn't be an issue
> for most sites, but for JS-scrolling sites it might prevent longpress+drag
> selection entirely.
> 
> > I talked with tdresser briefly about this, and he didn't think the current
> > implementation could support generating scroll gestures while preventing
touch
> > events from getting to the renderer (unless we host our own separate gesture
> > recognizer inside TSC). Do you agree with this assessment or are we missing
> > something? If you agree - we should probably start a discussion on how to
> solve
> > this. Incidentally we have a similar issue with GestureNav.
> 
> Yup, there's no clean way to do this with the existing touch/gesture pipeline.
> Note that the StylusTextSelector does in fact host its own gesture detector
for
> scroll/tap detection. We could do a similar thing here, but it wouldn't solve
> the issue you raised with this particular code segment. We still need a way to
> "hand off" control of the touch/gesture sequence.
> 
> I felt like the above heuristic was a simple compromise that should handle the
> 99.9% case without too much machinery/plumbing, but long term there's likely a
> better fix out there.

Agreed.
So.. it seems like we could get a long press during a long press drag:
- long press
- lift the finger before long press is generated/processed by the browser and
quickly press in the same spot again
- generate the second long press and start moving

--> the second long press will arrive while we are long press-dragging.. and
then there will be OnSelectionChanged().. How bad will this screw us up? Is
there any way we could look at some sort of time stamps to prevent this?

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:501:
consume_remaining_motion_for_longpress_drag_ = false;
When the user lifts the finger at the end of the longpress-drag - do we want to
show the handles? If not, I think subsequently tapping on the selection should
show the handles...

[jdduke (slow), 2015-04-21 22:29:34]

On 2015/04/21 22:20:29, mfomitchev wrote:
> Agreed.
> So.. it seems like we could get a long press during a long press drag:
> - long press
> - lift the finger before long press is generated/processed by the browser and
> quickly press in the same spot again
> - generate the second long press and start moving
> 
> --> the second long press will arrive while we are long press-dragging.. and
> then there will be OnSelectionChanged().. How bad will this screw us up? Is
> there any way we could look at some sort of time stamps to prevent this?
> 

I couldn't think of any really terrible side effects in that case. I think the
worst case is that you scroll after the original selection, so then there's a
potential race between how the scroll transforms the original selection and how
the new touch sequence mutates the original selection. Using timestamps would be
another input we could use (i.e., drop the longpress if its timestamp is before
the current down timestamp). What do you think?

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:501:
consume_remaining_motion_for_longpress_drag_ = false;
On 2015/04/21 22:20:29, mfomitchev wrote:
> When the user lifts the finger at the end of the longpress-drag - do we want
to
> show the handles? If not, I think subsequently tapping on the selection should
> show the handles...

Yep, at least, yes if we want to match the native ANdroid implementation (as
implemented with |SetTemporarilyHiddenForLongPressDrag(false) here).

[mfomitchev, 2015-04-22 19:19:43]

On 2015/04/21 22:29:34, jdduke wrote:
> On 2015/04/21 22:20:29, mfomitchev wrote:
> > Agreed.
> > So.. it seems like we could get a long press during a long press drag:
> > - long press
> > - lift the finger before long press is generated/processed by the browser
and
> > quickly press in the same spot again
> > - generate the second long press and start moving
> > 
> > --> the second long press will arrive while we are long press-dragging.. and
> > then there will be OnSelectionChanged().. How bad will this screw us up? Is
> > there any way we could look at some sort of time stamps to prevent this?
> > 
> 
> I couldn't think of any really terrible side effects in that case. I think the
> worst case is that you scroll after the original selection, so then there's a
> potential race between how the scroll transforms the original selection and
how
> the new touch sequence mutates the original selection. Using timestamps would
be
> another input we could use (i.e., drop the longpress if its timestamp is
before
> the current down timestamp). What do you think?

If it's easy I think we should do it. But it would still be a band aid. The best
solution seems to be to address crbug.com/479834, add a user action bit, and
switch to scroll events to eliminate this issue/complexity entirely. So if using
time stamps is not easy, IMHO we should focus on the 'best solution'.

[mfomitchev, 2015-04-22 19:19:50]

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/20001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:501:
consume_remaining_motion_for_longpress_drag_ = false;
On 2015/04/21 22:29:34, jdduke wrote:
> On 2015/04/21 22:20:29, mfomitchev wrote:
> > When the user lifts the finger at the end of the longpress-drag - do we want
> to
> > show the handles? If not, I think subsequently tapping on the selection
should
> > show the handles...
> 
> Yep, at least, yes if we want to match the native ANdroid implementation (as
> implemented with |SetTemporarilyHiddenForLongPressDrag(false) here).

Ah, right. Ok, cool.

[jdduke (slow), 2015-04-22 22:05:39]

Patchset #3 (id:40001) has been deleted

[jdduke (slow), 2015-04-22 22:39:29]

I pulled out the logic into a standalone class that shares a concept of
"draggable selection" with the TouchHandle. It's not quite as clean as I'd like,
but other variants required sharing a bit more state than I'd like. Adding the
longpress timestamp was very straightforward.

The StylusTextSelector should fit in nicely with this modification. If you think
this looks reasonable I'll start adding a smoke text for TSC, and unit tests for
the LongpressDragSelector (also opening to renaming).

[mfomitchev, 2015-04-23 21:05:53]

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:71: // If initial motion is
upward, extend the starting selection bound.
By the similar logic, wouldn't we want to extend the end selection bound if the
initial motion is downward?

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:82: if
(gfx::DotProduct(start_delta, delta) >
Heh. Smart.
Long-press is going to select a single word, which I guess also means that both
start and end are going to be on the same line. So this should work fine. If
multiple words could be selected, then dealing with mixed RTL would be
problematic. Also selection starting in the end of one line and ending in the
beginning of the next line (so that both vectors are virtually in the same
direction) probably won't work very well.
So I wonder if it would make more sense to just make an explicit assumption that
start and end are on the same line and simplify this code by just comparing the
direction of horizontal movement to the relative x positions of start and end?

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:122: bool
LongPressDragSelector::IsActive() const {
This is similar to is_dragging() in TouchHandle. Perhaps we could use a common
name and put the method to the TouchSelectionDraggable interface?

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:24: float slop_length);
Can we put GetTapSlop() into TouchSelectionDraggableClient and use that instead
of passing to the constructor?

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:60: gfx::PointF selection_start_;
Hmm.. this is not DRY (http://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
Any good reason we don't add GetSelectionStart()/GetSelectionEnd() to 
TouchSelectionDraggableClient instead?

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:293: if ((drag_position -
GetStartPosition()).LengthSquared() <
Can we just pass an enum in OnDragBegin indicating whether start, end, or insert
markers were dragged instead of passing draggable and position? Then we wouldn't
need extra code for longpress_drag_selector_.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:490: void
TouchSelectionController::OnHandleVisibilityOverrideMaybeChanged() {
Can we call this something like UpdateHandleVisibility or
RefreshHandleVisibility? The current name is pretty heavy, and unless I am
missing something, it doesn't seem like it conveys much additional info..

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_draggable.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_draggable.h:30: virtual
~TouchSelectionDraggable() {}
Any reason not to add WillHandleTouchEvent to this interface?

[jdduke (slow), 2015-04-27 20:14:47]

Patchset #4 (id:80001) has been deleted

[jdduke (slow), 2015-04-27 20:24:25]

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:71: // If initial motion is
upward, extend the starting selection bound.
On 2015/04/23 21:05:53, mfomitchev wrote:
> By the similar logic, wouldn't we want to extend the end selection bound if
the
> initial motion is downward?

We do, the exceptional case is dragging 'backward', that's why we use the swap.
I changed the logic to use a bool that gets set which then dictates the extent
value, reducing some of the verbosity.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:82: if
(gfx::DotProduct(start_delta, delta) >
On 2015/04/23 21:05:53, mfomitchev wrote:
> Heh. Smart.
> Long-press is going to select a single word, which I guess also means that
both
> start and end are going to be on the same line. So this should work fine. If
> multiple words could be selected, then dealing with mixed RTL would be
> problematic. Also selection starting in the end of one line and ending in the
> beginning of the next line (so that both vectors are virtually in the same
> direction) probably won't work very well.
> So I wonder if it would make more sense to just make an explicit assumption
that
> start and end are on the same line and simplify this code by just comparing
the
> direction of horizontal movement to the relative x positions of start and end?

Yeah, without awareness of text direction, making the right call gets tricky.
Perhaps the best long term solution would be to preserve the RTL bits (we
already get them from WebSelection), and expose them here. Then we'd have
something like

1) If dominant motion up/down, select start/end.
2) Else if motion is left/right, select start/end (unless start is RTL, then
reverse).

That should do the "righter" thing for multiline selections.

For now, I'll make the comment that this may not be 100% accurate for mixed RTL
text and/or multiline longpress selections, and I'll remove the default
initialization.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:122: bool
LongPressDragSelector::IsActive() const {
On 2015/04/23 21:05:53, mfomitchev wrote:
> This is similar to is_dragging() in TouchHandle. Perhaps we could use a common
> name and put the method to the TouchSelectionDraggable interface?

Hmm, the current result would be slightly misleading as we may not technically
be dragging yet (i.e., we want to keep the handles hidden until the user lifts
their finger and/or ends the drag). There's no real polymorphic advantage to
doing that, but I'll make the change anyway.

I still need a separate kind of bit that triggers when we show/hide the handles,
as we want the handles to remain hidden while the user's finger is down after
the selection has occurred.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:24: float slop_length);
On 2015/04/23 21:05:53, mfomitchev wrote:
> Can we put GetTapSlop() into TouchSelectionDraggableClient and use that
instead
> of passing to the constructor?

Done (added a |IsWithinTapSlop| client routine).

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:60: gfx::PointF selection_start_;
On 2015/04/23 21:05:53, mfomitchev wrote:
> Hmm.. this is not DRY (http://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
> Any good reason we don't add GetSelectionStart()/GetSelectionEnd() to 
> TouchSelectionDraggableClient instead?

"DRY" is a guideline, not a religion. I consider this change to be of the
caching type, which doesn't violate "DRY"; there's still one true source of
truth and the ancestry is clear. Moreover, I'm not sure polling the selection is
any more clear than getting a "push", as we're really interested in the
coordinates when the selection is first shown. In general, I tend to prefer
"KISS" (I too can quote wikipedia...
http://en.wikipedia.org/wiki/KISS_principle) =/.

What I can do is put the getters on a LongPressDragSelector-specific interface
(I wanted to avoid creating one, but maybe that's just laziness).

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:293: if ((drag_position -
GetStartPosition()).LengthSquared() <
On 2015/04/23 21:05:53, mfomitchev wrote:
> Can we just pass an enum in OnDragBegin indicating whether start, end, or
insert
> markers were dragged instead of passing draggable and position? Then we
wouldn't
> need extra code for longpress_drag_selector_.

The problem is that the TouchHandle has no awareness of the start/end of a
selection, neither does it know to which it corresponds.

At some point, it might make sense to shift *all* of the responsibility of
determining the handle orientation to the handle itself, i.e., we tell it
whether it's start/end/insertion/, the text orientation (LTR), and the viewport,
and let it decide the best visual representation/orientation. To this end, we
could plumb the RTL bits through instead of the left/right bits (in
RenderWidgetCompositor), propagate those, and it will fit in nicely with
crrev.com/481683003. However, I'd prefer that happen as a separate patch.

The other reason I hesitated adding such was b/c eventually StylusTextSelector
will live here and be a TouchSelectionDraggable, however, its semantics are
quite different in that it triggers a drag selection *before* any selection is
active. However, no need to optimize that case now, so that was only a minor
factor.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:490: void
TouchSelectionController::OnHandleVisibilityOverrideMaybeChanged() {
On 2015/04/23 21:05:53, mfomitchev wrote:
> Can we call this something like UpdateHandleVisibility or
> RefreshHandleVisibility? The current name is pretty heavy, and unless I am
> missing something, it doesn't seem like it conveys much additional info..

Done.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_draggable.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_draggable.h:30: virtual
~TouchSelectionDraggable() {}
On 2015/04/23 21:05:53, mfomitchev wrote:
> Any reason not to add WillHandleTouchEvent to this interface?

Hmm, but what functional purpose would it serve? We'll never use these objects
polymorphically, so it would just be an indicator that these things tend to
handle touch events. I'll make the change anyway I guess.

[mfomitchev, 2015-04-28 02:17:30]

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:71: // If initial motion is
upward, extend the starting selection bound.
Ah, right, I misread the code.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:82: if
(gfx::DotProduct(start_delta, delta) >
On 2015/04/27 20:24:25, jdduke wrote:
> On 2015/04/23 21:05:53, mfomitchev wrote:
> > Heh. Smart.
> > Long-press is going to select a single word, which I guess also means that
> both
> > start and end are going to be on the same line. So this should work fine. If
> > multiple words could be selected, then dealing with mixed RTL would be
> > problematic. Also selection starting in the end of one line and ending in
the
> > beginning of the next line (so that both vectors are virtually in the same
> > direction) probably won't work very well.
> > So I wonder if it would make more sense to just make an explicit assumption
> that
> > start and end are on the same line and simplify this code by just comparing
> the
> > direction of horizontal movement to the relative x positions of start and
end?
> 
> Yeah, without awareness of text direction, making the right call gets tricky.
> Perhaps the best long term solution would be to preserve the RTL bits (we
> already get them from WebSelection), and expose them here. Then we'd have
> something like
> 
> 1) If dominant motion up/down, select start/end.
> 2) Else if motion is left/right, select start/end (unless start is RTL, then
> reverse).

We can already make the decision based on whether the motion is left/right. We
don't need to know if the text is RTL, we can just look at whether
(selection_start.x() - selection_end.x()) is the same sign as delta.x().

Something like this:

if ((delta.x() > 0) && (selection_start.x() - selection_end.x() < 0) ||
    (delta.x() < 0) && (selection_start.x() - selection_end.x() > 0)) {
  extend_selection_start = true;
}

In practice with long press both handles are going to be on the same line, so it
seems this should work fine, and it's a lot more readable than doing vector
algebra.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:122: bool
LongPressDragSelector::IsActive() const {
On 2015/04/27 20:24:25, jdduke wrote:
> On 2015/04/23 21:05:53, mfomitchev wrote:
> > This is similar to is_dragging() in TouchHandle. Perhaps we could use a
common
> > name and put the method to the TouchSelectionDraggable interface?
> 
> Hmm, the current result would be slightly misleading as we may not technically
> be dragging yet (i.e., we want to keep the handles hidden until the user lifts
> their finger and/or ends the drag). There's no real polymorphic advantage to
> doing that, but I'll make the change anyway.
> 
> I still need a separate kind of bit that triggers when we show/hide the
handles,
> as we want the handles to remain hidden while the user's finger is down after
> the selection has occurred.

Sorry, I should've clarified my comment. I was suggesting we come up with a name
that would work for both IsActive() for LongPressDragSelector (LPDS) and
is_dragging() for the TouchHandle (TH). E.g. simply using IsActive might work
okay, i.e. the handle becomes active when it you press on it, and the LPDS
becomes active when the selection appears. Note that we don't actually care if
the handle is being _dragged_ when we check is_dragging() on the handle from
TSC, we just care if it's the handle the user is currently interacting with. If
we dispatched BeginDrag in MOVE instead of DOWN, DOWN-based IsActive would still
work. I like the notion of making LPDS and TH look more alike from the outside -
I think this could make the opportunities for code reuse become more apparent,
which is why I made the comment.

If you don't like using the same name for IsActive() and is_dragging(), that's
fine. But I don't think we should add IsDragging() to LPDS - there's really no
benefit since it's only used once, inside LPDS. And I liked IsActive more than
IsDetectingDrag, since it returns true in DRAGGING state, and it's a more
appropriate level of abstraction: from TSC's POV we don't necessarily care which
state the longpress sequence is in - we just care if it's active or not.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:60: gfx::PointF selection_start_;
On 2015/04/27 20:24:25, jdduke wrote:
> On 2015/04/23 21:05:53, mfomitchev wrote:
> > Hmm.. this is not DRY
(http://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
> > Any good reason we don't add GetSelectionStart()/GetSelectionEnd() to 
> > TouchSelectionDraggableClient instead?
> 
> "DRY" is a guideline, not a religion. I consider this change to be of the
> caching type, which doesn't violate "DRY"; there's still one true source of
> truth and the ancestry is clear. Moreover, I'm not sure polling the selection
is
> any more clear than getting a "push", as we're really interested in the
> coordinates when the selection is first shown. In general, I tend to prefer
> "KISS" (I too can quote wikipedia...
> http://en.wikipedia.org/wiki/KISS_principle) =/.

Sorry if I came off as a design snob, I guess it's hard not to do it when
linking to wiki on design terms.

> What I can do is put the getters on a LongPressDragSelector-specific interface
> (I wanted to avoid creating one, but maybe that's just laziness).

I don't think it was just laziness :) I am definitely not a fan of adding an
extra interface here if it could be avoided. If we can get rid of
OnLongPressDragDetectionStateChanged() (I posted a comment on it separately), I
think we should nuke LongPressDragSelectorClient. 

Assuming we can get rid of OnLongPressDragDetectionStateChanged(), IMHO we can
put the getters into TouchSelectionDraggableClient. I mean if you are okay with
getting those through LongPressDragSelectorClient, getting them though
TouchSelectionDraggableClient should be okay as well? If you don't like it - I
am fine with caching.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:293: if ((drag_position -
GetStartPosition()).LengthSquared() <
Yeah, I was implying we'd pass the type to the TouchHandle on creation to
support this. What you say makes sense though.

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:354: void
TouchSelectionController::OnLongPressDragDetectionStateChanged() {
I don't understand why we need this. TSC::OnSelectionChanged() sets the handle
visibility based on GetStartVisible()/GetEndVisible(), so it should already
choose the appropriate visibility. Am I missing something?

[jdduke (slow), 2015-04-28 20:37:49]

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.cc (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:82: if
(gfx::DotProduct(start_delta, delta) >
On 2015/04/28 02:17:30, mfomitchev wrote:
> On 2015/04/27 20:24:25, jdduke wrote:
> > On 2015/04/23 21:05:53, mfomitchev wrote:
> > > Heh. Smart.
> > > Long-press is going to select a single word, which I guess also means that
> > both
> > > start and end are going to be on the same line. So this should work fine.
If
> > > multiple words could be selected, then dealing with mixed RTL would be
> > > problematic. Also selection starting in the end of one line and ending in
> the
> > > beginning of the next line (so that both vectors are virtually in the same
> > > direction) probably won't work very well.
> > > So I wonder if it would make more sense to just make an explicit
assumption
> > that
> > > start and end are on the same line and simplify this code by just
comparing
> > the
> > > direction of horizontal movement to the relative x positions of start and
> end?
> > 
> > Yeah, without awareness of text direction, making the right call gets
tricky.
> > Perhaps the best long term solution would be to preserve the RTL bits (we
> > already get them from WebSelection), and expose them here. Then we'd have
> > something like
> > 
> > 1) If dominant motion up/down, select start/end.
> > 2) Else if motion is left/right, select start/end (unless start is RTL, then
> > reverse).
> 
> We can already make the decision based on whether the motion is left/right. We
> don't need to know if the text is RTL, we can just look at whether
> (selection_start.x() - selection_end.x()) is the same sign as delta.x().
> 
> Something like this:
> 
> if ((delta.x() > 0) && (selection_start.x() - selection_end.x() < 0) ||
>     (delta.x() < 0) && (selection_start.x() - selection_end.x() > 0)) {
>   extend_selection_start = true;
> }
> 
> In practice with long press both handles are going to be on the same line, so
it
> seems this should work fine, and it's a lot more readable than doing vector
> algebra.

Hmm, yeah, the math isn't quite as clear. 

That said, I was hoping the vector dot product might be slightly more robust to
multiline longpress selections (the common case I was thinking of is when you
longpress the end of a line, and the selection start and selection end are both
to the left of your finger, but on different lines. In that case, if you drag
left, you want the selection start to move, and if you drag right, you want the
selection end to move, and the vector math actually handles that nicely (the
angle is slightly smaller for the selection bound on the following line when you
drag right.. for RTL text that may not be right though).

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.cc:122: bool
LongPressDragSelector::IsActive() const {
On 2015/04/28 02:17:30, mfomitchev wrote:
> On 2015/04/27 20:24:25, jdduke wrote:
> > On 2015/04/23 21:05:53, mfomitchev wrote:
> > > This is similar to is_dragging() in TouchHandle. Perhaps we could use a
> common
> > > name and put the method to the TouchSelectionDraggable interface?
> > 
> > Hmm, the current result would be slightly misleading as we may not
technically
> > be dragging yet (i.e., we want to keep the handles hidden until the user
lifts
> > their finger and/or ends the drag). There's no real polymorphic advantage to
> > doing that, but I'll make the change anyway.
> > 
> > I still need a separate kind of bit that triggers when we show/hide the
> handles,
> > as we want the handles to remain hidden while the user's finger is down
after
> > the selection has occurred.
> 
> Sorry, I should've clarified my comment. I was suggesting we come up with a
name
> that would work for both IsActive() for LongPressDragSelector (LPDS) and
> is_dragging() for the TouchHandle (TH). E.g. simply using IsActive might work
> okay, i.e. the handle becomes active when it you press on it, and the LPDS
> becomes active when the selection appears. Note that we don't actually care if
> the handle is being _dragged_ when we check is_dragging() on the handle from
> TSC, we just care if it's the handle the user is currently interacting with.
If
> we dispatched BeginDrag in MOVE instead of DOWN, DOWN-based IsActive would
still
> work. I like the notion of making LPDS and TH look more alike from the outside
-
> I think this could make the opportunities for code reuse become more apparent,
> which is why I made the comment.
> 
> If you don't like using the same name for IsActive() and is_dragging(), that's
> fine. But I don't think we should add IsDragging() to LPDS - there's really no
> benefit since it's only used once, inside LPDS. And I liked IsActive more than
> IsDetectingDrag, since it returns true in DRAGGING state, and it's a more
> appropriate level of abstraction: from TSC's POV we don't necessarily care
which
> state the longpress sequence is in - we just care if it's active or not.

IsActive() sounds good to me.

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/60001/ui/touch_selection/long...
ui/touch_selection/longpress_drag_selector.h:60: gfx::PointF selection_start_;
On 2015/04/28 02:17:30, mfomitchev wrote:
> On 2015/04/27 20:24:25, jdduke wrote:
> > On 2015/04/23 21:05:53, mfomitchev wrote:
> > > Hmm.. this is not DRY
> (http://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
> > > Any good reason we don't add GetSelectionStart()/GetSelectionEnd() to 
> > > TouchSelectionDraggableClient instead?
> > 
> > "DRY" is a guideline, not a religion. I consider this change to be of the
> > caching type, which doesn't violate "DRY"; there's still one true source of
> > truth and the ancestry is clear. Moreover, I'm not sure polling the
selection
> is
> > any more clear than getting a "push", as we're really interested in the
> > coordinates when the selection is first shown. In general, I tend to prefer
> > "KISS" (I too can quote wikipedia...
> > http://en.wikipedia.org/wiki/KISS_principle) =/.
> 
> Sorry if I came off as a design snob, I guess it's hard not to do it when
> linking to wiki on design terms.
> 
> > What I can do is put the getters on a LongPressDragSelector-specific
interface
> > (I wanted to avoid creating one, but maybe that's just laziness).
> 
> I don't think it was just laziness :) I am definitely not a fan of adding an
> extra interface here if it could be avoided. If we can get rid of
> OnLongPressDragDetectionStateChanged() (I posted a comment on it separately),
I
> think we should nuke LongPressDragSelectorClient. 
> 
> Assuming we can get rid of OnLongPressDragDetectionStateChanged(), IMHO we can
> put the getters into TouchSelectionDraggableClient. I mean if you are okay
with
> getting those through LongPressDragSelectorClient, getting them though
> TouchSelectionDraggableClient should be okay as well? If you don't like it - I
> am fine with caching.

Too late :) It's possible we already needed
OnLongPressDragDetectionStateChanged() as its semantics are slightly different
than OnDragBegin/End, and otherwise we'd have to poll the drag detection state
in random places. So maybe the new client interface isn't such a bad thing.

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:354: void
TouchSelectionController::OnLongPressDragDetectionStateChanged() {
On 2015/04/28 02:17:30, mfomitchev wrote:
> I don't understand why we need this. TSC::OnSelectionChanged() sets the handle
> visibility based on GetStartVisible()/GetEndVisible(), so it should already
> choose the appropriate visibility. Am I missing something?

The problem is that we want the handles to reappear when they user lifts their
finger (after a longpress, but without any subsequent drag motion). We won't get
another OnSelectionChanged in that case, neither will we get a DragEnd as the
user hasn't initiated a drag after their longpress.

So the options are either get an explicit notification from the longpress
draggable, or have an implicit agreement that its state will change only due to
one of DragEnd or ACTION_UP. I almost went with the latter, don't have a strong
preference either way.

We could make the OnLongPressDragActiveStateChanged a more general method if you
prefer, though it would only be used for the longpress drag code (unless we try
to refresh visibility another way, in which case we don't even need this):

OnActiveStateChanged(const Draggable&) = 0;

Let me know either way, again, I don't have a strong opinion here.

[mfomitchev, 2015-04-29 00:34:28]

Do you think there is there any chance we can break some web content with this?
E.g. something using a drag-and-drop polyfill? I can't think of anything
specific myself.

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1087893003/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.cc:354: void
TouchSelectionController::OnLongPressDragDetectionStateChanged() {
On 2015/04/28 20:37:48, jdduke wrote:
> On 2015/04/28 02:17:30, mfomitchev wrote:
> > I don't understand why we need this. TSC::OnSelectionChanged() sets the
handle
> > visibility based on GetStartVisible()/GetEndVisible(), so it should already
> > choose the appropriate visibility. Am I missing something?
> 
> The problem is that we want the handles to reappear when they user lifts their
> finger (after a longpress, but without any subsequent drag motion). We won't
get
> another OnSelectionChanged in that case, neither will we get a DragEnd as the
> user hasn't initiated a drag after their longpress.
> 
> So the options are either get an explicit notification from the longpress
> draggable, or have an implicit agreement that its state will change only due
to
> one of DragEnd or ACTION_UP. I almost went with the latter, don't have a
strong
> preference either way.
> 
> We could make the OnLongPressDragActiveStateChanged a more general method if
you
> prefer, though it would only be used for the longpress drag code (unless we
try
> to refresh visibility another way, in which case we don't even need this):
> 
> OnActiveStateChanged(const Draggable&) = 0;
> 
> Let me know either way, again, I don't have a strong opinion here.
> 

Ah, I see, thanks for explaining. I am fine with it the way it is. When you work
on tests, can you please add a test for this use case?

If/when we switch to using gestures to drive the drag selection, would
ET_GESTURE_LONG_TAP work for this case (so that we don't need to listen to
ACTION_UP)?

https://codereview.chromium.org/1087893003/diff/120001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/120001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector.h:50: bool IsDetectingDrag() const;
This should be removed

[jdduke (slow), 2015-04-29 16:39:15]

Thanks for review. I'll add some tests and let you know when they're ready.

On 2015/04/29 00:34:28, mfomitchev wrote:
> Do you think there is there any chance we can break some web content with
this?
> E.g. something using a drag-and-drop polyfill? I can't think of anything
> specific myself.

Yeah, I've wondered about this too. Any page that consumes the touch events to
support drag/drop will prevent the longpress/selection entirely. In fact, on
Android, we previously would just cancel the whole touch sequence if we detected
a longpress-triggered selection, so I guess this is no less web content
friendly?

> Ah, I see, thanks for explaining. I am fine with it the way it is. When you
work
> on tests, can you please add a test for this use case?
> 
> If/when we switch to using gestures to drive the drag selection, would
> ET_GESTURE_LONG_TAP work for this case (so that we don't need to listen to
> ACTION_UP)?

Ideally, yes. I suppose the risk is that the page can preventDefault the
touchend event, which may supress the LONG_TAP. However, maybe we could just
make sure we get a TAP_CANCEL in that case, which would effectively reset the
active state? 

Thinking about it some more, I think the only real barrier to me using gestures
now is that it would involve converting WebGestureEvents (here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...)
back to a format that ui/ can understand. Sigh, these different formats are a
continual pain.

https://codereview.chromium.org/1087893003/diff/120001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector.h (right):

https://codereview.chromium.org/1087893003/diff/120001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector.h:50: bool IsDetectingDrag() const;
On 2015/04/29 00:34:28, mfomitchev wrote:
> This should be removed

Done.

[jdduke (slow), 2015-04-29 20:30:33]

Hmm, so one subtle issue here is that the TSC on Android gets a shot at touch
events *before* they ever hit the gesture detector, whereas the TSC on Aura is
offered touch events *after* they hit the gesture detector.

In practice, this means that on Aura we might get a fling event (because this
change doesn't "consume" the touchend event) that would need to be filtered
(converted to a GestureScrollEnd). I guess the simplest fix might be using the
gesture events to drive the longpress drag, though even then we'd need a way to
filter the fling into a scroll ending event.

[mfomitchev, 2015-04-29 20:42:15]

On 2015/04/29 20:30:33, jdduke wrote:
> Hmm, so one subtle issue here is that the TSC on Android gets a shot at touch
> events *before* they ever hit the gesture detector, whereas the TSC on Aura is
> offered touch events *after* they hit the gesture detector.
> 
> In practice, this means that on Aura we might get a fling event (because this
> change doesn't "consume" the touchend event) that would need to be filtered
> (converted to a GestureScrollEnd). I guess the simplest fix might be using the
> gesture events to drive the longpress drag, though even then we'd need a way
to
> filter the fling into a scroll ending event.

Can we send ACTION_CANCEL to the renderer when we enter DRAG_PENDING state and
then consume the real ACTION_UP/ACTION_CANCEL?
That actually seems more correct regardless of this issue. From renderer's POV -
the moment the drag starts is the moment the touch sequence ends. Plus with the
current code we consume all the moves, but the coordinates of the UP/CANCEL the
renderer gets are still adjusted for all the moves.

[jdduke (slow), 2015-04-29 20:47:26]

On 2015/04/29 20:42:15, mfomitchev wrote:
> On 2015/04/29 20:30:33, jdduke wrote:
> > Hmm, so one subtle issue here is that the TSC on Android gets a shot at
touch
> > events *before* they ever hit the gesture detector, whereas the TSC on Aura
is
> > offered touch events *after* they hit the gesture detector.
> > 
> > In practice, this means that on Aura we might get a fling event (because
this
> > change doesn't "consume" the touchend event) that would need to be filtered
> > (converted to a GestureScrollEnd). I guess the simplest fix might be using
the
> > gesture events to drive the longpress drag, though even then we'd need a way
> to
> > filter the fling into a scroll ending event.
> 
> Can we send ACTION_CANCEL to the renderer when we enter DRAG_PENDING state and
> then consume the real ACTION_UP/ACTION_CANCEL?
> That actually seems more correct regardless of this issue. From renderer's POV
-
> the moment the drag starts is the moment the touch sequence ends. Plus with
the
> current code we consume all the moves, but the coordinates of the UP/CANCEL
the
> renderer gets are still adjusted for all the moves.

Hmm, yeah I like that. I'll add a new event type.

[jdduke (slow), 2015-05-01 21:33:48]

Tests added.

[chromium-reviews, 2015-05-01 21:45:50]

Hey Jared,

Sorry, I probably won't be able to review it today and I am on vacation out
of country next week..

Mikhail

On Fri, May 1, 2015, 5:33 PM  <jdduke@chromium.org> wrote:

Tests added.

https://codereview.chromium.org/1087893003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jdduke (slow), 2015-05-01 21:49:21]

On 2015/05/01 21:45:50, chromium-reviews wrote:
> Hey Jared,
> 
> Sorry, I probably won't be able to review it today and I am on vacation out
> of country next week..
> 
> Mikhail
> 
> On Fri, May 1, 2015, 5:33 PM  <mailto:jdduke@chromium.org> wrote:
> 
> Tests added.
> 
> https://codereview.chromium.org/1087893003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

No problem, this isn't urgent. Have a great vacation, and I'll sit on this until
you're back.

[jdduke (slow), 2015-05-12 19:59:01]

On 2015/05/01 21:49:21, jdduke wrote:
> On 2015/05/01 21:45:50, chromium-reviews wrote:
> > Hey Jared,
> > 
> > Sorry, I probably won't be able to review it today and I am on vacation out
> > of country next week..
> > 
> > Mikhail
> > 
> > On Fri, May 1, 2015, 5:33 PM  <mailto:jdduke@chromium.org> wrote:
> > 
> > Tests added.
> > 
> > https://codereview.chromium.org/1087893003/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> No problem, this isn't urgent. Have a great vacation, and I'll sit on this
until
> you're back.

Welcome back :) Any chance you could take a look at the tests? Thanks.

[mfomitchev, 2015-05-13 17:32:58]

What about sending ACTION_CANCEL upon entering DRAG_PENDING and consuming
ACTION_UP/ACTION_CANCEL as per the discussion above?

[jdduke (slow), 2015-05-13 17:40:13]

On 2015/05/13 17:32:58, mfomitchev wrote:
> What about sending ACTION_CANCEL upon entering DRAG_PENDING and consuming
> ACTION_UP/ACTION_CANCEL as per the discussion above?

We already insert a touchcancel when we detect a longpress-triggered selection,
but I've changed that to insert the touchcancel at the gesture detector/content
level rather than somewhere higher. That effectively prevents the downstream
from handling any more touches of the current sequence (i.e., they get filtered
out), but I'm still slightly nervous about consuming the touchend. I'm not
entirely sure how this will work with Aura (the ordering of the TSC relative to
the GestureProvider isn't yet clear to me).

[mfomitchev, 2015-05-13 20:52:25]

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:27: //
LongPressDragSelectorClient implementation.
Can we put the LongPressDragSelectorClient methods at the end? Otherwise it's
harder to tell which ones are from LongPressDragSelectorClient and which ones
aren't.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:98: // Initiate drag
motion. The motion is downward, so the end selection point
The comment about downward motion would probably fit better with the non-zero
move below.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:143:
EXPECT_TRUE(selector.WillHandleTouchEvent(event.MovePoint(0, kSlop, 0)));
I don't get it - kSlop is greater than 0, so why is the motion leftward, not
rightward? Or is this just establishing the starting point even though we've had
the PressPoint @ (0, 0)?

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:219: // Activate a
longpress-triggered selection, but at a time different than the
different -> before ?

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:229:
selector.OnLongPressEvent(event.GetEventTime(), gfx::PointF(kSlop * 2, 0));
Can we do kSlop here instead of kSlop * 2?

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.h:113: bool
GetStartVisibleForTesting() const;
How about creating TouchSelectionControllerTestApi friend class and making
GetStartVisible/GetEndVisible public there? E.g. see EventTestApi. It's a nice
pattern to keep the core class clean.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller_unittest.cc:987:
TEST_F(TouchSelectionControllerTest, LongPressDrag) {
Can we also add a test that the handles reappear if the user lifts the finger
after a long press without any drag motion?

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller_unittest.cc:1013:
EXPECT_TRUE(controller().WillHandleTouchEvent(event.MovePoint(0, 0, 10)));
Can we use kDefaulTapSlop instead of ints here?

[jdduke (slow), 2015-05-14 18:28:32]

Patchset #11 (id:240001) has been deleted

[jdduke (slow), 2015-05-14 18:29:27]

Also switched back to cancelling the downstream touch sequence when we observe
SELECTION_SHOWN.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:27: //
LongPressDragSelectorClient implementation.
On 2015/05/13 20:52:25, mfomitchev wrote:
> Can we put the LongPressDragSelectorClient methods at the end? Otherwise it's
> harder to tell which ones are from LongPressDragSelectorClient and which ones
> aren't.

Done.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:98: // Initiate drag
motion. The motion is downward, so the end selection point
On 2015/05/13 20:52:25, mfomitchev wrote:
> The comment about downward motion would probably fit better with the non-zero
> move below.

Done.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:143:
EXPECT_TRUE(selector.WillHandleTouchEvent(event.MovePoint(0, kSlop, 0)));
On 2015/05/13 20:52:25, mfomitchev wrote:
> I don't get it - kSlop is greater than 0, so why is the motion leftward, not
> rightward? Or is this just establishing the starting point even though we've
had
> the PressPoint @ (0, 0)?

Yeah, the comment is in a bad position, I'll change it.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:219: // Activate a
longpress-triggered selection, but at a time different than the
On 2015/05/13 20:52:25, mfomitchev wrote:
> different -> before ?

Done.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:229:
selector.OnLongPressEvent(event.GetEventTime(), gfx::PointF(kSlop * 2, 0));
On 2015/05/13 20:52:25, mfomitchev wrote:
> Can we do kSlop here instead of kSlop * 2?

Done.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller.h:113: bool
GetStartVisibleForTesting() const;
On 2015/05/13 20:52:25, mfomitchev wrote:
> How about creating TouchSelectionControllerTestApi friend class and making
> GetStartVisible/GetEndVisible public there? E.g. see EventTestApi. It's a nice
> pattern to keep the core class clean.

Sure.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller_unittest.cc:987:
TEST_F(TouchSelectionControllerTest, LongPressDrag) {
On 2015/05/13 20:52:25, mfomitchev wrote:
> Can we also add a test that the handles reappear if the user lifts the finger
> after a long press without any drag motion?

Done.

https://codereview.chromium.org/1087893003/diff/220001/ui/touch_selection/tou...
ui/touch_selection/touch_selection_controller_unittest.cc:1013:
EXPECT_TRUE(controller().WillHandleTouchEvent(event.MovePoint(0, 0, 10)));
On 2015/05/13 20:52:25, mfomitchev wrote:
> Can we use kDefaulTapSlop instead of ints here?

Done.

[mfomitchev, 2015-05-14 21:55:28]

Looks good, just one question.

[mfomitchev, 2015-05-14 21:58:32]

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:153:
EXPECT_EQ(selection_start + gfx::Vector2dF(kSlop, -kSlop), DragPosition());
Sorry, I don't get it: we've moved by (-kSlop, -kSlop). The move within the slop
region is ignored, so shouldn't the expected move vector be (0, -kSlop)?, why is
x in the expected vector +kSlop if we were moving left and down?

[jdduke (slow), 2015-05-14 22:19:29]

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:153:
EXPECT_EQ(selection_start + gfx::Vector2dF(kSlop, -kSlop), DragPosition());
On 2015/05/14 21:58:32, mfomitchev wrote:
> Sorry, I don't get it: we've moved by (-kSlop, -kSlop). The move within the
slop
> region is ignored, so shouldn't the expected move vector be (0, -kSlop)?, why
is
> x in the expected vector +kSlop if we were moving left and down? 

Hmm, maybe switching from x to y coordinate is confusing.

The drag starts when the touch is at (-kSlop, 0). We then move the touch to (0,
kSlop), so the delta is (kSlop, -kSlop). Note that the slop is boundary
exclusive, so as soon as we hit kSlop, we're outside the slop region and the
drag starts.

[mfomitchev, 2015-05-14 22:33:57]

LGTM

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
File ui/touch_selection/longpress_drag_selector_unittest.cc (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/touch_selection/lon...
ui/touch_selection/longpress_drag_selector_unittest.cc:153:
EXPECT_EQ(selection_start + gfx::Vector2dF(kSlop, -kSlop), DragPosition());
On 2015/05/14 22:19:28, jdduke wrote:
> On 2015/05/14 21:58:32, mfomitchev wrote:
> > Sorry, I don't get it: we've moved by (-kSlop, -kSlop). The move within the
> slop
> > region is ignored, so shouldn't the expected move vector be (0, -kSlop)?,
why
> is
> > x in the expected vector +kSlop if we were moving left and down? 
> 
> Hmm, maybe switching from x to y coordinate is confusing.
> 
> The drag starts when the touch is at (-kSlop, 0). We then move the touch to
(0,
> kSlop), so the delta is (kSlop, -kSlop). Note that the slop is boundary
> exclusive, so as soon as we hit kSlop, we're outside the slop region and the
> drag starts.

AH, right. For some reason I was reading this as relative move coordinates, but
they are absolute, thanks for explaining!

[jdduke (slow), 2015-05-14 22:38:42]

On 2015/05/14 22:33:57, mfomitchev wrote:
> AH, right. For some reason I was reading this as relative move coordinates,
but
> they are absolute, thanks for explaining!

Thanks for review!

Oh, yeah, I've been tripped up by that in the past =/. In hindsight probably
should have had |MovePointTo| + |MovePointBy| methods.

[jdduke (slow), 2015-05-14 22:39:36]

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

[jdduke (slow), 2015-05-14 22:39:37]

+sadrul for ui/events/test/

[sadrul, 2015-05-14 22:44:36]

LGTM

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
ui/events/test/motion_event_test_utils.h:55: 
Interesting. I haven't seen this pattern used elsewhere in chrome ... are there
any?

[jdduke (slow), 2015-05-14 22:49:47]

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
ui/events/test/motion_event_test_utils.h:55: 
On 2015/05/14 22:44:36, sadrul wrote:
> Interesting. I haven't seen this pattern used elsewhere in chrome ... are
there
> any?

Isn't this pretty common with builder objects (confessedly I haven't actually
used builders much in Chrome...)? Otherwise I haven't seen it used much either.
Regardless, it's convenient, and in tests it's sometimes fun to rebel against
conformity ;).

[sadrul, 2015-05-14 22:55:33]

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/1087893003/diff/260001/ui/events/test/motion_...
ui/events/test/motion_event_test_utils.h:55: 
On 2015/05/14 22:49:46, jdduke wrote:
> On 2015/05/14 22:44:36, sadrul wrote:
> > Interesting. I haven't seen this pattern used elsewhere in chrome ... are
> there
> > any?
> 
> Isn't this pretty common with builder objects (confessedly I haven't actually
> used builders much in Chrome...)? Otherwise I haven't seen it used much
either.
> Regardless, it's convenient, and in tests it's sometimes fun to rebel against
> conformity ;).

Not sure. I don't think we have that many builders [that do this] in chrome. But
yeah, I quite like this.

[jdduke (slow), 2015-05-18 18:51:05]

Just a heads up, I'm going to land this small fix first:
https://codereview.chromium.org/1140693004. It addresses a concern here with
WebView while also addressing a different (but somewhat related) issue with
selection handle manipulation in WebView. I'd like to land it separately as it
will be useful for M44 regardless of if/when this change gets cherry-picked to
M44.

[jdduke (slow), 2015-05-20 18:49:00]

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

[jdduke (slow), 2015-05-20 18:49:01]

+sievers for content/public/common

[no sievers, 2015-05-21 18:09:24]

On 2015/05/20 18:49:01, jdduke wrote:
> +sievers for content/public/common

sorry, missed this. lgtm.

[jdduke (slow), 2015-05-21 18:25:37]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-05-21 18:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
mfomitchev@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1087893003/#ps360001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-21 18:26:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087893003/360001

[jdduke (slow), 2015-05-21 19:01:39]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-05-27 19:56:12]

On 2015/05/21 18:26:31, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1087893003/360001

Just an FYI, I'm teasing a few self-contained bits and several minor fixes into
a separate patch (https://codereview.chromium.org/1129193007/) that we need to
cherry-pick to M44 regardless. Once that lands I'll put this back in the CQ.

[jdduke (slow), 2015-06-02 16:16:17]

Patchset #16 (id:360001) has been deleted

[jdduke (slow), 2015-06-02 16:16:29]

Patchset #16 (id:380001) has been deleted

[jdduke (slow), 2015-06-02 16:57:47]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-06-02 16:57:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
mfomitchev@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1087893003/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-02 16:59:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087893003/420001

[commit-bot: I haz the power, 2015-06-02 19:01:13]

Committed patchset #17 (id:420001)

[commit-bot: I haz the power, 2015-06-02 19:02:04]

Patchset 17 (id:??) landed as
https://crrev.com/3f2a3abd4acdd336e4c76ce963663507bd05e7bd
Cr-Commit-Position: refs/heads/master@{#332439}