Reenable Tabdragging tests failing because of IsWindowPositionManaged()

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 10d80815a0cfdcafb4aebf88a3522c4285ead267..366070c9813316b9c7ac868d1d13b7a2ee4b937c 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
@@ -796,18 +796,54 @@ bool HasUserChangedWindowPositionOrSize(gfx::NativeWindow window) {
 }
 #endif
 
+// Encapsulates waiting for the browser window to become maximized. This is
+// needed for example on Chrome desktop linux, where window maximization is done
+// asynchronously as an event received from a different process.
+class MaximizedBrowserWindowWaiter {
+ public:
+  explicit MaximizedBrowserWindowWaiter(BrowserWindow* window)
+      : window_(window) {}
+  ~MaximizedBrowserWindowWaiter() = default;
+
+  // Blocks until the browser window becomes maximized.
+  void Wait() {
+    base::RunLoop run_loop;
+    quit_ = run_loop.QuitClosure();
+    base::MessageLoop::current()->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&MaximizedBrowserWindowWaiter::CheckMaximized,
+                              base::Unretained(this)));

[Peter Kasting, 2016/11/17 01:32:49]

Optional nit: We can save the repeated ugly PostTask() by doing something like
this:

  if (!CheckMaximized()) {  // Relies on |quit_| not being set yet
    quit_ = run_loop.QuitClosure();
    run_loop.Run();
  }

  ...

  bool CheckMaximized() {
    const bool maximized = window_->IsMaximized();
    if (maximized) {
      if (!quit_.is_null())
        quit_.Run();
    } else {
      base::MessageLoop::current()->task_runner()->PostTask(...
    }
    return maximized;
  }

This only breaks if Wait() can be called repeatedly for the same object.  To
avoid this, you could eliminate Wait() and move its code into the constructor
body, so callers cannot wait more than once.  (Or you could fix it, but that's
not actually necessary today.)

[afakhry, 2016/11/17 22:51:05]

This is a great suggestion! Thanks! 
Done. 
I'm not quite clear on why calling Wait() on the same object breaks it? Can you
please clarify?

[Peter Kasting, 2016/11/17 23:05:24]

After the first Wait(), |quit_| will remain initialized.  If Wait() is then
called a second time, when the window is already maximized, CheckMaximized()
will call quit_.Run() when the message loop is not running.  This seems bad
(reading the code, I think this causes the loop to immediately quit the next
time someone tries to run it).

[afakhry, 2016/11/18 16:45:36]

Yes, I also thought that's what you meant. I attempted to fix this in the new
patch by:

    if (!quit_.is_null())
      base::ResetAndReturn(&quit_).Run();

+    run_loop.Run();
+    CHECK(window_->IsMaximized());

[Peter Kasting, 2016/11/17 01:32:49]

Nit: CHECK in test code can causes tests to shut down uncleanly, which can leave
the bot in a bad state.  Generally something like ASSERT() is preferred, though
in this case I'd simply omit this line entirely, since you EXPECT it in the
callers already.

[afakhry, 2016/11/17 22:51:05]

Done. Removed.

+  }
+
+ private:
+  void CheckMaximized() {
+    if (!window_->IsMaximized()) {
+      base::MessageLoop::current()->task_runner()->PostTask(
+          FROM_HERE, base::Bind(&MaximizedBrowserWindowWaiter::CheckMaximized,
+                                base::Unretained(this)));
+      return;
+    }
+
+    // Quit the run_loop to end the wait.
+    if (!quit_.is_null())
+      quit_.Run();
+  }
+
+  // The browser window observed by this waiter.
+  BrowserWindow* window_;
+
+  // The waiter's RunLoop quit closure.
+  base::Closure quit_;
+
+  DISALLOW_COPY_AND_ASSIGN(MaximizedBrowserWindowWaiter);
+};
+
 }  // namespace
 
-#if defined(OS_CHROMEOS) || defined(OS_LINUX)
-// TODO(sky,sad): Disabled as it fails due to resize locks with a real
-// compositor. crbug.com/331924
-#define MAYBE_DetachToOwnWindow DISABLED_DetachToOwnWindow
-#else
-#define MAYBE_DetachToOwnWindow DetachToOwnWindow
-#endif
 // Drags from browser to separate window and releases mouse.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
-                       MAYBE_DetachToOwnWindow) {
+                       DetachToOwnWindow) {
   const gfx::Rect initial_bounds(browser()->window()->GetBounds());
   // Add another tab.
   AddTabAndResetBrowser(browser());
@@ -860,9 +896,13 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   EXPECT_FALSE(tab_strip2->GetWidget()->HasCapture());
 }
 
-#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX)
-// TODO(sky,sad): Disabled as it fails due to resize locks with a real
-// compositor. crbug.com/331924
+#if defined(OS_LINUX) || defined(OS_MACOSX)
+// TODO(afakhry,varkha): Disabled on Linux as it fails on the bot because
+// setting the window bounds to the work area bounds in
+// DesktopWindowTreeHostX11::SetBounds() always insets it by one pixel in both
+// width and height. This results in considering the source browser window not
+// being full size, and the test is not as expected.
+// crbug.com/626761, crbug.com/331924.
 // TODO(tapted,mblsha): Disabled as the Mac IsMaximized() behavior is not
 // consistent with other platforms. crbug.com/603562
 #define MAYBE_DetachFromFullsizeWindow DISABLED_DetachFromFullsizeWindow
@@ -923,6 +963,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
 
   // Only second window should be maximized.
   EXPECT_FALSE(browser()->window()->IsMaximized());
+  MaximizedBrowserWindowWaiter(new_browser->window()).Wait();
   EXPECT_TRUE(new_browser->window()->IsMaximized());
 
   // The tab strip should no longer have capture because the drag was ended and
@@ -931,9 +972,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   EXPECT_FALSE(tab_strip2->GetWidget()->HasCapture());
 }
 
-#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX)
-// TODO(sky,sad): Disabled as it fails due to resize locks with a real
-// compositor. crbug.com/331924
+#if defined(OS_MACOSX)
 // TODO(tapted,mblsha): Disabled as the Mac IsMaximized() behavior is not
 // consistent with other platforms. crbug.com/603562
 #define MAYBE_DetachToOwnWindowFromMaximizedWindow \
@@ -947,6 +986,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
                        MAYBE_DetachToOwnWindowFromMaximizedWindow) {
   // Maximize the initial browser window.
   browser()->window()->Maximize();
+  MaximizedBrowserWindowWaiter(browser()->window()).Wait();
   ASSERT_TRUE(browser()->window()->IsMaximized());
 
   // Add another tab.
@@ -990,6 +1030,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
       new_browser->window()->GetNativeWindow()));
 
   // The new window should be maximized.
+  MaximizedBrowserWindowWaiter(new_browser->window()).Wait();
   EXPECT_TRUE(new_browser->window()->IsMaximized());
 }
 
@@ -2429,7 +2470,7 @@ void DetachToDockedWindowNextStep(
 
 // Drags from browser to separate window, docks that window and releases mouse.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
-                       DISABLED_DetachToDockedWindowFromMaximizedWindow) {
+                       DetachToDockedWindowFromMaximizedWindow) {
   // Maximize the initial browser window.
   browser()->window()->Maximize();
   ASSERT_TRUE(browser()->window()->IsMaximized());