Reenable Tabdragging tests failing because of IsWindowPositionManaged()

chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.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_interactive_uitest.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 
@@ skipping 778 matching lines @@
 }
 #else
 bool IsWindowPositionManaged(gfx::NativeWindow window) {
   return true;
 }
 bool HasUserChangedWindowPositionOrSize(gfx::NativeWindow window) {
   return false;
 }
 #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());
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
 
   // Move to the first tab and drag it enough so that it detaches.
   gfx::Point tab_0_center(
       GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
   ASSERT_TRUE(PressInput(tab_0_center));
   ASSERT_TRUE(DragInputToNotifyWhenDone(
@@ skipping 32 matching lines @@
   // Both windows should not be maximized
   EXPECT_FALSE(browser()->window()->IsMaximized());
   EXPECT_FALSE(new_browser->window()->IsMaximized());
 
   // The tab strip should no longer have capture because the drag was ended and
   // mouse/touch was released.
   EXPECT_FALSE(tab_strip->GetWidget()->HasCapture());
   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
 #else
 #define MAYBE_DetachFromFullsizeWindow DetachFromFullsizeWindow
 #endif
 // Tests that a tab can be dragged from a browser window that is resized to full
 // screen.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
                        MAYBE_DetachFromFullsizeWindow) {
@@ skipping 40 matching lines @@
 
   EXPECT_FALSE(GetIsDragged(browser()));
   EXPECT_FALSE(GetIsDragged(new_browser));
   // After this both windows should still be manageable.
   EXPECT_TRUE(IsWindowPositionManaged(browser()->window()->GetNativeWindow()));
   EXPECT_TRUE(
       IsWindowPositionManaged(new_browser->window()->GetNativeWindow()));
 
   // 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
   // mouse/touch was released.
   EXPECT_FALSE(tab_strip->GetWidget()->HasCapture());
   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 \
   DISABLED_DetachToOwnWindowFromMaximizedWindow
 #else
 #define MAYBE_DetachToOwnWindowFromMaximizedWindow \
   DetachToOwnWindowFromMaximizedWindow
 #endif
 // Drags from browser to a separate window and releases mouse.
 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.
   AddTabAndResetBrowser(browser());
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
 
   // Move to the first tab and drag it enough so that it detaches.
   gfx::Point tab_0_center(
       GetCenterInScreenCoordinates(tab_strip->tab_at(0)));
   ASSERT_TRUE(PressInput(tab_0_center));
@@ skipping 23 matching lines @@
   EXPECT_TRUE(browser()->window()->IsMaximized());
 
   EXPECT_FALSE(GetIsDragged(browser()));
   EXPECT_FALSE(GetIsDragged(new_browser));
   // After this both windows should still be manageable.
   EXPECT_TRUE(IsWindowPositionManaged(browser()->window()->GetNativeWindow()));
   EXPECT_TRUE(IsWindowPositionManaged(
       new_browser->window()->GetNativeWindow()));
 
   // The new window should be maximized.
+  MaximizedBrowserWindowWaiter(new_browser->window()).Wait();
   EXPECT_TRUE(new_browser->window()->IsMaximized());
 }
 
 // Deletes a tab being dragged before the user moved enough to start a drag.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
                        DeleteBeforeStartedDragging) {
   // Add another tab.
   AddTabAndResetBrowser(browser());
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
 
@@ skipping 1419 matching lines @@
       base::Bind(&DetachToDockedWindowNextStep,
                  test,
                  gfx::Point(target_point.x(), 1 + target_point.y()),
                  iteration - 1)));
 }
 
 }  // namespace
 
 // 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());
 
   // Add another tab.
   AddTabAndResetBrowser(browser());
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
 
   // Move to the first tab and drag it enough so that it detaches.
   gfx::Point tab_0_center(
@@ skipping 56 matching lines @@
                         DetachToBrowserTabDragControllerTest,
                         ::testing::Values("mouse", "touch"));
 INSTANTIATE_TEST_CASE_P(TabDragging,
                         DetachToBrowserTabDragControllerTestTouch,
                         ::testing::Values("touch"));
 #else
 INSTANTIATE_TEST_CASE_P(TabDragging,
                         DetachToBrowserTabDragControllerTest,
                         ::testing::Values("mouse"));
 #endif