MacViews: Implement Tab Dragging

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

Index: chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
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 40b5a0357f5b58040447b12673dc9ea9fa4fcf86..3d79cd84f0efbc0bc946bf8f2b95346da3071a86 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
@@ -17,6 +17,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -171,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));

[tapted, 2016/04/13 08:28:12]

Let's move this to TabDragControllerTest::SetUpOnMainThread and ensure
DetachToBrowserTabDragControllerTest::SetUpOnMainThread calls its superclass's
TabDragControllerTest::SetUpOnMainThread()

nothing else should have to call BringBrowserWindowToFront then

[themblsha, 2016/04/18 09:30:00]

Done.

+#endif  // OS_MACOSX
 }
 
 Browser* TabDragControllerTest::CreateAnotherWindowBrowserAndRelayout() {
@@ -181,7 +188,8 @@ Browser* TabDragControllerTest::CreateAnotherWindowBrowserAndRelayout() {
   // Resize the two windows so they're right next to each other.
   gfx::Rect work_area =
       gfx::Screen::GetScreen()
-          ->GetDisplayNearestWindow(browser()->window()->GetNativeWindow())
+          ->GetDisplayNearestWindow(platform_util::GetViewForWindow(
+              browser()->window()->GetNativeWindow()))
           .work_area();
   gfx::Size half_size =
       gfx::Size(work_area.width() / 3 - 10, work_area.height() / 2 - 10);
@@ -249,7 +257,7 @@ class ScreenEventGeneratorDelegate
 
 #endif
 
-#if !defined(OS_CHROMEOS)
+#if !defined(OS_CHROMEOS) && defined(USE_AURA)
 
 // Following classes verify a crash scenario. Specifically on Windows when focus
 // changes it can trigger capture being lost. This was causing a crash in tab
@@ -862,7 +870,8 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   // Resize the browser window so that it is as big as the work area.
   gfx::Rect work_area =
       gfx::Screen::GetScreen()
-          ->GetDisplayNearestWindow(browser()->window()->GetNativeWindow())
+          ->GetDisplayNearestWindow(platform_util::GetViewForWindow(
+              browser()->window()->GetNativeWindow()))
           .work_area();
   browser()->window()->SetBounds(work_area);
   const gfx::Rect initial_bounds(browser()->window()->GetBounds());
@@ -1222,6 +1231,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest, MAYBE_DragAll) {
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
   browser()->tab_strip_model()->AddTabAtToSelection(0);
   browser()->tab_strip_model()->AddTabAtToSelection(1);
+  const gfx::Rect initial_bounds = browser()->window()->GetBounds();
 
   // Move to the first tab and drag it enough so that it would normally
   // detach.
@@ -1245,7 +1255,150 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest, MAYBE_DragAll) {
 
   // Remaining browser window should not be maximized
   EXPECT_FALSE(browser()->window()->IsMaximized());
+
+  const gfx::Rect final_bounds = browser()->window()->GetBounds();

[tapted, 2016/04/13 08:28:12]

Comment ~

// Size unchanged, but it should have moved down.

[themblsha, 2016/04/18 09:30:01]

Done.

+  EXPECT_EQ(initial_bounds.size(), final_bounds.size());
+  EXPECT_EQ(initial_bounds.origin().x(), final_bounds.origin().x());
+  EXPECT_EQ(initial_bounds.origin().y() + GetDetachY(tab_strip),
+            final_bounds.origin().y());
+}
+
+#if defined(OS_MACOSX)
+// Makes sure we can drag the window using WindowServer by dragging on a tab.
+//
+// All other tests move the windows without relying on the WindowServer, because
+// BridgedNativeWidget::RunMoveLoop() immediately offsets the window on first
+// mouse drag. If we generate more mouse move events after that, it's presumed
+// that the WindowServer will move the window. In order for that to work we need
+// to generate global CGEvents instead of app-specific NSEvents, that's what
+// ui_test_utils::DragAndDrop() is for.
+//

[tapted, 2016/04/13 08:28:12]

Perhaps a note here, since it will be confusing for those unfamiliar, to say

// Note that Mac doesn't simply rely on NSEvents, since they would not allow
// correct interactions with the window server. E.g., a window could not be
// dragged to another desktop Space.

[themblsha, 2016/04/18 09:30:01]

Done.

+// If the WindowServer fails to move the window, the final window bounds won't
+// match the expected ones.
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,

[tapted, 2016/04/13 08:28:12]

A comment specific to this test?

E.g. 

// Test dragging all the tabs in the window.

[themblsha, 2016/04/18 09:30:01]

Done.

+                       MacDragsWindowUsingCocoaMoveLoop) {
+  // Add another tab.
+  AddTabAndResetBrowser(browser());
+
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+  browser()->tab_strip_model()->AddTabAtToSelection(0);
+  browser()->tab_strip_model()->AddTabAtToSelection(1);
+  const gfx::Rect initial_bounds = browser()->window()->GetBounds();
+
+  // Move to the first tab and drag it enough so that it would normally
+  // detach.
+  gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
+
+  // If we don't make the interactive_ui_tests the active application, we won't
+  // be able to monitor NSMouseMoved events and ui_test_utils::DragAndDrop()
+  // will deadlock.
+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
+  // We need to move the window using multiple intermediate events in order
+  // to verify that CocoaWindowMoveLoop is working correctly.
+  const int steps = 10;
+  ui_test_utils::DragAndDrop(
+      tab_0_center,
+      gfx::Point(tab_0_center.x(), tab_0_center.y() + GetDetachY(tab_strip)),
+      steps);
+
+  // Should not be dragging.
+  EXPECT_FALSE(tab_strip->IsDragSessionActive());
+  EXPECT_FALSE(TabDragController::IsActive());
+  EXPECT_FALSE(GetIsDragged(browser()));
+
+  // And there should only be one window.
+  EXPECT_EQ(1u, browser_list->size());
+  EXPECT_EQ("0 1", IDString(browser()->tab_strip_model()));
+
+  // Remaining browser window should not be maximized
+  EXPECT_FALSE(browser()->window()->IsMaximized());
+
+  const gfx::Rect final_bounds = browser()->window()->GetBounds();
+  EXPECT_EQ(initial_bounds.size(), final_bounds.size());
+  EXPECT_EQ(initial_bounds.origin().x(), final_bounds.origin().x());
+  EXPECT_EQ(initial_bounds.origin().y() + GetDetachY(tab_strip),
+            final_bounds.origin().y());
+}
+
+// Tests that when the first mouse event that starts RunMoveLoop does not
+// overlap the Mac menu bar. BridgedNativeWidget::RunMoveLoop() shifts both the
+// mouse position and the expected window frame after detachment.
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
+                       MacDetachesWindowOnTopOfMacMenuBar) {
+  // Add another tab.
+  AddTabAndResetBrowser(browser());
+
+  // Make sure there's enough space to trigger detachment.
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+  browser()->window()->SetBounds(gfx::Rect(100, 100, 400, 200));
+  DCHECK_GT(browser()->window()->GetBounds().y(), GetDetachY(tab_strip));
+
+  // If we don't make the interactive_ui_tests the active application, we won't
+  // be able to monitor NSMouseMoved events and ui_test_utils::DragAndDrop()
+  // will deadlock.
+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
+  gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
+  gfx::Point on_top_of_menu_bar(tab_0_center.x(), 0);
+  ui_test_utils::DragAndDrop(tab_0_center, on_top_of_menu_bar);

[tapted, 2016/04/13 08:28:12]

using a sequence of DragAndDropOperation in place of this causes a failure for
me. E.g. I did


  browser()->window()->SetBounds(gfx::Rect(100, 200, 400, 200));
  DCHECK_GT(browser()->window()->GetBounds().y(), GetDetachY(tab_strip));

  gfx::Point tab_0_center(GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
  gfx::Point at_top_of_screen(tab_0_center.x(), 0);
  gfx::Point below_top_of_screen(tab_0_center.x(), 100);

  std::list<DragAndDropOperation> operations;
  operations.push_back(DragAndDropOperation::Move(tab_0_center));
  operations.push_back(DragAndDropOperation::MouseDown());
  operations.push_back(DragAndDropOperation::Move(at_top_of_screen));
  operations.push_back(DragAndDropOperation::Move(below_top_of_screen));
  operations.push_back(DragAndDropOperation::MouseUp());
  ui_test_utils::DragAndDrop(operations);

If at_top_of_screen is 50px instead of 0px from the top, the test works fine.
But with 0px the test fails - the window moves up, but doesn't initiate a drag.

[themblsha, 2016/04/18 09:30:01]

Added additional check to my code:

  EXPECT_GE(work_area.y(), 22)
      << "Without the MenuBar on top of work_area this test is pointless.";

Maybe somehow you don't have the menu bar on top of your screen?

[tapted, 2016/04/20 07:30:22]

On one of them :).

If you run two screens side-by-side, and don't use "Displays have separate
spaces" then one of the monitors will not have a menu bar at the top. And the
screen edge restricts movement in the same way as the menubar does.

But the point of my comment was that the test previously failed (regardless of
the menu bar presence). The checks were not enough to pick up the problem that
required fixes to move the cursor not just into the work area, but to ensure it
was in the window in the same place as the work area would have forced it to
move to.

So we should make sure we have a test case for this. In fact I wrote one already
at 
https://codereview.chromium.org/1877043003/diff2/20001:60001/chrome/browser/u...

[themblsha, 2016/04/20 13:38:40]

Curiously enough, your test only passes when I have two 27" monitors plugged in,
when only a single one is plugged it fails. But after replacing 2500 with 100 it
works fine even in single-monitor configuration.

And your test will fail if the BridgedNativeWidget::RunMoveLoop() mouse y
position shifting is removed even when the menu bar is not present, so it's
better than my version.

Replaced my test function with yours.

+
+  // Should not be dragging.
+  EXPECT_FALSE(tab_strip->IsDragSessionActive());
+  EXPECT_FALSE(TabDragController::IsActive());
+  EXPECT_FALSE(GetIsDragged(browser()));
+
+  // Second tab should successfully detach.
+  EXPECT_EQ(2u, browser_list->size());
+  EXPECT_EQ("1", IDString(browser()->tab_strip_model()));
+}
+
+// Detaches and reattaches second tab. Will fail if detached window is not moved
+// synchronously at the start of BridgedNativeWidget::RunMoveLoop(): the check
+// for expected WindowServerFrame() will fail.
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
+                       MacDetachesAndReattachesSecondTab) {
+  using namespace ui_test_utils;

[tapted, 2016/04/13 08:28:12]

nit: using ui_test_utils::DragAndDropOperation, and move it to above the
std::list declaration

[themblsha, 2016/04/18 09:30:01]

Done.

+
+  // Add another tab.
+  AddTabAndResetBrowser(browser());
+
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+  browser()->window()->SetBounds(gfx::Rect(100, 100, 400, 200));
+  const gfx::Rect initial_bounds = browser()->window()->GetBounds();
+
+  gfx::Point tab_1_center(GetCenterInScreenCoordinates(tab_strip->tab_at(1)));
+  gfx::Point tab_1_shake(tab_1_center.x(),
+                         tab_1_center.y() + GetDetachY(tab_strip) * 2);
+
+  // If we don't make the interactive_ui_tests the active application, we won't
+  // be able to monitor NSMouseMoved events and ui_test_utils::DragAndDrop()
+  // will deadlock.

[tapted, 2016/04/13 08:28:12]

nit: this comment doesn't need to repeat

[themblsha, 2016/04/18 09:30:01]

Done.

+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
+  std::list<DragAndDropOperation> operations;
+  operations.emplace_back(DragAndDropOperation::Move(tab_1_center));

[tapted, 2016/04/13 08:28:12]

The rule at Google is to use push_back unless that doesn't work.
DragAndDropOperation is copyable, so it should be push_back here (The downside
of emplace_back is that it invisibly calls explicit constructors which can lead
to unexpected stuff like

vector<vector<int>> baz;
baz.emplace_back(1 << 10);  // Inserts a vector of size 1024.

[themblsha, 2016/04/18 09:30:00]

Done. Style Guide is absolute :-)

+  operations.emplace_back(DragAndDropOperation::MouseDown());
+  operations.emplace_back(DragAndDropOperation::Move(tab_1_shake));
+  operations.emplace_back(DragAndDropOperation::Move(tab_1_center));
+  operations.emplace_back(DragAndDropOperation::MouseUp());
+
+  ui_test_utils::DragAndDrop(operations);
+
+  // Should not be dragging.
+  EXPECT_FALSE(tab_strip->IsDragSessionActive());
+  EXPECT_FALSE(TabDragController::IsActive());
+  EXPECT_FALSE(GetIsDragged(browser()));
+
+  // And there should only be one window.
+  EXPECT_EQ(1u, browser_list->size());
+  EXPECT_EQ("0 1", IDString(browser()->tab_strip_model()));
+
+  // The window should remain in its original place.
+  const gfx::Rect final_bounds = browser()->window()->GetBounds();
+  EXPECT_EQ(initial_bounds, final_bounds);
 }
+#endif  // OS_MACOSX
 
 namespace {
 
@@ -1475,6 +1628,8 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   TabStrip* tab_strip2 = GetTabStripForBrowser(browser2);
   const gfx::Rect initial_bounds(browser2->window()->GetBounds());
 
+  EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
   // Move to the first tab and drag it enough so that it detaches, but not
   // enough that it attaches to browser2.
   gfx::Point tab_0_center(
@@ -1522,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).

[tapted, 2016/04/13 08:28:12]

This comment should move as well, since it talks about the bug
(http://crbug.com/474082) which has "a new webcontents being created in the
middle of a drag, resulting in a canceled drag where the tabstrip view and its
model are out of sync." - i.e. it needs the LOAD_STOP stuff to exercise that
properly

[themblsha, 2016/04/18 09:30:00]

Done, thanks!

-  content::WindowedNotificationObserver observer(
-      content::NOTIFICATION_LOAD_STOP,
-      content::NotificationService::AllSources());
   chrome::AddTabAt(browser_list->GetLastActive(), GURL(url::kAboutBlankURL),
                    0, false);
-  observer.Wait();
 }
 
 }  // namespace
@@ -1552,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.

[tapted, 2016/04/13 08:28:12]

can you say more about this? (i.e. why it ends early on Mac)

[themblsha, 2016/04/18 09:30:00]

Maybe after moving the stuff around it's not Mac-specific anymore but I haven't
tested. Previously it issued an async command to drag, then immediately executed
QuitWhenNotDragging().

Now on Mac NOTIFICATION_LOAD_STOP executes after the drag'n'drop was cancelled.

Thinking about it now I think this should be removed, and testbots will show if
my assumption is wrong.

+  if (TabDragController::IsActive()) {

[tapted, 2016/04/13 08:28:12]

nit: no curlies

+    QuitWhenNotDragging();
+  }
 
   // Should be two windows and not dragging.
   ASSERT_FALSE(TabDragController::IsActive());
@@ -2141,6 +2300,7 @@ class DetachToBrowserInSeparateDisplayAndCancelTabDragControllerTest
   }
 
   void QuitWhenNotDragging() {
+    DCHECK(TabDragController::IsActive());
     test::QuitWhenNotDraggingImpl();
     base::MessageLoop::current()->Run();
   }