fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to devtools command.

BUG=chromedriver:1779

Review-Url: https://codereview.chromium.org/2836023003
Cr-Commit-Position: refs/heads/master@{#467893}

chrome/test/chromedriver/chrome/chrome_desktop_impl.cc

@@ -264,19 +264,47 @@ Status ChromeDesktopImpl::GetWindowSize(const std::string& target_id,
 Status ChromeDesktopImpl::SetWindowPosition(const std::string& target_id,
                                             int x,
                                             int y) {
+  Window window;
+  Status status = GetWindow(target_id, &window);
+  if (status.IsError())
+    return status;
+
+  if (window.state != "normal") {
+    // restore window to normal first to allow position change.
+    auto bounds = base::MakeUnique<base::DictionaryValue>();
+    bounds->SetString("windowState", "normal");
+    status = SetWindowBounds(window.id, std::move(bounds));
+    if (status.IsError())
+      return status;
+  }
+
   auto bounds = base::MakeUnique<base::DictionaryValue>();
   bounds->SetInteger("left", x);
   bounds->SetInteger("top", y);
-  return SetWindowBounds(target_id, std::move(bounds));
+  return SetWindowBounds(window.id, std::move(bounds));
 }
 
 Status ChromeDesktopImpl::SetWindowSize(const std::string& target_id,
                                         int width,
                                         int height) {
+  Window window;
+  Status status = GetWindow(target_id, &window);
+  if (status.IsError())
+    return status;
+
+  if (window.state != "normal") {
+    // restore window to normal first to allow size change.
+    auto bounds = base::MakeUnique<base::DictionaryValue>();
+    bounds->SetString("windowState", "normal");
+    status = SetWindowBounds(window.id, std::move(bounds));
+    if (status.IsError())
+      return status;
+  }
+
   auto bounds = base::MakeUnique<base::DictionaryValue>();
   bounds->SetInteger("width", width);
   bounds->SetInteger("height", height);
-  return SetWindowBounds(target_id, std::move(bounds));
+  return SetWindowBounds(window.id, std::move(bounds));
 }
 
 Status ChromeDesktopImpl::MaximizeWindow(const std::string& target_id) {
@@ -291,20 +319,21 @@ Status ChromeDesktopImpl::MaximizeWindow(const std::string& target_id) {
   if (window.state != "normal") {
     // always restore window to normal first, since chrome ui doesn't allow
     // maximizing a minimized or fullscreen window.
-    status = SetWindowState(window.id, "normal");
+    auto bounds = base::MakeUnique<base::DictionaryValue>();
+    bounds->SetString("windowState", "normal");
+    status = SetWindowBounds(window.id, std::move(bounds));
     if (status.IsError())
       return status;
   }
 
-  return SetWindowState(window.id, "maximized");
+  auto bounds = base::MakeUnique<base::DictionaryValue>();
+  bounds->SetString("windowState", "maximized");
+  return SetWindowBounds(window.id, std::move(bounds));
 }
 
-Status ChromeDesktopImpl::ParseWindow(
+Status ChromeDesktopImpl::ParseWindowBounds(
     std::unique_ptr<base::DictionaryValue> params,
     Window* window) {
-  if (!params->GetInteger("windowId", &window->id))
-    return Status(kUnknownError, "no window id in response");
-
   const base::Value* value = nullptr;
   const base::DictionaryValue* bounds_dict = nullptr;
   if (!params->Get("bounds", &value) || !value->GetAsDictionary(&bounds_dict))
@@ -325,6 +354,15 @@ Status ChromeDesktopImpl::ParseWindow(
   return Status(kOk);
 }
 
+Status ChromeDesktopImpl::ParseWindow(
+    std::unique_ptr<base::DictionaryValue> params,
+    Window* window) {
+  if (!params->GetInteger("windowId", &window->id))
+    return Status(kUnknownError, "no window id in response");
+
+  return ParseWindowBounds(std::move(params), window);
+}
+
 Status ChromeDesktopImpl::GetWindow(const std::string& target_id,
                                     Window* window) {
   Status status = devtools_websocket_client_->ConnectIfNecessary();
@@ -342,47 +380,48 @@ Status ChromeDesktopImpl::GetWindow(const std::string& target_id,
   return ParseWindow(std::move(result), window);
 }
 
-Status ChromeDesktopImpl::SetWindowState(int window_id,
-                                         const std::string& window_state) {
+Status ChromeDesktopImpl::GetWindowBounds(int window_id, Window* window) {
   Status status = devtools_websocket_client_->ConnectIfNecessary();
   if (status.IsError())
     return status;
 
   base::DictionaryValue params;
   params.SetInteger("windowId", window_id);
-  auto bounds_object = base::MakeUnique<base::DictionaryValue>();
-  bounds_object->SetString("windowState", window_state);
-  params.Set("bounds", std::move(bounds_object));
-  status = devtools_websocket_client_->SendCommand("Browser.setWindowBounds",
-                                                   params);
+  std::unique_ptr<base::DictionaryValue> result;
+  status = devtools_websocket_client_->SendCommandAndGetResult(
+      "Browser.getWindowBounds", params, &result);
   if (status.IsError())
     return status;
 
-  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
-  return Status(kOk);
+  return ParseWindowBounds(std::move(result), window);
 }
 
 Status ChromeDesktopImpl::SetWindowBounds(
-    const std::string& target_id,
+    int window_id,
     std::unique_ptr<base::DictionaryValue> bounds) {
-  Window window;
-  Status status = GetWindow(target_id, &window);
+  Status status = devtools_websocket_client_->ConnectIfNecessary();
   if (status.IsError())
     return status;
 
-  if (window.state != "normal") {
-    status = SetWindowState(window.id, "normal");
-    if (status.IsError())
-      return status;
-  }
-
   base::DictionaryValue params;
-  params.SetInteger("windowId", window.id);
-  params.Set("bounds", std::move(bounds));
+  params.SetInteger("windowId", window_id);
+  params.Set("bounds", bounds->CreateDeepCopy());
   status = devtools_websocket_client_->SendCommand("Browser.setWindowBounds",
                                                    params);
   if (status.IsError())
     return status;
 
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+  std::string state;
+  if (!bounds->GetString("windowState", &state))
+    return Status(kOk);
+
+  Window window;
+  status = GetWindowBounds(window_id, &window);
+  if (status.IsError())
+    return status;
+  if (window.state != state)
+    return Status(kUnknownError, "failed to change window state to " + state +
+                                     ", current state is " + window.state);
   return Status(kOk);
 }

chrome/test/chromedriver/chrome/chrome_desktop_impl.h

@@ -82,12 +82,14 @@ class ChromeDesktopImpl : public ChromeImpl {
     int width;
     int height;
   };
+  Status ParseWindowBounds(std::unique_ptr<base::DictionaryValue> params,
+                           Window* window);
   Status ParseWindow(std::unique_ptr<base::DictionaryValue> params,
                      Window* window);
 
   Status GetWindow(const std::string& target_id, Window* window);
-  Status SetWindowState(int window_id, const std::string& window_state);
-  Status SetWindowBounds(const std::string& target_id,
+  Status GetWindowBounds(int window_id, Window* window);
+  Status SetWindowBounds(int window_id,
                          std::unique_ptr<base::DictionaryValue> bounds);
 
   base::Process process_;

chrome/test/chromedriver/test/run_py_tests.py

@@ -99,10 +99,7 @@ _OS_SPECIFIC_FILTER['linux'] = [
     # Xvfb doesn't support maximization.
     'ChromeDriverTest.testWindowMaximize',
 ]
-_OS_SPECIFIC_FILTER['mac'] = [
-    # https://bugs.chromium.org/p/chromedriver/issues/detail?id=1779
-    'ChromeDriverTest.testWindowMaximize',
-]
+_OS_SPECIFIC_FILTER['mac'] = []
 
 _DESKTOP_NEGATIVE_FILTER = [
     # Desktop doesn't support touch (without --touch-events).
@@ -864,11 +861,11 @@ class ChromeDriverTest(ChromeDriverBaseTestWithWebServer):
 
   def testWindowMaximize(self):
     self._driver.SetWindowPosition(100, 200)
-    self._driver.SetWindowSize(600, 400)
+    self._driver.SetWindowSize(500, 300)
     self._driver.MaximizeWindow()
 
     self.assertNotEqual([100, 200], self._driver.GetWindowPosition())
-    self.assertNotEqual([600, 400], self._driver.GetWindowSize())
+    self.assertNotEqual([500, 300], self._driver.GetWindowSize())
     # Set size first so that the window isn't moved offscreen.
     # See https://bugs.chromium.org/p/chromedriver/issues/detail?id=297.
     self._driver.SetWindowSize(600, 400)