FormatErrorMessage() functions are now publicly available from ExtensionErrorUtils.

ExtensionTabsModule implements a bunch of error_messages.

Extension Calls now always deliver a response to the calling context and route error messages if any to the window.console.error log.

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

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

7 files changed, 146 insertions, 80 deletions

diff --git a/chrome/renderer/extensions/extension_api_client_unittest.cc b/chrome/renderer/extensions/extension_api_client_unittest.cc
index 7e786b7..6104bab 100644
--- a/chrome/renderer/extensions/extension_api_client_unittest.cc
+++ b/chrome/renderer/extensions/extension_api_client_unittest.cc
@@ -91,8 +91,9 @@ TEST_F(ExtensionAPIClientTest, CallbackDispatching) {
   ASSERT_TRUE(callback_id >= 0);
 
   // Now send the callback a response
-  ExtensionProcessBindings::ExecuteCallbackInFrame(
-    GetMainFrame(), callback_id, "{\"foo\":\"bar\"}");
+  ExtensionProcessBindings::CallContext call(GetMainFrame(), "CreateTab");
+  ExtensionProcessBindings::ExecuteResponseInFrame(
+    &call, callback_id, true, "{\"foo\":\"bar\"}", "");
 
   // And verify that it worked
   ASSERT_EQ("pass", GetConsoleMessage());
@@ -138,19 +139,19 @@ TEST_F(ExtensionAPIClientTest, GetCurentWindow) {
                "GetCurrentWindow", "null");
 }
 
-TEST_F(ExtensionAPIClientTest, GetFocusedWindow) {
-  ExpectJsFail("chrome.windows.getFocused(function(){}, 20);",
+TEST_F(ExtensionAPIClientTest, GetLastFocusedWindow) {
+  ExpectJsFail("chrome.windows.getLastFocused(function(){}, 20);",
                "Uncaught Error: Too many arguments.");
 
-  ExpectJsFail("chrome.windows.getFocused();",
+  ExpectJsFail("chrome.windows.getLastFocused();",
                "Uncaught Error: Parameter 0 is required.");
 
-  ExpectJsFail("chrome.windows.getFocused('abc');",
+  ExpectJsFail("chrome.windows.getLastFocused('abc');",
                "Uncaught Error: Invalid value for argument 0. "
                "Expected 'function' but got 'string'.");
 
-  ExpectJsPass("chrome.windows.getFocused(function(){})",
-               "GetFocusedWindow", "null");
+  ExpectJsPass("chrome.windows.getLastFocused(function(){})",
+               "GetLastFocusedWindow", "null");
 }
 
 TEST_F(ExtensionAPIClientTest, GetAllWindows) {
@@ -302,10 +303,23 @@ TEST_F(ExtensionAPIClientTest, MoveTab) {
 }
 
 TEST_F(ExtensionAPIClientTest, RemoveTab) {
-  ExpectJsFail("chrome.tabs.remove('foobar', function(){});",
+  ExpectJsFail("chrome.tabs.remove(32, function(){}, 20);",
                "Uncaught Error: Too many arguments.");
 
-  ExpectJsPass("chrome.tabs.remove(21)", "RemoveTab", "21");
+
+  ExpectJsFail("chrome.tabs.remove('abc', function(){});",
+               "Uncaught Error: Invalid value for argument 0. "
+               "Expected 'integer' but got 'string'.");
+
+  ExpectJsFail("chrome.tabs.remove(1, 1);",
+               "Uncaught Error: Invalid value for argument 1. "
+               "Expected 'function' but got 'integer'.");
+
+  ExpectJsPass("chrome.tabs.remove(2, function(){})",
+               "RemoveTab", "2");
+
+  ExpectJsPass("chrome.tabs.remove(2)",
+               "RemoveTab", "2");
 }
 
 // Bookmark API tests
diff --git a/chrome/renderer/extensions/extension_process_bindings.cc b/chrome/renderer/extensions/extension_process_bindings.cc
index 032d4570..dccc32a 100644
--- a/chrome/renderer/extensions/extension_process_bindings.cc
+++ b/chrome/renderer/extensions/extension_process_bindings.cc
@@ -46,8 +46,8 @@ class ExtensionImpl : public v8::Extension {
       v8::Handle<v8::String> name) {
     std::set<std::string>* names = GetFunctionNameSet();
 
-    if (name->Equals(v8::String::New("GetNextCallbackId")))
-      return v8::FunctionTemplate::New(GetNextCallbackId);
+    if (name->Equals(v8::String::New("GetNextRequestId")))
+      return v8::FunctionTemplate::New(GetNextRequestId);
     else if (names->find(*v8::String::AsciiValue(name)) != names->end())
       return v8::FunctionTemplate::New(StartRequest, name);
 
@@ -63,9 +63,9 @@ class ExtensionImpl : public v8::Extension {
     return &Singleton<SingletonData>()->function_names_;
   }
 
-  static v8::Handle<v8::Value> GetNextCallbackId(const v8::Arguments& args) {
-    static int next_callback_id = 0;
-    return v8::Integer::New(next_callback_id++);
+  static v8::Handle<v8::Value> GetNextRequestId(const v8::Arguments& args) {
+    static int next_request_id = 0;
+    return v8::Integer::New(next_request_id++);
   }
 
   static v8::Handle<v8::Value> StartRequest(const v8::Arguments& args) {
@@ -76,14 +76,17 @@ class ExtensionImpl : public v8::Extension {
     if (!webframe || !renderview)
       return v8::Undefined();
 
-    if (args.Length() != 2 || !args[0]->IsString() || !args[1]->IsInt32())
+    if (args.Length() != 3 || !args[0]->IsString() || !args[1]->IsInt32() ||
+        !args[2]->IsBoolean())
       return v8::Undefined();
 
-    int callback_id = args[1]->Int32Value();
+    int request_id = args[1]->Int32Value();
+    bool has_callback = args[2]->BooleanValue();
+
     renderview->SendExtensionRequest(
         std::string(*v8::String::AsciiValue(args.Data())),
         std::string(*v8::String::Utf8Value(args[0])),
-        callback_id, webframe);
+        request_id, has_callback, webframe);
 
     return v8::Undefined();
   }
@@ -100,17 +103,31 @@ void ExtensionProcessBindings::SetFunctionNames(
   ExtensionImpl::SetFunctionNames(names);
 }
 
-void ExtensionProcessBindings::ExecuteCallbackInFrame(
-    WebFrame* frame, int callback_id, const std::string& response) {
-  std::string code = "chrome.dispatchCallback_(";
-  code += IntToString(callback_id);
-  code += ", '";
+void ExtensionProcessBindings::ExecuteResponseInFrame(
+    CallContext *call, int request_id, bool success,
+    const std::string& response,
+    const std::string& error) {
+  std::string code = "chrome.handleResponse_(";
+  code += IntToString(request_id);
+
+  code += ", '" + call->name_;
+
+  if (success)
+    code += "', true";
+  else
+    code += "', false";
 
+  code += ", '";
   size_t offset = code.length();
   code += response;
   ReplaceSubstringsAfterOffset(&code, offset, "\\", "\\\\");
   ReplaceSubstringsAfterOffset(&code, offset, "'", "\\'");
+  code += "', '";
+  offset = code.length();
+  code += error;
+  ReplaceSubstringsAfterOffset(&code, offset, "\\", "\\\\");
+  ReplaceSubstringsAfterOffset(&code, offset, "'", "\\'");
   code += "')";
 
-  frame->ExecuteScript(WebScriptSource(WebString::fromUTF8(code)));
+  call->frame_->ExecuteScript(WebScriptSource(WebString::fromUTF8(code)));
 }
diff --git a/chrome/renderer/extensions/extension_process_bindings.h b/chrome/renderer/extensions/extension_process_bindings.h
index 51b13a7..a24e4e5 100644
--- a/chrome/renderer/extensions/extension_process_bindings.h
+++ b/chrome/renderer/extensions/extension_process_bindings.h
@@ -16,10 +16,21 @@ class WebFrame;
 
 class ExtensionProcessBindings {
  public:
+  struct CallContext {
+   public :
+     CallContext(WebFrame *frame, const std::string& name)
+        : frame_(frame),
+          name_(name) {}
+    WebFrame* frame_;
+    std::string name_;
+  };
+
   static void SetFunctionNames(const std::vector<std::string>& names);
   static v8::Extension* Get();
-  static void ExecuteCallbackInFrame(WebFrame* frame, int callback_id,
-                                     const std::string& response);
+  static void ExecuteResponseInFrame(CallContext *call, int request_id,
+                                     bool success,
+                                     const std::string& response,
+                                     const std::string& error);
 };
 
 #endif  // CHROME_RENDERER_EXTENSIONS_EXTENSION_PROCESS_BINDINGS_H_
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index d28faa7..8f36c76 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -1461,20 +1461,21 @@ void RenderView::DidCancelClientRedirect(WebView* webview,
 
 void RenderView::WillCloseFrame(WebView* view, WebFrame* frame) {
   // Remove all the pending extension callbacks for this frame.
-  if (pending_extension_callbacks_.IsEmpty())
+  if (pending_extension_requests_.IsEmpty())
     return;
 
-  std::vector<int> orphaned_callbacks;
-  for (IDMap<WebFrame>::const_iterator iter =
-       pending_extension_callbacks_.begin();
-       iter != pending_extension_callbacks_.end(); ++iter) {
-    if (iter->second == frame)
-      orphaned_callbacks.push_back(iter->first);
+  std::vector<int> orphaned_requests;
+  for (IDMap<ExtensionProcessBindings::CallContext>::const_iterator iter =
+       pending_extension_requests_.begin();
+       iter != pending_extension_requests_.end(); ++iter) {
+    if (iter->second->frame_ == frame)
+      orphaned_requests.push_back(iter->first);
   }
 
-  for (std::vector<int>::const_iterator iter = orphaned_callbacks.begin();
-       iter != orphaned_callbacks.end(); ++iter) {
-    pending_extension_callbacks_.Remove(*iter);
+  for (std::vector<int>::const_iterator iter = orphaned_requests.begin();
+       iter != orphaned_requests.end(); ++iter) {
+    delete pending_extension_requests_.Lookup(*iter);
+    pending_extension_requests_.Remove(*iter);
   }
 }
 
@@ -2908,25 +2909,33 @@ void RenderView::OnSetBackground(const SkBitmap& background) {
 
 void RenderView::SendExtensionRequest(const std::string& name,
                                       const std::string& args,
-                                      int callback_id,
-                                      WebFrame* callback_frame) {
-  if (callback_id != -1) {
-    DCHECK(callback_frame) << "Callback specified without frame";
-    pending_extension_callbacks_.AddWithID(callback_frame, callback_id);
-  }
-
-  Send(new ViewHostMsg_ExtensionRequest(routing_id_, name, args, callback_id));
-}
-
-void RenderView::OnExtensionResponse(int callback_id,
-                                     const std::string& response) {
-  WebFrame* web_frame = pending_extension_callbacks_.Lookup(callback_id);
-  if (!web_frame)
+                                      int request_id,
+                                      bool has_callback,
+                                      WebFrame* request_frame) {
+  DCHECK(request_frame) << "Request specified without frame";
+  pending_extension_requests_.AddWithID(
+      new ExtensionProcessBindings::CallContext(request_frame, name),
+      request_id);
+
+  Send(new ViewHostMsg_ExtensionRequest(routing_id_, name, args, request_id,
+      has_callback));
+}
+
+void RenderView::OnExtensionResponse(int request_id,
+                                     bool success,
+                                     const std::string& response,
+                                     const std::string& error) {
+  ExtensionProcessBindings::CallContext* call =
+      pending_extension_requests_.Lookup(request_id);
+
+  if (!call)
     return;  // The frame went away.
 
-  ExtensionProcessBindings::ExecuteCallbackInFrame(web_frame, callback_id,
-                                                   response);
-  pending_extension_callbacks_.Remove(callback_id);
+  ExtensionProcessBindings::ExecuteResponseInFrame(call, request_id,
+                                                   success, response,
+                                                   error);
+  pending_extension_requests_.Remove(request_id);
+  delete call;
 }
 
 // Dump all load time histograms.
diff --git a/chrome/renderer/render_view.h b/chrome/renderer/render_view.h
index 3463ad8..7b5665d 100644
--- a/chrome/renderer/render_view.h
+++ b/chrome/renderer/render_view.h
@@ -20,6 +20,7 @@
 #include "build/build_config.h"
 #include "chrome/renderer/automation/dom_automation_controller.h"
 #include "chrome/renderer/dom_ui_bindings.h"
+#include "chrome/renderer/extensions/extension_process_bindings.h"
 #include "chrome/renderer/external_host_bindings.h"
 #include "chrome/renderer/render_widget.h"
 #include "skia/include/SkBitmap.h"
@@ -374,8 +375,11 @@ class RenderView : public RenderWidget,
   void OnClearFocusedNode();
 
   void SendExtensionRequest(const std::string& name, const std::string& args,
-                            int callback_id, WebFrame* web_frame);
-  void OnExtensionResponse(int callback_id, const std::string& response);
+                            int request_id, bool has_callback,
+                            WebFrame* web_frame);
+  void OnExtensionResponse(int request_id, bool success,
+                           const std::string& response,
+                           const std::string& error);
 
  protected:
   // RenderWidget override.
@@ -784,8 +788,8 @@ class RenderView : public RenderWidget,
   // change but is overridden by tests.
   int delay_seconds_for_form_state_sync_;
 
-  // Maps pending callback IDs to their frames.
-  IDMap<WebFrame> pending_extension_callbacks_;
+  // Maps pending request IDs to their frames.
+  IDMap<ExtensionProcessBindings::CallContext> pending_extension_requests_;
 
   scoped_refptr<AudioMessageFilter> audio_message_filter_;
 
diff --git a/chrome/renderer/renderer_resources.grd b/chrome/renderer/renderer_resources.grd
index cda253b..c59e98c 100644
--- a/chrome/renderer/renderer_resources.grd
+++ b/chrome/renderer/renderer_resources.grd
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- This comment is only here because changes to resources are not picked up
-without changes to the corresponding grd file. -->
+without changes to the corresponding grd file.  -->
 <grit latest_public_release="0" current_release="1">
   <outputs>
     <output filename="grit/renderer_resources.h" type="rc_header">
diff --git a/chrome/renderer/resources/extension_process_bindings.js b/chrome/renderer/resources/extension_process_bindings.js
index 70df0fb..8b17e63 100644
--- a/chrome/renderer/resources/extension_process_bindings.js
+++ b/chrome/renderer/resources/extension_process_bindings.js
@@ -9,10 +9,10 @@
 
 var chrome;
 (function() {
-  native function GetNextCallbackId();
+  native function GetNextRequestId();
   native function GetWindow();
   native function GetCurrentWindow();
-  native function GetFocusedWindow();
+  native function GetLastFocusedWindow();
   native function CreateWindow();
   native function RemoveWindow();
   native function GetAllWindows();
@@ -71,27 +71,37 @@ var chrome;
   // TODO(aa): This function should not be publicly exposed. Pass it into V8
   // instead and hold one per-context. See the way event_bindings.js works.
   var callbacks = [];
-  chrome.dispatchCallback_ = function(callbackId, str) {
+  chrome.handleResponse_ = function(requestId, name, success, response, error) {
     try {
-      if (str) {
-        callbacks[callbackId](goog.json.parse(str));
-      } else {
-        callbacks[callbackId]();
+      if (!success) {
+        if (!error)
+          error = "Unknown error."
+        console.error("Error during " + name + ": " + error);
+        return;
+      }
+      
+      if (callbacks[requestId]) {
+        if (response) {
+          callbacks[requestId](goog.json.parse(response));
+        } else {
+          callbacks[requestId]();
+        }
       }
     } finally {
-      delete callbacks[callbackId];
+      delete callbacks[requestId];
     }
   };
 
   // Send an API request and optionally register a callback.
   function sendRequest(request, args, callback) {
     var sargs = goog.json.serialize(args);
-    var callbackId = -1;
+    var requestId = GetNextRequestId();
+    var hasCallback = false;
     if (callback) {
-      callbackId = GetNextCallbackId();
-      callbacks[callbackId] = callback;
+      hasCallback = true;
+      callbacks[requestId] = callback;
     }
-    request(sargs, callbackId);
+    request(sargs, requestId, hasCallback);
   }
 
   //----------------------------------------------------------------------------
@@ -118,12 +128,12 @@ var chrome;
     chrome.types.fun
   ];
   
-  chrome.windows.getFocused = function(callback) {
+  chrome.windows.getLastFocused = function(callback) {
     validate(arguments, arguments.callee.params);
-    sendRequest(GetFocusedWindow, null, callback);
+    sendRequest(GetLastFocusedWindow, null, callback);
   };
 
-  chrome.windows.getFocused.params = [
+  chrome.windows.getLastFocused.params = [
     chrome.types.fun
   ];
 
@@ -137,11 +147,11 @@ var chrome;
     chrome.types.fun
   ];
   
-  chrome.windows.createWindow = function(createData, callback) {
+  chrome.windows.create = function(createData, callback) {
     validate(arguments, arguments.callee.params);
     sendRequest(CreateWindow, createData, callback);
   };
-  chrome.windows.createWindow.params = [
+  chrome.windows.create.params = [
     {
       type: "object",
       properties: {
@@ -156,12 +166,12 @@ var chrome;
     chrome.types.optFun
   ];
   
-  chrome.windows.removeWindow = function(windowId, callback) {
+  chrome.windows.remove = function(windowId, callback) {
     validate(arguments, arguments.callee.params);
     sendRequest(RemoveWindow, windowId, callback);
   };
 
-  chrome.windows.removeWindow.params = [
+  chrome.windows.remove.params = [
     chrome.types.pInt,
     chrome.types.optFun
   ];
@@ -267,13 +277,14 @@ var chrome;
     chrome.types.optFun
   ];
   
-  chrome.tabs.remove = function(tabId) {
+  chrome.tabs.remove = function(tabId, callback) {
     validate(arguments, arguments.callee.params);
-    sendRequest(RemoveTab, tabId);
+    sendRequest(RemoveTab, tabId, callback);
   };
 
   chrome.tabs.remove.params = [
-    chrome.types.pInt
+    chrome.types.pInt,
+    chrome.types.optFun
   ];
 
   // Sends ({Tab}).