Implementing directional text selection handles

This change implements support for directional selection handles, i.e. handles that look differently depending if they are located at the left or right edge of the selection, or at the cursor. For web contents, the type of the handle is determined based on the information passed up from Blink in ViewHostMsg_SelectionBounds_Params. For Aura Textfield, it is calculated directly. New image assets (submitted in https://codereview.chromium.org/694873002) are used.

New struct ui::SelectionBound is introduced to encapsulate the information about a selection bound. This struct is purposefully made similar to cc::ViewportSelectionBound which is used by TouchSelectionController, and which we'll likely be using for RenderWidgetHostViewAura once we switch to the unified touch selection implementation between Android and Aura.

Depends on:
https://codereview.chromium.org/686513004/
https://codereview.chromium.org/694873002/

BUG=398053, 419898
Committed: https://crrev.com/218e811496df0cd6660d2f86e13ca04d10d5ef4f
Cr-Commit-Position: refs/heads/master@{#304524}

[mfomitchev, 2014-11-03 17:34:25]

mfomitchev@chromium.org changed reviewers:
+ mohsen@chromium.org, sadrul@chromium.org

[sadrul, 2014-11-03 17:56:26]

Can you add some details about the change in the CL description?

[mfomitchev, 2014-11-03 18:11:25]

Done

[mohsen, 2014-11-07 16:52:27]

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

https://codereview.chromium.org/700563002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:934: gfx::Rect
focus_rect(params.focus_rect);
I think the above copies are not needed and you can get rid of them.

https://codereview.chromium.org/700563002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:942: if
(params.anchor_rect.x() || params.anchor_rect.y())
What does this line exactly check? And, what happens if the condition does not
hold?

https://codereview.chromium.org/700563002/diff/1/content/browser/web_contents...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(right):

https://codereview.chromium.org/700563002/diff/1/content/browser/web_contents...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:144:
content->GetRootWindow(), &screen_position_client_);
Can you move this to SetUp()? (the same for similar cases in other tests)

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
ui/base/touch/touch_editing_controller.cc:34: const SelectionBound& b2) {
Can you rewrite this for general case (where handle is not necessarily
vertical)? (probably using gfx::BoundingRect())

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
File ui/base/touch/touch_editing_controller.h (right):

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
ui/base/touch/touch_editing_controller.h:19: struct UI_BASE_EXPORT
SelectionBound {
Can you make fields private and add setter/getters if needed?

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:74: 
The only difference between following 3 functions is the resource id. Can you
write a function using resource id as parameter? You can also merge the new
function with GetHandleImage() below.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:103: gfx::Image*
GetHandleImage(const ui::SelectionBound& bound) {
GetHandleImage(ui::SelectionBound::Type bound_type)?

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
Can't we just use SelectionBound::Type enum?

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:161: static gfx::Size*
max_size = nullptr;
Is this caching to prevent image load every time or just to prevent max
calculation? If the latter, I don't think it is worth it.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:205: gfx::Size(0,
bound.edge_bottom.y() - bound.edge_top.y()));
gfx::BoundingRect()

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:237:
image_(GetCenterHandleImage()),
Can't we initialize image_ with nullptr?

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:254: const
ui::SelectionBound& selection_bound() { return selection_bound_; }
Apparently it is not used anywhere. So, you can remove it.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:309: gfx::Point
selection_bound_center(selection_bound_.edge_top.x(),
Height of selection bound might change during drag. We should add this height to
calculations in scroll-update instead of scroll-begin.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:361: // This should be
possible to fix once crbug.com/399721 is resolved.
How is it possible to fix after crbug.com/399721?

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:469: if (dragging_handle_) {
During a drag handles might temporarily overlap, in which case I think both
handles would be of type center. However, we still want to show left/right
handles which doesn't seem to be the case here.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:556: p2.Offset(0,
(anchor_bound.edge_bottom.y() - anchor_bound.edge_top.y()) / 2);
anchor_bound.GetHeight() / 2

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:582: if
(bound.edge_bottom.y() - bound.edge_top.y() < kSelectionHandleBarMinHeight)
GetHeight()

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:587:
gfx::Rect(bound.edge_top, gfx::Size(0, bound.GetHeight()));
gfx::BoundingRect()

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.h:60: // Convenience method to
set a handle's selection rect and hide it if it is
There are a few comments here and in other files that need to be updated to
reflect new variable/function names.

[mfomitchev, 2014-11-10 04:04:13]

Mohsen, can you please take another look? Thanks!

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

https://codereview.chromium.org/700563002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:934: gfx::Rect
focus_rect(params.focus_rect);
On 2014/11/07 16:52:26, mohsen wrote:
> I think the above copies are not needed and you can get rid of them.

Done.

https://codereview.chromium.org/700563002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:942: if
(params.anchor_rect.x() || params.anchor_rect.y())
Got rid of this. I took this from the Clank code, but there was nothing like
this in the old Aura code.

https://codereview.chromium.org/700563002/diff/1/content/browser/web_contents...
File content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
(right):

https://codereview.chromium.org/700563002/diff/1/content/browser/web_contents...
content/browser/web_contents/touch_editable_impl_aura_browsertest.cc:144:
content->GetRootWindow(), &screen_position_client_);
On 2014/11/07 16:52:27, mohsen wrote:
> Can you move this to SetUp()? (the same for similar cases in other tests)

Done.

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
ui/base/touch/touch_editing_controller.cc:34: const SelectionBound& b2) {
On 2014/11/07 16:52:27, mohsen wrote:
> Can you rewrite this for general case (where handle is not necessarily
> vertical)? (probably using gfx::BoundingRect())

Done.

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
File ui/base/touch/touch_editing_controller.h (right):

https://codereview.chromium.org/700563002/diff/1/ui/base/touch/touch_editing_...
ui/base/touch/touch_editing_controller.h:19: struct UI_BASE_EXPORT
SelectionBound {
I thought it would be good to keep this similar to similar to
cc::ViewportSelectionBound, which is just a struct with direct access to
members.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:74: 
I kind of like not having to pass id. And not having to handle the case where
the id doesn't match one of the expected ones. The way it's now, the code is
really simple. And we are not even going to save on LOC much by having one
common method. Either way, I don't think it's a big deal.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:103: gfx::Image*
GetHandleImage(const ui::SelectionBound& bound) {
On 2014/11/07 16:52:27, mohsen wrote:
> GetHandleImage(ui::SelectionBound::Type bound_type)?

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
Not sure what are you suggesting - have GetCursorAlignment return
ui::SelectionBound::Type? Or get rid of this method altogether? The semantics of
the two types are different. Also having this method makes things more
obvious/easier to understand IMHO.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:161: static gfx::Size*
max_size = nullptr;
On 2014/11/07 16:52:27, mohsen wrote:
> Is this caching to prevent image load every time or just to prevent max
> calculation? If the latter, I don't think it is worth it.

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:205: gfx::Size(0,
bound.edge_bottom.y() - bound.edge_top.y()));
Done

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:237:
image_(GetCenterHandleImage()),
I had that initially, but I feel better not having to worry about image_ being
used before it's initialized.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:254: const
ui::SelectionBound& selection_bound() { return selection_bound_; }
On 2014/11/07 16:52:27, mohsen wrote:
> Apparently it is not used anywhere. So, you can remove it.

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:309: gfx::Point
selection_bound_center(selection_bound_.edge_top.x(),
That's a great point. I have tried a couple of things, and fixing this is
actually not as simple as updating the offset on based on the current selection
height. If we do that, the same point may map to completely different locations
in the document based on whether we are taking the current line height or the
previous line height into teh calculations. As a result, as you are moving the
handle through lines of different height, it can jump back and forth between
them, resulting in some pretty terrible UX. I think we really need to take the
previous state into account to fix this properly, and it's not simple, so I will
tackle this in a separate CL.

For now I am going back to the old approach.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:361: // This should be
possible to fix once crbug.com/399721 is resolved.
Got rid of this comment, since the offset no longer depends on the selection
height.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:469: if (dragging_handle_) {
Another good catch. Fixed.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:556: p2.Offset(0,
(anchor_bound.edge_bottom.y() - anchor_bound.edge_top.y()) / 2);
On 2014/11/07 16:52:27, mohsen wrote:
> anchor_bound.GetHeight() / 2

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:582: if
(bound.edge_bottom.y() - bound.edge_top.y() < kSelectionHandleBarMinHeight)
On 2014/11/07 16:52:27, mohsen wrote:
> GetHeight()

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:587:
gfx::Rect(bound.edge_top, gfx::Size(0, bound.GetHeight()));
On 2014/11/07 16:52:27, mohsen wrote:
> gfx::BoundingRect()

Done.

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.h:60: // Convenience method to
set a handle's selection rect and hide it if it is
On 2014/11/07 16:52:27, mohsen wrote:
> There are a few comments here and in other files that need to be updated to
> reflect new variable/function names.

Done for this file I think. What are the comments in other files that need
fixing? I couldn't find anything wrong from the quick skim through the changes.

[mohsen, 2014-11-10 22:26:00]

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
On 2014/11/10 04:04:12, mfomitchev wrote:
> Not sure what are you suggesting - have GetCursorAlignment return
> ui::SelectionBound::Type? Or get rid of this method altogether? The semantics
of
> the two types are different. Also having this method makes things more
> obvious/easier to understand IMHO.

You have codes like this:

  Alignment alignment = GetCursorAlignment(bound.type);
  switch (alignment) {
    case ALIGN_LEFT:
      ...
  }

My suggestion is to have this instead:

  switch (bound.type) {
    case ui::SelectionBound::RIGHT:
      ...
  }

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.h:60: // Convenience method to
set a handle's selection rect and hide it if it is
On 2014/11/10 04:04:12, mfomitchev wrote:
> On 2014/11/07 16:52:27, mohsen wrote:
> > There are a few comments here and in other files that need to be updated to
> > reflect new variable/function names.
> 
> Done for this file I think. What are the comments in other files that need
> fixing? I couldn't find anything wrong from the quick skim through the
changes.

SetHandleSelectionRect() -> SetHandleBound() in
touch_selection_controller_impl.cc which now you've fixed.

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:963: anchor_bound,
focus_bound);
nit: Curly braces for multi-line block.

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:37: return gfx::UnionRects(rect_1,
rect_2);
This might not work properly if width of one of rect_1 and rect_2 is zero.

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:216:
&screen_position_client_);
Can't this be moved to SetUp(), too?

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:60: // in the touch target
to make dragging the handle easier (see pic below).
below -> above

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:66: 
nit: Style guide suggests minimizing use of vertical whitespace.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:412: bool is_cursor_handle_;
Can we just use current type of handle to prevent a CENTER handle become LEFT or
RIGHT (and vice versa)?

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:88:
SetUpRootWindowClients(textfield_widget_->GetNativeView()->GetRootWindow());
Can be moved to SetUp()?

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:99:
SetUpRootWindowClients(widget_->GetNativeView()->GetRootWindow());
Can be moved to SetUp()?

[mfomitchev, 2014-11-10 23:07:30]

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
GetCursorAlignment is used twice, so doing this would mean that essentially the
logic for GetCursorAlignment(bound.type) is implicitly implemented twice. Also,
I think it's a bit easier to understand the code this way when bound type ->
alignment conversion step is explicit.

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:963: anchor_bound,
focus_bound);
On 2014/11/10 22:25:59, mohsen wrote:
> nit: Curly braces for multi-line block.

Acknowledged.

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:37: return gfx::UnionRects(rect_1,
rect_2);
One of {rect_1, rect_2} will be equal to the union of rect_1 and rect_2, so I
think it's fine.

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:216:
&screen_position_client_);
I think the widget's window is also the root window in this case, and the widget
is not created until this method is called, so we don't have anything to install
the client on in SetUp().

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:60: // in the touch target
to make dragging the handle easier (see pic below).
On 2014/11/10 22:25:59, mohsen wrote:
> below -> above

Acknowledged.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:66: 
On 2014/11/10 22:25:59, mohsen wrote:
> nit: Style guide suggests minimizing use of vertical whitespace.

Acknowledged.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:412: bool is_cursor_handle_;
I guess we could do this.. We'd still need to pass is_cursor_handle to the
constructor. And then we'd need to initialize selection_bound_.type to, lets
say, ui::SelectionBound::LEFT and also initialize image_ to,
GetHandleImage(selection_bound_.type). I am not sure it's that much better, but
if you prefer it that way I can change it - let me know.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:88:
SetUpRootWindowClients(textfield_widget_->GetNativeView()->GetRootWindow());
Same as in textfield_unittest.cc - don't think the window exists yet in SetUp().

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:99:
SetUpRootWindowClients(widget_->GetNativeView()->GetRootWindow());
Same as above

[mohsen, 2014-11-11 01:24:39]

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:37: return gfx::UnionRects(rect_1,
rect_2);
On 2014/11/10 23:07:29, mfomitchev wrote:
> One of {rect_1, rect_2} will be equal to the union of rect_1 and rect_2, so I
> think it's fine.

At least, if both have width of zero (which for example happens when both
handles have the same x coordinate), gfx::UnionRect() will arbitrarily return
one which might not be the one you expect. (Also, I'm not sure if your first
sentence is correct in the general case).

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:216:
&screen_position_client_);
On 2014/11/10 23:07:29, mfomitchev wrote:
> I think the widget's window is also the root window in this case, and the
widget
> is not created until this method is called, so we don't have anything to
install
> the client on in SetUp().

I don't think so. You can use GetContext() in SetUp() to access root window
(after calling base SetUp()).

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:412: bool is_cursor_handle_;
On 2014/11/10 23:07:29, mfomitchev wrote:
> I guess we could do this.. We'd still need to pass is_cursor_handle to the
> constructor. And then we'd need to initialize selection_bound_.type to, lets
> say, ui::SelectionBound::LEFT and also initialize image_ to,
> GetHandleImage(selection_bound_.type). I am not sure it's that much better,
but
> if you prefer it that way I can change it - let me know.

What if we allow an EMPTY handle become anything else, but after that preventing
unwanted changes? Am I missing something here?

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:88:
SetUpRootWindowClients(textfield_widget_->GetNativeView()->GetRootWindow());
On 2014/11/10 23:07:30, mfomitchev wrote:
> Same as in textfield_unittest.cc - don't think the window exists yet in
SetUp().

Same as my response to your comment in textfield_unittest.cc. :-)

[mfomitchev, 2014-11-12 18:32:49]

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/700563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:963: anchor_bound,
focus_bound);
On 2014/11/10 22:25:59, mohsen wrote:
> nit: Curly braces for multi-line block.

Done.

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:37: return gfx::UnionRects(rect_1,
rect_2);
Yes, good point. Ok, I'll just go with the straightforward approach then.

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:216:
&screen_position_client_);
Great, good to know, thanks!

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:60: // in the touch target
to make dragging the handle easier (see pic below).
On 2014/11/10 22:25:59, mohsen wrote:
> below -> above

Done.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:66: 
On 2014/11/10 23:07:30, mfomitchev wrote:
> On 2014/11/10 22:25:59, mohsen wrote:
> > nit: Style guide suggests minimizing use of vertical whitespace.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.cc:412: bool is_cursor_handle_;
Added a DCHECK as per our discussion

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:88:
SetUpRootWindowClients(textfield_widget_->GetNativeView()->GetRootWindow());
On 2014/11/11 01:24:39, mohsen wrote:
> On 2014/11/10 23:07:30, mfomitchev wrote:
> > Same as in textfield_unittest.cc - don't think the window exists yet in
> SetUp().
> 
> Same as my response to your comment in textfield_unittest.cc. :-)

Done.

https://codereview.chromium.org/700563002/diff/20001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:99:
SetUpRootWindowClients(widget_->GetNativeView()->GetRootWindow());
On 2014/11/10 23:07:30, mfomitchev wrote:
> Same as above

Done.

[mohsen, 2014-11-12 21:41:46]

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:166:
test_cursor_client_->EnableMouseEvents();
Can you use EventGenerator here?

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:210:
point.Offset(rect.width() / 2, rect.height() / 2 + cursor_height);
rect.CenterPoint() might be helpful here.

[mfomitchev, 2014-11-13 22:11:48]

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:166:
test_cursor_client_->EnableMouseEvents();
There are complications with this like configuring the min scroll distance, and
even after resolving them right now there is a case where the handles overlap,
and we can't use the event generator to send events to the handle which is
covered.

https://codereview.chromium.org/700563002/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:210:
point.Offset(rect.width() / 2, rect.height() / 2 + cursor_height);
On 2014/11/12 21:41:46, mohsen wrote:
> rect.CenterPoint() might be helpful here.

Done.

[mohsen, 2014-11-13 22:59:57]

LGTM

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
On 2014/11/10 23:07:29, mfomitchev wrote:
> GetCursorAlignment is used twice, so doing this would mean that essentially
the
> logic for GetCursorAlignment(bound.type) is implicitly implemented twice.
Also,
> I think it's a bit easier to understand the code this way when bound type ->
> alignment conversion step is explicit.  

What I was suggesting was to remove Alignment enum and GetCursorAlignment()
function and directly use bound.type in your switch statements.

[mfomitchev, 2014-11-13 23:36:22]

mfomitchev@chromium.org changed reviewers:
- sadrul@chromium.org

[mfomitchev, 2014-11-13 23:36:47]

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

[mfomitchev, 2014-11-13 23:36:47]

+sky for owner review

[mfomitchev, 2014-11-13 23:49:51]

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/700563002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:117: enum Alignment
{ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
On 2014/11/13 22:59:56, mohsen wrote:
> On 2014/11/10 23:07:29, mfomitchev wrote:
> > GetCursorAlignment is used twice, so doing this would mean that essentially
> the
> > logic for GetCursorAlignment(bound.type) is implicitly implemented twice.
> Also,
> > I think it's a bit easier to understand the code this way when bound type ->
> > alignment conversion step is explicit.  
> 
> What I was suggesting was to remove Alignment enum and GetCursorAlignment()
> function and directly use bound.type in your switch statements.

Yup, that's how I understood it. Whether the alignment enum is there or not, the
concept of alignment semantically is still present. The bound type by itself
doesn't necessarily impact the cursor positioning relative to the handle image.
It happens to do so because of the specific handle image shapes. How exactly
this positioning is done is currently explicitly captured in GetAlignment()
method. And it's used in two places. If we remove the method, we will
essentially make this conversion implicit, and we'll have to repeat it in two
separate places. I.e. if we change the handle shapes, we will have to change the
code in two places instead of one.

[sky, 2014-11-14 02:02:47]

I didn't look at ui/views/touchui. I'm assuming mohsen did. Can you add mohsen
to ui/views/touchui/OWNERS?

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:41: const int num_elements = 4;
How about a compile assert that the size of the arrays matches? And use
arraysize rather than 4.

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.h (right):

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.h:26: virtual ~SelectionBound();
Why is this virtual?

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.h:41: UI_BASE_EXPORT gfx::Rect
RectBetweenSelectionBounds(const SelectionBound& b1,
How about some test coverage?

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1127: } else if (ltr ^ (range_start <
range_end)) {
WHy ^ and not || ?

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:165:
aura::client::SetScreenPositionClient(GetContext(),
If the aura usesage.

https://codereview.chromium.org/700563002/diff/60001/ui/views/widget/widget_i...
File ui/views/widget/widget_interactive_uitest.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/widget/widget_i...
ui/views/widget/widget_interactive_uitest.cc:785:
aura::client::SetScreenPositionClient(widget.GetNativeView()->GetRootWindow(),
Ifdef aura here too.

[mfomitchev, 2014-11-14 19:56:27]

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.cc:41: const int num_elements = 4;
On 2014/11/14 02:02:47, sky wrote:
> How about a compile assert that the size of the arrays matches? And use
> arraysize rather than 4.

Done.

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
File ui/base/touch/touch_editing_controller.h (right):

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.h:26: virtual ~SelectionBound();
Fixed

https://codereview.chromium.org/700563002/diff/60001/ui/base/touch/touch_edit...
ui/base/touch/touch_editing_controller.h:41: UI_BASE_EXPORT gfx::Rect
RectBetweenSelectionBounds(const SelectionBound& b1,
On 2014/11/14 02:02:47, sky wrote:
> How about some test coverage?

Done.

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1127: } else if (ltr ^ (range_start <
range_end)) {
Any one condition is not enough.. ltr means range indexes grow from left to
right, while rtl means that they decrease. So we need to both know the direction
and which index is greater/less. I've renamed the vars to clarify. Or do you
mean expand it like this:
(ltr && (anchor_position_index > focus_position_index)) || (!ltr &&
(anchor_position_index < focus_position_index))?

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:165:
aura::client::SetScreenPositionClient(GetContext(),
On 2014/11/14 02:02:47, sky wrote:
> If the aura usesage.

Done.

https://codereview.chromium.org/700563002/diff/60001/ui/views/widget/widget_i...
File ui/views/widget/widget_interactive_uitest.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/widget/widget_i...
ui/views/widget/widget_interactive_uitest.cc:785:
aura::client::SetScreenPositionClient(widget.GetNativeView()->GetRootWindow(),
On 2014/11/14 02:02:47, sky wrote:
> Ifdef aura here too.

Done.

[mfomitchev, 2014-11-14 19:59:16]

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

[mfomitchev, 2014-11-14 19:59:17]

+sadrul for changes content/browser

[sky, 2014-11-14 20:30:57]

LGTM

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1127: } else if (ltr ^ (range_start <
range_end)) {
On 2014/11/14 19:56:27, mfomitchev wrote:
> Any one condition is not enough.. ltr means range indexes grow from left to
> right, while rtl means that they decrease. So we need to both know the
direction
> and which index is greater/less. I've renamed the vars to clarify. Or do you
> mean expand it like this:
> (ltr && (anchor_position_index > focus_position_index)) || (!ltr &&
> (anchor_position_index < focus_position_index))?

Yes. I would rather have the more readable version.

[mfomitchev, 2014-11-14 20:46:04]

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/700563002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1127: } else if (ltr ^ (range_start <
range_end)) {
Ok, done. I updated render_widget_host_view_aura.cc similarly as well.

[mfomitchev, 2014-11-17 20:02:57]

mfomitchev@chromium.org changed reviewers:
- sadrul@chromium.org

[mfomitchev, 2014-11-17 20:03:43]

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

[mfomitchev, 2014-11-17 20:03:43]

+creis@chromium.org: Please review changes in 
content/browser

[Charlie Reis, 2014-11-17 21:19:08]

content/browser LGTM

[mfomitchev, 2014-11-17 21:38:11]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-11-17 21:39:37]

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

[commit-bot: I haz the power, 2014-11-17 21:59:33]

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

[commit-bot: I haz the power, 2014-11-17 21:59:34]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[mfomitchev, 2014-11-17 22:27:39]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-11-17 22:29:30]

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

[commit-bot: I haz the power, 2014-11-18 00:13:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-18 00:14:47]

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