DevTools: brush up remote debugging browser target: use callbacks, make handlers implementation a m…

content/browser/devtools/devtools_browser_target.cc

Index: content/browser/devtools/devtools_browser_target.cc
diff --git a/content/browser/devtools/devtools_browser_target.cc b/content/browser/devtools/devtools_browser_target.cc
index f3de3e2141ddea349f0e15ab5c645dd4e9c57881..59ee903e6a5ef73907639e4f7b9956d21a7bce62 100644
--- a/content/browser/devtools/devtools_browser_target.cc
+++ b/content/browser/devtools/devtools_browser_target.cc
@@ -14,79 +14,73 @@
 #include "base/values.h"
 #include "net/server/http_server.h"
 
-namespace content {
-
 namespace {
 
-class NotifierImpl : public DevToolsBrowserTarget::Notifier {
- public:
-  NotifierImpl(
-      base::MessageLoopProxy* message_loop_proxy,
-      net::HttpServer* http_server,
-      int connection_id)
-     : message_loop_proxy_(message_loop_proxy),
-       server_(http_server),
-       connection_id_(connection_id) {
-  }
-
-  virtual ~NotifierImpl() {}
-
-  // DevToolsBrowserTarget::Notifier
-  virtual void Notify(const std::string& data) OVERRIDE {
-    message_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&net::HttpServer::SendOverWebSocket,
-                   server_,
-                   connection_id_,
-                   data));
-  }
+base::Value* CreateErrorObject(int error_code, const std::string& message) {
+  base::DictionaryValue* error_object = new base::DictionaryValue();
+  error_object->SetInteger("code", error_code);
+  error_object->SetString("message", message);
+  return error_object;
+}
 
- private:
-  base::MessageLoopProxy* const message_loop_proxy_;
-  net::HttpServer* const server_;
-  const int connection_id_;
+}  // namespace
 
-  DISALLOW_COPY_AND_ASSIGN(NotifierImpl);
-};
+namespace content {
 
-}  // namespace
+DevToolsBrowserTarget::DomainHandler::~DomainHandler() {
+}
 
-DevToolsBrowserTarget::Handler::Handler()
-    : notifier_(NULL) {
+void DevToolsBrowserTarget::DomainHandler::RegisterCommandHandler(
+    const std::string& command,
+    CommandHandler handler) {
+  command_handlers_[command] = handler;
 }
 
-DevToolsBrowserTarget::Handler::~Handler() {
+DevToolsBrowserTarget::DomainHandler::DomainHandler(const std::string& domain)
+    : domain_(domain) {
 }
 
-void DevToolsBrowserTarget::Handler::set_notifier(
-    DevToolsBrowserTarget::Notifier* notifier) {
-  notifier_ = notifier;
+base::DictionaryValue* DevToolsBrowserTarget::DomainHandler::HandleCommand(
+    const std::string& command,
+    const base::DictionaryValue* params,
+    base::Value** error_out) {
+  CommandHandlers::iterator it = command_handlers_.find(command);
+  if (it == command_handlers_.end()) {
+    *error_out = CreateErrorObject(-1, "Invalid method");
+    return NULL;
+  }
+  return (it->second).Run(params, error_out);
 }
 
-DevToolsBrowserTarget::Notifier* DevToolsBrowserTarget::Handler::notifier()
-    const {
-  return notifier_;
+void DevToolsBrowserTarget::DomainHandler::SendNotification(
+    const std::string& method,
+    base::DictionaryValue* params,
+    base::Value* error) {
+  notifier_.Run(method, params, error);
 }
 
 DevToolsBrowserTarget::DevToolsBrowserTarget(
     base::MessageLoopProxy* message_loop_proxy,
     net::HttpServer* http_server,
     int connection_id)
-    : notifier_(
-          new NotifierImpl(message_loop_proxy, http_server, connection_id)),
-      connection_id_(connection_id) {
+    : message_loop_proxy_(message_loop_proxy),
+      http_server_(http_server),
+      connection_id_(connection_id),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 DevToolsBrowserTarget::~DevToolsBrowserTarget() {
-  for (HandlersMap::iterator i = handlers_.begin(); i != handlers_.end(); ++i)
+  for (DomainHandlerMap::iterator i = handlers_.begin(); i != handlers_.end();
+       ++i)
     delete i->second;
 }
 
-void DevToolsBrowserTarget::RegisterHandler(Handler* handler) {
-  std::string domain = handler->Domain();
+void DevToolsBrowserTarget::RegisterDomainHandler(DomainHandler* handler) {
+  std::string domain = handler->domain();
   DCHECK(handlers_.find(domain) == handlers_.end());
   handlers_[domain] = handler;
-  handler->set_notifier(notifier_.get());
+  handler->set_notifier(Bind(&DevToolsBrowserTarget::SendNotification,
+                             weak_factory_.GetWeakPtr()));
 }
 
 std::string DevToolsBrowserTarget::HandleMessage(const std::string& data) {
@@ -126,19 +120,35 @@ std::string DevToolsBrowserTarget::HandleMessage(const std::string& data) {
         request_id, CreateErrorObject(-1, "Domain unsupported"));
 
   base::Value* error_object = NULL;
-  base::Value* domain_result = handlers_[domain]->OnProtocolCommand(
+  base::DictionaryValue* domain_result = handlers_[domain]->HandleCommand(
       method, params, &error_object);
 
   if (error_object)
     return SerializeErrorResponse(request_id, error_object);
 
-  if (!domain_result)
-    return SerializeErrorResponse(
-        request_id, CreateErrorObject(-1, "Invalid call"));
+  return SerializeResponse(request_id, domain_result);
+}
 
-  DictionaryValue* response = new DictionaryValue();
-  response->Set("result", domain_result);
-  return SerializeResponse(request_id, response);
+void DevToolsBrowserTarget::SendNotification(const std::string& method,
+                                             DictionaryValue* params,
+                                             Value* error) {
+  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
+  response->SetString("method", method);
+  if (error)

[bulach, 2013/01/16 14:56:21]

ultra nit: seems that if (params) .. else if (error) would be neater..

[pfeldman, 2013/01/16 15:33:38]

Actually did it on purpose, so that error had precedence over semi-populated
parameters.

+    response->Set("error", error);
+  else if (params)
+    response->Set("params", params);
+
+  // Serialize response.
+  std::string json_response;
+  base::JSONWriter::Write(response.get(), &json_response);
+
+  message_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(&net::HttpServer::SendOverWebSocket,
+                 http_server_,
+                 connection_id_,
+                 json_response));
 }
 
 std::string DevToolsBrowserTarget::SerializeErrorResponse(
@@ -152,23 +162,16 @@ std::string DevToolsBrowserTarget::SerializeErrorResponse(
   return json_response;
 }
 
-base::Value* DevToolsBrowserTarget::CreateErrorObject(
-    int error_code, const std::string& message) {
-  base::DictionaryValue* error_object = new base::DictionaryValue();
-  error_object->SetInteger("code", error_code);
-  error_object->SetString("message", message);
-  return error_object;
-}
-
 std::string DevToolsBrowserTarget::SerializeResponse(
-    int request_id, base::Value* response) {
-  scoped_ptr<base::DictionaryValue> ret(new base::DictionaryValue());
-  ret->SetInteger("id", request_id);
-  ret->Set("response", response);
+    int request_id, base::DictionaryValue* result) {
+  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
+  response->SetInteger("id", request_id);
+  if (result)
+    response->Set("result", result);
 
   // Serialize response.
   std::string json_response;
-  base::JSONWriter::Write(ret.get(), &json_response);
+  base::JSONWriter::Write(response.get(), &json_response);
   return json_response;
 }