Implementing directional text selection handles

ui/views/touchui/touch_selection_controller_impl.cc

Index: ui/views/touchui/touch_selection_controller_impl.cc
diff --git a/ui/views/touchui/touch_selection_controller_impl.cc b/ui/views/touchui/touch_selection_controller_impl.cc
index 978592c349683bf24580de4ff16b37a05015a033..aec6050212a81c3022f811fa2625e9fc3415ddbf 100644
--- a/ui/views/touchui/touch_selection_controller_impl.cc
+++ b/ui/views/touchui/touch_selection_controller_impl.cc
@@ -18,41 +18,31 @@
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/widget/widget.h"
+#include "ui/wm/core/coordinate_conversion.h"
 #include "ui/wm/core/masked_window_targeter.h"
 
 namespace {
 
 // Constants defining the visual attributes of selection handles
-const int kSelectionHandleLineWidth = 1;
+
+// Controls the width of the cursor line associated with the selection handle.
+const int kSelectionHandleLineWidth = 2;
+
 const SkColor kSelectionHandleLineColor =
     SkColorSetRGB(0x42, 0x81, 0xf4);
 
-// When a handle is dragged, the drag position reported to the client view is
-// offset vertically to represent the cursor position. This constant specifies
-// the offset in  pixels above the "O" (see pic below). This is required because
-// say if this is zero, that means the drag position we report is the point
-// right above the "O" or the bottom most point of the cursor "|". In that case,
-// a vertical movement of even one pixel will make the handle jump to the line
-// below it. So when the user just starts dragging, the handle will jump to the
-// next line if the user makes any vertical movement. It is correct but
-// looks/feels weird. So we have this non-zero offset to prevent this jumping.
-//
-// Editing handle widget showing the difference between the position of the
-// ET_GESTURE_SCROLL_UPDATE event and the drag position reported to the client:
+// Padding around the selection handle defining the area that will be included
+// in the touch target to make dragging the handle easier (see pic below).
 //                                  _____
-//                                 |  |<-|---- Drag position reported to client
+//                                 |  |  |
 //                              _  |  O  |
-//          Vertical Padding __|   |   <-|---- ET_GESTURE_SCROLL_UPDATE position
-//                             |_  |_____|<--- Editing handle widget
+//          Vertical Padding __|   |     |<--- Editing handle widget
+//                             |_  |_____|
 //
 //                                 | |
 //                                  T
 //                          Horizontal Padding
 //
-const int kSelectionHandleVerticalDragOffset = 5;
-
-// Padding around the selection handle defining the area that will be included
-// in the touch target to make dragging the handle easier (see pic above).
 const int kSelectionHandleHorizPadding = 10;
 const int kSelectionHandleVertPadding = 20;
 
@@ -82,43 +72,137 @@ views::Widget* CreateTouchSelectionPopupWidget(
   return widget;
 }
 

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

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.

[mfomitchev, 2014/11/10 04:04:12]

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.

-gfx::Image* GetHandleImage() {
-  static gfx::Image* handle_image = NULL;
+gfx::Image* GetCenterHandleImage() {
+  static gfx::Image* handle_image = nullptr;
   if (!handle_image) {
     handle_image = &ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-        IDR_TEXT_SELECTION_HANDLE);
+        IDR_TEXT_SELECTION_HANDLE_CENTER);
   }
   return handle_image;
 }
 
-gfx::Size GetHandleImageSize() {
-  return GetHandleImage()->Size();
+gfx::Image* GetLeftHandleImage() {
+  static gfx::Image* handle_image = nullptr;
+  if (!handle_image) {
+    handle_image = &ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+        IDR_TEXT_SELECTION_HANDLE_LEFT);
+  }
+  return handle_image;
 }
 
-// Cannot use gfx::UnionRect since it does not work for empty rects.
-gfx::Rect Union(const gfx::Rect& r1, const gfx::Rect& r2) {
-  int rx = std::min(r1.x(), r2.x());
-  int ry = std::min(r1.y(), r2.y());
-  int rr = std::max(r1.right(), r2.right());
-  int rb = std::max(r1.bottom(), r2.bottom());
+gfx::Image* GetRightHandleImage() {
+  static gfx::Image* handle_image = nullptr;
+  if (!handle_image) {
+    handle_image = &ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+        IDR_TEXT_SELECTION_HANDLE_RIGHT);
+  }
+  return handle_image;
+}
 
-  return gfx::Rect(rx, ry, rr - rx, rb - ry);
+// Return the appropriate handle image based on the bound's type
+gfx::Image* GetHandleImage(const ui::SelectionBound& bound) {

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

GetHandleImage(ui::SelectionBound::Type bound_type)?

[mfomitchev, 2014/11/10 04:04:12]

Done.

+  switch(bound.type) {
+    case ui::SelectionBound::LEFT:
+      return GetLeftHandleImage();
+    case ui::SelectionBound::CENTER:
+      return GetCenterHandleImage();
+    case ui::SelectionBound::RIGHT:
+      return GetRightHandleImage();
+    default:
+      NOTREACHED() << "Invalid touch handle bound type.";
+      return nullptr;
+  };
+}
+
+enum Alignment {ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};

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

Can't we just use SelectionBound::Type enum?

[mfomitchev, 2014/11/10 04:04:12]

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.

[mohsen, 2014/11/10 22:25:59]

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:
      ...
  }

[mfomitchev, 2014/11/10 23:07:29]

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.  

[mohsen, 2014/11/13 22:59:56]

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:49:51]

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.

+
+// Determine the cursor line alignment to the handle image based on the bound
+// type. Depends on the visual shapes of the handle image assets.
+Alignment GetCursorAlignment(ui::SelectionBound::Type bound_type) {
+  switch (bound_type) {
+    case ui::SelectionBound::LEFT:
+      return ALIGN_RIGHT;
+    case ui::SelectionBound::RIGHT:
+      return ALIGN_LEFT;
+    case ui::SelectionBound::CENTER:
+      return ALIGN_CENTER;
+    default:
+      NOTREACHED() << "Undefined bound type for cursor alignment.";
+      return ALIGN_LEFT;
+  };
+}
+
+// Calculates the bounds of the widget containing the selection handle based
+// on the SelectionBound's type and location
+gfx::Rect GetSelectionWidgetBounds(const ui::SelectionBound& bound) {
+  Alignment cursor_alignment = GetCursorAlignment(bound.type);
+  gfx::Size image_size = GetHandleImage(bound)->Size();
+  int widget_width =  image_size.width() + 2 * kSelectionHandleHorizPadding;
+  int widget_height = bound.GetHeight() + image_size.height() +
+      kSelectionHandleVertPadding;
+  int widget_left = 0;
+  switch (cursor_alignment) {
+    case ALIGN_LEFT:
+      widget_left = bound.edge_top.x() - kSelectionHandleHorizPadding;
+      break;
+    case ALIGN_RIGHT:
+      widget_left = bound.edge_top.x() - image_size.width() -
+          kSelectionHandleHorizPadding;
+      break;
+    case ALIGN_CENTER:
+      widget_left = bound.edge_top.x() - widget_width / 2;
+      break;
+  };
+  return gfx::Rect(
+      widget_left, bound.edge_top.y(), widget_width, widget_height);
+}
+
+gfx::Size GetMaxHandleImageSize() {
+  static gfx::Size* max_size = nullptr;

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

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.

[mfomitchev, 2014/11/10 04:04:12]

Done.

+  if (!max_size) {
+    gfx::Rect center_rect = gfx::Rect(GetCenterHandleImage()->Size());
+    gfx::Rect left_rect = gfx::Rect(GetLeftHandleImage()->Size());
+    gfx::Rect right_rect = gfx::Rect(GetRightHandleImage()->Size());
+    gfx::Rect union_rect = center_rect;
+    union_rect.Union(left_rect);
+    union_rect.Union(right_rect);
+    max_size = new gfx::Size(union_rect.size());
+  }
+  return *max_size;
 }
 
-// Convenience methods to convert a |rect| from screen to the |client|'s
+// Convenience methods to convert a |bound| from screen to the |client|'s
 // coordinate system and vice versa.
 // Note that this is not quite correct because it does not take into account
 // transforms such as rotation and scaling. This should be in TouchEditable.
 // TODO(varunjain): Fix this.
-gfx::Rect ConvertFromScreen(ui::TouchEditable* client, const gfx::Rect& rect) {
-  gfx::Point origin = rect.origin();
-  client->ConvertPointFromScreen(&origin);
-  return gfx::Rect(origin, rect.size());
-}
-gfx::Rect ConvertToScreen(ui::TouchEditable* client, const gfx::Rect& rect) {
-  gfx::Point origin = rect.origin();
-  client->ConvertPointToScreen(&origin);
-  return gfx::Rect(origin, rect.size());
+ui::SelectionBound ConvertFromScreen(ui::TouchEditable* client,
+                                     const ui::SelectionBound& bound) {
+  ui::SelectionBound result = bound;
+  gfx::Point edge_bottom = bound.edge_bottom;
+  gfx::Point edge_top = bound.edge_top;
+  client->ConvertPointFromScreen(&edge_bottom);
+  client->ConvertPointFromScreen(&edge_top);
+  result.edge_bottom = edge_bottom;
+  result.edge_top = edge_top;
+  return result;
+}
+
+ui::SelectionBound ConvertToScreen(ui::TouchEditable* client,
+                                   const ui::SelectionBound& bound) {
+  ui::SelectionBound result = bound;
+  gfx::Point edge_bottom = bound.edge_bottom;
+  gfx::Point edge_top = bound.edge_top;
+  client->ConvertPointToScreen(&edge_bottom);
+  client->ConvertPointToScreen(&edge_top);
+  result.edge_bottom = edge_bottom;
+  result.edge_top = edge_top;
+  return result;
+}
+
+gfx::Rect BoundToRect(const ui::SelectionBound& bound) {
+  return gfx::Rect(bound.edge_top,
+                   gfx::Size(0, bound.edge_bottom.y() - bound.edge_top.y()));

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

gfx::BoundingRect()

[mfomitchev, 2014/11/10 04:04:12]

Done

 }
 
 }  // namespace
@@ -150,7 +234,7 @@ class TouchSelectionControllerImpl::EditingHandleView
   EditingHandleView(TouchSelectionControllerImpl* controller,
                     gfx::NativeView context)
       : controller_(controller),
-        drag_offset_(0),
+        image_(GetCenterHandleImage()),

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

Can't we initialize image_ with nullptr?

[mfomitchev, 2014/11/10 04:04:12]

I had that initially, but I feel better not having to worry about image_ being
used before it's initialized.

         draw_invisible_(false) {
     widget_.reset(CreateTouchSelectionPopupWidget(context, this));
     widget_->SetContentsView(this);
@@ -165,15 +249,21 @@ class TouchSelectionControllerImpl::EditingHandleView
 
   ~EditingHandleView() override { SetWidgetVisible(false, false); }
 
+  const gfx::Image* image() const { return image_; }
+
+  const ui::SelectionBound& selection_bound() { return selection_bound_; }

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

Apparently it is not used anywhere. So, you can remove it.

[mfomitchev, 2014/11/10 04:04:12]

Done.

+
   // Overridden from views::WidgetDelegateView:
   bool WidgetHasHitTestMask() const override { return true; }
 
   void GetWidgetHitTestMask(gfx::Path* mask) const override {
-    gfx::Size image_size = GetHandleImageSize();
-    mask->addRect(SkIntToScalar(0), SkIntToScalar(selection_rect_.height()),
+    gfx::Size image_size = image_->Size();
+    mask->addRect(
+        SkIntToScalar(0),
+        SkIntToScalar(selection_bound_.GetHeight()),
         SkIntToScalar(image_size.width()) + 2 * kSelectionHandleHorizPadding,
-        SkIntToScalar(selection_rect_.height() + image_size.height() +
-            kSelectionHandleVertPadding));
+        SkIntToScalar(selection_bound_.GetHeight() + image_size.height() +
+                      kSelectionHandleVertPadding));
   }
 
   void DeleteDelegate() override {
@@ -184,40 +274,51 @@ class TouchSelectionControllerImpl::EditingHandleView
   void OnPaint(gfx::Canvas* canvas) override {
     if (draw_invisible_)
       return;
-    gfx::Size image_size = GetHandleImageSize();
-    int cursor_pos_x = image_size.width() / 2 - kSelectionHandleLineWidth +
-        kSelectionHandleHorizPadding;
 
+    Alignment cursor_alignment = GetCursorAlignment(selection_bound_.type);
+    int cursor_x = 0;
+    switch (cursor_alignment) {
+      case ALIGN_RIGHT:
+        cursor_x = selection_bound_.edge_top.x() - kSelectionHandleLineWidth;
+        break;
+      case ALIGN_LEFT:
+        cursor_x = selection_bound_.edge_top.x();
+        break;
+      case ALIGN_CENTER:
+        cursor_x =
+            selection_bound_.edge_top.x() - kSelectionHandleLineWidth / 2;
+        break;
+    };
     // Draw the cursor line.
-    canvas->FillRect(
-        gfx::Rect(cursor_pos_x, 0,
-                  2 * kSelectionHandleLineWidth + 1, selection_rect_.height()),
-        kSelectionHandleLineColor);
-
+    canvas->FillRect(gfx::Rect(cursor_x,
+                               0,
+                               kSelectionHandleLineWidth,
+                               selection_bound_.GetHeight()),
+                     kSelectionHandleLineColor);
     // Draw the handle image.
-    canvas->DrawImageInt(*GetHandleImage()->ToImageSkia(),
-        kSelectionHandleHorizPadding, selection_rect_.height());
+    canvas->DrawImageInt(*image_->ToImageSkia(),
+        kSelectionHandleHorizPadding, selection_bound_.GetHeight());
   }
 
   void OnGestureEvent(ui::GestureEvent* event) override {
     event->SetHandled();
     switch (event->type()) {
-      case ui::ET_GESTURE_SCROLL_BEGIN:
+      case ui::ET_GESTURE_SCROLL_BEGIN: {
         widget_->SetCapture(this);
         controller_->SetDraggingHandle(this);
-        drag_offset_ = event->y() - selection_rect_.height() +
-            kSelectionHandleVerticalDragOffset;
+        gfx::Point selection_bound_center(selection_bound_.edge_top.x(),

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

Height of selection bound might change during drag. We should add this height to
calculations in scroll-update instead of scroll-begin.

[mfomitchev, 2014/11/10 04:04:12]

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.

+                                          selection_bound_.GetHeight() / 2);
+        drag_offset_ = selection_bound_center - event->location();
         break;
+      }
       case ui::ET_GESTURE_SCROLL_UPDATE: {
-        gfx::Point drag_pos(event->location().x(),
-            event->location().y() - drag_offset_);
-        controller_->SelectionHandleDragged(drag_pos);
+        controller_->SelectionHandleDragged(event->location() + drag_offset_);
         break;
       }
       case ui::ET_GESTURE_SCROLL_END:
       case ui::ET_SCROLL_FLING_START:
         widget_->ReleaseCapture();
-        controller_->SetDraggingHandle(NULL);
+        controller_->SetDraggingHandle(nullptr);
         break;
       default:
         break;
@@ -225,9 +326,9 @@ class TouchSelectionControllerImpl::EditingHandleView
   }
 
   gfx::Size GetPreferredSize() const override {
-    gfx::Size image_size = GetHandleImageSize();
+    gfx::Size image_size = image_->Size();
     return gfx::Size(image_size.width() + 2 * kSelectionHandleHorizPadding,
-                     image_size.height() + selection_rect_.height() +
+                     image_size.height() + selection_bound_.GetHeight() +
                          kSelectionHandleVertPadding);
   }
 
@@ -247,19 +348,24 @@ class TouchSelectionControllerImpl::EditingHandleView
       widget_->Hide();
   }
 
-  void SetSelectionRectInScreen(const gfx::Rect& rect) {
-    gfx::Size image_size = GetHandleImageSize();
-    selection_rect_ = rect;
-    gfx::Rect widget_bounds(
-        rect.x() - image_size.width() / 2 - kSelectionHandleHorizPadding,
-        rect.y(),
-        image_size.width() + 2 * kSelectionHandleHorizPadding,
-        rect.height() + image_size.height() + kSelectionHandleVertPadding);
-    widget_->SetBounds(widget_bounds);
-  }
-
-  gfx::Point GetScreenPosition() {
-    return widget_->GetClientAreaBoundsInScreen().origin();
+  void SetBoundInScreen(const ui::SelectionBound& bound) {
+    if (bound.type != selection_bound_.type) {
+      image_ = GetHandleImage(bound);
+      // TODO(mfomitchev): Ideally we should update drag_offset_ as well: as the
+      // bound type changes, the position of the line cursor in the handle view
+      // changes as well. We don't do this, because it would cause a "flicker"
+      // effect every time the handle orientation changes as the user is
+      // dragging the handle. So instead the issue we have is that the position
+      // of the handle relative to your finger changes slightly every time the
+      // handle changes its orientation.
+      // This should be possible to fix once crbug.com/399721 is resolved.

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

How is it possible to fix after crbug.com/399721?

[mfomitchev, 2014/11/10 04:04:12]

Got rid of this comment, since the offset no longer depends on the selection
height.

+      SchedulePaint();
+    }
+    widget_->SetBounds(GetSelectionWidgetBounds(bound));
+    selection_bound_ = bound;
+    aura::Window* window = widget_->GetNativeView();
+    wm::ConvertPointFromScreen(window, &selection_bound_.edge_top);
+    wm::ConvertPointFromScreen(window, &selection_bound_.edge_bottom);
   }
 
   void SetDrawInvisible(bool draw_invisible) {
@@ -269,17 +375,17 @@ class TouchSelectionControllerImpl::EditingHandleView
     SchedulePaint();
   }
 
-  const gfx::Rect& selection_rect() const { return selection_rect_; }
-
  private:
   scoped_ptr<Widget> widget_;
   TouchSelectionControllerImpl* controller_;
-  gfx::Rect selection_rect_;
 
-  // Vertical offset between the scroll event position and the drag position
-  // reported to the client view (see the ASCII figure at the top of the file
-  // and its description for more details).
-  int drag_offset_;
+  // In local coordinates
+  ui::SelectionBound selection_bound_;
+  gfx::Image* image_;
+
+  // Distance from the touch-drag point to the middle of the handle's cursor
+  // line.
+  gfx::Vector2d drag_offset_;
 
   // If set to true, the handle will not draw anything, hence providing an empty
   // widget. We need this because we may want to stop showing the handle while
@@ -299,27 +405,22 @@ TouchHandleWindowTargeter::TouchHandleWindowTargeter(
 
 bool TouchHandleWindowTargeter::GetHitTestMask(aura::Window* window,
                                                gfx::Path* mask) const {
-  const gfx::Rect& selection_rect = handle_view_->selection_rect();
-  gfx::Size image_size = GetHandleImageSize();
-  mask->addRect(SkIntToScalar(0), SkIntToScalar(selection_rect.height()),
-      SkIntToScalar(image_size.width()) + 2 * kSelectionHandleHorizPadding,
-      SkIntToScalar(selection_rect.height() + image_size.height() +
-                    kSelectionHandleVertPadding));
+  handle_view_->GetWidgetHitTestMask(mask);
   return true;
 }
 
 TouchSelectionControllerImpl::TouchSelectionControllerImpl(
     ui::TouchEditable* client_view)
     : client_view_(client_view),
-      client_widget_(NULL),
+      client_widget_(nullptr),
       selection_handle_1_(new EditingHandleView(this,
                           client_view->GetNativeView())),
       selection_handle_2_(new EditingHandleView(this,
                           client_view->GetNativeView())),
       cursor_handle_(new EditingHandleView(this,
                      client_view->GetNativeView())),
-      context_menu_(NULL),
-      dragging_handle_(NULL) {
+      context_menu_(nullptr),
+      dragging_handle_(nullptr) {
   aura::Window* client_window = client_view_->GetNativeView();
   client_window->AddObserver(this);
   client_widget_ = Widget::GetTopLevelWidgetForNativeView(client_window);
@@ -337,30 +438,34 @@ TouchSelectionControllerImpl::~TouchSelectionControllerImpl() {
 }
 
 void TouchSelectionControllerImpl::SelectionChanged() {
-  gfx::Rect r1, r2;
-  client_view_->GetSelectionEndPoints(&r1, &r2);
-  gfx::Rect screen_rect_1 = ConvertToScreen(client_view_, r1);
-  gfx::Rect screen_rect_2 = ConvertToScreen(client_view_, r2);
+  ui::SelectionBound anchor, focus;
+  client_view_->GetSelectionEndPoints(&anchor, &focus);
+  ui::SelectionBound screen_bound_anchor =
+      ConvertToScreen(client_view_, anchor);
+  ui::SelectionBound screen_bound_focus = ConvertToScreen(client_view_, focus);
   gfx::Rect client_bounds = client_view_->GetBounds();
-  if (r1.y() < client_bounds.y())
-    r1.Inset(0, client_bounds.y() - r1.y(), 0, 0);
-  if (r2.y() < client_bounds.y())
-    r2.Inset(0, client_bounds.y() - r2.y(), 0, 0);
-  gfx::Rect screen_rect_1_clipped = ConvertToScreen(client_view_, r1);
-  gfx::Rect screen_rect_2_clipped = ConvertToScreen(client_view_, r2);
-  if (screen_rect_1_clipped == selection_end_point_1_clipped_ &&
-      screen_rect_2_clipped == selection_end_point_2_clipped_)
+  if (anchor.edge_top.y() < client_bounds.y())
+    anchor.edge_top.set_y(client_bounds.y());
+  if (focus.edge_top.y() < client_bounds.y())
+    focus.edge_top.set_y(client_bounds.y());
+  ui::SelectionBound screen_bound_anchor_clipped =
+      ConvertToScreen(client_view_, anchor);
+  ui::SelectionBound screen_bound_focus_clipped =
+      ConvertToScreen(client_view_, focus);
+  if (screen_bound_anchor_clipped == selection_bound_1_clipped_ &&
+      screen_bound_focus_clipped == selection_bound_2_clipped_)
     return;
 
-  selection_end_point_1_ = screen_rect_1;
-  selection_end_point_2_ = screen_rect_2;
-  selection_end_point_1_clipped_ = screen_rect_1_clipped;
-  selection_end_point_2_clipped_ = screen_rect_2_clipped;
+  selection_bound_1_ = screen_bound_anchor;
+  selection_bound_2_ = screen_bound_focus;
+  selection_bound_1_clipped_ = screen_bound_anchor_clipped;
+  selection_bound_2_clipped_ = screen_bound_focus_clipped;
 
   if (client_view_->DrawsHandles()) {
     UpdateContextMenu();
     return;
   }
+
   if (dragging_handle_) {

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

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.

[mfomitchev, 2014/11/10 04:04:12]

Another good catch. Fixed.

     // We need to reposition only the selection handle that is being dragged.
     // The other handle stays the same. Also, the selection handle being dragged
@@ -369,14 +474,14 @@ void TouchSelectionControllerImpl::SelectionChanged() {
     // If the new location of this handle is out of client view, its widget
     // should not get hidden, since it should still receive touch events.
     // Hence, we are not using |SetHandleSelectionRect()| method here.
-    dragging_handle_->SetSelectionRectInScreen(screen_rect_2_clipped);
+    dragging_handle_->SetBoundInScreen(screen_bound_focus_clipped);
 
     // Temporary fix for selection handle going outside a window. On a webpage,
     // the page should scroll if the selection handle is dragged outside the
     // window. That does not happen currently. So we just hide the handle for
     // now.
     // TODO(varunjain): Fix this: crbug.com/269003
-    dragging_handle_->SetDrawInvisible(!ShouldShowHandleFor(r2));
+    dragging_handle_->SetDrawInvisible(!ShouldShowHandleFor(focus));
 
     if (dragging_handle_ != cursor_handle_.get()) {
       // The non-dragging-handle might have recently become visible.
@@ -385,29 +490,29 @@ void TouchSelectionControllerImpl::SelectionChanged() {
         non_dragging_handle = selection_handle_2_.get();
         // if handle 1 is being dragged, it is corresponding to the end of
         // selection and the other handle to the start of selection.
-        selection_end_point_1_ = screen_rect_2;
-        selection_end_point_2_ = screen_rect_1;
-        selection_end_point_1_clipped_ = screen_rect_2_clipped;
-        selection_end_point_2_clipped_ = screen_rect_1_clipped;
+        selection_bound_1_ = screen_bound_focus;
+        selection_bound_2_ = screen_bound_anchor;
+        selection_bound_1_clipped_ = screen_bound_focus_clipped;
+        selection_bound_2_clipped_ = screen_bound_anchor_clipped;
       }
-      SetHandleSelectionRect(non_dragging_handle, r1, screen_rect_1_clipped);
+      SetHandleBound(non_dragging_handle, anchor, screen_bound_anchor_clipped);
     }
   } else {
     UpdateContextMenu();
 
     // Check if there is any selection at all.
-    if (screen_rect_1.origin() == screen_rect_2.origin()) {
+    if (screen_bound_anchor.edge_top == screen_bound_focus.edge_top) {
       selection_handle_1_->SetWidgetVisible(false, false);
       selection_handle_2_->SetWidgetVisible(false, false);
-      SetHandleSelectionRect(cursor_handle_.get(), r1, screen_rect_1_clipped);
+      SetHandleBound(cursor_handle_.get(), anchor, screen_bound_anchor_clipped);
       return;
     }
 
     cursor_handle_->SetWidgetVisible(false, false);
-    SetHandleSelectionRect(selection_handle_1_.get(), r1,
-                           screen_rect_1_clipped);
-    SetHandleSelectionRect(selection_handle_2_.get(), r2,
-                           screen_rect_2_clipped);
+    SetHandleBound(
+        selection_handle_1_.get(), anchor, screen_bound_anchor_clipped);
+    SetHandleBound(
+        selection_handle_2_.get(), focus, screen_bound_focus_clipped);
   }
 }
 
@@ -442,13 +547,13 @@ void TouchSelectionControllerImpl::SelectionHandleDragged(
   }
 
   // Find the stationary selection handle.
-  gfx::Rect fixed_handle_rect = selection_end_point_1_;
-  if (selection_handle_1_ == dragging_handle_)
-    fixed_handle_rect = selection_end_point_2_;
+  ui::SelectionBound anchor_bound =
+      selection_handle_1_ == dragging_handle_ ? selection_bound_2_
+                                              : selection_bound_1_;
 
   // Find selection end points in client_view's coordinate system.
-  gfx::Point p2 = fixed_handle_rect.origin();
-  p2.Offset(0, fixed_handle_rect.height() / 2);
+  gfx::Point p2 = anchor_bound.edge_top;
+  p2.Offset(0, (anchor_bound.edge_bottom.y() - anchor_bound.edge_top.y()) / 2);

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

anchor_bound.GetHeight() / 2

[mfomitchev, 2014/11/10 04:04:12]

Done.

   client_view_->ConvertPointFromScreen(&p2);
 
   // Instruct client_view to select the region between p1 and p2. The position
@@ -463,22 +568,24 @@ void TouchSelectionControllerImpl::ConvertPointToClientView(
   client_view_->ConvertPointFromScreen(point);
 }
 
-void TouchSelectionControllerImpl::SetHandleSelectionRect(
+void TouchSelectionControllerImpl::SetHandleBound(
     EditingHandleView* handle,
-    const gfx::Rect& rect,
-    const gfx::Rect& rect_in_screen) {
-  handle->SetWidgetVisible(ShouldShowHandleFor(rect), false);
+    const ui::SelectionBound& bound,
+    const ui::SelectionBound& bound_in_screen) {
+  handle->SetWidgetVisible(ShouldShowHandleFor(bound), false);
   if (handle->IsWidgetVisible())
-    handle->SetSelectionRectInScreen(rect_in_screen);
+    handle->SetBoundInScreen(bound_in_screen);
 }
 
 bool TouchSelectionControllerImpl::ShouldShowHandleFor(
-    const gfx::Rect& rect) const {
-  if (rect.height() < kSelectionHandleBarMinHeight)
+    const ui::SelectionBound& bound) const {
+  if (bound.edge_bottom.y() - bound.edge_top.y() < kSelectionHandleBarMinHeight)

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

GetHeight()

[mfomitchev, 2014/11/10 04:04:12]

Done.

     return false;
-  gfx::Rect bounds = client_view_->GetBounds();
-  bounds.Inset(0, 0, 0, -kSelectionHandleBarBottomAllowance);
-  return bounds.Contains(rect);
+  gfx::Rect client_bounds = client_view_->GetBounds();
+  client_bounds.Inset(0, 0, 0, -kSelectionHandleBarBottomAllowance);
+  gfx::Rect selection_rect =
+      gfx::Rect(bound.edge_top, gfx::Size(0, bound.GetHeight()));

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

gfx::BoundingRect()

[mfomitchev, 2014/11/10 04:04:12]

Done.

+  return client_bounds.Contains(selection_rect);
 }
 
 bool TouchSelectionControllerImpl::IsCommandIdEnabled(int command_id) const {
@@ -501,7 +608,7 @@ void TouchSelectionControllerImpl::OpenContextMenu() {
 
 void TouchSelectionControllerImpl::OnMenuClosed(TouchEditingMenuView* menu) {
   if (menu == context_menu_)
-    context_menu_ = NULL;
+    context_menu_ = nullptr;
 }
 
 void TouchSelectionControllerImpl::OnAncestorWindowTransformed(
@@ -512,7 +619,7 @@ void TouchSelectionControllerImpl::OnAncestorWindowTransformed(
 
 void TouchSelectionControllerImpl::OnWidgetClosing(Widget* widget) {
   DCHECK_EQ(client_widget_, widget);
-  client_widget_ = NULL;
+  client_widget_ = nullptr;
 }
 
 void TouchSelectionControllerImpl::OnWidgetBoundsChanged(
@@ -539,37 +646,30 @@ void TouchSelectionControllerImpl::OnScrollEvent(ui::ScrollEvent* event) {
 
 void TouchSelectionControllerImpl::ContextMenuTimerFired() {
   // Get selection end points in client_view's space.
-  gfx::Rect end_rect_1_in_screen;
-  gfx::Rect end_rect_2_in_screen;
-  if (cursor_handle_->IsWidgetVisible()) {
-    end_rect_1_in_screen = selection_end_point_1_clipped_;
-    end_rect_2_in_screen = end_rect_1_in_screen;
-  } else {
-    end_rect_1_in_screen = selection_end_point_1_clipped_;
-    end_rect_2_in_screen = selection_end_point_2_clipped_;
-  }
-
+  ui::SelectionBound b1_in_screen = selection_bound_1_clipped_;
+  ui::SelectionBound b2_in_screen =
+      cursor_handle_->IsWidgetVisible() ? b1_in_screen
+                                        : selection_bound_2_clipped_;
   // Convert from screen to client.
-  gfx::Rect end_rect_1(ConvertFromScreen(client_view_, end_rect_1_in_screen));
-  gfx::Rect end_rect_2(ConvertFromScreen(client_view_, end_rect_2_in_screen));
+  ui::SelectionBound b1 = ConvertFromScreen(client_view_, b1_in_screen);
+  ui::SelectionBound b2 = ConvertFromScreen(client_view_, b2_in_screen);
 
   // if selection is completely inside the view, we display the context menu
   // in the middle of the end points on the top. Else, we show it above the
   // visible handle. If no handle is visible, we do not show the menu.
   gfx::Rect menu_anchor;
-  if (ShouldShowHandleFor(end_rect_1) &&
-      ShouldShowHandleFor(end_rect_2))
-    menu_anchor = Union(end_rect_1_in_screen,end_rect_2_in_screen);
-  else if (ShouldShowHandleFor(end_rect_1))
-    menu_anchor = end_rect_1_in_screen;
-  else if (ShouldShowHandleFor(end_rect_2))
-    menu_anchor = end_rect_2_in_screen;
+  if (ShouldShowHandleFor(b1) && ShouldShowHandleFor(b2))
+    menu_anchor = ui::RectBetweenSelectionBounds(b1_in_screen, b2_in_screen);
+  else if (ShouldShowHandleFor(b1))
+    menu_anchor = BoundToRect(b1_in_screen);
+  else if (ShouldShowHandleFor(b2))
+    menu_anchor = BoundToRect(b2_in_screen);
   else
     return;
 
   DCHECK(!context_menu_);
   context_menu_ = TouchEditingMenuView::Create(this, menu_anchor,
-                                               GetHandleImageSize(),
+                                               GetMaxHandleImageSize(),
                                                client_view_->GetNativeView());
 }
 
@@ -592,7 +692,7 @@ void TouchSelectionControllerImpl::UpdateContextMenu() {
 void TouchSelectionControllerImpl::HideContextMenu() {
   if (context_menu_)
     context_menu_->Close();
-  context_menu_ = NULL;
+  context_menu_ = nullptr;
   context_menu_timer_.Stop();
 }
 
@@ -600,16 +700,16 @@ gfx::NativeView TouchSelectionControllerImpl::GetCursorHandleNativeView() {
   return cursor_handle_->GetWidget()->GetNativeView();
 }
 
-gfx::Point TouchSelectionControllerImpl::GetSelectionHandle1Position() {
-  return selection_handle_1_->GetScreenPosition();
+gfx::Rect TouchSelectionControllerImpl::GetSelectionHandle1Bounds() {
+  return selection_handle_1_->GetBoundsInScreen();
 }
 
-gfx::Point TouchSelectionControllerImpl::GetSelectionHandle2Position() {
-  return selection_handle_2_->GetScreenPosition();
+gfx::Rect TouchSelectionControllerImpl::GetSelectionHandle2Bounds() {
+  return selection_handle_2_->GetBoundsInScreen();
 }
 
-gfx::Point TouchSelectionControllerImpl::GetCursorHandlePosition() {
-  return cursor_handle_->GetScreenPosition();
+gfx::Rect TouchSelectionControllerImpl::GetCursorHandleBounds() {
+  return cursor_handle_->GetBoundsInScreen();
 }
 
 bool TouchSelectionControllerImpl::IsSelectionHandle1Visible() {
@@ -624,4 +724,17 @@ bool TouchSelectionControllerImpl::IsCursorHandleVisible() {
   return cursor_handle_->IsWidgetVisible();
 }
 
+gfx::Rect TouchSelectionControllerImpl::GetExpectedHandleBounds(
+    const ui::SelectionBound& bound) {
+  return GetSelectionWidgetBounds(bound);
+}
+
+views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle1View() {
+  return selection_handle_1_.get();
+}
+
+views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle2View() {
+  return selection_handle_2_.get();
+}
+
 }  // namespace views