Implementation of AutomationEventQueue and associated framework to support generic non-blocking aut…

chrome/browser/automation/automation_event_queue.cc

Index: chrome/browser/automation/automation_event_queue.cc
diff --git a/chrome/browser/automation/automation_event_queue.cc b/chrome/browser/automation/automation_event_queue.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8cb07df9718b61d8a33f811a29c53902a38c819d
--- /dev/null
+++ b/chrome/browser/automation/automation_event_queue.cc
@@ -0,0 +1,123 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/automation/automation_event_observer.h"
+#include "chrome/browser/automation/automation_event_queue.h"
+
+#include "chrome/browser/automation/automation_provider_json.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+
+AutomationEventQueue::AutomationEventQueue()
+    : observer_id_count_(0),
+      wait_automation_reply_(NULL),
+      wait_observer_id_(-1) {}
+
+AutomationEventQueue::~AutomationEventQueue() {
+  Clear();
+}
+
+AutomationEventQueue::AutomationEvent::AutomationEvent(
+    int observer_id, DictionaryValue* event_value)
+    : event_value_(event_value), observer_id_(observer_id) {}

[frankf, 2012/02/29 02:29:00]

order of member value initialization should match function signature.

[craigdh, 2012/02/29 22:53:43]

Whoops. Done.

+
+void AutomationEventQueue::GetNextEvent(AutomationJSONReply* reply,

[frankf, 2012/02/29 02:29:00]

You're making a copy of reply, why can't you make it const?

[craigdh, 2012/02/29 22:53:43]

I'm not making a copy, a shared_ptr is taking ownership of this pointer.

+                                        int observer_id,
+                                        bool blocking) {
+  wait_automation_reply_.reset(reply);
+  wait_observer_id_ = observer_id;
+  if (!CheckReturnEvent() && !blocking && wait_automation_reply_.get()) {
+    wait_automation_reply_->SendSuccess(NULL);
+    wait_automation_reply_.reset();
+  }
+}
+
+void AutomationEventQueue::Clear() {
+  ClearObservers();
+  ClearEvents();
+}
+
+bool AutomationEventQueue::IsEmpty() {
+  return event_queue_.empty();
+}
+
+AutomationEventQueue::AutomationEvent* AutomationEventQueue::PopEvent() {
+  if (event_queue_.empty()) {
+    return NULL;
+  }
+  AutomationEvent* event = event_queue_.back();
+  event_queue_.pop_back();
+  return event;
+}
+
+AutomationEventQueue::AutomationEvent* AutomationEventQueue::PopEvent(
+    int observer_id) {
+  AutomationEvent* event = NULL;
+  std::list<AutomationEvent*>::reverse_iterator it;
+  for (it = event_queue_.rbegin(); it != event_queue_.rend(); it++) {
+    if (observer_id < 0 || (*it)->GetId() == observer_id) {
+      event = *it;
+      break;

[frankf, 2012/02/29 02:29:00]

you could use find_if here.

[Nirnimesh, 2012/02/29 03:22:08]

You could directly call .erase(it) here, instead of the .remove() below

[craigdh, 2012/02/29 22:53:43]

Actually, erase() won't take a reverse_iterator and converting between them is
messy (I think it would be something like event_queue_.erase(--(it.base())).

[craigdh, 2012/02/29 22:53:43]

I'd have to write a predicate class to check the id (insert long in-person
discussion). Done.

+    }
+  }
+  if (event) {

[Nirnimesh, 2012/02/29 03:22:08]

event wil already be NULL if this if is not fired, so the return inside this if
block is not necessary.

if (event)
  event_queue.remove(event);

return event;

+    event_queue_.remove(event);
+    return event;
+  }
+  return NULL;
+}
+
+void AutomationEventQueue::NotifyEvent(
+    AutomationEventQueue::AutomationEvent* event) {
+  event_queue_.push_front(event);
+  CheckReturnEvent();
+}
+
+int AutomationEventQueue::AddObserver(AutomationEventObserver* observer) {
+  int id = observer_id_count_++;
+  observer->Init(id);
+  observers_[id] = observer;
+  return id;
+}
+
+bool AutomationEventQueue::RemoveObserver(int observer_id) {
+  if (observers_.count(observer_id)) {

[Nirnimesh, 2012/02/29 03:22:08]

You don't really need the count.
Use observers.find(observer_id) != observers_.end()

[craigdh, 2012/02/29 22:53:43]

observers_ is a map so count just returns whether or not the key exists, but if
you think it's clearer I'll switch to find.

+    delete observers_[observer_id];
+    observers_.erase(observer_id);
+    return true;
+  }
+  return false;
+}
+
+void AutomationEventQueue::ClearObservers() {
+  std::map<int, AutomationEventObserver*>::iterator it;
+  for (it = observers_.begin(); it != observers_.end(); it++) {
+    delete it->second;
+  }
+  observers_.clear();
+}
+
+void AutomationEventQueue::ClearEvents() {
+  std::list<AutomationEvent*>::iterator i;

[Nirnimesh, 2012/02/29 03:22:08]

s/i/it/

[craigdh, 2012/02/29 22:53:43]

Done.

+  for (i = event_queue_.begin(); i != event_queue_.end(); i++) {
+    delete *i;
+  }
+  event_queue_.clear();
+}
+
+bool AutomationEventQueue::CheckReturnEvent() {
+  if (wait_automation_reply_.get()) {
+    AutomationEventQueue::AutomationEvent* event = wait_observer_id_ < 0 ?
+                             PopEvent() :
+                             PopEvent(wait_observer_id_);

[frankf, 2012/02/29 02:29:00]

alignment issue

[craigdh, 2012/02/29 22:53:43]

Done.

+    if (event) {
+      wait_automation_reply_->SendSuccess(event->GetValue());
+      wait_automation_reply_.reset();
+      wait_observer_id_ = -1;
+      delete event;
+      return true;
+    }
+  }
+  return false;
+}