Add enum for TabDragController mode and replace various state flags with it.

Also changes what happens when the drag ends during a nested move loop.  It
now exits the move loop (which triggers a drag end) instead of immediately
ending the drag.

This is the first step in a larger refactoring for TabDragController:
go/tabdragcontroller-untangling

Change-Id: I6cabc5237a604320c934bb8b5d4bbc9ff6d231f0
Reviewed-on: https://chromium-review.googlesource.com/c/1285085
Commit-Queue: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611352}

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

@@ -323,28 +323,24 @@ const int TabDragController::kTouchVerticalDetachMagnetism = 50;
 const int TabDragController::kVerticalDetachMagnetism = 15;
 
 TabDragController::TabDragController()
-    : event_source_(EVENT_SOURCE_MOUSE),
+    : current_state_(DragState::kNotStarted),
+      event_source_(EVENT_SOURCE_MOUSE),
       source_tabstrip_(NULL),
       attached_tabstrip_(NULL),
       can_release_capture_(true),
       offset_to_width_ratio_(0),
       old_focused_view_tracker_(std::make_unique<views::ViewTracker>()),
       last_move_screen_loc_(0),
-      started_drag_(false),
-      active_(true),
       source_tab_index_(std::numeric_limits<size_t>::max()),
       initial_move_(true),
       detach_behavior_(DETACHABLE),
       move_behavior_(REORDER),
       mouse_has_ever_moved_left_(false),
       mouse_has_ever_moved_right_(false),
-      is_dragging_window_(false),
       is_dragging_new_browser_(false),
       was_source_maximized_(false),
       was_source_fullscreen_(false),
       did_restore_window_(false),
-      end_run_loop_behavior_(END_RUN_LOOP_STOP_DRAGGING),
-      waiting_for_run_loop_to_exit_(false),
       tab_strip_to_attach_to_after_exit_(NULL),
       move_loop_widget_(NULL),
       is_mutating_(false),
@@ -462,9 +458,7 @@ TabStrip* TabDragController::GetSourceTabStrip() {
 }
 
 void TabDragController::SetMoveBehavior(MoveBehavior behavior) {
-  if (started_drag())
-    return;
-
+  if (current_state_ == DragState::kNotStarted)
     move_behavior_ = behavior;
 }
 
@@ -483,10 +477,12 @@ void TabDragController::Drag(const gfx::Point& point_in_screen) {
   bring_to_front_timer_.Stop();
   move_stacked_timer_.Stop();
 
-  if (waiting_for_run_loop_to_exit_)
+  if (current_state_ == DragState::kWaitingToDragTabs ||
+      current_state_ == DragState::kWaitingToStop ||
+      current_state_ == DragState::kStopped)
     return;
 
-  if (!started_drag_) {
+  if (current_state_ == DragState::kNotStarted) {
     if (!CanStartDrag(point_in_screen))
       return;  // User hasn't dragged far enough yet.
 
@@ -497,7 +493,7 @@ void TabDragController::Drag(const gfx::Point& point_in_screen) {
       if (!ref)
         return;
     }
-    started_drag_ = true;
+    current_state_ = DragState::kDraggingTabs;
     Attach(source_tabstrip_, gfx::Point());
     if (static_cast<int>(drag_data_.size()) ==
         GetModel(source_tabstrip_)->count()) {
@@ -548,8 +544,18 @@ void TabDragController::EndDrag(EndDragReason reason) {
   // If we're dragging a window ignore capture lost since it'll ultimately
   // trigger the move loop to end and we'll revert the drag when RunMoveLoop()
   // finishes.
-  if (reason == END_DRAG_CAPTURE_LOST && is_dragging_window_)
+  if (reason == END_DRAG_CAPTURE_LOST &&
+      current_state_ == DragState::kDraggingWindow) {
+    return;
+  }
+
+  // If we're dragging a window, end the move loop, returning control to
+  // RunMoveLoop() which will end the drag.
+  if (current_state_ == DragState::kDraggingWindow) {
+    current_state_ = DragState::kWaitingToStop;
+    GetAttachedBrowserWidget()->EndMoveLoop();
     return;
+  }
 
 #if defined(OS_CHROMEOS)
   // It's possible that in Chrome OS we defer the windows that are showing in
@@ -697,7 +703,9 @@ TabDragController::Liveness TabDragController::ContinueDragging(
   // it till the drag ends and reset |target_tabstrip| here.
   if (ShouldAttachOnEnd(target_tabstrip)) {
     SetDeferredTargetTabstrip(target_tabstrip);
-    target_tabstrip = is_dragging_window_ ? attached_tabstrip_ : nullptr;
+    target_tabstrip = current_state_ == DragState::kDraggingWindow
+                          ? attached_tabstrip_
+                          : nullptr;
   } else {
     SetDeferredTargetTabstrip(nullptr);
   }
@@ -721,14 +729,14 @@ TabDragController::Liveness TabDragController::ContinueDragging(
       return Liveness::ALIVE;
     }
   }
-  if (is_dragging_window_) {
+  if (current_state_ == DragState::kDraggingWindow) {
     bring_to_front_timer_.Start(
         FROM_HERE, base::TimeDelta::FromMilliseconds(kBringToFrontDelay),
         base::Bind(&TabDragController::BringWindowUnderPointToFront,
                    base::Unretained(this), point_in_screen));
   }
 
-  if (!is_dragging_window_ && attached_tabstrip_) {
+  if (current_state_ == DragState::kDraggingTabs) {
     if (move_only()) {
       DragActiveTabStacked(point_in_screen);
     } else {
@@ -770,7 +778,7 @@ TabDragController::DragBrowserToNewTabStrip(TabStrip* target_tabstrip,
       ui::TransferTouchesBehavior::kDontCancel);
 #endif
 
-  if (is_dragging_window_) {
+  if (current_state_ == DragState::kDraggingWindow) {
     // ReleaseCapture() is going to result in calling back to us (because it
     // results in a move). That'll cause all sorts of problems.  Reset the
     // observer so we don't get notified and process the event.
@@ -806,17 +814,16 @@ TabDragController::DragBrowserToNewTabStrip(TabStrip* target_tabstrip,
     // (crbug.com/116329).
     if (can_release_capture_) {
       tab_strip_to_attach_to_after_exit_ = target_tabstrip;
+      current_state_ = DragState::kWaitingToDragTabs;
     } else {
-      is_dragging_window_ = false;
       Detach(DONT_RELEASE_CAPTURE);
       Attach(target_tabstrip, point_in_screen);
+      current_state_ = DragState::kDraggingTabs;
       // Move the tabs into position.
       MoveAttached(point_in_screen);
       attached_tabstrip_->GetWidget()->Activate();
     }
 
-    waiting_for_run_loop_to_exit_ = true;
-    end_run_loop_behavior_ = END_RUN_LOOP_CONTINUE_DRAGGING;
     return DRAG_BROWSER_RESULT_STOP;
   }
   Detach(DONT_RELEASE_CAPTURE);
@@ -858,7 +865,7 @@ void TabDragController::MoveAttachedToPreviousStackedIndex(
 
 void TabDragController::MoveAttached(const gfx::Point& point_in_screen) {
   DCHECK(attached_tabstrip_);
-  DCHECK(!is_dragging_window_);
+  DCHECK_EQ(current_state_, DragState::kDraggingTabs);
 
   gfx::Point dragged_view_point = GetAttachedDragPoint(point_in_screen);
 
@@ -1017,7 +1024,8 @@ TabDragController::Liveness TabDragController::GetTargetTabStripForPoint(
   }
   gfx::NativeWindow local_window;
   const Liveness state = GetLocalProcessWindow(
-      point_in_screen, is_dragging_window_, &local_window);
+      point_in_screen, current_state_ == DragState::kDraggingWindow,
+      &local_window);
   if (state == Liveness::DELETED)
     return Liveness::DELETED;
 
@@ -1040,7 +1048,8 @@ TabDragController::Liveness TabDragController::GetTargetTabStripForPoint(
     }
   }
 
-  *tab_strip = is_dragging_window_ ? attached_tabstrip_ : nullptr;
+  *tab_strip = current_state_ == DragState::kDraggingWindow ? attached_tabstrip_
+                                                            : nullptr;
   return Liveness::ALIVE;
 }
 
@@ -1283,7 +1292,7 @@ void TabDragController::RunMoveLoop(const gfx::Vector2d& drag_offset) {
   move_loop_widget_ = GetAttachedBrowserWidget();
   DCHECK(move_loop_widget_);
   move_loop_widget_->AddObserver(this);
-  is_dragging_window_ = true;
+  current_state_ = DragState::kDraggingWindow;
   base::WeakPtr<TabDragController> ref(weak_factory_.GetWeakPtr());
   if (can_release_capture_) {
     // Running the move loop releases mouse capture, which triggers destroying
@@ -1315,14 +1324,17 @@ void TabDragController::RunMoveLoop(const gfx::Vector2d& drag_offset) {
     move_loop_widget_->RemoveObserver(this);
     move_loop_widget_ = nullptr;
   }
-  is_dragging_window_ = false;
-  waiting_for_run_loop_to_exit_ = false;
-  if (end_run_loop_behavior_ == END_RUN_LOOP_CONTINUE_DRAGGING) {
-    end_run_loop_behavior_ = END_RUN_LOOP_STOP_DRAGGING;
-    if (tab_strip_to_attach_to_after_exit_) {
+
+  if (current_state_ == DragState::kDraggingWindow) {
+    current_state_ = DragState::kWaitingToStop;
+  }
+
+  if (current_state_ == DragState::kWaitingToDragTabs) {
+    DCHECK(tab_strip_to_attach_to_after_exit_);
     gfx::Point point_in_screen(GetCursorScreenPoint());
     Detach(DONT_RELEASE_CAPTURE);
     Attach(tab_strip_to_attach_to_after_exit_, point_in_screen);
+    current_state_ = DragState::kDraggingTabs;
     // Move the tabs into position.
     MoveAttached(point_in_screen);
     attached_tabstrip_->GetWidget()->Activate();
@@ -1330,10 +1342,7 @@ void TabDragController::RunMoveLoop(const gfx::Vector2d& drag_offset) {
     if (!ref)
       return;
     tab_strip_to_attach_to_after_exit_ = NULL;
-    }
-    DCHECK(attached_tabstrip_);
-    attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_);
-  } else if (active_) {
+  } else if (current_state_ == DragState::kWaitingToStop) {
     EndDrag(result == views::Widget::MOVE_LOOP_CANCELED ?
             END_DRAG_CANCEL : END_DRAG_COMPLETE);
   }
@@ -1395,33 +1404,17 @@ std::vector<gfx::Rect> TabDragController::CalculateBoundsForDraggedTabs() {
 }
 
 void TabDragController::EndDragImpl(EndDragType type) {
-  DCHECK(active_);
-  active_ = false;
+  DragState previous_state = current_state_;
+  current_state_ = DragState::kStopped;
 
   bring_to_front_timer_.Stop();
   move_stacked_timer_.Stop();
 
-  if (move_loop_widget_) {
-    // This function is only called when the drag is ending. At this point we
-    // don't care about any subsequent moves to the widget, so we remove the
-    // observer. If we didn't do this we could get told the widget moved and
-    // attempt to do the wrong thing.
-    move_loop_widget_->RemoveObserver(this);
-    move_loop_widget_ = nullptr;
-  }
-
-  if (is_dragging_window_) {
-    waiting_for_run_loop_to_exit_ = true;
-
-    // End the nested drag loop.
-    GetAttachedBrowserWidget()->EndMoveLoop();
-  }
-
   if (type != TAB_DESTROYED) {
     // We only finish up the drag if we were actually dragging. If start_drag_
     // is false, the user just clicked and released and didn't move the mouse
     // enough to trigger a drag.
-    if (started_drag_) {
+    if (previous_state != DragState::kNotStarted) {
       // After the drag ends, if |attached_tabstrip_| is showing in overview
       // mode, do not restore focus, otherwise overview mode may be ended
       // unexpectly because of the window activation.
@@ -1435,7 +1428,7 @@ void TabDragController::EndDragImpl(EndDragType type) {
     }
   } else if (drag_data_.size() > 1) {
     initial_selection_model_.Clear();
-    if (started_drag_)
+    if (previous_state != DragState::kNotStarted)
       RevertDrag();
   }  // else case the only tab we were dragging was deleted. Nothing to do.
 
@@ -1567,7 +1560,7 @@ void TabDragController::RestoreInitialSelection() {
 }
 
 void TabDragController::RevertDragAt(size_t drag_index) {
-  DCHECK(started_drag_);
+  DCHECK_NE(current_state_, DragState::kNotStarted);
   DCHECK(source_tabstrip_);
 
   base::AutoReset<bool> setter(&is_mutating_, true);
@@ -1604,7 +1597,7 @@ void TabDragController::RevertDragAt(size_t drag_index) {
 }
 
 void TabDragController::CompleteDrag() {
-  DCHECK(started_drag_);
+  DCHECK_NE(current_state_, DragState::kNotStarted);
 
   if (attached_tabstrip_) {
     if (is_dragging_new_browser_ || did_restore_window_) {
@@ -1727,7 +1720,7 @@ void TabDragController::BringWindowUnderPointToFront(
 
     // The previous call made the window appear on top of the dragged window,
     // move the dragged window to the front.
-    if (is_dragging_window_)
+    if (current_state_ == DragState::kDraggingWindow)
       attached_tabstrip_->GetWidget()->StackAtTop();
   }
 }
@@ -1967,7 +1960,8 @@ void TabDragController::SetTabDraggingInfo() {
 #if defined(OS_CHROMEOS)
   TabStrip* dragged_tabstrip =
       attached_tabstrip_ ? attached_tabstrip_ : source_tabstrip_;
-  DCHECK(dragged_tabstrip->IsDragSessionActive() && active_);
+  DCHECK(dragged_tabstrip->IsDragSessionActive() &&
+         current_state_ != DragState::kStopped);
 
   aura::Window* dragged_window =
       GetWindowForTabDraggingProperties(dragged_tabstrip);
@@ -1985,7 +1979,8 @@ void TabDragController::ClearTabDraggingInfo() {
 #if defined(OS_CHROMEOS)
   TabStrip* dragged_tabstrip =
       attached_tabstrip_ ? attached_tabstrip_ : source_tabstrip_;
-  DCHECK(!dragged_tabstrip->IsDragSessionActive() || !active_);
+  DCHECK(!dragged_tabstrip->IsDragSessionActive() ||
+         current_state_ == DragState::kStopped);
   // Do not clear the dragging info properties for a to-be-destroyed window.
   // They will be cleared later in Window's destructor. It's intentional as
   // ash::SplitViewController::TabDraggedWindowObserver listens to both

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

@@ -114,18 +114,20 @@ class TabDragController : public views::WidgetObserver,
   EventSource event_source() const { return event_source_; }
 
   // See description above fields for details on these.
-  bool active() const { return active_; }
+  bool active() const { return current_state_ != DragState::kStopped; }
   const TabStrip* attached_tabstrip() const { return attached_tabstrip_; }
 
   // Returns true if a drag started.
-  bool started_drag() const { return started_drag_; }
+  bool started_drag() const { return current_state_ != DragState::kNotStarted; }
 
   // Returns true if mutating the TabStripModel.
   bool is_mutating() const { return is_mutating_; }
 
   // Returns true if we've detached from a tabstrip and are running a nested
   // move message loop.
-  bool is_dragging_window() const { return is_dragging_window_; }
+  bool is_dragging_window() const {
+    return current_state_ == DragState::kDraggingWindow;
+  }
 
   // Returns true if currently dragging a tab with |contents|.
   bool IsDraggingTab(content::WebContents* contents);
@@ -143,6 +145,24 @@ class TabDragController : public views::WidgetObserver,
   static const int kMovedMouseLeft  = 1 << 0;
   static const int kMovedMouseRight = 1 << 1;
 
+  enum class DragState {
+    // The drag has not yet started; the user has not dragged far enough to
+    // begin a session.
+    kNotStarted,
+    // The session is dragging a set of tabs within |attached_tabstrip_|.
+    kDraggingTabs,
+    // The session is dragging a window; |attached_tabstrip_| is that window's
+    // tabstrip.
+    kDraggingWindow,
+    // The session is waiting for the nested move loop to exit to transition
+    // to kDraggingTabs.  Not used on all platforms.
+    kWaitingToDragTabs,
+    // The session is waiting for the nested move loop to exit to end the drag.
+    kWaitingToStop,
+    // The drag session has completed or been canceled.
+    kStopped
+  };
+
   enum class Liveness {
     ALIVE,
     DELETED,
@@ -166,15 +186,6 @@ class TabDragController : public views::WidgetObserver,
     DONT_RELEASE_CAPTURE,
   };
 
-  // Specifies what should happen when RunMoveLoop completes.
-  enum EndRunLoopBehavior {
-    // Indicates the drag should end.
-    END_RUN_LOOP_STOP_DRAGGING,
-
-    // Indicates the drag should continue.
-    END_RUN_LOOP_CONTINUE_DRAGGING
-  };
-
   // Enumeration of the possible positions the detached tab may detach from.
   enum DetachPosition {
     DETACH_BEFORE,
@@ -473,6 +484,8 @@ class TabDragController : public views::WidgetObserver,
   // property.
   void SetDeferredTargetTabstrip(TabStrip* deferred_target_tabstrip);
 
+  DragState current_state_;
+
   // Whether a drag to |window| should be blocked (for example, if the window
   // is showing a modal).
   bool ShouldDisallowDrag(gfx::NativeWindow window);
@@ -544,12 +557,6 @@ class TabDragController : public views::WidgetObserver,
   // StartMoveStackedTimerIfNecessary().
   base::OneShotTimer move_stacked_timer_;
 
-  // Did the mouse move enough that we started a drag?
-  bool started_drag_;
-
-  // Is the drag active?
-  bool active_;
-
   DragData drag_data_;
 
   // Index of the source tab in |drag_data_|.
@@ -591,9 +598,6 @@ class TabDragController : public views::WidgetObserver,
   // The following are needed when detaching into a browser
   // (|detach_into_browser_| is true).
 
-  // See description above getter.
-  bool is_dragging_window_;
-
   // True if |attached_tabstrip_| is in a browser specifically created for
   // the drag.
   bool is_dragging_new_browser_;
@@ -608,11 +612,6 @@ class TabDragController : public views::WidgetObserver,
   // maximized).
   bool did_restore_window_;
 
-  EndRunLoopBehavior end_run_loop_behavior_;
-
-  // If true, we're waiting for a move loop to complete.
-  bool waiting_for_run_loop_to_exit_;
-
   // The TabStrip to attach to after the move loop completes.
   TabStrip* tab_strip_to_attach_to_after_exit_;
 

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

@@ -246,6 +246,11 @@ void TabDragControllerTest::HandleGestureEvent(TabStrip* tab_strip,
   tab_strip->OnGestureEvent(event);
 }
 
+bool TabDragControllerTest::HasDragStarted(const TabStrip* tab_strip) const {
+  return tab_strip->drag_controller_ &&
+         tab_strip->drag_controller_->started_drag();
+}
+
 void TabDragControllerTest::SetUp() {
 #if defined(USE_AURA)
   // This needs to be disabled as it can interfere with when events are
@@ -1210,6 +1215,31 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   EXPECT_FALSE(GetIsDragged(browser()));
 }
 
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
+                       DragDoesntStartFromClick) {
+  // Add another tab.
+  AddTabAndResetBrowser(browser());
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+
+  // Click on the first tab, but don't move it.
+  gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
+  EXPECT_TRUE(PressInput(tab_0_center));
+
+  // A drag session should exist, but the drag should not have started.
+  EXPECT_TRUE(tab_strip->IsDragSessionActive());
+  EXPECT_TRUE(TabDragController::IsActive());
+  EXPECT_FALSE(HasDragStarted(tab_strip));
+
+  // Move the mouse enough to start the drag.  It doesn't matter whether this
+  // is enough to create a window or not.
+  EXPECT_TRUE(DragInputTo(gfx::Point(tab_0_center.x() + 20, tab_0_center.y())));
+
+  // Drag should now have started.
+  EXPECT_TRUE(HasDragStarted(tab_strip));
+
+  EXPECT_TRUE(ReleaseInput());
+}
+
 #if defined(OS_CHROMEOS)
 // TODO(sky,sad): Disabled as it fails due to resize locks with a real
 // compositor. crbug.com/331924

chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h

@@ -50,6 +50,8 @@ class TabDragControllerTest : public InProcessBrowserTest {
  protected:
   void HandleGestureEvent(TabStrip* tab_strip, ui::GestureEvent* event);
 
+  bool HasDragStarted(const TabStrip* tab_strip) const;
+
   // InProcessBrowserTest:
   void SetUp() override;