Update touch selection to only modify one selection point at a time.

Update touch selection to only modify one selection point at a time.

This is made in preparation for enabling selection auto scroll using touch
handles.

BUG=340658
Committed: https://crrev.com/aeed9866ec8ef2172404784edeebc85aaafbf330
Cr-Commit-Position: refs/heads/master@{#302776}

[christiank, 2014-10-15 09:48:56]

christiank@opera.com changed reviewers:
+ jdduke@chromium.org

[christiank, 2014-10-15 09:52:57]

christiank@opera.com changed reviewers:
+ jamesr@chromium.org, palmer@chromium.org

[christiank, 2014-10-15 09:55:03]

First patch in preparation for touch selection auto scroll on Android. After
some discussion we reached the conclusion that we must update the browser -
renderer API to only manipulate one selection point at a time for auto scrolling
to work.

Thanks!

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:227:
client_->SelectBetweenCoordinates(fixed_handle_position, handle_position);
Instead of adding a new code path for swapping the base and extent I thought I
could as well use SelectBetweenCoordinates to set the desired base and extent.
SelectBetweenCoordinates calls its parameters start and end, however they
correspond to base and extent when ultimately reaching
WebLocalFrameImpl::selectRange.

Not sure what you think about this the Jared?

I think setting both points in one message is more elegant than issuing a
"switch base and extent" message. An option could be to add a new code path that
makes it more explicit that we're setting the base and extent, not start and
end.

[jdduke (slow), 2014-10-15 15:46:07]

Thanks! This looks pretty sensible, just a few minor notes.

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:320: if
(move_selection_extent_pending_) {
There's probably some common logic between SelectRange/MoveCaret/MoveExtent we
could factor out, but that's probably best left as a follow-up patch. Maybe add
a:

// TODO(jdduke): Factor out common logic between selection and caret-related IPC
messages.

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:223:
gfx::Vector2dF line_offset = &handle == end_selection_handle_.get()
This could might be more simply folded into the above if statements, so you only
have a single branch point, e.g.,

gfx::PointF base, extent;
if (&handle == start_selection_handle_.get()) {
  base = end_selection_handle_->position() + GetEndLineOffset();
  extent = start_selection_handle_->position() + GetStartLineOffset();
} ...

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:227:
client_->SelectBetweenCoordinates(fixed_handle_position, handle_position);
On 2014/10/15 09:55:03, christiank wrote:
> Instead of adding a new code path for swapping the base and extent I thought I
> could as well use SelectBetweenCoordinates to set the desired base and extent.
> SelectBetweenCoordinates calls its parameters start and end, however they
> correspond to base and extent when ultimately reaching
> WebLocalFrameImpl::selectRange.
> 
> Not sure what you think about this the Jared?
> 
> I think setting both points in one message is more elegant than issuing a
> "switch base and extent" message. An option could be to add a new code path
that
> makes it more explicit that we're setting the base and extent, not start and
> end.

Ah, I think what you have is reasonable, maybe we just need to update the
start/end names in the SelectRange pipeline to read base/extent? Then I think
the intent is a bit more clear?

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:243:
client_->MoveSelectionExtent(line_position);
Could add a test (or update an existing test) in
touch_selection_controller_unittest.cc to track the SelectRange +
MoveSelectionExtent flow?

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.h:35: virtual
void SelectBetweenCoordinates(const gfx::PointF& start,
While you're digging around here, maybe we could rename all of the
|SelectBetweenCoordinates| methods to |SelectRange(base, extent)|? Makes
grepping a lot easier to find all the connected paths =/

https://codereview.chromium.org/657803002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/657803002/diff/1/content/common/view_messages...
content/common/view_messages.h:1336:
IPC_MESSAGE_ROUTED0(ViewHostMsg_MoveSelectionExtent_ACK)
Hmm, these should really be InputHostMsg types, would you mind moving them into
input_messages.h (and updating input_router_impl.cc/render_frame_impl.cc
accordingly)? Thanks.

[christiank, 2014-10-17 14:33:05]

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:320: if
(move_selection_extent_pending_) {
On 2014/10/15 15:46:07, jdduke (OOO until Oct 21) wrote:
> There's probably some common logic between SelectRange/MoveCaret/MoveExtent we
> could factor out, but that's probably best left as a follow-up patch. Maybe
add
> a:
> 
> // TODO(jdduke): Factor out common logic between selection and caret-related
IPC
> messages.

Yes, I noticed this. I didn't address it in this CL since I wanted to keep it
small and focused :) A TODO makes perfect sense!

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_selection_controller.cc (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:223:
gfx::Vector2dF line_offset = &handle == end_selection_handle_.get()
On 2014/10/15 15:46:07, jdduke wrote:
> This could might be more simply folded into the above if statements, so you
only
> have a single branch point, e.g.,
> 
> gfx::PointF base, extent;
> if (&handle == start_selection_handle_.get()) {
>   base = end_selection_handle_->position() + GetEndLineOffset();
>   extent = start_selection_handle_->position() + GetStartLineOffset();
> } ...

Good idea!

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:227:
client_->SelectBetweenCoordinates(fixed_handle_position, handle_position);
On 2014/10/15 15:46:07, jdduke wrote:
> On 2014/10/15 09:55:03, christiank wrote:
> > Instead of adding a new code path for swapping the base and extent I thought
I
> > could as well use SelectBetweenCoordinates to set the desired base and
extent.
> > SelectBetweenCoordinates calls its parameters start and end, however they
> > correspond to base and extent when ultimately reaching
> > WebLocalFrameImpl::selectRange.
> > 
> > Not sure what you think about this the Jared?
> > 
> > I think setting both points in one message is more elegant than issuing a
> > "switch base and extent" message. An option could be to add a new code path
> that
> > makes it more explicit that we're setting the base and extent, not start and
> > end.
> 
> Ah, I think what you have is reasonable, maybe we just need to update the
> start/end names in the SelectRange pipeline to read base/extent? Then I think
> the intent is a bit more clear?

I agree, I'll update the names.

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.cc:243:
client_->MoveSelectionExtent(line_position);
On 2014/10/15 15:46:07, jdduke wrote:
> Could add a test (or update an existing test) in
> touch_selection_controller_unittest.cc to track the SelectRange +
> MoveSelectionExtent flow?

Sure, will do!

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_selection_controller.h (right):

https://codereview.chromium.org/657803002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_selection_controller.h:35: virtual
void SelectBetweenCoordinates(const gfx::PointF& start,
On 2014/10/15 15:46:07, jdduke wrote:
> While you're digging around here, maybe we could rename all of the
> |SelectBetweenCoordinates| methods to |SelectRange(base, extent)|? Makes
> grepping a lot easier to find all the connected paths =/

Sure, makes sense.

https://codereview.chromium.org/657803002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/657803002/diff/1/content/common/view_messages...
content/common/view_messages.h:1336:
IPC_MESSAGE_ROUTED0(ViewHostMsg_MoveSelectionExtent_ACK)
On 2014/10/15 15:46:07, jdduke wrote:
> Hmm, these should really be InputHostMsg types, would you mind moving them
into
> input_messages.h (and updating input_router_impl.cc/render_frame_impl.cc
> accordingly)? Thanks.

Sure!

[palmer, 2014-10-17 21:47:03]

IPC security LGTM.

[jdduke (slow), 2014-10-21 16:57:36]

Thanks, this is looking pretty good.

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

https://codereview.chromium.org/657803002/diff/40001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:781: const gfx::PointF&
position) {
Nit: Let's call this |extent| instead of |position| (sorry for the code
thrashing!).

https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:319:
DCHECK(message->type() == InputMsg_MoveRangeSelectionExtent::ID);
Hmm, so, in theory |SelectRange| and |MoveRangeSelectionExtent| messages could
be interleaved, arriving out-of-order in the render process (e.g., if a
SelectRange is pending and we get a new MoveRangeSelectionExtent message).

The simplest solution is probably: 

1) Keep a unified queue of IPCs for both messages types.
2) If the tail message of the queue is the same type as the new IPC, replace the
tail with the new IPC. Otherwise, enqueue it.
3) When an ack is received, dispatch the head of the queue.

Thoughts? Sorry I didn't think of this sooner, but hopefully it's not too crazy
to implement. Unfortunately, I don't think there's a ScopedDeque in base/ yet.

If that's confusing, feel free to TODO(jdduke) it and I'll implement it in a
follow-up patch.

https://codereview.chromium.org/657803002/diff/40001/content/common/input_mes...
File content/common/input_messages.h (right):

https://codereview.chromium.org/657803002/diff/40001/content/common/input_mes...
content/common/input_messages.h:208: gfx::Point /* point */)
Nit: Let's call this extent for consistency.

[christiank, 2014-10-22 12:16:06]

https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:319:
DCHECK(message->type() == InputMsg_MoveRangeSelectionExtent::ID);
On 2014/10/21 16:57:36, jdduke wrote:
> Hmm, so, in theory |SelectRange| and |MoveRangeSelectionExtent| messages could
> be interleaved, arriving out-of-order in the render process (e.g., if a
> SelectRange is pending and we get a new MoveRangeSelectionExtent message).
> 
> The simplest solution is probably: 
> 
> 1) Keep a unified queue of IPCs for both messages types.
> 2) If the tail message of the queue is the same type as the new IPC, replace
the
> tail with the new IPC. Otherwise, enqueue it.
> 3) When an ack is received, dispatch the head of the queue.
> 
> Thoughts? Sorry I didn't think of this sooner, but hopefully it's not too
crazy
> to implement. Unfortunately, I don't think there's a ScopedDeque in base/ yet.
> 
> If that's confusing, feel free to TODO(jdduke) it and I'll implement it in a
> follow-up patch.

I think you're right. Maybe we could use the queue for caret messages as well?
While there is no dependency between selection and caret messages it would be
possible to remove some code (and the other TODO that I added).

I'll attempt a fix for this using a std::deque. Unfortunately I suspect some
manual memory management will be required.

[christiank, 2014-10-27 07:06:09]

On 2014/10/21 16:57:36, jdduke wrote:
> Thanks, this is looking pretty good.
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:781: const gfx::PointF&
> position) {
> Nit: Let's call this |extent| instead of |position| (sorry for the code
> thrashing!).
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl.cc:319:
> DCHECK(message->type() == InputMsg_MoveRangeSelectionExtent::ID);
> Hmm, so, in theory |SelectRange| and |MoveRangeSelectionExtent| messages could
> be interleaved, arriving out-of-order in the render process (e.g., if a
> SelectRange is pending and we get a new MoveRangeSelectionExtent message).
> 
> The simplest solution is probably: 
> 
> 1) Keep a unified queue of IPCs for both messages types.
> 2) If the tail message of the queue is the same type as the new IPC, replace
the
> tail with the new IPC. Otherwise, enqueue it.
> 3) When an ack is received, dispatch the head of the queue.
> 
> Thoughts? Sorry I didn't think of this sooner, but hopefully it's not too
crazy
> to implement. Unfortunately, I don't think there's a ScopedDeque in base/ yet.
> 
> If that's confusing, feel free to TODO(jdduke) it and I'll implement it in a
> follow-up patch.
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/common/input_mes...
> File content/common/input_messages.h (right):
> 
>
https://codereview.chromium.org/657803002/diff/40001/content/common/input_mes...
> content/common/input_messages.h:208: gfx::Point /* point */)
> Nit: Let's call this extent for consistency.

Jared, what do you think about my approach for addressing this?

[jdduke (slow), 2014-10-27 15:22:13]

content/browser/android lgtm.
content/browser/renderer_host/input lgtm with an additional input_router_impl
test for the interleaved message types.

Thanks! Sorry for the delayed review. Rietveld doesn't automatically send out an
update when you post a new patch, so I didn't realize you had an update.

https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:87: for (auto* message
: pending_select_messages_) {
Woo, range for!

For now, let's stick with STLDeleteElements(&pending_select_messages_);

I'll see if we can get a ScopedDeque smart container in base (or just wait until
we get proper move support with standard containers... =/).

https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:512:
pending_select_messages_.pop_front();
As a precaution, let's store |front()| in a local scoped_ptr, then
|pop_front()|, then forward the local scoped_ptr (this shouldn't be an issue in
practice currently, but we've had issues with re-entrancy in the past so best to
make sure it works now).

https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl_unittest.cc:357:
scoped_ptr<IPC::Message> response(new InputHostMsg_SelectRange_ACK(0));
Maybe a short test that interleaves RangeSelection and MoveRangeSelectionExtent?

[christiank, 2014-10-28 13:50:39]

christiank@opera.com changed reviewers:
+ sievers@chromium.org

[christiank, 2014-10-28 13:53:36]

On 2014/10/27 15:22:13, jdduke wrote:
> content/browser/android lgtm.
> content/browser/renderer_host/input lgtm with an additional input_router_impl
> test for the interleaved message types.
> 
> Thanks! Sorry for the delayed review. Rietveld doesn't automatically send out
an
> update when you post a new patch, so I didn't realize you had an update.
> 
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl.cc:87: for (auto*
message
> : pending_select_messages_) {
> Woo, range for!
> 
> For now, let's stick with STLDeleteElements(&pending_select_messages_);
> 
> I'll see if we can get a ScopedDeque smart container in base (or just wait
until
> we get proper move support with standard containers... =/).
> 
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl.cc:512:
> pending_select_messages_.pop_front();
> As a precaution, let's store |front()| in a local scoped_ptr, then
> |pop_front()|, then forward the local scoped_ptr (this shouldn't be an issue
in
> practice currently, but we've had issues with re-entrancy in the past so best
to
> make sure it works now).
> 
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl_unittest.cc:357:
> scoped_ptr<IPC::Message> response(new InputHostMsg_SelectRange_ACK(0));
> Maybe a short test that interleaves RangeSelection and
MoveRangeSelectionExtent?

No worries!

Thanks for the feedback, I have now addressed the issues you pointed out and
added a few more unit tests.

Also added sievers as reviewer to cover the changes in
content/browser/renderer_host/ and content/browser/web_contents/.

Thanks!

[christiank, 2014-11-03 07:06:13]

On 2014/10/28 13:53:36, christiank wrote:
> On 2014/10/27 15:22:13, jdduke wrote:
> > content/browser/android lgtm.
> > content/browser/renderer_host/input lgtm with an additional
input_router_impl
> > test for the interleaved message types.
> > 
> > Thanks! Sorry for the delayed review. Rietveld doesn't automatically send
out
> an
> > update when you post a new patch, so I didn't realize you had an update.
> > 
> >
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> > File content/browser/renderer_host/input/input_router_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> > content/browser/renderer_host/input/input_router_impl.cc:87: for (auto*
> message
> > : pending_select_messages_) {
> > Woo, range for!
> > 
> > For now, let's stick with STLDeleteElements(&pending_select_messages_);
> > 
> > I'll see if we can get a ScopedDeque smart container in base (or just wait
> until
> > we get proper move support with standard containers... =/).
> > 
> >
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> > content/browser/renderer_host/input/input_router_impl.cc:512:
> > pending_select_messages_.pop_front();
> > As a precaution, let's store |front()| in a local scoped_ptr, then
> > |pop_front()|, then forward the local scoped_ptr (this shouldn't be an issue
> in
> > practice currently, but we've had issues with re-entrancy in the past so
best
> to
> > make sure it works now).
> > 
> >
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> > File content/browser/renderer_host/input/input_router_impl_unittest.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/657803002/diff/80001/content/browser/renderer...
> > content/browser/renderer_host/input/input_router_impl_unittest.cc:357:
> > scoped_ptr<IPC::Message> response(new InputHostMsg_SelectRange_ACK(0));
> > Maybe a short test that interleaves RangeSelection and
> MoveRangeSelectionExtent?
> 
> No worries!
> 
> Thanks for the feedback, I have now addressed the issues you pointed out and
> added a few more unit tests.
> 
> Also added sievers as reviewer to cover the changes in
> content/browser/renderer_host/ and content/browser/web_contents/.
> 
> Thanks!

sievers, Jared? What do you think?

[jdduke (slow), 2014-11-03 17:58:49]

Wow, I'm a bad person, I had this all reviewed but I forgot to publish =/.

content/browser/android and content/browser/renderer_host/input lgtm (with one
question).

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

https://codereview.chromium.org/657803002/diff/100001/content/browser/android...
content/browser/android/content_view_core_impl.cc:784:
web_contents_->MoveRangeSelectionExtent(gfx::Point(extent.x(), extent.y()));
I wonder, will the renderer guard against the selection collapsing if base ==
extent here?

[christiank, 2014-11-04 09:44:28]

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

https://codereview.chromium.org/657803002/diff/100001/content/browser/android...
content/browser/android/content_view_core_impl.cc:784:
web_contents_->MoveRangeSelectionExtent(gfx::Point(extent.x(), extent.y()));
On 2014/11/03 17:58:48, jdduke wrote:
> I wonder, will the renderer guard against the selection collapsing if base ==
> extent here?

Yes it should. WebLocalFrameImpl::moveRangeSelectionExtent should prevent the
selection from collapsing with the following lines:
if (comparePositions(basePosition, extentPosition) == 0)
  return;

I have tested it and it appears to be working.

[jamesr, 2014-11-04 19:09:54]

content/renderer/ lgtm

[no sievers, 2014-11-04 22:12:10]

lgtm

https://codereview.chromium.org/657803002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/657803002/diff/100001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:87:
STLDeleteElements(&pending_select_messages_);
You can use STLElementDeleter to auto-delete when the container goes
out-of-scope :)

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

https://codereview.chromium.org/657803002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1146:
content_view_core_->MoveRangeSelectionExtent(extent);
It seems a bit odd that we are going back from RWHV to CVC/WebContents here for
a browser-side initiated action, but was like that before your change.

[christiank, 2014-11-05 09:38:41]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2014-11-05 09:39:03]

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

[commit-bot: I haz the power, 2014-11-05 10:49:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-05 10:49:59]

Patchset 7 (id:??) landed as
https://crrev.com/aeed9866ec8ef2172404784edeebc85aaafbf330
Cr-Commit-Position: refs/heads/master@{#302776}