Modified DomAutomationController to expose a new Javascript method sendWithId() and updated the event queue tests to use it.

All DomAutomationController messages need to be coupled with an automation
routing id. This was previously done by setting the id through
a separate javascript functional call. However, events introduce a race
condition where an event raised between the call to set the automation id
and actually sending the message would leave the DomAutomationController
with a cleared Automation Id and the send() message would fail. A new
method sendWithId() allows events to send a message with an automation id
in an atomic fashion, without disrupting the automation id held by the
DomAutomationController.

Change-Id: Ia6be9108ce881ee1a063fc6ff1844587bf122f50

BUG=
TEST=functional/apptest.py

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

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

8 files changed, 138 insertions, 53 deletions

diff --git a/chrome/browser/automation/automation_event_observer.cc b/chrome/browser/automation/automation_event_observer.cc
index feadd29..4ebe32a 100644
--- a/chrome/browser/automation/automation_event_observer.cc
+++ b/chrome/browser/automation/automation_event_observer.cc
@@ -7,12 +7,16 @@
 #include "chrome/browser/automation/automation_event_queue.h"
 #include "chrome/browser/automation/automation_provider.h"
 #include "chrome/browser/automation/automation_provider_json.h"
+#include "content/public/browser/dom_operation_notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 
 AutomationEventObserver::AutomationEventObserver(
-    AutomationEventQueue* event_queue)
-    : event_queue_(event_queue), observer_id_(-1) {
+    AutomationEventQueue* event_queue, bool recurring)
+    : event_queue_(event_queue),
+      recurring_(recurring),
+      observer_id_(-1),
+      event_count_(0) {
   DCHECK(event_queue_ != NULL);
 }
 
@@ -23,6 +27,7 @@ void AutomationEventObserver::NotifyEvent(DictionaryValue* value) {
     event_queue_->NotifyEvent(
         new AutomationEventQueue::AutomationEvent(
             GetId(), value));
+    event_count_++;
   }
 }
 
@@ -36,28 +41,44 @@ int AutomationEventObserver::GetId() const {
   return observer_id_;
 }
 
-DomRaisedEventObserver::DomRaisedEventObserver(
-    AutomationEventQueue* event_queue, const std::string& event_name)
-    : AutomationEventObserver(event_queue), event_name_(event_name) {}
-
-DomRaisedEventObserver::~DomRaisedEventObserver() {}
-
-void DomRaisedEventObserver::OnDomOperationCompleted(const std::string& json) {
-  DictionaryValue* dict = new DictionaryValue;
-  dict->SetString("type", "raised");
-  dict->SetString("name", json);
-  dict->SetInteger("observer_id", GetId());
-  NotifyEvent(dict);
+void AutomationEventObserver::RemoveIfDone() {
+  if (!recurring_ && event_count_ > 0 && event_queue_) {
+    event_queue_->RemoveObserver(GetId());
+  }
 }
 
-void DomRaisedEventObserver::OnModalDialogShown() {
-  DictionaryValue* dict = new DictionaryValue;
-  dict->SetString("error", "Blocked by modal dialogue");
-  NotifyEvent(dict);
+DomRaisedEventObserver::DomRaisedEventObserver(
+    AutomationEventQueue* event_queue,
+    const std::string& event_name,
+    int automation_id,
+    bool recurring)
+    : AutomationEventObserver(event_queue, recurring),
+      event_name_(event_name),
+      automation_id_(automation_id) {
+  registrar_.Add(this, content::NOTIFICATION_DOM_OPERATION_RESPONSE,
+                 content::NotificationService::AllSources());
 }
 
-void DomRaisedEventObserver::OnJavascriptBlocked() {
-  DictionaryValue* dict = new DictionaryValue;
-  dict->SetString("error", "Javascript execution was blocked");
-  NotifyEvent(dict);
+DomRaisedEventObserver::~DomRaisedEventObserver() {}
+
+void DomRaisedEventObserver::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  if (type == content::NOTIFICATION_DOM_OPERATION_RESPONSE) {
+    content::Details<content::DomOperationNotificationDetails> dom_op_details(
+        details);
+    if ((dom_op_details->automation_id == automation_id_ ||
+         automation_id_ == -1) &&
+        (event_name_.length() == 0 ||
+         event_name_.compare(dom_op_details->json) == 0)) {
+      DictionaryValue* dict = new DictionaryValue;
+      dict->SetString("type", "raised_event");
+      dict->SetString("name", dom_op_details->json);
+      dict->SetInteger("observer_id", GetId());
+      NotifyEvent(dict);
+    }
+  }
+  // Nothing should happen after RemoveIfDone() as it may delete the object.
+  RemoveIfDone();
 }
diff --git a/chrome/browser/automation/automation_event_observer.h b/chrome/browser/automation/automation_event_observer.h
index 3915648..3d02231 100644
--- a/chrome/browser/automation/automation_event_observer.h
+++ b/chrome/browser/automation/automation_event_observer.h
@@ -16,38 +16,47 @@
 // AutomationEvent into the AutomationEventQueue for each occurance.
 class AutomationEventObserver {
  public:
-  explicit AutomationEventObserver(AutomationEventQueue* event_queue);
+  explicit AutomationEventObserver(AutomationEventQueue* event_queue,
+                                   bool recurring);
   virtual ~AutomationEventObserver();
 
   void Init(int observer_id);
   void NotifyEvent(DictionaryValue* value);
   int GetId() const;
 
+ protected:
+  void RemoveIfDone();  // This may delete the object.
+
  private:
   AutomationEventQueue* event_queue_;
+  bool recurring_;
   int observer_id_;
+  // TODO(craigdh): Add a PyAuto hook to retrieve the number of times an event
+  // has occurred.
+  int event_count_;
 
   DISALLOW_COPY_AND_ASSIGN(AutomationEventObserver);
 };
 
 // AutomationEventObserver implementation that listens for explicitly raised
-// events. A webpage currently raises events by calling:
-// window.domAutomationController.setAutomationId(42); // Any integer works.
-// window.domAutomationController("EVENT_NAME");
-// TODO(craigdh): This method is a temporary hack.
+// events. A webpage explicitly raises events by calling:
+// window.domAutomationController.raiseEvent("EVENT_NAME");
 class DomRaisedEventObserver
-    : public AutomationEventObserver, public DomOperationObserver {
+    : public AutomationEventObserver, public content::NotificationObserver {
  public:
   DomRaisedEventObserver(AutomationEventQueue* event_queue,
-                         const std::string& event_name);
+                         const std::string& event_name,
+                         int automation_id,
+                         bool recurring);
   virtual ~DomRaisedEventObserver();
 
-  virtual void OnDomOperationCompleted(const std::string& json) OVERRIDE;
-  virtual void OnModalDialogShown() OVERRIDE;
-  virtual void OnJavascriptBlocked() OVERRIDE;
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
 
  private:
   std::string event_name_;
+  int automation_id_;
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(DomRaisedEventObserver);
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
index 38d8121..38ad540 100644
--- a/chrome/browser/automation/testing_automation_provider.cc
+++ b/chrome/browser/automation/testing_automation_provider.cc
@@ -6465,16 +6465,29 @@ void TestingAutomationProvider::AddDomRaisedEventObserver(
 
   AutomationJSONReply reply(this, reply_message);
   std::string event_name;
+  int automation_id;
+  bool recurring;
   if (!args->GetString("event_name", &event_name)) {
     reply.SendError("'event_name' missing or invalid");
     return;
   }
+  if (!args->GetInteger("automation_id", &automation_id)) {
+    reply.SendError("'automation_id' missing or invalid");
+    return;
+  }
+  if (!args->GetBoolean("recurring", &recurring)) {
+    reply.SendError("'recurring' missing or invalid");
+    return;
+  }
 
   if (!automation_event_queue_.get())
     automation_event_queue_.reset(new AutomationEventQueue);
 
   int observer_id = automation_event_queue_->AddObserver(
-      new DomRaisedEventObserver(automation_event_queue_.get(), event_name));
+      new DomRaisedEventObserver(automation_event_queue_.get(),
+                                 event_name,
+                                 automation_id,
+                                 recurring));
   scoped_ptr<DictionaryValue> return_value(new DictionaryValue);
   return_value->SetInteger("observer_id", observer_id);
   reply.SendSuccess(return_value.get());
@@ -6521,7 +6534,7 @@ void TestingAutomationProvider::GetNextEvent(
   }
   if (!automation_event_queue_.get()) {
     reply->SendError(
-        "No observers are attached to the queue. Did you forget to add one?");
+        "No observers are attached to the queue. Did you create any?");
     return;
   }
 
diff --git a/chrome/test/data/apptest/basic.html b/chrome/test/data/apptest/basic.html
index 2bce174..606cc95 100644
--- a/chrome/test/data/apptest/basic.html
+++ b/chrome/test/data/apptest/basic.html
@@ -23,8 +23,7 @@
       /* Adds an event with the given name to the AutomationEventQueue. */
       function raiseEvent(str) {
         if (window.domAutomationController) {
-          window.domAutomationController.setAutomationId(424242)
-          window.domAutomationController.send(str);
+          window.domAutomationController.sendWithId(4444, str);
         }
       }
 
@@ -49,6 +48,7 @@
       function loginSuccess() {
         write("Login succeeded!");
         raiseEvent("login done");
+        raiseEvent("test success");
       }
 
       function fail() {
diff --git a/chrome/test/functional/apptest.py b/chrome/test/functional/apptest.py
index fb351c0..e4fd940 100644
--- a/chrome/test/functional/apptest.py
+++ b/chrome/test/functional/apptest.py
@@ -2,36 +2,33 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import json
-
 import pyauto_functional  # must be imported before pyauto
 import pyauto
 
 
 class PyAutoEventsTest(pyauto.PyUITest):
+  """Tests using the event queue."""
 
   def testBasicEvents(self):
     """Basic test for the event queue."""
     url = self.GetHttpURLForDataPath('apptest', 'basic.html')
     driver = self.NewWebDriver()
-    event_id = self.AddDomRaisedEventObserver();
+    event_id = self.AddDomRaisedEventObserver(automation_id=4444);
+    success_id = self.AddDomRaisedEventObserver('test success',
+                                                automation_id=4444);
     self.NavigateToURL(url)
     self._ExpectEvent(event_id, 'init')
     self._ExpectEvent(event_id, 'login ready')
     driver.find_element_by_id('login').click()
     self._ExpectEvent(event_id, 'login start')
     self._ExpectEvent(event_id, 'login done')
+    self.GetNextEvent(success_id)
 
   def _ExpectEvent(self, event_id, event_name):
-    # TODO(craigdh): Temporary hack to ignore unexpected events generated by
-    # chromedriver's use of DomAutomationController. The upcoming revision to
-    # RaisedEvents will fix this. Note this isn't polling, just ignoring
-    # chromedriver events.
-    while True:
-      event = json.loads(self.GetNextEvent(event_id).get('name'))
-      if event.find('{') == -1:
-        break
-    self.assertEqual(event, event_name)
+    """Checks that the next event is expected."""
+    e = self.GetNextEvent(event_id)
+    self.assertEqual(e.get('name'), event_name,
+                     msg="unexpected event: %s" % e)
 
 
 if __name__ == '__main__':
diff --git a/chrome/test/pyautolib/pyauto.py b/chrome/test/pyautolib/pyauto.py
index fb95557..d145a77 100755
--- a/chrome/test/pyautolib/pyauto.py
+++ b/chrome/test/pyautolib/pyauto.py
@@ -2831,12 +2831,24 @@ class PyUITest(pyautolib.PyUITestBase, unittest.TestCase):
     return self._GetResultFromJSONRequest(cmd_dict, windex=windex,
                                           timeout=timeout)
 
-  def AddDomRaisedEventObserver(self, event_name=''):
+  def AddDomRaisedEventObserver(self, event_name='', automation_id=-1,
+                                recurring=True):
     """Adds a DomRaisedEventObserver associated with the AutomationEventQueue.
 
+    An app raises a matching event in Javascript by calling:
+    window.domAutomationController.sendWithId(automation_id, event_name)
+
     Args:
-      event_name: The raised event name to watch for. By default all raised
-                  events are observed.
+      event_name: The event name to watch for. By default an event is raised
+                  for every message.
+      automation_id: The Automation Id of the sent message. By default all
+                     messages sent from the window.domAutomationController are
+                     observed. Note that other PyAuto functions also send
+                     messages through window.domAutomationController with
+                     arbirary Automation Ids and they will be observed.
+      recurring: If False the observer will be removed after it generates one
+                 event, otherwise it will continue observing and generating
+                 events until explicity removed with RemoveEventObserver(id).
 
     Returns:
       The id of the created observer, which can be used with GetNextEvent(id)
@@ -2845,11 +2857,11 @@ class PyUITest(pyautolib.PyUITestBase, unittest.TestCase):
     Raises:
       pyauto_errors.JSONInterfaceError if the automation call returns an error.
     """
-    # TODO(craigdh): Add documentation for raising an event once it has been
-    #                implemented.
     cmd_dict = {
       'command': 'AddDomRaisedEventObserver',
       'event_name': event_name,
+      'automation_id': automation_id,
+      'recurring': recurring,
     }
     return self._GetResultFromJSONRequest(cmd_dict, windex=None)['observer_id']
 
diff --git a/content/renderer/dom_automation_controller.cc b/content/renderer/dom_automation_controller.cc
index 0955a6a..85e2fcd 100644
--- a/content/renderer/dom_automation_controller.cc
+++ b/content/renderer/dom_automation_controller.cc
@@ -21,6 +21,8 @@ DomAutomationController::DomAutomationController()
                           base::Unretained(this)));
   BindCallback("sendJSON", base::Bind(&DomAutomationController::SendJSON,
                                       base::Unretained(this)));
+  BindCallback("sendWithId", base::Bind(&DomAutomationController::SendWithId,
+                                        base::Unretained(this)));
 }
 
 void DomAutomationController::Send(const CppArgumentList& args,
@@ -121,6 +123,29 @@ void DomAutomationController::SendJSON(const CppArgumentList& args,
   automation_id_ = MSG_ROUTING_NONE;
 }
 
+void DomAutomationController::SendWithId(const CppArgumentList& args,
+                                         CppVariant* result) {
+  if (args.size() != 2) {
+    result->SetNull();
+    return;
+  }
+
+  if (!sender_) {
+    NOTREACHED();
+    result->SetNull();
+    return;
+  }
+
+  if (!args[0].isNumber() || args[1].type != NPVariantType_String) {
+    result->SetNull();
+    return;
+  }
+
+  result->Set(sender_->Send(
+      new ViewHostMsg_DomOperationResponse(routing_id_, args[1].ToString(),
+                                           args[0].ToInt32())));
+}
+
 void DomAutomationController::SetAutomationId(
     const CppArgumentList& args, CppVariant* result) {
   if (args.size() != 1) {
diff --git a/content/renderer/dom_automation_controller.h b/content/renderer/dom_automation_controller.h
index 576e09d..f1b06f4 100644
--- a/content/renderer/dom_automation_controller.h
+++ b/content/renderer/dom_automation_controller.h
@@ -90,6 +90,14 @@ class DomAutomationController : public CppBoundClass {
   // This function does not modify/escape the returned string in any way.
   void SendJSON(const CppArgumentList& args, CppVariant* result);
 
+  // Sends a string with a provided Automation Id.
+  // Expects two javascript values; the first must be a number type and will be
+  // used as the Automation Id, the second must be of type NPString.
+  // The function returns true/false to the javascript based on the success
+  // of the send over IPC. It sets the javascript return value to null on
+  // unexpected errors or arguments.
+  void SendWithId(const CppArgumentList& args, CppVariant* result);
+
   void SetAutomationId(const CppArgumentList& args, CppVariant* result);
 
   // TODO(vibhor): Implement later