Make eventArgumentMassagers asynchronous so custom bindings don't need to...

Make eventArgumentMassagers asynchronous so custom bindings don't need to override chrome.Event.prototype.dispatch.


BUG=121479


Review URL: https://chromiumcodereview.appspot.com/10704073

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

chrome/common/extensions/api/input_ime.json

@@ -469,6 +469,12 @@
         "name": "onKeyEvent",
         "type": "function",
         "description": "This event is sent if this extension owns the active IME.",
+        "options": {
+          "supportsFilters": false,
+          "supportsListeners": true,
+          "supportsRules": false,
+          "maxListeners": 1
+        },
         "parameters": [
           {
             "type": "string",

chrome/renderer/extensions/chrome_v8_context_set.cc

@@ -145,15 +145,5 @@ void ChromeV8ContextSet::DispatchChromeHiddenMethod(
     v8::Handle<v8::Value> retval;
     (*it)->CallChromeHiddenMethod(
         method_name, v8_arguments.size(), &v8_arguments[0], &retval);
-    // In debug, the js will validate the event parameters and return a
-    // string if a validation error has occured.
-    // TODO(rafaelw): Consider only doing this check if function_name ==
-    // "Event.dispatchJSON".
-#ifndef NDEBUG
-    if (!retval.IsEmpty() && !retval->IsUndefined()) {
-      std::string error = *v8::String::AsciiValue(retval);
-      DCHECK(false) << error;
-    }
-#endif
   }
 }

chrome/renderer/extensions/event_unittest.cc

@@ -65,6 +65,8 @@ class EventUnittest : public ModuleSystemTest {
         "exports.sendRequest = function() {};");
     OverrideNativeHandler("apiDefinitions",
         "exports.GetExtensionAPIDefinition = function() {};");
+    OverrideNativeHandler("logging",
+        "exports.DCHECK = function() {};");
   }
 };
 
@@ -148,7 +150,7 @@ TEST_F(EventUnittest, NamedEventDispatch) {
       "var e = new event.Event('myevent');"
       "var called = false;"
       "e.addListener(function() { called = true; });"
-      "chromeHidden.Event.dispatch('myevent', []);"
+      "chromeHidden.Event.dispatchJSON('myevent', []);"
       "assert.AssertTrue(called);");
   module_system_->Require("test");
 }
@@ -246,4 +248,28 @@ TEST_F(EventUnittest, AddingFilterWithUrlFieldNotAListThrowsException) {
   module_system_->Require("test");
 }
 
+TEST_F(EventUnittest, MaxListeners) {
+  ModuleSystem::NativesEnabledScope natives_enabled_scope(module_system_.get());
+  RegisterModule("test",
+      "var event = require('event');"
+      "var assert = requireNative('assert');"
+      "var eventOpts = {supportsListeners: true, maxListeners: 1};"
+      "var e = new event.Event('myevent', undefined, eventOpts);"
+      "var cb = function() {};"
+      "var caught = false;"
+      "try {"
+      "  e.addListener(cb);"
+      "} catch (e) {"
+      "  caught = true;"
+      "}"
+      "assert.AssertTrue(!caught);"
+      "try {"
+      "  e.addListener(cb);"
+      "} catch (e) {"
+      "  caught = true;"
+      "}"
+      "assert.AssertTrue(caught);");
+  module_system_->Require("test");
+}
+
 }  // namespace

chrome/renderer/extensions/extension_dispatcher.cc

@@ -211,6 +211,35 @@ class ChannelNativeHandler : public NativeHandler {
   chrome::VersionInfo::Channel channel_;
 };
 
+class LoggingNativeHandler : public NativeHandler {
+ public:
+  LoggingNativeHandler() {
+    RouteFunction("DCHECK",
+        base::Bind(&LoggingNativeHandler::Dcheck,
+                   base::Unretained(this)));
+  }
+
+  v8::Handle<v8::Value> Dcheck(const v8::Arguments& args) {
+    CHECK_LE(args.Length(), 2);
+    bool check_value = args[0]->BooleanValue();
+    std::string error_message;
+    if (args.Length() == 2)
+      error_message += "Error: " + std::string(*v8::String::AsciiValue(args[1]))
+          + "\n";
+
+    v8::Handle<v8::Array> stack_trace(
+        v8::StackTrace::CurrentStackTrace(10)->AsArray());
+    error_message += "Stack trace: {\n";
+    for (size_t i = 0; i < stack_trace->Length(); i++) {
+      error_message += "  "
+          + std::string(*v8::String::AsciiValue(stack_trace->Get(i))) + "\n";
+    }
+    error_message += "}";
+    DCHECK(check_value) << error_message;
+    return v8::Undefined();
+  }
+};
+
 void InstallAppBindings(ModuleSystem* module_system,
                         v8::Handle<v8::Object> chrome,
                         v8::Handle<v8::Object> chrome_hidden) {
@@ -229,7 +258,7 @@ void InstallWebstoreBindings(ModuleSystem* module_system,
                               "chromeHiddenWebstore");
 }
 
-}
+}  // namespace
 
 ExtensionDispatcher::ExtensionDispatcher()
     : is_webkit_initialized_(false),
@@ -690,6 +719,8 @@ void ExtensionDispatcher::DidCreateScriptContext(
   module_system->RegisterNativeHandler("channel",
       scoped_ptr<NativeHandler>(new ChannelNativeHandler(
           static_cast<chrome::VersionInfo::Channel>(chrome_channel_))));
+  module_system->RegisterNativeHandler("logging",
+      scoped_ptr<NativeHandler>(new LoggingNativeHandler()));
   // Create the 'chrome' variable if it doesn't already exist.
   {
     v8::HandleScope handle_scope;

chrome/renderer/module_system.cc

@@ -60,12 +60,15 @@ bool ModuleSystem::IsPresentInCurrentContext() {
 }
 
 // static
-void ModuleSystem::DumpException(v8::Handle<v8::Message> message) {
+void ModuleSystem::DumpException(const v8::TryCatch& try_catch) {
+  v8::Handle<v8::Message> message(try_catch.Message());
+
   LOG(ERROR) << "["
              << *v8::String::Utf8Value(
                  message->GetScriptResourceName()->ToString())
              << "(" << message->GetLineNumber() << ")] "
-             << *v8::String::Utf8Value(message->Get());
+             << *v8::String::Utf8Value(message->Get())
+             << "{" << *v8::String::Utf8Value(try_catch.StackTrace()) << "}";
 }
 
 void ModuleSystem::Require(const std::string& module_name) {
@@ -96,8 +99,10 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
       v8::Handle<v8::String>::Cast(source)));
   v8::Handle<v8::Function> func =
       v8::Handle<v8::Function>::Cast(RunString(wrapped_source, module_name));
-  if (func.IsEmpty())
-    return handle_scope.Close(v8::Handle<v8::Value>());
+  if (func.IsEmpty()) {
+    return ThrowException(std::string(*v8::String::AsciiValue(module_name)) +
+        ": Bad source");
+  }
 
   exports = v8::Object::New();
   v8::Handle<v8::Object> natives(NewInstance());
@@ -108,7 +113,12 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   };
   {
     WebKit::WebScopedMicrotaskSuppression suppression;
+    v8::TryCatch try_catch;
     func->Call(global, 3, args);
+    if (try_catch.HasCaught()) {
+      DumpException(try_catch);
+      return v8::Undefined();
+    }
   }
   modules->Set(module_name, exports);
   return handle_scope.Close(exports);
@@ -184,16 +194,15 @@ v8::Handle<v8::Value> ModuleSystem::RunString(v8::Handle<v8::String> code,
   WebKit::WebScopedMicrotaskSuppression suppression;
   v8::Handle<v8::Value> result;
   v8::TryCatch try_catch;
-  try_catch.SetCaptureMessage(true);
   v8::Handle<v8::Script> script(v8::Script::New(code, name));
   if (try_catch.HasCaught()) {
-    DumpException(try_catch.Message());
+    DumpException(try_catch);
     return handle_scope.Close(result);
   }
 
   result = script->Run();
   if (try_catch.HasCaught())
-    DumpException(try_catch.Message());
+    DumpException(try_catch);
 
   return handle_scope.Close(result);
 }

chrome/renderer/module_system.h

@@ -58,8 +58,8 @@ class ModuleSystem : public NativeHandler {
   // Returns true if the current context has a ModuleSystem installed in it.
   static bool IsPresentInCurrentContext();
 
-  // Dumps the given exception message to LOG(ERROR).
-  static void DumpException(v8::Handle<v8::Message> message);
+  // Dumps the debug info from |try_catch| to LOG(ERROR).
+  static void DumpException(const v8::TryCatch& try_catch);
 
   // Require the specified module. This is the equivalent of calling
   // require('module_name') from the loaded JS files.

chrome/renderer/resources/extensions/event.js

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+  var DCHECK = requireNative('logging').DCHECK;
   var eventBindingsNatives = requireNative('event_bindings');
   var AttachEvent = eventBindingsNatives.AttachEvent;
   var DetachEvent = eventBindingsNatives.DetachEvent;
@@ -208,10 +209,13 @@
 
   chromeHidden.Event = {};
 
-  chromeHidden.Event.registerArgumentMassager = function(name, fn) {
+  // callback is a function(args, dispatch). args are the args we receive from
+  // dispatchJSON(), and dispatch is a function(args) that dispatches args to
+  // its listeners.
+  chromeHidden.Event.registerArgumentMassager = function(name, callback) {
     if (eventArgumentMassagers[name])
       throw new Error("Massager already registered for event: " + name);
-    eventArgumentMassagers[name] = fn;
+    eventArgumentMassagers[name] = callback;
   };
 
   // Dispatches a named event with the given JSON array, which is deserialized
@@ -220,43 +224,29 @@
   chromeHidden.Event.dispatchJSON = function(name, args, filteringInfo) {
     var listenerIDs = null;
 
-    if (filteringInfo) {
+    if (filteringInfo)
       listenerIDs = MatchAgainstEventFilter(name, filteringInfo);
-    }
-    if (attachedNamedEvents[name]) {
-      if (args) {
-        // TODO(asargent): This is an antiquity. Until all callers of
-        // dispatchJSON use actual values, this must remain here to catch the
-        // cases where a caller has hard-coded a JSON string to pass in.
-        if (typeof(args) == "string") {
-          args = chromeHidden.JSON.parse(args);
-        }
-        if (eventArgumentMassagers[name])
-          eventArgumentMassagers[name](args);
-      }
 
-      var event = attachedNamedEvents[name];
-      var result;
-      // TODO(koz): We have to do this differently for unfiltered events (which
-      // have listenerIDs = null) because some bindings write over
-      // event.dispatch (eg: experimental.app.custom_bindings.js) and so expect
-      // events to go through it. These places need to be fixed so that they
-      // expect a listenerIDs parameter.
-      if (listenerIDs)
-        result = event.dispatch_(args, listenerIDs);
-      else
-        result = event.dispatch.apply(event, args);
-      if (result && result.validationErrors)
-        return result.validationErrors;
-    }
-  };
+    var event = attachedNamedEvents[name];
+    if (!event)
+      return;
 
-  // Dispatches a named event with the given arguments, supplied as an array.
-  chromeHidden.Event.dispatch = function(name, args) {
-    if (attachedNamedEvents[name]) {
-      attachedNamedEvents[name].dispatch.apply(
-          attachedNamedEvents[name], args);
-    }
+    // TODO(asargent): This is an antiquity. Until all callers of
+    // dispatchJSON use actual values, this must remain here to catch the
+    // cases where a caller has hard-coded a JSON string to pass in.
+    if (typeof(args) == "string")
+      args = chromeHidden.JSON.parse(args);
+
+    var dispatchArgs = function(args) {
+      result = event.dispatch_(args, listenerIDs);
+      if (result)
+        DCHECK(!result.validationErrors, result.validationErrors);
+    };
+
+    if (eventArgumentMassagers[name])
+      eventArgumentMassagers[name](args, dispatchArgs);
+    else
+      dispatchArgs(args);
   };
 
   // Test if a named event has any listeners.
@@ -269,6 +259,9 @@
   chrome.Event.prototype.addListener = function(cb, filters) {
     if (!this.eventOptions_.supportsListeners)
       throw new Error("This event does not support listeners.");
+    if (this.eventOptions_.maxListeners &&
+        this.getListenerCount() >= this.eventOptions_.maxListeners)
+      throw new Error("Too many listeners for " + this.eventName_);
     if (filters) {
       if (!this.eventOptions_.supportsFilters)
         throw new Error("This event does not support filters.");
@@ -333,9 +326,14 @@
 
   // Test if any callbacks are registered for this event.
   chrome.Event.prototype.hasListeners = function() {
+    return this.getListenerCount() > 0;
+  };
+
+  // Return the number of listeners on this event.
+  chrome.Event.prototype.getListenerCount = function() {
     if (!this.eventOptions_.supportsListeners)
       throw new Error("This event does not support listeners.");
-    return this.listeners_.length > 0;
+    return this.listeners_.length;
   };
 
   // Returns the index of the given callback if registered, or -1 if not
@@ -364,7 +362,7 @@
     var results = [];
     for (var i = 0; i < listeners.length; i++) {
       try {
-        var result = listeners[i].callback.apply(null, args);
+        var result = this.dispatchToListener(listeners[i].callback, args);
         if (result !== undefined)
           results.push(result);
       } catch (e) {
@@ -376,6 +374,11 @@
       return {results: results};
   }
 
+  // Can be overridden to support custom dispatching.
+  chrome.Event.prototype.dispatchToListener = function(callback, args) {
+    return callback.apply(null, args);
+  }
+
   // Dispatches this event object to all listeners, passing all supplied
   // arguments to this function each listener.
   chrome.Event.prototype.dispatch = function(varargs) {

chrome/renderer/resources/extensions/experimental.app_custom_bindings.js

@@ -11,52 +11,52 @@ var appNatives = requireNative('experimental_app');
 var DeserializeString = appNatives.DeserializeString;
 var CreateBlob = appNatives.CreateBlob;
 
-chromeHidden.registerCustomHook('experimental.app', function(bindingsAPI) {
-  chrome.experimental.app.onLaunched.dispatch =
-      function(launchData, intentData) {
-    if (launchData && intentData) {
-      switch(intentData.format) {
-        case('fileEntry'):
-          var event = this;
-          var fs = GetIsolatedFileSystem(intentData.fileSystemId);
-          try {
-            fs.root.getFile(intentData.baseName, {}, function(fileEntry) {
-              launchData.intent.data = fileEntry;
-              launchData.intent.postResult = function() {};
-              launchData.intent.postFailure = function() {};
-              chrome.Event.prototype.dispatch.call(event, launchData);
-            }, function(fileError) {
-              console.error('Error getting fileEntry, code: ' + fileError.code);
-              chrome.Event.prototype.dispatch.call(event);
-            });
-          } catch (e) {
-            console.error('Error in event handler for onLaunched: ' + e.stack);
-            chrome.Event.prototype.dispatch.call(event);
-          }
-          break;
-        case('serialized'):
-          var deserializedData = DeserializeString(intentData.data);
-          launchData.intent.data = deserializedData;
-          launchData.intent.postResult = function() {};
-          launchData.intent.postFailure = function() {};
-          chrome.Event.prototype.dispatch.call(this, launchData);
-          break;
-        case('blob'):
-          var blobData = CreateBlob(intentData.blobFilePath,
-                                    intentData.blobLength);
-          launchData.intent.data = blobData;
-          launchData.intent.postResult = function() {};
-          launchData.intent.postFailure = function() {};
-          chrome.Event.prototype.dispatch.call(this, launchData);
-          break;
-        default:
-          console.error('Unexpected launch data format');
-          chrome.Event.prototype.dispatch.call(this);
-      }
-    } else if (launchData) {
-      chrome.Event.prototype.dispatch.call(this, launchData);
-    } else {
-      chrome.Event.prototype.dispatch.call(this);
+chromeHidden.Event.registerArgumentMassager('experimental.app.onLaunched',
+    function(args, dispatch) {
+  var launchData = args[0];
+  var intentData = args[1];
+
+  if (launchData && intentData) {
+    switch(intentData.format) {
+      case('fileEntry'):
+        var fs = GetIsolatedFileSystem(intentData.fileSystemId);
+        try {
+          fs.root.getFile(intentData.baseName, {}, function(fileEntry) {
+            launchData.intent.data = fileEntry;
+            launchData.intent.postResult = function() {};
+            launchData.intent.postFailure = function() {};
+            dispatch([launchData]);
+          }, function(fileError) {
+            console.error('Error getting fileEntry, code: ' + fileError.code);
+            dispatch([]);
+          });
+        } catch (e) {
+          console.error('Error in event handler for onLaunched: ' + e.stack);
+          dispatch([]);
+        }
+        break;
+      case('serialized'):
+        var deserializedData = DeserializeString(intentData.data);
+        launchData.intent.data = deserializedData;
+        launchData.intent.postResult = function() {};
+        launchData.intent.postFailure = function() {};
+        dispatch([launchData]);
+        break;
+      case('blob'):
+        var blobData = CreateBlob(intentData.blobFilePath,
+                                  intentData.blobLength);
+        launchData.intent.data = blobData;
+        launchData.intent.postResult = function() {};
+        launchData.intent.postFailure = function() {};
+        dispatch([launchData]);
+        break;
+      default:
+        console.error('Unexpected launch data format');
+        dispatch([]);
     }
-  };
+  } else if (launchData) {
+    dispatch([launchData]);
+  } else {
+    dispatch([]);
+  }
 });

chrome/renderer/resources/extensions/input.ime_custom_bindings.js

@@ -8,29 +8,16 @@
 var chromeHidden = requireNative('chrome_hidden').GetChromeHidden();
 
 chromeHidden.registerCustomHook('input.ime', function() {
-  chrome.input.ime.onKeyEvent.dispatch = function(engineID, keyData) {
-    var args = Array.prototype.slice.call(arguments);
-    if (this.validate_) {
-      var validationErrors = this.validate_(args);
-      if (validationErrors) {
-        chrome.input.ime.eventHandled(requestId, false);
-        return validationErrors;
-      }
-    }
-    if (this.listeners_.length > 1) {
-      console.error('Too many listeners for onKeyEvent: ' + e.stack);
-      chrome.input.ime.eventHandled(requestId, false);
-      return;
-    }
-    for (var i = 0; i < this.listeners_.length; i++) {
-      try {
-        var requestId = keyData.requestId;
-        var result = this.listeners_[i].callback.apply(null, args);
-        chrome.input.ime.eventHandled(requestId, result);
-      } catch (e) {
-        console.error('Error in event handler for onKeyEvent: ' + e.stack);
-        chrome.input.ime.eventHandled(requestId, false);
-      }
+  chrome.input.ime.onKeyEvent.dispatchToListener = function(callback, args) {
+    var engineID = args[0];
+    var keyData = args[1];
+
+    var result = false;
+    try {
+      result = chrome.Event.prototype.dispatchToListener(callback, args);
+    } catch (e) {
+      console.error('Error in event handler for onKeyEvent: ' + e.stack);
     }
+    chrome.input.ime.eventHandled(keyData.requestId, result);
   };
 });

chrome/renderer/resources/extensions/omnibox_custom_bindings.js

@@ -97,12 +97,14 @@ chromeHidden.registerCustomHook('omnibox', function(bindingsAPI) {
     }
     return [requestId, suggestions];
   });
+});
 
-  chrome.omnibox.onInputChanged.dispatch =
-      function(text, requestId) {
-    var suggestCallback = function(suggestions) {
-      chrome.omnibox.sendSuggestions(requestId, suggestions);
-    };
-    chrome.Event.prototype.dispatch.apply(this, [text, suggestCallback]);
+chromeHidden.Event.registerArgumentMassager('omnibox.onInputChanged',
+    function(args, dispatch) {
+  var text = args[0];
+  var requestId = args[1];
+  var suggestCallback = function(suggestions) {
+    chrome.omnibox.sendSuggestions(requestId, suggestions);
   };
+  dispatch([text, suggestCallback]);
 });

chrome/renderer/resources/extensions/tts_engine_custom_bindings.js

@@ -6,12 +6,13 @@
 
 var chromeHidden = requireNative('chrome_hidden').GetChromeHidden();
 
-chromeHidden.registerCustomHook('ttsEngine', function() {
-  chrome.ttsEngine.onSpeak.dispatch = function(text, options, requestId) {
-    var sendTtsEvent = function(event) {
-      chrome.ttsEngine.sendTtsEvent(requestId, event);
-    };
-    chrome.Event.prototype.dispatch.apply(
-        this, [text, options, sendTtsEvent]);
+chromeHidden.Event.registerArgumentMassager('ttsEngine.onSpeak',
+    function(args, dispatch) {
+  var text = args[0];
+  var options = args[1];
+  var requestId = args[2];
+  var sendTtsEvent = function(event) {
+    chrome.ttsEngine.sendTtsEvent(requestId, event);
   };
+  dispatch([text, options, sendTtsEvent]);
 });

chrome/test/base/module_system_test.cc

@@ -112,7 +112,7 @@ void ModuleSystemTest::OverrideNativeHandler(const std::string& name,
 
 void ModuleSystemTest::TearDown() {
   if (try_catch_.HasCaught())
-    ModuleSystem::DumpException(try_catch_.Message());
+    ModuleSystem::DumpException(try_catch_);
   EXPECT_FALSE(try_catch_.HasCaught());
   // All tests must assert at least once unless otherwise specified.
   EXPECT_EQ(should_assertions_be_made_,