Bindings layer for declarative events API

chrome/renderer/resources/extensions/event.js

Index: chrome/renderer/resources/extensions/event.js
diff --git a/chrome/renderer/resources/extensions/event.js b/chrome/renderer/resources/extensions/event.js
index 45d7080a6c478b2fbc9002e6ccba527ea80c57a2..b1ab1e13ecbdd7fb45d5ce012cabddce2bfcd9e4 100644
--- a/chrome/renderer/resources/extensions/event.js
+++ b/chrome/renderer/resources/extensions/event.js
@@ -58,9 +58,14 @@ var chrome = chrome || {};
   //   chrome.tabs.onChanged.addListener(function(data) { alert(data); });
   //   chromeHidden.Event.dispatch("tab-changed", "hi");
   // will result in an alert dialog that says 'hi'.
-  chrome.Event = function(opt_eventName, opt_argSchemas) {
+  //
+  // If opt_eventOptions exists, it is a dictionary that contains the boolean
+  // entries "supportsListeners" and "supportsRules".
+  chrome.Event = function(opt_eventName, opt_argSchemas, opt_eventOptions) {

[not at google - send to devlin, 2012/02/01 07:23:12]

eventCapabilities might be a better name?

[battre, 2012/02/01 17:35:39]

Even if we continue adding stuff, such in this example?
        "options": {
          "supportsListeners": false,
          "supportsRules": true,
          "conditions": ["RequestMatcher"],
          "actions": ["CancelRequest", "ModifyRequest"]
        }
Would you call the schema features "capabilities"?

[not at google - send to devlin, 2012/02/01 23:07:23]

Yep, never mind. They're also defined as "options" in the JSON I suppose.

     this.eventName_ = opt_eventName;
     this.listeners_ = [];
+    this.eventOptions_ = opt_eventOptions ||
+        {"supportsListeners": true, "supportsRules": false};
 
     // Validate event parameters if we are in debug.
     if (opt_argSchemas &&
@@ -75,7 +80,12 @@ var chrome = chrome || {};
                  exception;
         }
       };
+    } else {
+      this.validate_ = function() {}
     }
+
+    this.ruleIds_ = {};
+    this.lastGeneratedRuleId_ = 0;
   };
 
   // A map of event names to the event object that is registered to that name.
@@ -84,6 +94,9 @@ var chrome = chrome || {};
   // An array of all attached event objects, used for detaching on unload.
   var allAttachedEvents = [];
 
+  // An array of all events that have had rules added the them at any time.
+  var allEventsWithRules = [];
+
   // A map of functions that massage event arguments before they are dispatched.
   // Key is event name, value is function.
   var eventArgumentMassagers = {};
@@ -127,6 +140,8 @@ var chrome = chrome || {};
 
   // Registers a callback to be called when this event is dispatched.
   chrome.Event.prototype.addListener = function(cb) {
+    if (!this.eventOptions_.supportsListeners)
+      throw new Error("This event does not support listeners.");
     if (this.listeners_.length == 0) {
       this.attach_();
     }
@@ -135,6 +150,8 @@ var chrome = chrome || {};
 
   // Unregisters a callback.
   chrome.Event.prototype.removeListener = function(cb) {
+    if (!this.eventOptions_.supportsListeners)
+      throw new Error("This event does not support listeners.");
     var idx = this.findListener_(cb);
     if (idx == -1) {
       return;
@@ -148,11 +165,15 @@ var chrome = chrome || {};
 
   // Test if the given callback is registered for this event.
   chrome.Event.prototype.hasListener = function(cb) {
+    if (!this.eventOptions_.supportsListeners)
+      throw new Error("This event does not support listeners.");
     return this.findListener_(cb) > -1;
   };
 
   // Test if any callbacks are registered for this event.
-  chrome.Event.prototype.hasListeners = function(cb) {
+  chrome.Event.prototype.hasListeners = function() {
+    if (!this.eventOptions_.supportsListeners)
+      throw new Error("This event does not support listeners.");
     return this.listeners_.length > 0;
   };
 
@@ -171,6 +192,8 @@ var chrome = chrome || {};
   // Dispatches this event object to all listeners, passing all supplied
   // arguments to this function each listener.
   chrome.Event.prototype.dispatch = function(varargs) {
+    if (!this.eventOptions_.supportsListeners)
+      throw new Error("This event does not support listeners.");
     var args = Array.prototype.slice.call(arguments);
     if (this.validate_) {

[not at google - send to devlin, 2012/02/01 07:23:12]

now that there's always set a validate_ method, this check is unnecessary

       var validationErrors = this.validate_(args);
@@ -227,6 +250,105 @@ var chrome = chrome || {};
     this.detach_();
   };
 
+  // Takes a list of JSON datatype identifiers and returns a schema fragment
+  // that verifies that a JSON object corresponds to an array of only these
+  // data types.
+  chrome.Event.prototype.buildArrayOfChoicesSchema_ = function(typesList) {
+    return {
+      "type": "array",
+      "items": {
+        "choices": typesList.forEach(function(el) {return {"$ref": el};})

[not at google - send to devlin, 2012/02/01 08:15:57]

What does this do? I thought forEach has no return value?

[battre, 2012/02/01 17:35:39]

This should have been "map" instead of "forEach".

Done.

+      }
+    };
+  }
+
+  // Validate conditions and actions against specific schemas of this
+  // event object type.
+  // |rules| is an array of JSON objects that follow the Rule type of the
+  // declarative extension APIs. |conditions| is an array of JSON type
+  // identifiers that are allowed to occur in the conditions attribute of each
+  // rule. Likewise, |actions| is an array of JSON type identifiers that are
+  // allowed to occur in the actions attribute of each rule.
+  chrome.Event.prototype.validateRules_ = function(rules, conditions, actions) {
+    if (!conditions || !actions) {
+      throw new Error("Error in API specification.");
+    }
+    var conditionsSchema = this.buildArrayOfChoicesSchema_(conditions);
+    var actionsSchema = this.buildArrayOfChoicesSchema_(actions);
+    rules.forEach(function(rule) {
+      chromeHidden.validate([rule.conditions], [conditionsSchema]);
+      chromeHidden.validate([rule.actions], [actionsSchema]);
+    })
+  }
+
+  chrome.Event.prototype.addMissingIds_ = function(rules) {

[not at google - send to devlin, 2012/02/01 07:23:12]

This, and addMissingPriorities, should be done in a custom_bindings.js file; see
omnibox_custom_bindings.js as an example, but it would look something like:

=== experimental.declarative_custom_bindings.js === 


(function() {

native function GetChromeHidden();

GetChromeHidden().registerCustomHook(
    'experimental.declarative',
    function(bindingsAPI) {
  var apiFunctions = bindingsAPI.apiFunctions;

  apiFunctions.setUpdateArgumentsPostValidate(
      "experimental.declarative.addRules",
      function(event, rules) {
    addMissingIds(rules);
    addMissingPriorities(rules);
    return [event, rules];
  }); 
});

}());

[battre, 2012/02/01 17:35:39]

This does not work:
We want to tell the extension which IDs were assigned to the rules. The custom
hooks work on a copy of the parameters passed to the function. I.e. these
updates are sent to the browser but are invisible to the extension. -> I will
change this so that extension implementation in the browser is responsible for
ID generation and returns the new rules to the extension. I think this is
cleaner.

+    for (var i = 0; i < rules.length; ++i) {
+      // TODO(battre): check for "".
+      if (!("id" in rules[i])) {
+        // Generate a unique ID.
+        var newRuleId = "";
+        do {
+          newRuleId = "_" + (this.lastGeneratedRuleId_++) + "_"
+        } while (newRuleId in this.ruleIds_);
+        // And store it.
+        rules[i]["id"] = newRuleId;
+      }
+      this.ruleIds_[rules[i]["id"]] = 1;
+    }
+  }
+
+  chrome.Event.prototype.addMissingPriorities_ = function(rules) {
+    for (var i = 0; i < rules.length; ++i) {
+      if (!("priority" in rules[i]))
+        rules[i]["priority"] = 100;
+    }
+  }
+
+  chrome.Event.prototype.addRules = function(rules, opt_cb) {

[not at google - send to devlin, 2012/02/01 07:23:12]

I think that this should just be a thin wrapper around
experimental.declarative.addRules which basically curries in the event name. So
just

chrome.experimental.declarative.addRules(this.eventName_, rules, opt_cb);

plus maybe any extra validation.

[battre, 2012/02/01 17:35:39]

Isn't it a pretty thin wrapper? Which part would you like to get rid of?

[not at google - send to devlin, 2012/02/01 23:07:23]

Yep, it is now. By "thin wrapper" I meant... what I said, something which
curries in the appropriate fields into an already-self-describing
"experimental.declarative" API.

Which is what this looks like now, so cool.

+    if (!this.eventOptions_.supportsRules)
+      throw new Error("This event does not support rules.");
+
+    var found = false;
+    for (var i = 0; i < allEventsWithRules.length; ++i) {
+      if (allEventsWithRules === this) {
+        found = true;
+        break;
+      }
+    }
+    if (!found)
+      allEventsWithRules.push(this);

[not at google - send to devlin, 2012/02/01 07:23:12]

This implies an O(n) iteration over all events each time you call addRules,
right? How about setting a property on the event, like

if (rules.length > 0)
  this.hasRules_ = true;

[battre, 2012/02/01 17:35:39]

I have removed this logic, see below.

+
+    this.validateRules_(rules,

[not at google - send to devlin, 2012/02/01 07:23:12]

I would have thought that the schema-based validation should be the
responsibility of the rule factories, like net.RequestMatcher or whatever.
You'll have to do similar work in there to pull out the necessary fields anyway.

It might help to see what implementations of those classes would look like, but
I'd imagine something like

function RequestMatcher(configuration) {
  validateAgainstSchema(schema, configuration);
  this.buildConditionFromRequestMatcher_(configuration);
}

I imagine you'd still want to validate here that the filters/actions or whatever
part of the rules are the correct instances?

[not at google - send to devlin, 2012/02/01 08:15:57]

I may have gotten my wires a little bit crossed when I made this comment. Let me
rephrase:

Going by the design doc, the caller of the webRequest API (or whtaever), when
setting up rules, will need to go like "new RequestMatcher()" or whatever, and
pass in an object.  You'll need to provide a way of actually implementing these;
will it be something along the lines of

function RequestMatcher(o) {
  var self = this;
  forEach(o, function(key, value) {
    self[key] = value;
  });
  validate(getRequestMatcherSchema(), this);
  this.id = 'net.RequestMatcher'; // not sure about this step?
}

I'm just a bit fuzzy on the details without having an example.
  

[battre, 2012/02/01 17:35:39]

I am not sure how how we will implement these constructors exactly. However, we
need to check that we don't pass "WindowSizeChanged" conditions into the
webrequest API. Therefore, I think that the validation at this place makes
sense.

+                        this.eventOptions_.conditions,
+                        this.eventOptions_.actions);
+    this.addMissingIds_(rules);
+    this.addMissingPriorities_(rules);
+    var callback = opt_cb ? opt_cb.bind(undefined, rules) : undefined;
+
+    return chromeHidden.validatingSendRequest.call(

[not at google - send to devlin, 2012/02/01 07:23:12]

This is interesting. All you really need to do, at least at the moment, is call
the API function itself directly (if it exists; which might not happen if the
extension doesn't have the experimental permission)

so like

if (chrome.experimental && chrome.experimental.declarative)
  chrome.experimental.declarative.addRules(this.eventName_, rules, callback);

However, this brings up the point that as it stands the experimental.declarative
API will be defined as a directly-callable API, as well as one that uses Events.
I'm not sure this is desirable. I guess I don't really see a problem, though
it's a bit ambiguous given you can also do it through the Event objects.

Anyway, to not generate the API we'd need to add an extra property to the schema
like disableBindingsGeneration or something.

However, if we DO go along with having the API callable, then most of this logic
will need to be moved to a custom_bindings file, as I've commented above.

[battre, 2012/02/01 17:35:39]

Wow, this became significantly simpler.

Done.

+        this,
+        "experimental.declarative.addRules",
+        [this.eventName_, rules, callback]);
+  }
+
+  chrome.Event.prototype.removeRules = function(ruleIdentifiers, opt_cb) {
+    if (!this.eventOptions_.supportsRules)
+      throw new Error("This event does not support rules.");
+    return chromeHidden.validatingSendRequest.call(
+        this,
+        "experimental.declarative.removeRules",
+        [this.eventName_, ruleIdentifiers, opt_cb]);
+  }
+
+  chrome.Event.prototype.getRules = function(ruleIdentifiers, cb) {
+    if (!this.eventOptions_.supportsRules)
+      throw new Error("This event does not support rules.");
+    return chromeHidden.validatingSendRequest.call(
+        this,
+        "experimental.declarative.getRules",
+        [this.eventName_, ruleIdentifiers, cb]);
+  }
+
   // Special load events: we don't use the DOM unload because that slows
   // down tab shutdown.  On the other hand, onUnload might not always fire,
   // since Chrome will terminate renderers on shutdown (SuddenTermination).
@@ -246,6 +368,11 @@ var chrome = chrome || {};
       if (event)
         event.detach_();
     }
+    for (var i = 0; i < allEventsWithRules.length; ++i) {
+      var event = allEventsWithRules[i];
+      if (event && event.eventOptions_.supportsRules)
+        event.removeRules([]);

[not at google - send to devlin, 2012/02/01 07:23:12]

Why [] ? Is this to signify removing all rules?

This seems surprising. It would be consistent for other APIs for removing all
rules to be "event.removeRules(null)" instead. Removing [] would remove no
rules, I would have thought.

[battre, 2012/02/01 17:35:39]

I have removed this logic because
a) it did not work reliably (dispatchOnUnload was not called in my browser
tests) and 
b) we can trigger the removal of rules from the "extension unloaded" signal sent
by the ExtensionService in the browser

+    }
   };
 
   chromeHidden.dispatchError = function(msg) {