[ChromeDriver] Refactored window state normalization code

Previously, each function MinimizeWindow, MaximizeWindow, etc... had
their own code to change window state to normal before calling
GetWindowBounds. This changelist refactors the code so window state
normalization only occurs in one place.

R=johnchen@chromium.org

Change-Id: I0a865adf6e9e1d64afc0694930a6982789f6656a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1696451
Commit-Queue: Julian Kung <juliankung@google.com>
Reviewed-by: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676502}

chrome/test/chromedriver/chrome/chrome_impl.cc

@@ -182,19 +182,10 @@ Status ChromeImpl::SetWindowPosition(const std::string& target_id,
   if (status.IsError())
     return status;
 
-  if (window.state != "normal") {
-    // restore window to normal first to allow position change.
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   auto bounds = std::make_unique<base::DictionaryValue>();
   bounds->SetInteger("left", x);
   bounds->SetInteger("top", y);
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::MaximizeWindow(const std::string& target_id) {
@@ -206,19 +197,9 @@ Status ChromeImpl::MaximizeWindow(const std::string& target_id) {
   if (window.state == "maximized")
     return Status(kOk);
 
-  if (window.state != "normal") {
-    // always restore window to normal first, since chrome ui doesn't allow
-    // maximizing a minimized or fullscreen window.
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   auto bounds = std::make_unique<base::DictionaryValue>();
   bounds->SetString("windowState", "maximized");
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::MinimizeWindow(const std::string& target_id) {
@@ -230,18 +211,9 @@ Status ChromeImpl::MinimizeWindow(const std::string& target_id) {
   if (window.state == "minimized")
     return Status(kOk);
 
-  if (window.state != "normal") {
-    // restore window to normal first
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   auto bounds = std::make_unique<base::DictionaryValue>();
   bounds->SetString("windowState", "minimized");
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::FullScreenWindow(const std::string& target_id) {
@@ -253,17 +225,9 @@ Status ChromeImpl::FullScreenWindow(const std::string& target_id) {
   if (window.state == "fullscreen")
     return Status(kOk);
 
-  if (window.state != "normal") {
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   auto bounds = std::make_unique<base::DictionaryValue>();
   bounds->SetString("windowState", "fullscreen");
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::SetWindowRect(const std::string& target_id,
@@ -275,15 +239,6 @@ Status ChromeImpl::SetWindowRect(const std::string& target_id,
 
   auto bounds = std::make_unique<base::DictionaryValue>();
 
-  // fully exit fullscreen
-  if (window.state != "normal") {
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   // window position
   int x = 0;
   int y = 0;
@@ -300,7 +255,7 @@ Status ChromeImpl::SetWindowRect(const std::string& target_id,
     bounds->SetInteger("height", height);
   }
 
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::GetWindowSize(const std::string& target_id,
@@ -333,14 +288,23 @@ Status ChromeImpl::GetWindowBounds(int window_id, Window* window) {
 }
 
 Status ChromeImpl::SetWindowBounds(
-    int window_id,
+    Window* window,
     std::unique_ptr<base::DictionaryValue> bounds) {
   Status status = devtools_websocket_client_->ConnectIfNecessary();
   if (status.IsError())
     return status;
 
   base::DictionaryValue params;
-  params.SetInteger("windowId", window_id);
+  params.SetInteger("windowId", window->id);
+  if (window->state != "normal") {
+    params.SetString("bounds.windowState", "normal");
+    status = devtools_websocket_client_->SendCommand("Browser.setWindowBounds",
+                                                     params);
+    if (status.IsError())
+      return status;
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+  }
+
   params.Set("bounds", bounds->CreateDeepCopy());
   status = devtools_websocket_client_->SendCommand("Browser.setWindowBounds",
                                                    params);
@@ -352,14 +316,13 @@ Status ChromeImpl::SetWindowBounds(
   if (!bounds->GetString("windowState", &state))
     return Status(kOk);
 
-  Window window;
-  status = GetWindowBounds(window_id, &window);
+  status = GetWindowBounds(window->id, window);
   if (status.IsError())
     return status;
-  if (window.state == state)
+  if (window->state == state)
     return Status(kOk);
 
-  if (state == "maximized" && window.state == "normal") {
+  if (state == "maximized" && window->state == "normal") {
     // Maximize window is not supported in some environment, such as Mac Chrome
     // version 70 and above, or Linux without a window manager.
     // In these cases, we simulate window maximization by setting window size
@@ -391,7 +354,7 @@ Status ChromeImpl::SetWindowBounds(
                                                    params);
   } else {
     return Status(kUnknownError, "failed to change window state to " + state +
-                                     ", current state is " + window.state);
+                                     ", current state is " + window->state);
   }
 }
 
@@ -404,19 +367,10 @@ Status ChromeImpl::SetWindowSize(const std::string& target_id,
   if (status.IsError())
     return status;
 
-  if (window.state != "normal") {
-    // restore window to normal first to allow size change.
-    auto bounds = std::make_unique<base::DictionaryValue>();
-    bounds->SetString("windowState", "normal");
-    status = SetWindowBounds(window.id, std::move(bounds));
-    if (status.IsError())
-      return status;
-  }
-
   auto bounds = std::make_unique<base::DictionaryValue>();
   bounds->SetInteger("width", width);
   bounds->SetInteger("height", height);
-  return SetWindowBounds(window.id, std::move(bounds));
+  return SetWindowBounds(&window, std::move(bounds));
 }
 
 Status ChromeImpl::ParseWindow(std::unique_ptr<base::DictionaryValue> params,

chrome/test/chromedriver/chrome/chrome_impl.h

@@ -79,7 +79,7 @@ class ChromeImpl : public Chrome {
   Status ParseWindowBounds(std::unique_ptr<base::DictionaryValue> params,
                            Window* window);
   Status GetWindowBounds(int window_id, Window* window);
-  Status SetWindowBounds(int window_id,
+  Status SetWindowBounds(Window* window,
                          std::unique_ptr<base::DictionaryValue> bounds);
 
   bool quit_;