Bindings layer for declarative events API

Bindings layer for declarative events API


BUG=112155
TEST=no

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120962

[battre, 2012-01-19 22:11:10]

Hi.

this is a new CL that adds "addRules", "removeRules", ... functions into the
event type. Could you please have a look, whether you think I am on track with
this approach.

The CL is work in progress.


This is what an example application would look like:

var rule = {
  'conditions': [
    {'url': "http://www.google.com", "filter_type": "net.RequestMatcher"}
  ],
  'actions': [ {"action_type": "net.CancelRequest"} ]
};
chrome.webRequest.onRequest.addRules(
    [rule],
    function(rules) {console.log("Done!";});

[Aaron Boodman, 2012-01-19 23:09:46]

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:71: {'supports_listeners': true,
'supports_rules': false};
Nit: don't need the single quotes in the object literal here.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:176:
chrome.Event.prototype.hasListeners = function(cb) {
whoops: the cb param here not required.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:264: if
(!this.eventOptions_.supports_rules)
nit: maybe just put supports_rules_ on |this|?

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:265: throw new Error("This event
does not support listeners.");
s/listener/rule/

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:279:
chromeHidden.validate([rule.actions], [actionsSchema]);
Interesting :) I hadn't considered that we could just use JSON Schema so
directly here.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:283:
this.getFunctionDefinition_("experimental.declarative", "addRules");
I think it might be easier to just use raw IPC here. Define a set of native
functions like addRule/removeRule/etc, and then define IPC messages that get
picked up in the browser and send to the right registry.

I don't think we get that much by reusing ExtensionFunctionDispatcher.

Matt, what do you think?

[battre, 2012-01-19 23:45:47]

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:283:
this.getFunctionDefinition_("experimental.declarative", "addRules");
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> I think it might be easier to just use raw IPC here. Define a set of native
> functions like addRule/removeRule/etc, and then define IPC messages that get
> picked up in the browser and send to the right registry.
> 
> I don't think we get that much by reusing ExtensionFunctionDispatcher.
> 
> Matt, what do you think?

I started off with this approach. Here is the deal:
If you register rules, these need to be sent to the RuleRegistry, which lives in
the Browser, they need to be validated and a response needs to be generated. If
we use native functions we are replicating everything that is already available
to sendRequest:
- validation of arguments
- asynchronous messaging with callbacks (addRules needs to "return" something to
the callback function)
- association of messages to their extensions?
- possibly some security features?

And we need to do a lot of plumbing with messages. I preferred this approach.

[Aaron Boodman, 2012-01-19 23:57:36]

On Thu, Jan 19, 2012 at 3:45 PM,  <battre@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
> File chrome/renderer/resources/extensions/event.js (right):
>
>
https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
> chrome/renderer/resources/extensions/event.js:283:
> this.getFunctionDefinition_("experimental.declarative", "addRules");
> On 2012/01/19 23:09:46, Aaron Boodman wrote:
>>
>> I think it might be easier to just use raw IPC here. Define a set of
>
> native
>>
>> functions like addRule/removeRule/etc, and then define IPC messages
>
> that get
>>
>> picked up in the browser and send to the right registry.
>
>
>> I don't think we get that much by reusing ExtensionFunctionDispatcher.
>
>
>> Matt, what do you think?
>
>
> I started off with this approach. Here is the deal:
> If you register rules, these need to be sent to the RuleRegistry, which
> lives in the Browser, they need to be validated and a response needs to
> be generated. If we use native functions we are replicating everything
> that is already available to sendRequest:
> - validation of arguments

The event.addRule() call would still be validated the typical way, but
internally in the js bindings, we would call a special native
function.

That native function wouldn't need to do careful validation of the JS
because it is our own code calling it, which we can assume is correct.

> - asynchronous messaging with callbacks (addRules needs to "return"
> something to the callback function)
> - association of messages to their extensions?
> - possibly some security features?
>
> And we need to do a lot of plumbing with messages. I preferred this
> approach.

Hm, yeah. I see your point of view. OK.

- a

[Matt Perry, 2012-01-20 00:07:55]

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:71: {'supports_listeners': true,
'supports_rules': false};
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> Nit: don't need the single quotes in the object literal here.

also, use camelCase for dict keys

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:283:
this.getFunctionDefinition_("experimental.declarative", "addRules");
On 2012/01/19 23:45:48, battre wrote:
> On 2012/01/19 23:09:46, Aaron Boodman wrote:
> > I think it might be easier to just use raw IPC here. Define a set of native
> > functions like addRule/removeRule/etc, and then define IPC messages that get
> > picked up in the browser and send to the right registry.
> > 
> > I don't think we get that much by reusing ExtensionFunctionDispatcher.
> > 
> > Matt, what do you think?
> 
> I started off with this approach. Here is the deal:
> If you register rules, these need to be sent to the RuleRegistry, which lives
in
> the Browser, they need to be validated and a response needs to be generated.
If
> we use native functions we are replicating everything that is already
available
> to sendRequest:
> - validation of arguments
> - asynchronous messaging with callbacks (addRules needs to "return" something
to
> the callback function)
> - association of messages to their extensions?
> - possibly some security features?
> 
> And we need to do a lot of plumbing with messages. I preferred this approach.

This seems reasonable to me. No sense rolling custom IPC messages when
sendRequest does what we need.

I admit that I'm kind of confused on how you're going about this, though. The
getFunctionDefinition_ followed by sendRequest_ seems complex and hard to
understand. Why doesn't the standard usage of sendRequest work here?

[battre, 2012-01-24 20:36:15]

I think this is a sensible size for the first CL to be reviewed (and hopefully
landed).

Unit tests will follow as soon as we have 
- a RulesRegistry interface,
- the dispatcher implementation,
- a test implementation of the RulesRegistry
- registration of the test implementation

It would probably double the size of this CL and small CLs are beautiful. :-)

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:71: {'supports_listeners': true,
'supports_rules': false};
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> Nit: don't need the single quotes in the object literal here.

Done.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:71: {'supports_listeners': true,
'supports_rules': false};
On 2012/01/20 00:07:55, Matt Perry wrote:
> On 2012/01/19 23:09:46, Aaron Boodman wrote:
> > Nit: don't need the single quotes in the object literal here.
> 
> also, use camelCase for dict keys

Done.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:176:
chrome.Event.prototype.hasListeners = function(cb) {
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> whoops: the cb param here not required.

Done.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:264: if
(!this.eventOptions_.supports_rules)
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> nit: maybe just put supports_rules_ on |this|?

hm... I think having this as a separate dictionary is not a bad idea because
that way we can document what we expect in the extensions_api.json file.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:265: throw new Error("This event
does not support listeners.");
On 2012/01/19 23:09:46, Aaron Boodman wrote:
> s/listener/rule/

Done.

https://chromiumcodereview.appspot.com/9192029/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:283:
this.getFunctionDefinition_("experimental.declarative", "addRules");
On 2012/01/20 00:07:55, Matt Perry wrote:
> On 2012/01/19 23:45:48, battre wrote:
> > On 2012/01/19 23:09:46, Aaron Boodman wrote:
> > > I think it might be easier to just use raw IPC here. Define a set of
native
> > > functions like addRule/removeRule/etc, and then define IPC messages that
get
> > > picked up in the browser and send to the right registry.
> > > 
> > > I don't think we get that much by reusing ExtensionFunctionDispatcher.
> > > 
> > > Matt, what do you think?
> > 
> > I started off with this approach. Here is the deal:
> > If you register rules, these need to be sent to the RuleRegistry, which
lives
> in
> > the Browser, they need to be validated and a response needs to be generated.
> If
> > we use native functions we are replicating everything that is already
> available
> > to sendRequest:
> > - validation of arguments
> > - asynchronous messaging with callbacks (addRules needs to "return"
something
> to
> > the callback function)
> > - association of messages to their extensions?
> > - possibly some security features?
> > 
> > And we need to do a lot of plumbing with messages. I preferred this
approach.
> 
> This seems reasonable to me. No sense rolling custom IPC messages when
> sendRequest does what we need.
> 
> I admit that I'm kind of confused on how you're going about this, though. The
> getFunctionDefinition_ followed by sendRequest_ seems complex and hard to
> understand. Why doesn't the standard usage of sendRequest work here?

We don't have access to these functions, they are implementation details of a
(function() {})() block in schema_generated_bindings.js

[Matt Perry, 2012-01-24 22:39:10]

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/rules_registry_dispatcher.cc
(right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/browser/exten...
chrome/browser/extensions/api/declarative/rules_registry_dispatcher.cc:16:
result_.reset(new ListValue());
Why do you even need to return anything?

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/common/extens...
File chrome/common/extensions/api/experimental.declarative.json (right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/common/extens...
chrome/common/extensions/api/experimental.declarative.json:48: "description":
"Rules to be registered. These does not replace previously registered rules."
"These do not"

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/common/extens...
chrome/common/extensions/api/experimental.declarative.json:77: "name":
"rule_identifiers",
camelCase

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/common/extens...
File chrome/common/extensions/api/webRequest.json (right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/common/extens...
chrome/common/extensions/api/webRequest.json:89: "filter_type": { "type":
"string", "enum": ["net.RequestMatcher"] }
camelCase

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/event.js:12: //native function
RemoveRules(rules);
dead code

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/event.js:98: this.last_generated_rule_id_ =
0;
camelCase

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/event.js:338:
console.log(JSON.stringify(functionDef));
remove debug code?

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
If we need to expose these identifiers to other modules, lets just put them on
chromeHidden.

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:34:
{"supportsListeners": true, "supportsRules": false};
indent 2 more

https://chromiumcodereview.appspot.com/9192029/diff/9005/chrome/renderer/reso...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:136: throw
new Error("This event does not support listeners.");
s/listeners/rules

[battre, 2012-01-25 19:25:08]

Thanks for the review.

It took me a year to get from camelCase to leet_hacker_style. It will probably
take me another year to remember that JavaScript wants camelCase. ;-)

http://codereview.chromium.org/9192029/diff/9005/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/rules_registry_dispatcher.cc
(right):

http://codereview.chromium.org/9192029/diff/9005/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/rules_registry_dispatcher.cc:16:
result_.reset(new ListValue());
On 2012/01/24 22:39:10, Matt Perry wrote:
> Why do you even need to return anything?

Due to the validation check according to the API defintion.

http://codereview.chromium.org/9192029/diff/9005/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.declarative.json (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.declarative.json:48: "description":
"Rules to be registered. These does not replace previously registered rules."
On 2012/01/24 22:39:10, Matt Perry wrote:
> "These do not"

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.declarative.json:77: "name":
"rule_identifiers",
On 2012/01/24 22:39:10, Matt Perry wrote:
> camelCase

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/common/extensions/api...
File chrome/common/extensions/api/webRequest.json (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/common/extensions/api...
chrome/common/extensions/api/webRequest.json:89: "filter_type": { "type":
"string", "enum": ["net.RequestMatcher"] }
On 2012/01/24 22:39:10, Matt Perry wrote:
> camelCase

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/event.js:12: //native function
RemoveRules(rules);
On 2012/01/24 22:39:10, Matt Perry wrote:
> dead code

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/event.js:98: this.last_generated_rule_id_ =
0;
On 2012/01/24 22:39:10, Matt Perry wrote:
> camelCase

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/event.js:338:
console.log(JSON.stringify(functionDef));
On 2012/01/24 22:39:10, Matt Perry wrote:
> remove debug code?

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
On 2012/01/24 22:39:10, Matt Perry wrote:
> If we need to expose these identifiers to other modules, lets just put them on
> chromeHidden.

That would be a bigger change because the same mechanism is used for all custom
bindings. I have renamed the dictionary to bindingsAPI, though, to be more in
line with the rest of the code.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:34:
{"supportsListeners": true, "supportsRules": false};
On 2012/01/24 22:39:10, Matt Perry wrote:
> indent 2 more

Done.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:136: throw
new Error("This event does not support listeners.");
On 2012/01/24 22:39:10, Matt Perry wrote:
> s/listeners/rules

Done.

[Matt Perry, 2012-01-25 22:29:35]

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
On 2012/01/25 19:25:08, battre wrote:
> On 2012/01/24 22:39:10, Matt Perry wrote:
> > If we need to expose these identifiers to other modules, lets just put them
on
> > chromeHidden.
> 
> That would be a bigger change because the same mechanism is used for all
custom
> bindings.

I don't understand. sendRequest and apiDefinitions are essentially globals. Why
can't we just do:

chromeHidden.sendRequest = sendRequest;
chromeHidden.apiDefinitions = apiDefinitions

> I have renamed the dictionary to bindingsAPI, though, to be more in
> line with the rest of the code.

[battre, 2012-01-27 16:49:32]

On 2012/01/25 22:29:35, Matt Perry wrote:
>
http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
> File chrome/renderer/resources/extensions/schema_generated_bindings.js
(right):
> 
>
http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
> chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
> On 2012/01/25 19:25:08, battre wrote:
> > On 2012/01/24 22:39:10, Matt Perry wrote:
> > > If we need to expose these identifiers to other modules, lets just put
them
> on
> > > chromeHidden.
> > 
> > That would be a bigger change because the same mechanism is used for all
> custom
> > bindings.
> 
> I don't understand. sendRequest and apiDefinitions are essentially globals.
Why
> can't we just do:
> 
> chromeHidden.sendRequest = sendRequest;
> chromeHidden.apiDefinitions = apiDefinitions
> 
> > I have renamed the dictionary to bindingsAPI, though, to be more in
> > line with the rest of the code.

Done (I copied GetExtensionAPIDefinitions() into chromeHidden)

[Matt Perry, 2012-01-27 19:27:14]

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:361: var
apiDefinitions = GetExtensionAPIDefinition();
Just put the result from here on chromeHidden, rather than the function. If we
call the function each time, we're making a copy of the object for each
invocation.

[battre, 2012-01-27 19:42:18]

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:361: var
apiDefinitions = GetExtensionAPIDefinition();
On 2012/01/27 19:27:15, Matt Perry wrote:
> Just put the result from here on chromeHidden, rather than the function. If we
> call the function each time, we're making a copy of the object for each
> invocation.

Done.

[Matt Perry, 2012-01-27 19:44:36]

lgtm

[Aaron Boodman, 2012-01-31 00:11:17]

Sorry for the late review here.

I'm adding Ben and James here. Since they've been doing a lot of refactoring in
here, I want to make sure we don't start moving things in a different direction
than they were planning.

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
On 2012/01/25 22:29:35, Matt Perry wrote:
> On 2012/01/25 19:25:08, battre wrote:
> > On 2012/01/24 22:39:10, Matt Perry wrote:
> > > If we need to expose these identifiers to other modules, lets just put
them
> on
> > > chromeHidden.
> > 
> > That would be a bigger change because the same mechanism is used for all
> custom
> > bindings.
> 
> I don't understand. sendRequest and apiDefinitions are essentially globals.
Why
> can't we just do:
> 
> chromeHidden.sendRequest = sendRequest;
> chromeHidden.apiDefinitions = apiDefinitions
> 
> > I have renamed the dictionary to bindingsAPI, though, to be more in
> > line with the rest of the code.
> 

I kinda preferred it the way Dominic had it. Ben and James are in the process of
untangling this, and I think they want to move even more in the direction of
having explicit dependencies (they eventually want to be able to unit test these
js files outside of the v8::Extension system).

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/21001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:361: var
apiDefinitions = GetExtensionAPIDefinition();
On 2012/01/27 19:42:18, battre wrote:
> On 2012/01/27 19:27:15, Matt Perry wrote:
> > Just put the result from here on chromeHidden, rather than the function. If
we
> > call the function each time, we're making a copy of the object for each
> > invocation.
> 
> Done.

That isn't completely true. A new object is created each time, but only the top
level. The child objects are all cached across invocations of this function.

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:16:
result_.reset(new ListValue());
Nit: No need to return anything in that case then? (go easy on the IPC system
:-)).

http://codereview.chromium.org/9192029/diff/19018/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/19018/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:316:
this.getFunctionDefinition_("experimental.declarative", "addRules");
This seems like something sendRequest (or some variant of sendRequest) should
just know how to do.

It shouldn't be the caller's responsibility to dig around and find the schema
for the specified function.

[not at google - send to devlin, 2012-01-31 11:43:43]

Sorry I didn't get time to look at this today, but I'd like to tomorrow (as in
Sydney time, so I'll start after lunch your time).

Can you point me at a bit of context for the patch?

[battre, 2012-01-31 11:46:07]

On 2012/01/31 11:43:43, kalman wrote:
> Sorry I didn't get time to look at this today, but I'd like to tomorrow (as in
> Sydney time, so I'll start after lunch your time).
> 
> Can you point me at a bit of context for the patch?

Please see http://goto.google.com/xvvyv for a design document.

[battre, 2012-01-31 12:28:50]

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/9005/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/schema_generated_bindings.js:461: };
On 2012/01/31 00:11:17, Aaron Boodman wrote:
> On 2012/01/25 22:29:35, Matt Perry wrote:
> > On 2012/01/25 19:25:08, battre wrote:
> > > On 2012/01/24 22:39:10, Matt Perry wrote:
> > > > If we need to expose these identifiers to other modules, lets just put
> them
> > on
> > > > chromeHidden.
> > > 
> > > That would be a bigger change because the same mechanism is used for all
> > custom
> > > bindings.
> > 
> > I don't understand. sendRequest and apiDefinitions are essentially globals.
> Why
> > can't we just do:
> > 
> > chromeHidden.sendRequest = sendRequest;
> > chromeHidden.apiDefinitions = apiDefinitions
> > 
> > > I have renamed the dictionary to bindingsAPI, though, to be more in
> > > line with the rest of the code.
> > 
> 
> I kinda preferred it the way Dominic had it. Ben and James are in the process
of
> untangling this, and I think they want to move even more in the direction of
> having explicit dependencies (they eventually want to be able to unit test
these
> js files outside of the v8::Extension system).

Ok, I will wait for Ben's and James' call.

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:16:
result_.reset(new ListValue());
On 2012/01/31 00:11:17, Aaron Boodman wrote:
> Nit: No need to return anything in that case then? (go easy on the IPC system
> :-)).

If we don't return an empty list here, we get the following DCHECK:


[7123:7123:617285019860:FATAL:schema_generated_bindings.cc(392)] Check failed:
false. Callback validation error during experimental.declarative.addRules --
Error: Parameter 1 is required.
    at Object.validate (extensions/schema_generated_bindings.js:65:15)
    at Object.<anonymous> (extensions/schema_generated_bindings.js:110:26)
Backtrace:
        base::debug::StackTrace::StackTrace() [0x7ffc0c1cc63a]
        logging::LogMessage::~LogMessage() [0x7ffc0c1fb3a4]
        extensions::SchemaGeneratedBindings::HandleResponse() [0x7ffc10da1a8a]
        ExtensionHelper::OnExtensionResponse() [0x7ffc10d94ea1]
        DispatchToMethod<>() [0x7ffc10d96ec8]
        ExtensionMsg_Response::Dispatch<>() [0x7ffc10d962e1]
        ExtensionHelper::OnMessageReceived() [0x7ffc10d946b0]
        RenderViewImpl::OnMessageReceived() [0x7ffc0ef503a1]
        MessageRouter::RouteMessage() [0x7ffc0ee2037c]
        MessageRouter::OnMessageReceived() [0x7ffc0ee2031e]
        ChildThread::OnMessageReceived() [0x7ffc0ed0de71]
        IPC::ChannelProxy::Context::OnDispatchMessage() [0x7ffc0cfce7c4]
        base::internal::RunnableAdapter<>::Run() [0x7ffc0cfd1cef]
        base::internal::InvokeHelper<>::MakeItSo() [0x7ffc0cfd18b5]
        base::internal::Invoker<>::Run() [0x7ffc0cfd125b]
        base::Callback<>::Run() [0x7ffc0c1c491f]
        MessageLoop::RunTask() [0x7ffc0c200ef8]
        MessageLoop::DeferOrRunPendingTask() [0x7ffc0c20100f]
        MessageLoop::DoWork() [0x7ffc0c201831]
        base::MessagePumpDefault::Run() [0x7ffc0c209c20]
        MessageLoop::RunInternal() [0x7ffc0c200bb3]
        MessageLoop::RunHandler() [0x7ffc0c200a66]
        MessageLoop::Run() [0x7ffc0c20039b]
        RendererMain() [0x7ffc0ef933b9]
        (anonymous namespace)::RunZygote() [0x7ffc0e9eb728]
        (anonymous namespace)::RunNamedProcessTypeMain() [0x7ffc0e9eb8fd]
        content::ContentMain() [0x7ffc0e9ebe92]
        ChromeMain [0x7ffc10cb9e1d]
        main [0x7ffc10cb9ddc]
        0x7ffc00609c4d
        0x7ffc10cb9ce9

We may actually change this in the light of Matt's new event pages. I suppose we
don't want to store any state in the events on the JavaScript side if we allow
killing the renderer.

http://codereview.chromium.org/9192029/diff/19018/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/19018/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:316:
this.getFunctionDefinition_("experimental.declarative", "addRules");
On 2012/01/31 00:11:17, Aaron Boodman wrote:
> This seems like something sendRequest (or some variant of sendRequest) should
> just know how to do.
> 
> It shouldn't be the caller's responsibility to dig around and find the schema
> for the specified function.

Done.

[Aaron Boodman, 2012-02-01 00:27:25]

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/19018/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:16:
result_.reset(new ListValue());
On 2012/01/31 12:28:50, battre wrote:
> On 2012/01/31 00:11:17, Aaron Boodman wrote:
> > Nit: No need to return anything in that case then? (go easy on the IPC
system
> > :-)).
> 
> If we don't return an empty list here, we get the following DCHECK:
> 

Ah. OK.

> We may actually change this in the light of Matt's new event pages. I suppose
we
> don't want to store any state in the events on the JavaScript side if we allow
> killing the renderer.

The extension will still be allowed to get the result of these calls. It might
do something like store it, before its event page gets killed.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:196:
chromeHidden.validatingSendRequest = function(qualifiedFunctionName, args) {
I don't think this really validates. sendRequest doesn't normally do the
validation, it is the code that calls sendRequest. The only thing sendRequest
does with the schemas is call prepareRequest, which has special handling for the
callback.

Do you need validation to happen here? If so, a bit more refactoring is needed.
Otherwise, change the name of this function (or just make sendRequest do the
lookup if schemas are undefined).

[not at google - send to devlin, 2012-02-01 00:47:57]

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:196:
chromeHidden.validatingSendRequest = function(qualifiedFunctionName, args) {
On 2012/02/01 00:27:25, Aaron Boodman wrote:
> I don't think this really validates. sendRequest doesn't normally do the
> validation, it is the code that calls sendRequest. The only thing sendRequest
> does with the schemas is call prepareRequest, which has special handling for
the
> callback.
> 
> Do you need validation to happen here? If so, a bit more refactoring is
needed.
> Otherwise, change the name of this function (or just make sendRequest do the
> lookup if schemas are undefined).

I think we should get sendRequest to do the lookup itself, so a bit of
refactoring.  I'm looking at where sendRequest is called though (as in, existing
code), and that code seems to
 1. pull the schema definition of out of "this.definition"
 2. pass it to sendRequest
so basically, it uses "this.description" unnecessarily, not to mention the
wtfness of using "this" at all.  I think it would be fairly straightfoward for
sendRequest to pull out the schema information more or less how you've done it
for validatingSendRequest.

Anyway -- koz and I are going through this change in more detail (trying to
understand it etc), but just commenting on this now since aa's comment showed up
in my inbox.

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

Hm, that's a pretty epic code review there. Sorry :(

A couple of other questions:

1. Perhaps you should add a simple browser test at this point which exercises
some of these code paths?

2. How soon will you be writing the C++/browser side of this (perhaps you've
already started)? I ask because Chris (cc'ed) is currently writing a json_schema
-> C++ model compiler for use with extensions. I think this would be a great
time to start using it with a new API. See c/b/e/api/permissions_api.cc for an
example, and tools/json_schema_compiler for the code itself. It doesn't quite
support the declarative.json file (it doesn't support the "any" type), but we
can get it working soon if there's enough time.

http://codereview.chromium.org/9192029/diff/33001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:16:
result_.reset(new ListValue());
Why does the callback from addRules need to contain the rules it was passed,
anyway? I would have thought that it would be unusual for an extension to want
to know the rules, since it was the one which passed them in in the first place.
If the extension wants to know the rules, it should hold onto a reference to
them, as in,

var rules = [ ... ];
chrome.blah.someEvent.addRules(rules);

That would mean not needing to do new ListValue() here, and a simpler API
overall.

One reason I can see for passing the rules back through is if all state is lost,
for Matt's event pages. As I understand, "rules" would be lost if it's stored as
a local variable. In this case though, it would also be lost if kept as state of
the Event; so it should be the Browser which sends back the list of rules.

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.declarative.json (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.declarative.json:3: "namespace":
"experimental.declarative",
Should this have nodoc:true ?

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.declarative.json:56: "name": "rules",
See previous comment on why this might not be necessary.

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
File chrome/common/extensions/api/webRequest.json (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/webRequest.json:555: "name": "onRequest",
should be nodoc=true?

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:64: chrome.Event =
function(opt_eventName, opt_argSchemas, opt_eventOptions) {
eventCapabilities might be a better name?

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:198: if (this.validate_) {
now that there's always set a validate_ method, this check is unnecessary

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:284:
chrome.Event.prototype.addMissingIds_ = function(rules) {
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];
  }); 
});

}());

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:307:
chrome.Event.prototype.addRules = function(rules, opt_cb) {
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.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:319:
allEventsWithRules.push(this);
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;

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:321: this.validateRules_(rules,
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?

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:328: return
chromeHidden.validatingSendRequest.call(
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.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:367: var event =
allAttachedEvents[i];
I.e. with the above suggestion, this would be

var event = ...;
if (event.hasRules_)
  event.removeRules(?);

(I'm puzzled as to why this is necessary; it seems odd to me that events/rules
are unloaded by the renderers rather than the browser or whatever figuring that
out by itself. But whatever, I'm presumably missing a lot of prior context).

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:374: event.removeRules([]);
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.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:189: var
filterFunctionName = function (val) {return val.name === functionName;};
This is expensive, O(number of APIs * number of functions per API) each time
validatingSendRequest is called.

I like Aaron's suggestion in sendRequest. That is, check if argSchemas is
defined, if not then populate it like you've done here.

However... you'd need to transform the API definitions into a more useful form,
that doesn't involve a linear lookup each time. (like being able to do
schemas[namespace].functions[functionName] for this).

For now, I'd just cache the functionName->schema mapping in sendRequest, and put
that schema transformation as a TODO. It's part of a larger refactor related to
moving all the schemas from using lists-with-ids to using dictionaries... which
I will get around to doing, eventually.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:376:
chromeHidden.apiDefinitions = apiDefinitions;
When the logic from event.js is moved into
experimental.declarative_custom_bindings.js, you won't have to put
apiDefinitions or validatingSendRequest/sendRequest on chromeHidden.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:135: throw
new Error("This event does not support rules.");
I think a more self-documenting check would be

if (!this.eventForRules_)
  throw new Error("   ");

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

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:260: "choices":
typesList.forEach(function(el) {return {"$ref": el};})
What does this do? I thought forEach has no return value?

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:321: this.validateRules_(rules,
On 2012/02/01 07:23:12, kalman wrote:
> 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?


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:38]

Thanks for the review. I think this CL got significantly cleaner.

The browser tests require some additional C++ code and can be found in
http://codereview.chromium.org/9315010/.

http://codereview.chromium.org/9192029/diff/33001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:16:
result_.reset(new ListValue());
On 2012/02/01 07:23:12, kalman wrote:
> Why does the callback from addRules need to contain the rules it was passed,
> anyway? I would have thought that it would be unusual for an extension to want
> to know the rules, since it was the one which passed them in in the first
place.
> If the extension wants to know the rules, it should hold onto a reference to
> them, as in,
> 
> var rules = [ ... ];
> chrome.blah.someEvent.addRules(rules);
> 
> That would mean not needing to do new ListValue() here, and a simpler API
> overall.
> 
> One reason I can see for passing the rules back through is if all state is
lost,
> for Matt's event pages. As I understand, "rules" would be lost if it's stored
as
> a local variable. In this case though, it would also be lost if kept as state
of
> the Event; so it should be the Browser which sends back the list of rules.

The reasoning here was that rules are modified. The rules passed by the
extension don't need to contain identifiers and priorities. By the time of the
callback, these values are filled.

I am not completely convinced that the the strategy that the rules passed to
addRules() should require to have the same identity as the ones passed to the
callback. I think, the following would be sensible.

extension calls .addRules(rules)
--> C++ implementation validates rules and fills optional fields (such as rule
ids)
--> C++ passes new Dictionaries back to extension and calls the callback
function with this.

The reason I consider this better is
- With GetRules() we return new rules objects anyway
- The Extension background page can be killed and reopened with the new Event
pages mechanism, yet we need to be able to track which IDs have been used
already. We don't want to use a native function to generate unique rule IDs
because an extension might register 10.000 rules at a time and we don't want to
execute 10.000 IPC round trips for this.

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.declarative.json (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.declarative.json:3: "namespace":
"experimental.declarative",
On 2012/02/01 07:23:12, kalman wrote:
> Should this have nodoc:true ?

Done.

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.declarative.json:56: "name": "rules",
On 2012/02/01 07:23:12, kalman wrote:
> See previous comment on why this might not be necessary.

See my answer to previous comment.

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
File chrome/common/extensions/api/webRequest.json (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/common/extensions/ap...
chrome/common/extensions/api/webRequest.json:555: "name": "onRequest",
On 2012/02/01 07:23:12, kalman wrote:
> should be nodoc=true?

Done.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:64: chrome.Event =
function(opt_eventName, opt_argSchemas, opt_eventOptions) {
On 2012/02/01 07:23:12, kalman wrote:
> eventCapabilities might be a better name?

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"?

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:260: "choices":
typesList.forEach(function(el) {return {"$ref": el};})
On 2012/02/01 08:15:57, kalman wrote:
> What does this do? I thought forEach has no return value?

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

Done.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:284:
chrome.Event.prototype.addMissingIds_ = function(rules) {
On 2012/02/01 07:23:12, kalman wrote:
> 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];
>   }); 
> });
> 
> }());

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.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:307:
chrome.Event.prototype.addRules = function(rules, opt_cb) {
On 2012/02/01 07:23:12, kalman wrote:
> 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.

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

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:319:
allEventsWithRules.push(this);
On 2012/02/01 07:23:12, kalman wrote:
> 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;

I have removed this logic, see below.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:321: this.validateRules_(rules,
On 2012/02/01 08:15:57, kalman wrote:
> On 2012/02/01 07:23:12, kalman wrote:
> > 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?
> 
> 
> 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.
>   

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.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:328: return
chromeHidden.validatingSendRequest.call(
On 2012/02/01 07:23:12, kalman wrote:
> 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.

Wow, this became significantly simpler.

Done.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:367: var event =
allAttachedEvents[i];
On 2012/02/01 07:23:12, kalman wrote:
> I.e. with the above suggestion, this would be
> 
> var event = ...;
> if (event.hasRules_)
>   event.removeRules(?);
> 
> (I'm puzzled as to why this is necessary; it seems odd to me that events/rules
> are unloaded by the renderers rather than the browser or whatever figuring
that
> out by itself. But whatever, I'm presumably missing a lot of prior context).

Solved, see below.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:374: event.removeRules([]);
On 2012/02/01 07:23:12, kalman wrote:
> 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.

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

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:189: var
filterFunctionName = function (val) {return val.name === functionName;};
On 2012/02/01 07:23:12, kalman wrote:
> This is expensive, O(number of APIs * number of functions per API) each time
> validatingSendRequest is called.
> 
> I like Aaron's suggestion in sendRequest. That is, check if argSchemas is
> defined, if not then populate it like you've done here.
> 
> However... you'd need to transform the API definitions into a more useful
form,
> that doesn't involve a linear lookup each time. (like being able to do
> schemas[namespace].functions[functionName] for this).
> 
> For now, I'd just cache the functionName->schema mapping in sendRequest, and
put
> that schema transformation as a TODO. It's part of a larger refactor related
to
> moving all the schemas from using lists-with-ids to using dictionaries...
which
> I will get around to doing, eventually.

I have moved this logic into the custom bindings. Please have a look. I think
this is now sufficiently fast and simple.

Done.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:196:
chromeHidden.validatingSendRequest = function(qualifiedFunctionName, args) {
On 2012/02/01 00:47:57, kalman wrote:
> On 2012/02/01 00:27:25, Aaron Boodman wrote:
> > I don't think this really validates. sendRequest doesn't normally do the
> > validation, it is the code that calls sendRequest. The only thing
sendRequest
> > does with the schemas is call prepareRequest, which has special handling for
> the
> > callback.
> > 
> > Do you need validation to happen here? If so, a bit more refactoring is
> needed.
> > Otherwise, change the name of this function (or just make sendRequest do the
> > lookup if schemas are undefined).
> 
> I think we should get sendRequest to do the lookup itself, so a bit of
> refactoring.  I'm looking at where sendRequest is called though (as in,
existing
> code), and that code seems to
>  1. pull the schema definition of out of "this.definition"
>  2. pass it to sendRequest
> so basically, it uses "this.description" unnecessarily, not to mention the
> wtfness of using "this" at all.  I think it would be fairly straightfoward for
> sendRequest to pull out the schema information more or less how you've done it
> for validatingSendRequest.
> 
> Anyway -- koz and I are going through this change in more detail (trying to
> understand it etc), but just commenting on this now since aa's comment showed
up
> in my inbox.

Please have a look at the new implementation. This function is gone now.

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

LGTM (after those last comments have been addressed)

Looks really nice, thanks for clarifying those things.

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:64: chrome.Event =
function(opt_eventName, opt_argSchemas, opt_eventOptions) {
On 2012/02/01 17:35:39, battre wrote:
> On 2012/02/01 07:23:12, kalman wrote:
> > eventCapabilities might be a better name?
> 
> 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"?

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

http://codereview.chromium.org/9192029/diff/33001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:307:
chrome.Event.prototype.addRules = function(rules, opt_cb) {
On 2012/02/01 17:35:39, battre wrote:
> On 2012/02/01 07:23:12, kalman wrote:
> > 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.
> 
> Isn't it a pretty thin wrapper? Which part would you like to get rid of?

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.

http://codereview.chromium.org/9192029/diff/33005/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/33005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:14: ListValue*
rules_list;
= NULL

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:248:
chrome.experimental.declarative.addRules(this.eventName_, rules, opt_cb);
You'll still need to check that the experimental.declarative API exists.

This might be most easily done in the constructor, like

if (this.eventOptions_.supportsRules &&
    (!chrome.experimental || !chrome.experimental.declarative)) {
  throw new Error("You must have access to the experimental.declarative " +
                  "API to support rules in events");
}

Possibly when this API gets taken out of experimental the presence of events
with rules will imply permission to use the API? Or possibly not? Not a decision
that needs to be made right now though, extensions will definitely need the
experimental permission to use this.

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js
(right):

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js:15:
function(bindingsAPI) {
single line for all of this?

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js:61:
throw new Error("Error in API specification.");
it might be nice to have a slightly more helpful (i.e. contextual) error
message.

[battre, 2012-02-02 13:49:59]

Thanks again for the review. I will wait for a final approval by Matt and commit
then.

http://codereview.chromium.org/9192029/diff/33005/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9192029/diff/33005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/declarative_api.cc:14: ListValue*
rules_list;
On 2012/02/01 23:07:23, kalman wrote:
> = NULL

Done.

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:248:
chrome.experimental.declarative.addRules(this.eventName_, rules, opt_cb);
On 2012/02/01 23:07:23, kalman wrote:
> You'll still need to check that the experimental.declarative API exists.
> 
> This might be most easily done in the constructor, like
> 
> if (this.eventOptions_.supportsRules &&
>     (!chrome.experimental || !chrome.experimental.declarative)) {
>   throw new Error("You must have access to the experimental.declarative " +
>                   "API to support rules in events");
> }
> 
> Possibly when this API gets taken out of experimental the presence of events
> with rules will imply permission to use the API? Or possibly not? Not a
decision
> that needs to be made right now though, extensions will definitely need the
> experimental permission to use this.

Anticipating that experimental.declarative will become "declarative" and that
you don't need to request permission for "declarative" I have split this into
two if statements.

Done.

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js
(right):

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js:15:
function(bindingsAPI) {
On 2012/02/01 23:07:23, kalman wrote:
> single line for all of this?

Does not fit (3 characters too much). I have left it as is. We can move
everything into one line once the "experimental." is gone.

http://codereview.chromium.org/9192029/diff/33005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.declarative_custom_bindings.js:61:
throw new Error("Error in API specification.");
On 2012/02/01 23:07:23, kalman wrote:
> it might be nice to have a slightly more helpful (i.e. contextual) error
> message.

Done.

[Matt Perry, 2012-02-06 21:07:13]

lgtm with 1 potential fix

http://codereview.chromium.org/9192029/diff/43001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9192029/diff/43001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:65: this.eventName_ =
opt_eventName;
It seems like eventName is not optional in the case of a rule-based event. If
that's the case, you should enforce that here.

[commit-bot: I haz the power, 2012-02-07 20:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9192029/50002

[commit-bot: I haz the power, 2012-02-07 20:07:37]

Can't process patch for file
chrome/renderer/resources/extensions/web_request_custom_bindings.js.
File's status is None, patchset upload is incomplete.