[chromedriver] Fix 3 bugs about web view, window handle and target window.

Main change is the interface of chrome.h

BUG=chromedriver:241, chromedriver:248, chromedriver:254
NOTRY=true

Review URL: https://chromiumcodereview.appspot.com/12978003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@190537 0039d316-1c4b-4281-b951-d872f2087c98

chrome/test/chromedriver/chrome/chrome.h

@@ -17,8 +17,11 @@ class Chrome {
 
   virtual std::string GetVersion() = 0;
 
-  // Return a list of opened WebViews.
-  virtual Status GetWebViews(std::list<WebView*>* web_views) = 0;
+  // Return ids of opened WebViews in the same order as they are opened.
+  virtual Status GetWebViewIds(std::list<std::string>* web_view_ids) = 0;
+
+  // Return the WebView for the given id.
+  virtual Status GetWebViewById(const std::string& id, WebView** web_view) = 0;
 
   // Returns whether a JavaScript dialog is open.
   virtual Status IsJavaScriptDialogOpen(bool* is_open) = 0;

chrome/test/chromedriver/chrome/chrome_android_impl.cc

@@ -9,7 +9,6 @@
 #include "base/process_util.h"
 #include "base/string_number_conversions.h"
 #include "chrome/test/chromedriver/chrome/status.h"
-#include "chrome/test/chromedriver/chrome/web_view.h"
 #include "chrome/test/chromedriver/net/sync_websocket_impl.h"
 #include "chrome/test/chromedriver/net/url_request_context_getter.h"
 
@@ -45,13 +44,6 @@ Status ChromeAndroidImpl::Launch(const std::string& package_name) {
     Quit();
     return status;
   }
-  std::list<WebView*> web_views;
-  status = GetWebViews(&web_views);
-  if (status.IsError() || web_views.empty()) {
-    Quit();
-    return status.IsError() ? status :
-        Status(kUnknownError, "unable to discover open window in chrome");
-  }
 
   return Status(kOk);
 }

chrome/test/chromedriver/chrome/chrome_impl.cc

@@ -204,48 +204,78 @@ ChromeImpl::ChromeImpl(URLRequestContextGetter* context_getter,
       build_no_(0) {}
 
 ChromeImpl::~ChromeImpl() {
-  web_view_map_.clear();
+  web_views_.clear();
 }
 
 std::string ChromeImpl::GetVersion() {
   return version_;
 }
 
-Status ChromeImpl::GetWebViews(std::list<WebView*>* web_views) {
+Status ChromeImpl::GetWebViewIds(std::list<std::string>* web_view_ids) {
   WebViewInfoList info_list;
   Status status = FetchWebViewsInfo(
       context_getter_, port_, &info_list);
   if (status.IsError())
     return status;
 
-  std::list<WebView*> internal_web_views;
+  // Check if some web views are closed.
+  WebViewList::iterator it = web_views_.begin();
+  while (it != web_views_.end()) {
+    if (!GetWebViewFromList((*it)->GetId(), info_list)) {
+      it = web_views_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
+  // Check for newly-opened web views.
   for (WebViewInfoList::const_iterator it = info_list.begin();
        it != info_list.end(); ++it) {
     if (it->type != internal::WebViewInfo::kPage)
       continue;
-    WebViewMap::const_iterator found = web_view_map_.find(it->id);
-    if (found != web_view_map_.end()) {
-      internal_web_views.push_back(found->second.get());
-      continue;
-    }
 
-    std::string ws_url = base::StringPrintf(
-        "ws://127.0.0.1:%d/devtools/page/%s", port_, it->id.c_str());
-    DevToolsClientImpl::FrontendCloserFunc frontend_closer_func = base::Bind(
-        &CloseDevToolsFrontend, this, socket_factory_,
-        context_getter_, port_, it->id);
-    web_view_map_[it->id] = make_linked_ptr(new WebViewImpl(
-        it->id,
-        new DevToolsClientImpl(socket_factory_, ws_url, frontend_closer_func),
-        this,
-        base::Bind(&CloseWebView, context_getter_, port_, it->id)));
-    internal_web_views.push_back(web_view_map_[it->id].get());
+    bool found = false;
+    for (WebViewList::const_iterator web_view_iter = web_views_.begin();
+         web_view_iter != web_views_.end(); ++web_view_iter) {
+      if ((*web_view_iter)->GetId() == it->id) {
+        found = true;
+        break;
+      }
+    }
+    if (!found) {
+      std::string ws_url = base::StringPrintf(
+          "ws://127.0.0.1:%d/devtools/page/%s", port_, it->id.c_str());
+      DevToolsClientImpl::FrontendCloserFunc frontend_closer_func = base::Bind(
+          &CloseDevToolsFrontend, this, socket_factory_,
+          context_getter_, port_, it->id);
+      web_views_.push_back(make_linked_ptr(new WebViewImpl(
+          it->id,
+          new DevToolsClientImpl(socket_factory_, ws_url, frontend_closer_func),
+          this,
+          base::Bind(&CloseWebView, context_getter_, port_, it->id))));
+    }
   }
 
-  web_views->swap(internal_web_views);
+  std::list<std::string> web_view_ids_tmp;
+  for (WebViewList::const_iterator web_view_iter = web_views_.begin();
+       web_view_iter != web_views_.end(); ++web_view_iter) {
+    web_view_ids_tmp.push_back((*web_view_iter)->GetId());
+  }
+  web_view_ids->swap(web_view_ids_tmp);
   return Status(kOk);
 }
 
+Status ChromeImpl::GetWebViewById(const std::string& id, WebView** web_view) {
+  for (WebViewList::iterator it = web_views_.begin();
+       it != web_views_.end(); ++it) {
+    if ((*it)->GetId() == id) {
+      *web_view = (*it).get();
+      return Status(kOk);
+    }
+  }
+  return Status(kUnknownError, "web view not found");
+}
+
 Status ChromeImpl::IsJavaScriptDialogOpen(bool* is_open) {
   JavaScriptDialogManager* manager;
   Status status = GetDialogManagerForOpenDialog(&manager);
@@ -279,7 +309,13 @@ Status ChromeImpl::HandleJavaScriptDialog(bool accept,
 }
 
 void ChromeImpl::OnWebViewClose(WebView* web_view) {
-  web_view_map_.erase(web_view->GetId());
+  for (WebViewList::iterator it = web_views_.begin();
+       it != web_views_.end(); ++it) {
+    if ((*it)->GetId() == web_view->GetId()) {
+      web_views_.erase(it);
+      return;
+    }
+  }
 }
 
 Status ChromeImpl::Init() {
@@ -317,15 +353,19 @@ int ChromeImpl::GetPort() const {
 
 Status ChromeImpl::GetDialogManagerForOpenDialog(
     JavaScriptDialogManager** manager) {
-  std::list<WebView*> web_views;
-  Status status = GetWebViews(&web_views);
+  std::list<std::string> web_view_ids;
+  Status status = GetWebViewIds(&web_view_ids);
   if (status.IsError())
     return status;
 
-  for (std::list<WebView*>::const_iterator it = web_views.begin();
-       it != web_views.end(); ++it) {
-    if ((*it)->GetJavaScriptDialogManager()->IsDialogOpen()) {
-      *manager = (*it)->GetJavaScriptDialogManager();
+  for (std::list<std::string>::const_iterator it = web_view_ids.begin();
+       it != web_view_ids.end(); ++it) {
+    WebView* web_view;
+    status = GetWebViewById(*it, &web_view);
+    if (status.IsError())
+      return status;
+    if (web_view->GetJavaScriptDialogManager()->IsDialogOpen()) {
+      *manager = web_view->GetJavaScriptDialogManager();
       return Status(kOk);
     }
   }

chrome/test/chromedriver/chrome/chrome_impl.h

@@ -6,7 +6,6 @@
 #define CHROME_TEST_CHROMEDRIVER_CHROME_CHROME_IMPL_H_
 
 #include <list>
-#include <map>
 #include <string>
 
 #include "base/compiler_specific.h"
@@ -31,7 +30,9 @@ class ChromeImpl : public Chrome, public WebViewDelegate {
 
   // Overridden from Chrome:
   virtual std::string GetVersion() OVERRIDE;
-  virtual Status GetWebViews(std::list<WebView*>* web_views) OVERRIDE;
+  virtual Status GetWebViewIds(std::list<std::string>* web_view_ids) OVERRIDE;
+  virtual Status GetWebViewById(const std::string& id,
+                                WebView** web_view) OVERRIDE;
   virtual Status IsJavaScriptDialogOpen(bool* is_open) OVERRIDE;
   virtual Status GetJavaScriptDialogMessage(std::string* message) OVERRIDE;
   virtual Status HandleJavaScriptDialog(
@@ -46,7 +47,7 @@ class ChromeImpl : public Chrome, public WebViewDelegate {
   int GetPort() const;
 
  private:
-  typedef std::map<std::string, linked_ptr<WebViewImpl> > WebViewMap;
+  typedef std::list<linked_ptr<WebViewImpl> > WebViewList;
 
   Status GetDialogManagerForOpenDialog(JavaScriptDialogManager** manager);
   Status ParseAndCheckVersion(const std::string& version);
@@ -56,7 +57,8 @@ class ChromeImpl : public Chrome, public WebViewDelegate {
   SyncWebSocketFactory socket_factory_;
   std::string version_;
   int build_no_;
-  WebViewMap web_view_map_;
+  // Web views in this list are in the same order as they are opened.
+  WebViewList web_views_;
 };
 
 namespace internal {

chrome/test/chromedriver/chrome/stub_chrome.cc

@@ -14,7 +14,11 @@ std::string StubChrome::GetVersion() {
   return "";
 }
 
-Status StubChrome::GetWebViews(std::list<WebView*>* web_views) {
+Status StubChrome::GetWebViewIds(std::list<std::string>* web_view_ids) {
+  return Status(kOk);
+}
+
+Status StubChrome::GetWebViewById(const std::string& id, WebView** web_view) {
   return Status(kOk);
 }
 

chrome/test/chromedriver/chrome/stub_chrome.h

@@ -20,7 +20,9 @@ class StubChrome : public Chrome {
 
   // Overridden from Chrome:
   virtual std::string GetVersion() OVERRIDE;
-  virtual Status GetWebViews(std::list<WebView*>* web_views) OVERRIDE;
+  virtual Status GetWebViewIds(std::list<std::string>* web_view_ids) OVERRIDE;
+  virtual Status GetWebViewById(const std::string& id,
+                                WebView** web_view) OVERRIDE;
   virtual Status IsJavaScriptDialogOpen(bool* is_open) OVERRIDE;
   virtual Status GetJavaScriptDialogMessage(std::string* message) OVERRIDE;
   virtual Status HandleJavaScriptDialog(

chrome/test/chromedriver/commands.cc

@@ -118,14 +118,13 @@ Status ExecuteNewSession(
   if (status.IsError())
     return Status(kSessionNotCreatedException, status.message());
 
-  std::list<WebView*> web_views;
-  status = chrome->GetWebViews(&web_views);
-  if (status.IsError() || web_views.empty()) {
+  std::list<std::string> web_view_ids;
+  status = chrome->GetWebViewIds(&web_view_ids);
+  if (status.IsError() || web_view_ids.empty()) {
     chrome->Quit();
     return status.IsError() ? status :
         Status(kUnknownError, "unable to discover open window in chrome");
   }
-  WebView* default_web_view = web_views.front();
 
   std::string new_id = session_id;
   if (new_id.empty())
@@ -136,7 +135,7 @@ Status ExecuteNewSession(
     return Status(kUnknownError,
                   "failed to start a thread for the new session");
   }
-  session->window = default_web_view->GetId();
+  session->window = web_view_ids.front();
   out_value->reset(session->capabilities->DeepCopy());
   *out_session_id = new_id;
 

chrome/test/chromedriver/run_py_tests.py

@@ -154,11 +154,9 @@ class ChromeDriverTest(ChromeDriverBaseTest):
     while time.time() < timeout:
       new_handles = self._driver.GetWindowHandles()
       if len(new_handles) > len(old_handles):
-        for old_handle in old_handles:
-          self.assertTrue(old_handle in new_handles)
-          new_handles.remove(old_handle)
-        self.assertTrue(len(new_handles))
-        return new_handles[0]
+        for index, old_handle in enumerate(old_handles):
+          self.assertEquals(old_handle, new_handles[index])
+        return new_handles[len(old_handles)]
       time.sleep(0.01)
     return None
 

chrome/test/chromedriver/session.cc

@@ -45,19 +45,10 @@ Status Session::GetTargetWindow(WebView** web_view) {
   if (!chrome)
     return Status(kNoSuchWindow, "no chrome started in this session");
 
-  std::list<WebView*> web_views;
-  Status status = chrome->GetWebViews(&web_views);
+  Status status = chrome->GetWebViewById(window, web_view);
   if (status.IsError())
-    return status;
-
-  for (std::list<WebView*>::const_iterator it = web_views.begin();
-       it != web_views.end(); ++it) {
-    if ((*it)->GetId() == window) {
-      *web_view = *it;
-      return Status(kOk);
-    }
-  }
-  return Status(kNoSuchWindow, "target window already closed");
+    status = Status(kNoSuchWindow, "target window already closed", status);
+  return status;
 }
 
 void Session::SwitchToTopFrame() {

chrome/test/chromedriver/session_commands.cc

@@ -119,12 +119,12 @@ Status ExecuteClose(
     Session* session,
     const base::DictionaryValue& params,
     scoped_ptr<base::Value>* value) {
-  std::list<WebView*> web_views;
-  Status status = session->chrome->GetWebViews(&web_views);
+  std::list<std::string> web_view_ids;
+  Status status = session->chrome->GetWebViewIds(&web_view_ids);
   if (status.IsError())
     return status;
-  bool is_last_web_view = web_views.size() == 1u;
-  web_views.clear();
+  bool is_last_web_view = web_view_ids.size() == 1u;
+  web_view_ids.clear();
 
   WebView* web_view = NULL;
   status = session->GetTargetWindow(&web_view);
@@ -135,9 +135,9 @@ Status ExecuteClose(
   if (status.IsError())
     return status;
 
-  status = session->chrome->GetWebViews(&web_views);
+  status = session->chrome->GetWebViewIds(&web_view_ids);
   if ((status.code() == kChromeNotReachable && is_last_web_view) ||
-      (status.IsOk() && web_views.empty())) {
+      (status.IsOk() && web_view_ids.empty())) {
     CHECK(session_map->Remove(session->id));
     return Status(kOk);
   }
@@ -148,16 +148,16 @@ Status ExecuteGetWindowHandles(
     Session* session,
     const base::DictionaryValue& params,
     scoped_ptr<base::Value>* value) {
-  std::list<WebView*> web_views;
-  Status status = session->chrome->GetWebViews(&web_views);
+  std::list<std::string> web_view_ids;
+  Status status = session->chrome->GetWebViewIds(&web_view_ids);
   if (status.IsError())
     return status;
-  base::ListValue window_ids;
-  for (std::list<WebView*>::const_iterator it = web_views.begin();
-       it != web_views.end(); ++it) {
-    window_ids.AppendString(WebViewIdToWindowHandle((*it)->GetId()));
+  scoped_ptr<base::ListValue> window_ids(new base::ListValue());
+  for (std::list<std::string>::const_iterator it = web_view_ids.begin();
+       it != web_view_ids.end(); ++it) {
+    window_ids->AppendString(WebViewIdToWindowHandle(*it));
   }
-  value->reset(window_ids.DeepCopy());
+  value->reset(window_ids.release());
   return Status(kOk);
 }
 
@@ -169,20 +169,19 @@ Status ExecuteSwitchToWindow(
   if (!params.GetString("name", &name) || name.empty())
     return Status(kUnknownError, "'name' must be a nonempty string");
 
-  std::list<WebView*> web_views;
-  Status status = session->chrome->GetWebViews(&web_views);
+  std::list<std::string> web_view_ids;
+  Status status = session->chrome->GetWebViewIds(&web_view_ids);
   if (status.IsError())
     return status;
 
-  WebView* web_view = NULL;
-
   std::string web_view_id;
+  bool found = false;
   if (WindowHandleToWebViewId(name, &web_view_id)) {
     // Check if any web_view matches |web_view_id|.
-    for (std::list<WebView*>::const_iterator it = web_views.begin();
-         it != web_views.end(); ++it) {
-      if ((*it)->GetId() == web_view_id) {
-        web_view = *it;
+    for (std::list<std::string>::const_iterator it = web_view_ids.begin();
+         it != web_view_ids.end(); ++it) {
+      if (*it == web_view_id) {
+        found = true;
         break;
       }
     }
@@ -190,25 +189,33 @@ Status ExecuteSwitchToWindow(
     // Check if any of the tab window names match |name|.
     const char* kGetWindowNameScript = "function() { return window.name; }";
     base::ListValue args;
-    for (std::list<WebView*>::const_iterator it = web_views.begin();
-         it != web_views.end(); ++it) {
+    for (std::list<std::string>::const_iterator it = web_view_ids.begin();
+         it != web_view_ids.end(); ++it) {
       scoped_ptr<base::Value> result;
-      status = (*it)->CallFunction("", kGetWindowNameScript, args, &result);
+      WebView* web_view;
+      status = session->chrome->GetWebViewById(*it, &web_view);
+      if (status.IsError())
+        return status;
+      status = web_view->ConnectIfNecessary();
+      if (status.IsError())
+        return status;
+      status = web_view->CallFunction("", kGetWindowNameScript, args, &result);
       if (status.IsError())
         return status;
       std::string window_name;
       if (!result->GetAsString(&window_name))
         return Status(kUnknownError, "failed to get window name");
       if (window_name == name) {
-        web_view = *it;
+        web_view_id = *it;
+        found = true;
         break;
       }
     }
   }
 
-  if (!web_view)
+  if (!found)
     return Status(kNoSuchWindow);
-  session->window = web_view->GetId();
+  session->window = web_view_id;
   session->SwitchToTopFrame();
   session->mouse_position = WebPoint(0, 0);
   return Status(kOk);

chrome/test/chromedriver/session_unittest.cc

@@ -31,10 +31,13 @@ class MockChrome : public StubChrome {
   MockChrome() : web_view_("1") {}
   virtual ~MockChrome() {}
 
-  virtual Status GetWebViews(std::list<WebView*>* web_views) OVERRIDE {
-    web_views->clear();
-    web_views->push_back(&web_view_);
-    return Status(kOk);
+  virtual Status GetWebViewById(const std::string& id,
+                                WebView** web_view) OVERRIDE {
+    if (id == web_view_.GetId()) {
+      *web_view = &web_view_;
+      return Status(kOk);
+    }
+    return Status(kUnknownError);
   }
 
  private: