MacViews: Implement Tab Dragging

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/tabs/tab_drag_controller.h"
 
 #include <math.h>
 #include <set>
 
 #include "base/auto_reset.h"
@@ skipping 364 matching lines @@
                           point_in_screen.x() -
                           mouse_offset_.x(), 0);
         widget->SetVisibilityChangedAnimationsEnabled(false);
         widget->Restore();
         widget->SetBounds(new_bounds);
         AdjustBrowserAndTabBoundsForDrag(last_tabstrip_width,
                                          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;
     }
   }
 
   ContinueDragging(point_in_screen);
 }
 
 void TabDragController::EndDrag(EndDragReason reason) {
   TRACE_EVENT0("views", "TabDragController::EndDrag");
 
@@ skipping 121 matching lines @@
     return;
   old_focused_view->GetFocusManager()->SetFocusedView(old_focused_view);
 }
 
 bool TabDragController::CanStartDrag(const gfx::Point& point_in_screen) const {
   // Determine if the mouse has moved beyond a minimum elasticity distance in
   // any direction from the starting point.
   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) {
   TRACE_EVENT1("views", "TabDragController::ContinueDragging",
                "point_in_screen", point_in_screen.ToString());
 
   DCHECK(attached_tabstrip_);
 
   TabStrip* target_tabstrip = detach_behavior_ == DETACHABLE ?
       GetTargetTabStripForPoint(point_in_screen) : source_tabstrip_;
@@ skipping 62 matching lines @@
     // observer so we don't get notified and process the event.
 #if defined(USE_ASH)
     move_loop_widget_->RemoveObserver(this);
     move_loop_widget_ = nullptr;
 #endif  // USE_ASH
     views::Widget* browser_widget = GetAttachedBrowserWidget();
     // Need to release the drag controller before starting the move loop as it's
     // 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_)
       browser_widget->ReleaseCapture();
     else
       target_tabstrip->GetWidget()->SetCapture(attached_tabstrip_);
 
     // The window is going away. Since the drag is still on going we don't want
     // that to effect the position of any windows.
     SetWindowPositionManaged(browser_widget->GetNativeWindow(), false);
@@ skipping 1186 matching lines @@
   // window which was used for dragging is not hidden once all of its tabs are
   // attached to another browser window in DragBrowserToNewTabStrip().
   // TODO(pkotwicz): Fix this properly (crbug.com/358482)
   for (auto* browser : *BrowserList::GetInstance()) {
     if (browser->tab_strip_model()->empty())
       exclude.insert(browser->window()->GetNativeWindow());
   }
 #endif
   return GetLocalProcessWindowAtPoint(screen_point, exclude);
 }