Add generic "json dict" entry point for pyauto commands. Will prevent

the need to modify the automation proxy anymore.  New pyauto commands
will only need to edit pyauto.py (to add a new SendJSONCommand() call)
and browser_proxy.cc (to implement the other side).  Contrast with the
normal editing of ~8 files.

Also added WaitForAllDownloadsToComplete using new JSON path.

BUG=http://crbug.com/39274


Review URL: http://codereview.chromium.org/1547012

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

13 files changed, 255 insertions, 4 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index c6ee650..2b88a29 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/file_version_info.h"
 #include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/keyboard_codes.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
@@ -430,6 +431,8 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
                         SetBookmarkURL)
     IPC_MESSAGE_HANDLER(AutomationMsg_RemoveBookmark,
                         RemoveBookmark)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_SendJSONRequest,
+                                    SendJSONRequest)
     IPC_MESSAGE_HANDLER(AutomationMsg_GetInfoBarCount, GetInfoBarCount)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_ClickInfoBarAccept,
                                     ClickInfoBarAccept)
@@ -1500,6 +1503,112 @@ void AutomationProvider::RemoveBookmark(int handle,
   *success = false;
 }
 
+void AutomationProvider::WaitForDownloadsToComplete(
+    DictionaryValue* args,
+    IPC::Message* reply_message) {
+  std::string json_return;
+  bool reply_return = true;
+  AutomationProviderDownloadManagerObserver observer;
+  std::vector<DownloadItem*> downloads;
+
+  // Look for a quick return.
+  if (!profile_->HasCreatedDownloadManager()) {
+    json_return = "{'error': 'no download manager'}";
+    reply_return = false;
+  } else {
+    profile_->GetDownloadManager()->GetCurrentDownloads(&observer,
+                                                        FilePath());
+    downloads = observer.Downloads();
+    if (downloads.size() == 0) {
+      json_return = "{}";
+    }
+  }
+  if (!json_return.empty()) {
+    AutomationMsg_SendJSONRequest::WriteReplyParams(
+        reply_message, json_return, reply_return);
+    Send(reply_message);
+  }
+
+  // The observer owns itself.  When the last observed item pings, it
+  // deletes itself.
+  AutomationProviderDownloadItemObserver* item_observer =
+      new AutomationProviderDownloadItemObserver(
+          this, reply_message, downloads.size());
+  for (std::vector<DownloadItem*>::iterator i = downloads.begin();
+       i != downloads.end();
+       i++) {
+    (*i)->AddObserver(item_observer);
+  }
+}
+
+void AutomationProvider::SendJSONRequest(
+    int handle,
+    std::string json_request,
+    IPC::Message* reply_message) {
+  Browser* browser = NULL;
+  std::string error_string;
+  scoped_ptr<Value> values;
+
+  // Basic error checking.
+  if (browser_tracker_->ContainsHandle(handle)) {
+    browser = browser_tracker_->GetResource(handle);
+  }
+  if (!browser) {
+    error_string = "no browser object";
+  } else {
+    base::JSONReader reader;
+    std::string error;
+    values.reset(reader.ReadAndReturnError(json_request, true, &error));
+    if (!error.empty()) {
+      error_string = error;
+    }
+  }
+
+  // Make sure input is a dict with a string command.
+  std::string command;
+  DictionaryValue* dict_value = NULL;
+  if (error_string.empty()) {
+    if (values->GetType() != Value::TYPE_DICTIONARY) {
+      error_string = "not a dict or no command key in dict";
+    } else {
+      // Ownership remains with "values" variable.
+      dict_value = static_cast<DictionaryValue*>(values.get());
+      if (!dict_value->GetStringASCII(std::string("command"), &command)) {
+        error_string = "no command key in dict or not a string command";
+      }
+    }
+  }
+
+  if (error_string.empty()) {
+    // TODO(jrg): table of calls; async gets passed reply_message,
+    // sync methods gets passed an output json dict which we package
+    // up and send.  Right now we only have one.
+    if (command == "WaitForAllDownloadsToComplete") {
+      this->WaitForDownloadsToComplete(dict_value, reply_message);
+      return;
+    } else {
+      error_string = "unknown command";
+    }
+  }
+
+  // If we hit an error, return info.
+  // Return a dict of {'error', 'descriptive_string_for_error'}.
+  // Else return an empty dict.
+  std::string json_string;
+  bool success = true;
+  if (!error_string.empty()) {
+    scoped_ptr<DictionaryValue> dict(new DictionaryValue);
+    dict->SetString(L"error", error_string);
+    base::JSONWriter::Write(dict.get(), false, &json_string);
+    success = false;
+  } else {
+    json_string = "{}";
+  }
+  AutomationMsg_SendJSONRequest::WriteReplyParams(
+      reply_message, json_string, success);
+  Send(reply_message);
+}
+
 void AutomationProvider::HandleInspectElementRequest(
     int handle, int x, int y, IPC::Message* reply_message) {
   TabContents* tab_contents = GetTabContentsForHandle(handle, NULL);
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index 2d680b1..929bc20 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -17,6 +17,7 @@
 
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
+#include "base/values.h"
 #include "chrome/browser/automation/automation_autocomplete_edit_tracker.h"
 #include "chrome/browser/automation/automation_browser_tracker.h"
 #include "chrome/browser/automation/automation_resource_message_filter.h"
@@ -319,6 +320,16 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
                       int64 id,
                       bool* success);
 
+  // Wait for all downloads to complete.
+  void WaitForDownloadsToComplete(
+      DictionaryValue* args,
+      IPC::Message* reply_message);
+
+  // Generic pattern for pyautolib
+  void SendJSONRequest(int handle,
+                       std::string json_request,
+                       IPC::Message* reply_message);
+
   // Responds to InspectElement request
   void HandleInspectElementRequest(int handle,
                                    int x,
diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index 7e5ac24..da7537d 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -767,3 +767,14 @@ void AutomationProviderBookmarkModelObserver::ReplyAndDelete(bool success) {
   automation_provider_->Send(reply_message_);
   delete this;
 }
+
+void AutomationProviderDownloadItemObserver::OnDownloadFileCompleted(
+    DownloadItem* download) {
+  download->RemoveObserver(this);
+  if (--downloads_ == 0) {
+    AutomationMsg_SendJSONRequest::WriteReplyParams(
+        reply_message_, std::string("{}"), true);
+    provider_->Send(reply_message_);
+    delete this;
+  }
+}
diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h
index 0d75e70..892be51 100644
--- a/chrome/browser/automation/automation_provider_observers.h
+++ b/chrome/browser/automation/automation_provider_observers.h
@@ -9,6 +9,7 @@
 #include <set>
 
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
+#include "chrome/browser/download/download_manager.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_type.h"
@@ -452,4 +453,50 @@ class AutomationProviderBookmarkModelObserver : BookmarkModelObserver {
   DISALLOW_COPY_AND_ASSIGN(AutomationProviderBookmarkModelObserver);
 };
 
+// When asked for pending downloads, the DownloadManager places
+// results in a DownloadManager::Observer.
+class AutomationProviderDownloadManagerObserver :
+    public DownloadManager::Observer {
+ public:
+  AutomationProviderDownloadManagerObserver() : DownloadManager::Observer()  {}
+  virtual ~AutomationProviderDownloadManagerObserver() {}
+  virtual void ModelChanged() {}
+  virtual void SetDownloads(std::vector<DownloadItem*>& downloads) {
+    downloads_ = downloads;
+  }
+  std::vector<DownloadItem*> Downloads() {
+    return downloads_;
+  }
+ private:
+  std::vector<DownloadItem*> downloads_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutomationProviderDownloadManagerObserver);
+};
+
+
+// Allows the automation provider to wait for all downloads to finish.
+class AutomationProviderDownloadItemObserver : public DownloadItem::Observer {
+ public:
+  AutomationProviderDownloadItemObserver(
+      AutomationProvider* provider,
+      IPC::Message* reply_message,
+      int downloads) {
+    provider_ = provider;
+    reply_message_ = reply_message;
+    downloads_ = downloads;
+  }
+  virtual ~AutomationProviderDownloadItemObserver() {}
+
+  virtual void OnDownloadUpdated(DownloadItem* download) { }
+  virtual void OnDownloadFileCompleted(DownloadItem* download);
+  virtual void OnDownloadOpened(DownloadItem* download) { }
+
+ private:
+  AutomationProvider* provider_;
+  IPC::Message* reply_message_;
+  int downloads_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutomationProviderDownloadItemObserver);
+};
+
 #endif  // CHROME_BROWSER_AUTOMATION_AUTOMATION_PROVIDER_OBSERVERS_H_
diff --git a/chrome/test/automation/automation_messages_internal.h b/chrome/test/automation/automation_messages_internal.h
index bb13297..74dabad 100644
--- a/chrome/test/automation/automation_messages_internal.h
+++ b/chrome/test/automation/automation_messages_internal.h
@@ -1313,4 +1313,12 @@ IPC_BEGIN_MESSAGES(Automation)
   IPC_SYNC_MESSAGE_ROUTED0_1(AutomationMsg_WaitForPopupMenuToOpen,
                              bool /* success */)
 
+  // Generic pyauto pattern to help avoid future addition of
+  // automation messages.
+  IPC_SYNC_MESSAGE_ROUTED2_2(AutomationMsg_SendJSONRequest,
+                             int /* browser_handle */,
+                             std::string /* JSON request */,
+                             std::string /* JSON response */,
+                             bool /* success */)
+
 IPC_END_MESSAGES(Automation)
diff --git a/chrome/test/automation/browser_proxy.cc b/chrome/test/automation/browser_proxy.cc
index f0825a0..1b275d8 100644
--- a/chrome/test/automation/browser_proxy.cc
+++ b/chrome/test/automation/browser_proxy.cc
@@ -582,3 +582,15 @@ bool BrowserProxy::WaitForPopupMenuToOpen() {
     return false;
   return result;
 }
+
+bool BrowserProxy::SendJSONRequest(const std::string& request,
+                                   std::string* response) {
+  if (!is_valid())
+    return false;
+
+  bool result = false;
+  return sender_->Send(new AutomationMsg_SendJSONRequest(0, handle_,
+                                                         request, response,
+                                                         &result));
+  return result;
+}
diff --git a/chrome/test/automation/browser_proxy.h b/chrome/test/automation/browser_proxy.h
index 509a630..d0bb4d0 100644
--- a/chrome/test/automation/browser_proxy.h
+++ b/chrome/test/automation/browser_proxy.h
@@ -226,6 +226,10 @@ class BrowserProxy : public AutomationResourceProxy {
   bool StartTrackingPopupMenus() WARN_UNUSED_RESULT;
   bool WaitForPopupMenuToOpen() WARN_UNUSED_RESULT;
 
+  // Experimental generic pattern.
+  bool SendJSONRequest(const std::string& request,
+                       std::string* response) WARN_UNUSED_RESULT;
+
  protected:
   virtual ~BrowserProxy() {}
  private:
diff --git a/chrome/test/functional/PYAUTO_TESTS b/chrome/test/functional/PYAUTO_TESTS
index 69c83a1..124b846 100644
--- a/chrome/test/functional/PYAUTO_TESTS
+++ b/chrome/test/functional/PYAUTO_TESTS
@@ -21,6 +21,7 @@
     'bookmark_bar',
     'bookmarks',
     'test_basic.SimpleTest.testCanOpenGoogle',
+    'downloads'
   ],
 
   'win': [
diff --git a/chrome/test/functional/downloads.py b/chrome/test/functional/downloads.py
index d2bf5be..4be9128 100644
--- a/chrome/test/functional/downloads.py
+++ b/chrome/test/functional/downloads.py
@@ -22,6 +22,10 @@ class DownloadsTest(pyauto.PyUITest):
     md5.update(open(filename, 'rb').read())
     return md5.hexdigest()
 
+  def testNoDownloadWaitingNeeded(self):
+    """Make sure "wait for downloads" returns quickly if we have none."""
+    self.WaitForAllDownloadsToComplete()
+
   def testZip(self):
     """Download a zip and verify that it downloaded correctly.
        Also verify that the download shelf showed up.
@@ -38,17 +42,28 @@ class DownloadsTest(pyauto.PyUITest):
     # Download
     self.NavigateToURL(file_url)
 
-    # TODO: Remove this sleep. Wait until all downloads finish.
-    time.sleep(4)
+    # Wait for the download to finish
+    start = time.time()
+    self.WaitForAllDownloadsToComplete()
+    end = time.time()
+    print 'Wait for downloads to complete: %2.2fsec' % (end - start)
 
     # Verify that the download shelf is visible
     self.assertTrue(self.IsDownloadShelfVisible())
 
     # Verify that the file was correctly downloaded
     self.assertTrue(os.path.exists(downloaded_pkg))
+    # print 'Download size is %d' % os.path.getsize(downloaded_pkg)
     self.assertEqual(golden_md5sum, self._ComputeMD5sum(downloaded_pkg))
 
+  def testBigZip(self):
+    # TODO: download something "pretty big".  The above test will
+    # usually wortk even without the WaitForAllDownloadsToComplete().
+    # Manual testing shows it isn't just a noop, but an explicit test
+    # is needed here.  However, if the download is TOO big, we hit a
+    # 30sec timeout in our automation proxy / IPC (didn't track down
+    # where exactly).
+    pass
 
 if __name__ == '__main__':
   pyauto_functional.Main()
-
diff --git a/chrome/test/pyautolib/pyauto.py b/chrome/test/pyautolib/pyauto.py
index 4d45d65..fe5f9ab 100644
--- a/chrome/test/pyautolib/pyauto.py
+++ b/chrome/test/pyautolib/pyauto.py
@@ -69,6 +69,7 @@ except ImportError:
   raise
 
 # Should go after sys.path is set appropriately
+import simplejson as json  # found in third_party
 import bookmark_model
 
 
@@ -143,6 +144,13 @@ class PyUITest(pyautolib.PyUITestBase, unittest.TestCase):
     """
     return bookmark_model.BookmarkModel(self._GetBookmarksAsJSON())
 
+  def WaitForAllDownloadsToComplete(self):
+    """Wait for all downloads to complete."""
+    # Implementation detail: uses the generic "JSON command" model
+    # (experimental)
+    self._SendJSONRequest(0, json.dumps({'command':
+                                         'WaitForAllDownloadsToComplete'}))
+
 
 class PyUITestSuite(pyautolib.PyUITestSuiteBase, unittest.TestSuite):
   """Base TestSuite for PyAuto UI tests."""
@@ -310,4 +318,3 @@ class Main(object):
 
 if __name__ == '__main__':
   Main()
-
diff --git a/chrome/test/pyautolib/pyautolib.cc b/chrome/test/pyautolib/pyautolib.cc
index 4afc72e..84bdd18 100644
--- a/chrome/test/pyautolib/pyautolib.cc
+++ b/chrome/test/pyautolib/pyautolib.cc
@@ -274,3 +274,16 @@ scoped_refptr<BrowserProxy> PyUITestBase::GetBrowserWindow(int window_index) {
   return automation()->GetBrowserWindow(window_index);
 }
 
+
+std::string PyUITestBase::_SendJSONRequest(int window_index,
+                                            std::string& request) {
+  scoped_refptr<BrowserProxy> browser_proxy =
+      automation()->GetBrowserWindow(window_index);
+  EXPECT_TRUE(browser_proxy.get());
+  std::string response;
+  if (browser_proxy.get()) {
+    EXPECT_TRUE(browser_proxy->SendJSONRequest(request, &response));
+  }
+  return response;
+}
+
diff --git a/chrome/test/pyautolib/pyautolib.h b/chrome/test/pyautolib/pyautolib.h
index 1ec6b57..1093fda 100644
--- a/chrome/test/pyautolib/pyautolib.h
+++ b/chrome/test/pyautolib/pyautolib.h
@@ -139,6 +139,11 @@ class PyUITestBase : public UITestBase {
   // Get a handle to browser window at the given index, or NULL on failure.
   scoped_refptr<BrowserProxy> GetBrowserWindow(int window_index);
 
+  // Meta-method.  Experimental pattern of passing args and response as
+  // JSON dict to avoid future use of the SWIG interface and
+  // automation proxy additions.  Returns response as JSON dict.
+  std::string _SendJSONRequest(int window_index, std::string& request);
+
  private:
   // Enables PostTask to main thread.
   // Should be shared across multiple instances of PyUITestBase so that this
diff --git a/chrome/test/pyautolib/pyautolib.i b/chrome/test/pyautolib/pyautolib.i
index ed9de32..407aafc 100644
--- a/chrome/test/pyautolib/pyautolib.i
+++ b/chrome/test/pyautolib/pyautolib.i
@@ -309,5 +309,13 @@ class PyUITestBase {
   %feature("docstring", "Get a proxy to the browser window at the given "
                         "zero-based index.") GetBrowserWindow;
   scoped_refptr<BrowserProxy> GetBrowserWindow(int window_index);
+
+  // Meta-method
+  %feature("docstring", "Send a sync JSON request to Chrome.  "
+                        "Returns a JSON dict as a response.  "
+			"Internal method.")
+      _SendJSONRequest;
+  std::string _SendJSONRequest(int window_index, std::string request);
+
 };