MacViews: Implement Tab Dragging

chrome/browser/ui/views/tabs/tab_drag_controller.cc

Index: chrome/browser/ui/views/tabs/tab_drag_controller.cc
diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller.cc b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
index 5c5160367fccd04fa56076e1a5c9f61f7242fba8..7e426218c8a70f4ec1e0ec7a23e7534059af57c7 100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
@@ -382,7 +382,7 @@ void TabDragController::Drag(const gfx::Point& point_in_screen) {
                                          &drag_bounds);
         widget->SetVisibilityChangedAnimationsEnabled(true);
       }
-      RunMoveLoop(GetWindowOffset(point_in_screen));
+      RunMoveLoop(GetWindowOffset(start_point_in_screen_));

[tapted, 2016/03/10 11:51:18]

I think this should land first and separately with its own explainer since it
affects all platforms and might cause a subtle regression.. Also someone more
familiar with tab_drag_controller.cc might know a good reason why it was
point_in_screen rather than start_point_in_screen.

(although I seem to recall that Windows would even ignore its argument to
`RunMoveLoop` or something, but that was hard to fix -- if this just fixes the
"slippage" on Ash it's probably worth doing -- there is http://crbug.com/518740
for that)

[themblsha, 2016/03/10 17:18:58]

Without this fix the DetachToBrowserTabDragControllerTest.DragAll won't pass for
sure, and it'll also affect all other tests that rely on
CocoaWindowMoveLoop-related code to move the window.

Maybe it's better to ensure that it doesn't break no other platforms in this CL
and add one more reviewer for that?

[tapted, 2016/03/11 09:38:28]

The problem currently is that it's hard for a reviewer to understand why it's
needed and reason about whether it could break things. Maybe it just needs a
comment above it, like // Always use the start point <because reason>.

[themblsha, 2016/04/05 17:20:42]

Added this comment:

      // Always use the start point so the MacViews'
      // BridgedNativeWidget::RunMoveLoop() could position the window so the
      // mouse would be directly on top of start_point_in_screen_. Otherwise the
      // point by which the window is dragged will be wrong.
      //
      // The DetachToBrowserTabDragControllerTest.DragAll/0 test on MacViews
      // will detect the inconsistencies.

       return;
     }
   }
@@ -524,8 +524,9 @@ bool TabDragController::CanStartDrag(const gfx::Point& point_in_screen) const {
   static const int kMinimumDragDistance = 10;
   int x_offset = abs(point_in_screen.x() - start_point_in_screen_.x());
   int y_offset = abs(point_in_screen.y() - start_point_in_screen_.y());
-  return sqrt(pow(static_cast<float>(x_offset), 2) +
+  bool result = sqrt(pow(static_cast<float>(x_offset), 2) +

[tapted, 2016/03/10 11:51:18]

nit: this change not needed

[themblsha, 2016/03/10 17:18:58]

Done.

               pow(static_cast<float>(y_offset), 2)) > kMinimumDragDistance;
+  return result;
 }
 
 void TabDragController::ContinueDragging(const gfx::Point& point_in_screen) {
@@ -608,6 +609,7 @@ TabDragController::DragBrowserToNewTabStrip(
     // going to trigger capture lost, which cancels drag.
     attached_tabstrip_->ReleaseDragController();
     target_tabstrip->OwnDragController(this);
+

[tapted, 2016/03/10 11:51:18]

nit: remove added line

[themblsha, 2016/03/10 17:18:58]

Done.

     // Disable animations so that we don't see a close animation on aero.
     browser_widget->SetVisibilityChangedAnimationsEnabled(false);
     if (can_release_capture_)