https://codereview.chromium.org/1747803003/

patch.diff

diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller.cc b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
index a1389f8..3300ecf 100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
@@ -382,13 +382,9 @@ void TabDragController::Drag(const gfx::Point& point_in_screen) {
                                          &drag_bounds);
         widget->SetVisibilityChangedAnimationsEnabled(true);
       }
-      // 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.
+      // Use the start point to initialize the move loop, otherwise the point
+      // under the cursor will be offset by the drag threshold from where the
+      // user originally clicked, which could be entirely off the window.
       RunMoveLoop(GetWindowOffset(start_point_in_screen_));
       return;
     }
diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc b/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
index df083f8..3d79cd8 100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
@@ -172,6 +172,12 @@ void TabDragControllerTest::AddTabAndResetBrowser(Browser* browser) {
   AddBlankTabAndShow(browser);
   StopAnimating(GetTabStripForBrowser(browser));
   ResetIDs(browser->tab_strip_model(), 0);
+#if defined(OS_MACOSX)
+  // Currently MacViews' browser windows are shown in the background anc could
+  // be obscured by other windows if there are any. This should be fixed in
+  // order to be consistent with other platforms.
+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser));
+#endif  // OS_MACOSX
 }
 
 Browser* TabDragControllerTest::CreateAnotherWindowBrowserAndRelayout() {
@@ -1671,12 +1677,8 @@ void CancelOnNewTabWhenDraggingStep2(
 
   // Add another tab. This should trigger exiting the nested loop. Add at the
   // to exercise past crash when model/tabstrip got out of sync (474082).
-  content::WindowedNotificationObserver observer(
-      content::NOTIFICATION_LOAD_STOP,
-      content::NotificationService::AllSources());
   chrome::AddTabAt(browser_list->GetLastActive(), GURL(url::kAboutBlankURL),
                    0, false);
-  observer.Wait();
 }
 
 }  // namespace
@@ -1701,10 +1703,18 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   gfx::Point tab_0_center(
       GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
   ASSERT_TRUE(PressInput(tab_0_center));
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
   ASSERT_TRUE(DragInputToNotifyWhenDone(
       tab_0_center.x(), tab_0_center.y() + GetDetachY(tab_strip),
       base::Bind(&CancelOnNewTabWhenDraggingStep2, this, browser_list)));
-  QuitWhenNotDragging();
+  observer.Wait();
+
+  // On MacViews the Drag ends while waiting for NOTIFICATION_LOAD_STOP.
+  if (TabDragController::IsActive()) {
+    QuitWhenNotDragging();
+  }
 
   // Should be two windows and not dragging.
   ASSERT_FALSE(TabDragController::IsActive());
@@ -2290,6 +2300,7 @@ class DetachToBrowserInSeparateDisplayAndCancelTabDragControllerTest
   }
 
   void QuitWhenNotDragging() {
+    DCHECK(TabDragController::IsActive());
     test::QuitWhenNotDraggingImpl();
     base::MessageLoop::current()->Run();
   }
diff --git a/chrome/browser/ui/views/tabs/window_finder_mac.mm b/chrome/browser/ui/views/tabs/window_finder_mac.mm
index 9f8b7f2..0283a80 100644
--- a/chrome/browser/ui/views/tabs/window_finder_mac.mm
+++ b/chrome/browser/ui/views/tabs/window_finder_mac.mm
@@ -24,13 +24,11 @@ gfx::NativeWindow WindowFinder::GetLocalProcessWindowAtPoint(
 
     // NativeWidgetMac::Close() calls -orderOut: on NSWindows before actually
     // closing them.
-    if (![window isVisible]) {
+    if (![window isVisible])
       continue;
-    }
 
-    if (NSPointInRect(ns_point, [window frame])) {
+    if (NSPointInRect(ns_point, [window frame]))
       return window;
-    }
   }
 
   return nil;
diff --git a/ui/base/test/ui_controls_mac.mm b/ui/base/test/ui_controls_mac.mm
index 9f49cdf..3e03b6a 100644
--- a/ui/base/test/ui_controls_mac.mm
+++ b/ui/base/test/ui_controls_mac.mm
@@ -14,7 +14,6 @@
 #import "base/mac/scoped_objc_class_swizzler.h"
 #include "base/message_loop/message_loop.h"
 #include "base/thread_task_runner_handle.h"
-#include "content/public/browser/browser_thread.h"
 #include "ui/base/cocoa/cocoa_base_utils.h"
 #include "ui/events/keycodes/keyboard_code_conversion_mac.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
@@ -150,10 +149,10 @@ void RunTaskInTaskRunner(
 
 // A helper function to watch for the event queue. The specific task will be
 // fired when there is no more event in the queue.
-// NOTE: It should be run on the UI thread, as otherwise it will always report
-// there are no pending events.
 void EventQueueWatcher(const base::Closure& task) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK_EQ(dispatch_get_current_queue(), dispatch_get_main_queue())
+      << "It should be run on the UI thread, as otherwise it will always "
+         "report there are no pending events";
   NSEvent* event = [NSApp nextEventMatchingMask:NSAnyEventMask
                                       untilDate:nil
                                          inMode:NSDefaultRunLoopMode
@@ -216,21 +215,19 @@ class EventMonitor {
              "browser window prior to generating CGEvents.";
     }
 
-    dispatch_sync(queue_, ^{
-      tasks_.emplace_back(Task(event, task));
-    });
+    base::AutoLock auto_lock(lock_);
+    tasks_.emplace_back(Task(event, task));
   }
 
   void ProcessingEvent(NSEvent* event) {
-    dispatch_sync(queue_, ^{
-      auto it = std::find_if(
-          tasks_.begin(), tasks_.end(),
-          [&event](const Task& task) { return task.MatchesEvent(event); });
-      if (it != tasks_.end()) {
-        it->Run();
-        tasks_.erase(it);
-      }
-    });
+    base::AutoLock auto_lock(lock_);
+    auto it = std::find_if(
+        tasks_.begin(), tasks_.end(),
+        [&event](const Task& task) { return task.MatchesEvent(event); });
+    if (it != tasks_.end()) {
+      it->Run();
+      tasks_.erase(it);
+    }
   }
 
   static EventMonitor* Instance() {
@@ -261,11 +258,10 @@ class EventMonitor {
       // We get here before the event is actually processed. Run the
       // EventQueueWatcher on the main thread in order to wait for all events to
       // finish processing.
-      content::BrowserThread::PostTask(
-          content::BrowserThread::UI, FROM_HERE,
-          base::Bind(
-              &EventQueueWatcher,
-              base::Bind(&RunTaskInTaskRunner, task_runner_, finish_closure_)));
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&EventQueueWatcher,
+                                base::Bind(&RunTaskInTaskRunner, task_runner_,
+                                           finish_closure_)));
     }
 
    private:
@@ -277,19 +273,16 @@ class EventMonitor {
   };
 
   EventMonitor()
-      : queue_(dispatch_queue_create("ui_controls_mac.EventMonitor",
-                                     DISPATCH_QUEUE_SERIAL)),
-        send_event_swizzler_(
+      : send_event_swizzler_(
             new base::mac::ScopedObjCClassSwizzler([NSApplication class],
                                                    @selector(sendEvent:),
-                                                   @selector(cr_sendEvent:))) {
-  }
+                                                   @selector(cr_sendEvent:))) {}
 
   std::vector<Task> tasks_;
 
   // synchronizes access to the |tasks_| in case we spawn the events on a
   // different thread
-  dispatch_queue_t queue_ = 0;
+  base::Lock lock_;
 
   scoped_ptr<base::mac::ScopedObjCClassSwizzler>
       send_event_swizzler_;