Move declarative API into events API

Move declarative API into events API

The motivation of this CL is to bring the declarative API and the rest of the
events mechanism closer together in the API documentation. To achive this, we
move the content of declarative.json and everything related to events from
extension.json into the events.json API definition. The new "events" namespace
is automatically provided to all extensions.


BUG=126497, 112155
TEST=no

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

[battre, 2012-05-09 16:28:07]

Hi.

Could you please review this?

Ben: for the context see http://crbug.com/126497

Thanks,
Dominic

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/declarative/declarative_api.h (left):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/declarative/declarative_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_API_H__
I suppose it would be a god idea to move all files from c/b/e/a/declarative to
c/b/e/a/events. I will do that in a separate CL to simplify this codereview.

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/_permission_features.json (left):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
chrome/common/extensions/api/_permission_features.json:85: "extension_types":
["extension", "packaged_app"]
I did not introduce a new feature "events". I am not sure whether this is
necessary. I introduce an "events" permission but grant it to all extensions. If
this should contain "events", what would the "extension_types" be?

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/events.json (right):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
chrome/common/extensions/api/events.json:9: "unprivileged": true,
Is this ok? Otherwise my tests fail:

from chrome/test/data/extensions/api_test/webrequest/test_blocking.js

    navigateAndWait(getURLEchoUserAgent(), function() {
      chrome.test.listenOnce(chrome.extension.onRequest, function(request) {
        chrome.test.assertTrue(request.pass, "Request header was not set.");
      });
      chrome.tabs.executeScript(tabId,
        {
          code: "chrome.extension.sendRequest(" +
              "{pass: document.body.innerText.indexOf('FoobarUA') >= 0});"
        });
    });

The code is executed in an unprivileged context. If you look at extension.json,
you see that I have introduced a dependency to events.json. This dependency
cannot be fulfilled, if events.json is not unprivileged.

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/extension.json (left):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
chrome/common/extensions/api/extension.json:21: "id": "Event",
this went into events.json.

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/static/events.html (right):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/static/events.html:49: 
I have mostly just moved the text. It will need to be adapted to the new
context. I would do this in a separate CL.

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:44: value +
"'");
TODO(battre): update error message.

https://chromiumcodereview.appspot.com/10392008/diff/1/tools/json_schema_comp...
File tools/json_schema_compiler/cc_generator.py (left):

https://chromiumcodereview.appspot.com/10392008/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:107: '\nCannot generate both
functions and properties on a type')
The files generated after deleting this exception are sound. You cannot
instantiate the objects in C++ but you get proper function definitions, which
are helpful.

[battre, 2012-05-09 17:54:19]

You can find the generated documentation here:
http://www/~battre/no_crawl/docs2/events.html

On 2012/05/09 16:28:07, battre wrote:
> Hi.
> 
> Could you please review this?
> 
> Ben: for the context see http://crbug.com/126497
> 
> Thanks,
> Dominic
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/browser/extensi...
> File chrome/browser/extensions/api/declarative/declarative_api.h (left):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/browser/extensi...
> chrome/browser/extensions/api/declarative/declarative_api.h:5: #ifndef
> CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_API_H__
> I suppose it would be a god idea to move all files from c/b/e/a/declarative to
> c/b/e/a/events. I will do that in a separate CL to simplify this codereview.
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> File chrome/common/extensions/api/_permission_features.json (left):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> chrome/common/extensions/api/_permission_features.json:85: "extension_types":
> ["extension", "packaged_app"]
> I did not introduce a new feature "events". I am not sure whether this is
> necessary. I introduce an "events" permission but grant it to all extensions.
If
> this should contain "events", what would the "extension_types" be?
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> File chrome/common/extensions/api/events.json (right):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> chrome/common/extensions/api/events.json:9: "unprivileged": true,
> Is this ok? Otherwise my tests fail:
> 
> from chrome/test/data/extensions/api_test/webrequest/test_blocking.js
> 
>     navigateAndWait(getURLEchoUserAgent(), function() {
>       chrome.test.listenOnce(chrome.extension.onRequest, function(request) {
>         chrome.test.assertTrue(request.pass, "Request header was not set.");
>       });
>       chrome.tabs.executeScript(tabId,
>         {
>           code: "chrome.extension.sendRequest(" +
>               "{pass: document.body.innerText.indexOf('FoobarUA') >= 0});"
>         });
>     });
> 
> The code is executed in an unprivileged context. If you look at
extension.json,
> you see that I have introduced a dependency to events.json. This dependency
> cannot be fulfilled, if events.json is not unprivileged.
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> File chrome/common/extensions/api/extension.json (left):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> chrome/common/extensions/api/extension.json:21: "id": "Event",
> this went into events.json.
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> File chrome/common/extensions/docs/static/events.html (right):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/common/extensio...
> chrome/common/extensions/docs/static/events.html:49: 
> I have mostly just moved the text. It will need to be adapted to the new
> context. I would do this in a separate CL.
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/renderer/resour...
> File chrome/renderer/resources/extensions/schema_generated_bindings.js
(right):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/chrome/renderer/resour...
> chrome/renderer/resources/extensions/schema_generated_bindings.js:44: value +
> "'");
> TODO(battre): update error message.
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/tools/json_schema_comp...
> File tools/json_schema_compiler/cc_generator.py (left):
> 
>
https://chromiumcodereview.appspot.com/10392008/diff/1/tools/json_schema_comp...
> tools/json_schema_compiler/cc_generator.py:107: '\nCannot generate both
> functions and properties on a type')
> The files generated after deleting this exception are sound. You cannot
> instantiate the objects in C++ but you get proper function definitions, which
> are helpful.

[Aaron Boodman, 2012-05-09 19:29:45]

This is really excellent. Thank you for taking the time to figure out how this
should work properly.

I'm going to dig into event.js a little more closely now.

http://codereview.chromium.org/10392008/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/declarative/declarative_api.h (left):

http://codereview.chromium.org/10392008/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative/declarative_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_API_H__
On 2012/05/09 16:28:08, battre wrote:
> I suppose it would be a god idea to move all files from c/b/e/a/declarative to
> c/b/e/a/events. I will do that in a separate CL to simplify this codereview.

OK.

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/_...
File chrome/common/extensions/api/_permission_features.json (left):

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/_...
chrome/common/extensions/api/_permission_features.json:85: "extension_types":
["extension", "packaged_app"]
On 2012/05/09 16:28:08, battre wrote:
> I did not introduce a new feature "events". I am not sure whether this is
> necessary. I introduce an "events" permission but grant it to all extensions.
If
> this should contain "events", what would the "extension_types" be?

No permission is needed. Every extension is going to need "events".

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/events.json (right):

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/events.json:7: "namespace": "events",
This was a nice idea, I really like it.

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/events.json:9: "unprivileged": true,
On 2012/05/09 16:28:08, battre wrote:
> Is this ok? Otherwise my tests fail:
> 
> from chrome/test/data/extensions/api_test/webrequest/test_blocking.js
> 
>     navigateAndWait(getURLEchoUserAgent(), function() {
>       chrome.test.listenOnce(chrome.extension.onRequest, function(request) {
>         chrome.test.assertTrue(request.pass, "Request header was not set.");
>       });
>       chrome.tabs.executeScript(tabId,
>         {
>           code: "chrome.extension.sendRequest(" +
>               "{pass: document.body.innerText.indexOf('FoobarUA') >= 0});"
>         });
>     });
> 
> The code is executed in an unprivileged context. If you look at
extension.json,
> you see that I have introduced a dependency to events.json. This dependency
> cannot be fulfilled, if events.json is not unprivileged.

Yes, it's OK, so long as methods like addRules are no-ops in all but the places
they're used.

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/events.json:22: {
TODO(aa): Limit the rules APIs to privileged context. See crbug.com/120069.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:9: var sendRequest =
require('sendRequest').sendRequest;
TODO(aa): Look at this file more closely.

[not at google - send to devlin, 2012-05-10 09:01:24]

Looks good.

How does this fit into the question you asked over email about putting internal
APIs into a different namespace?  Is the addRules stuff what you were referring
to?

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/events.json (right):

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/events.json:9: "unprivileged": true,
On 2012/05/09 19:29:46, Aaron Boodman wrote:
> On 2012/05/09 16:28:08, battre wrote:
> > Is this ok? Otherwise my tests fail:
> > 
> > from chrome/test/data/extensions/api_test/webrequest/test_blocking.js
> > 
> >     navigateAndWait(getURLEchoUserAgent(), function() {
> >       chrome.test.listenOnce(chrome.extension.onRequest, function(request) {
> >         chrome.test.assertTrue(request.pass, "Request header was not set.");
> >       });
> >       chrome.tabs.executeScript(tabId,
> >         {
> >           code: "chrome.extension.sendRequest(" +
> >               "{pass: document.body.innerText.indexOf('FoobarUA') >= 0});"
> >         });
> >     });
> > 
> > The code is executed in an unprivileged context. If you look at
> extension.json,
> > you see that I have introduced a dependency to events.json. This dependency
> > cannot be fulfilled, if events.json is not unprivileged.
> 
> Yes, it's OK, so long as methods like addRules are no-ops in all but the
places
> they're used.

We have this problem in a couple of other places where unprivileged APIs require
types defined in privileged APIs.  At least, I think we do.

And I think in all cases it's just the types that we depend on.  Unfortunately
there's no convenient way to do that.

Long story short, this is ok I guess though I think you might be able to put
"unprivileged: false" on all the functions?

Even better though... can we just leave Event on chrome.extension and move it
here when we "fix" features?

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/declarative_webrequest_custom_bindings.js
(right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/declarative_webrequest_custom_bindings.js:13:
'namespace', namespace);
nit: arguments should line up, but this could be

var apiSchema =
    chromeHidden...

and then it would fix on 2 lines.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:64: chrome.Event =
function(opt_eventName, opt_argSchemas, opt_eventOptions,
nit: arguments should line up or fix on 1 line :(

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:100: 'id', 'Event');
nit: argument lineup thing (and on the lines below)

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:320: this.addRulesParams_);
did we lose the ability to do chromeHidden.internalAPIs.events.addRules(...)
like before?

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/schema_generated_bindings.js:45: }
hmmmm I want to avoid putting random utilities on chromeHidden if possible.  It
implies an execution order on these custom bindings files rather than
modularity.

So: how about creating a new file like "util.js", put this on that, then
require() it (similar to how we require() sendRequest)

We could also put forEach in there for good measure.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/schema_generated_bindings.js:379:
'namespace', 'events');
nit: arguments 1 line or vertical thing

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:38:
opt_eventsSchema);
nit: 1 line or vertical arguments

(I can see that other lines don't adhere to this... eh.)

http://codereview.chromium.org/10392008/diff/1/tools/json_schema_compiler/cc_...
File tools/json_schema_compiler/cc_generator.py (left):

http://codereview.chromium.org/10392008/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:107: '\nCannot generate both
functions and properties on a type')
On 2012/05/09 16:28:08, battre wrote:
> The files generated after deleting this exception are sound. You cannot
> instantiate the objects in C++ but you get proper function definitions, which
> are helpful.

I don't quite understand what you mean, can you give an example (for my own
peace of mind)?  But cool.

[battre, 2012-05-10 16:38:00]

Addressed comments

[battre, 2012-05-10 16:40:28]

I have addressed your comments. I haven't figured out, yet, how to address the
issue of making the declarative Web Request API functions unprivileged.

Best regards,
Dominic

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/events.json (right):

http://codereview.chromium.org/10392008/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/events.json:9: "unprivileged": true,
On 2012/05/10 09:01:25, kalman wrote:
> On 2012/05/09 19:29:46, Aaron Boodman wrote:
> > On 2012/05/09 16:28:08, battre wrote:
> > > Is this ok? Otherwise my tests fail:
> > > 
> > > from chrome/test/data/extensions/api_test/webrequest/test_blocking.js
> > > 
> > >     navigateAndWait(getURLEchoUserAgent(), function() {
> > >       chrome.test.listenOnce(chrome.extension.onRequest, function(request)
{
> > >         chrome.test.assertTrue(request.pass, "Request header was not
set.");
> > >       });
> > >       chrome.tabs.executeScript(tabId,
> > >         {
> > >           code: "chrome.extension.sendRequest(" +
> > >               "{pass: document.body.innerText.indexOf('FoobarUA') >= 0});"
> > >         });
> > >     });
> > > 
> > > The code is executed in an unprivileged context. If you look at
> > extension.json,
> > > you see that I have introduced a dependency to events.json. This
dependency
> > > cannot be fulfilled, if events.json is not unprivileged.
> > 
> > Yes, it's OK, so long as methods like addRules are no-ops in all but the
> places
> > they're used.
> 
> We have this problem in a couple of other places where unprivileged APIs
require
> types defined in privileged APIs.  At least, I think we do.
> 
> And I think in all cases it's just the types that we depend on.  Unfortunately
> there's no convenient way to do that.
> 
> Long story short, this is ok I guess though I think you might be able to put
> "unprivileged: false" on all the functions?

No, I think that does not work because "unprivileged: false" is only supported
one level under the namespace, i.e. for functions and events. These functions
are defined as part of a data type.

> Even better though... can we just leave Event on chrome.extension and move it
> here when we "fix" features?

No, the entire exercise is to move Event into chrome.events to improve the
documentation.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/declarative_webrequest_custom_bindings.js
(right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/declarative_webrequest_custom_bindings.js:13:
'namespace', namespace);
On 2012/05/10 09:01:25, kalman wrote:
> nit: arguments should line up, but this could be
> 
> var apiSchema =
>     chromeHidden...
> 
> and then it would fix on 2 lines.

Done.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:64: chrome.Event =
function(opt_eventName, opt_argSchemas, opt_eventOptions,
On 2012/05/10 09:01:25, kalman wrote:
> nit: arguments should line up or fix on 1 line :(

According to the JavaScript Style Guide, the rule is:

"When possible, all function arguments should be listed on the same line. If
doing so would exceed the 80-column limit, the arguments must be line-wrapped in
a readable way. To save space, you may wrap as close to 80 as possible, or put
each argument on its own line to enhance readability. The indentation may be
either four spaces, or aligned to the parenthesis. Below are the most common
patterns for argument wrapping"

So it is not necessary that all arguments line up. I have added a line break
after function( to put all of them on one line.

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:100: 'id', 'Event');
On 2012/05/10 09:01:25, kalman wrote:
> nit: argument lineup thing (and on the lines below)

See above

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/event.js:320: this.addRulesParams_);
On 2012/05/10 09:01:25, kalman wrote:
> did we lose the ability to do chromeHidden.internalAPIs.events.addRules(...)
> like before?

Yes, because there is no such function anymore. "events.addRules" is the
function that is described in the "Event" type of events.json as "addRules".

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10392008/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/schema_generated_bindings.js:45: }
On 2012/05/10 09:01:25, kalman wrote:
> hmmmm I want to avoid putting random utilities on chromeHidden if possible. 
It
> implies an execution order on these custom bindings files rather than
> modularity.
> 
> So: how about creating a new file like "util.js", put this on that, then
> require() it (similar to how we require() sendRequest)
> 
> We could also put forEach in there for good measure.

I like that idea very much. Done.

http://codereview.chromium.org/10392008/diff/1/tools/json_schema_compiler/cc_...
File tools/json_schema_compiler/cc_generator.py (left):

http://codereview.chromium.org/10392008/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:107: '\nCannot generate both
functions and properties on a type')
On 2012/05/10 09:01:25, kalman wrote:
> On 2012/05/09 16:28:08, battre wrote:
> > The files generated after deleting this exception are sound. You cannot
> > instantiate the objects in C++ but you get proper function definitions,
which
> > are helpful.
> 
> I don't quite understand what you mean, can you give an example (for my own
> peace of mind)?  But cool.

Look at the events.json of this CL. The Event object is considered to have
functions and properties. By disabling this check, the following code is
generated, which is useful for me:

namespace Event {

  namespace GetRules {
    struct Params {
      // Name of the event this function affects.
      std::string event_name;

      // If an array is passed, only rules with identifiers contained in this
array
      // are returned.
      scoped_ptr<std::vector<std::string> > rule_identifiers;

      ~Params();

      static scoped_ptr<Params> Create(const ListValue& args);

     private:
      Params();

      DISALLOW_COPY_AND_ASSIGN(Params);
    };

    namespace Result {
      // Rules that were registered, the optional parameters are filled with
      // values.
      Value* Create(const std::vector<linked_ptr<Rule> >& rules);
    };

  };

[Aaron Boodman, 2012-05-10 17:14:49]

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/event.js:65: opt_eventName, opt_argSchemas,
opt_eventOptions, opt_eventsSchema) {
The need for the 'event' type's schema is an implementation detail of this
object. Can we just have it do it itself? You could add a method to
ApiDefinitionsNatives to get a particular API definition by name.

Alternately, you could export a function from event.js like setEventSchema()
which schema_generated_bindings.js calls once.

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/event.js:99: var eventType =
lookup(this.eventsSchema_.types, 'id', 'Event');
This lookup is done for every event instance. It would be better to do it once
and cache it.

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/schema_generated_bindings.js:10: var
GetExtensionAPIDefinition =
I would like to propose the following rule:

When importing with require, always reference the imported function with their
module name, so like:

var apiDefinitions = requireNative('apiDefinitions');
var sendRequest = require('sendRequest');
var utils = require('utils');

var definition = apiDefinitions.GetExtensionAPIDefinition(..);

util.forEach(...);

It seems more clear to me to always see these functions prefixed by their module
name. You can see from looking at them that they are an imported function.

[battre, 2012-05-10 18:05:02]

Don't provide scheme to each event individually and create util module

[battre, 2012-05-10 18:05:22]

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/event.js:65: opt_eventName, opt_argSchemas,
opt_eventOptions, opt_eventsSchema) {
On 2012/05/10 17:14:49, Aaron Boodman wrote:
> The need for the 'event' type's schema is an implementation detail of this
> object. Can we just have it do it itself? You could add a method to
> ApiDefinitionsNatives to get a particular API definition by name.

I am just concerned that we might end up with multiple copies of the schema in
memory. 

> Alternately, you could export a function from event.js like setEventSchema()
> which schema_generated_bindings.js calls once.

I prefer this approach.

Done.

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/event.js:99: var eventType =
lookup(this.eventsSchema_.types, 'id', 'Event');
On 2012/05/10 17:14:49, Aaron Boodman wrote:
> This lookup is done for every event instance. It would be better to do it once
> and cache it.

Done.

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/schema_generated_bindings.js:10: var
GetExtensionAPIDefinition =
On 2012/05/10 17:14:49, Aaron Boodman wrote:
> I would like to propose the following rule:
> 
> When importing with require, always reference the imported function with their
> module name, so like:
> 
> var apiDefinitions = requireNative('apiDefinitions');
> var sendRequest = require('sendRequest');
> var utils = require('utils');
> 
> var definition = apiDefinitions.GetExtensionAPIDefinition(..);
> 
> util.forEach(...);
> 
> It seems more clear to me to always see these functions prefixed by their
module
> name. You can see from looking at them that they are an imported function.

I like that.

I have done this for "utils" and added a TODO for the rest to handle that in a
separate CL.

[Aaron Boodman, 2012-05-10 18:27:50]

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/event.js:65: opt_eventName, opt_argSchemas,
opt_eventOptions, opt_eventsSchema) {
On 2012/05/10 18:05:22, battre wrote:
> On 2012/05/10 17:14:49, Aaron Boodman wrote:
> > The need for the 'event' type's schema is an implementation detail of this
> > object. Can we just have it do it itself? You could add a method to
> > ApiDefinitionsNatives to get a particular API definition by name.
> 
> I am just concerned that we might end up with multiple copies of the schema in
> memory. 

V8SchemaRegistry caches the js objects across the entire process, so that won't
happen.

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:14: var addRulesParams =
undefined;
undefined is weird in JavaScript. Suffice to say, it would be better and 'less
defined' to do:

var addRulesParams;
var foo;
var bar;

[battre, 2012-05-10 19:16:50]

On 2012/05/10 18:27:50, Aaron Boodman wrote:
>
http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
> File chrome/renderer/resources/extensions/event.js (right):
> 
>
http://codereview.chromium.org/10392008/diff/38/chrome/renderer/resources/ext...
> chrome/renderer/resources/extensions/event.js:65: opt_eventName,
opt_argSchemas,
> opt_eventOptions, opt_eventsSchema) {
> On 2012/05/10 18:05:22, battre wrote:
> > On 2012/05/10 17:14:49, Aaron Boodman wrote:
> > > The need for the 'event' type's schema is an implementation detail of this
> > > object. Can we just have it do it itself? You could add a method to
> > > ApiDefinitionsNatives to get a particular API definition by name.
> > 
> > I am just concerned that we might end up with multiple copies of the schema
in
> > memory. 
> 
> V8SchemaRegistry caches the js objects across the entire process, so that
won't
> happen.
> 
>
http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
> File chrome/renderer/resources/extensions/event.js (right):
> 
>
http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
> chrome/renderer/resources/extensions/event.js:14: var addRulesParams =
> undefined;
> undefined is weird in JavaScript. Suffice to say, it would be better and 'less
> defined' to do:
> 
> var addRulesParams;
> var foo;
> var bar;

Do you have any preferences regarding the "unprivileged"?

- I could modify extension_api.cc to look for unprivileged functions not only at
the top level namespace but also in all object definitions and fill
partially_unprivileged_apis_ accordingly. This might be computationally somewhat
expensive, in particular for ExtensionAPI::IsChildNamePrivileged. 

- I could hack around by hardcoding my functions as unprivileged.

- And I could try to modify declarative_api.cc to do a late check and make the
functions NOPs.

Do you see another strategy?

[battre, 2012-05-11 13:07:21]

Extend apiDefinitions.GetExtensionAPIDefinition to return individual apis

[battre, 2012-05-11 13:08:46]

I have extend apiDefinitions.GetExtensionAPIDefinition to return individual
apis.

I think we are done with the CL. PTAL.

Thanks,
Dominic

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:14: var addRulesParams =
undefined;
On 2012/05/10 18:27:51, Aaron Boodman wrote:
> undefined is weird in JavaScript. Suffice to say, it would be better and 'less
> defined' to do:
> 
> var addRulesParams;
> var foo;
> var bar;

Done.

[Aaron Boodman, 2012-05-11 19:16:55]

lgtm

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/extensions/...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/extensions/...
chrome/renderer/extensions/api_definitions_natives.cc:33: } else {
You can eliminate the else block because of the early return.

[not at google - send to devlin, 2012-05-14 07:29:38]

lgtm, I think my comments in event.js are worth looking at though.

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:14: var addRulesParams =
undefined;
Can you also prefix them with Schema?  I wasn't sure what I was looking at at
first.

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:21: function
storeFunctionSchemes(eventsSchema) {
Schemes -> Schemas?

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

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:18: var removeRulesParams;
I find this a bit funny to look at out of context. This is specifically for the
rule-based events right? Could you store these in a single object like
"ruleFunctionSchemas" or something with keys "addRules", "getRules", and
"removeRules"?

I find the comment on this quite hard to parse actually, maybe something like

// Schemas for the rule-style functions on the events API that
// only need to be generated occasionally, so populate them lazily.

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:24: // Schema definition of Events
object.
Schemes -> Schemas?

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:115:
storeFunctionSchemes(GetExtensionAPIDefinition("events")[0]);
You really only need to load these right before calling sendRequest right?  You
could move all of this logic into the storeFunctionSchemes method; call it like
"ensureRuleSchemasLoaded" which early-exists if the ruleFunctionSchemas object
hasn't been populated yet. Then call it right before calling sendRequest each
time.

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

http://codereview.chromium.org/10392008/diff/6040/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:12: var events
= require('event_bindings');
Why this change? Doesn't look like it's being used?

[not at google - send to devlin, 2012-05-14 07:30:34]

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10392008/diff/10002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/event.js:14: var addRulesParams =
undefined;
On 2012/05/14 07:29:38, kalman wrote:
> Can you also prefix them with Schema?  I wasn't sure what I was looking at at
> first.

(ignore this, forgot to delete, sorry)

[commit-bot: I haz the power, 2012-05-15 16:47:24]

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

[commit-bot: I haz the power, 2012-05-15 16:47:57]

Presubmit check for 10392008-3008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
This change modifies the extension docs but the generated docs have not been
updated properly. See chrome/common/extensions/docs/README.txt for more info.
 - Changes to chrome/common/extensions/docs/static/declarativeWebRequest.html
not reflected in generated doc.
 - Changes to chrome/common/extensions/docs/static/events.html not reflected in
generated doc.
First build DumpRenderTree, then update the docs by running:
  chrome/common/extensions/docs/build/build.py

Presubmit checks took 5.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[battre, 2012-05-15 16:50:09]

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/ext...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/ext...
chrome/renderer/extensions/api_definitions_natives.cc:33: } else {
On 2012/05/11 19:16:56, Aaron Boodman wrote:
> You can eliminate the else block because of the early return.

Done.

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:18: var removeRulesParams;
On 2012/05/14 07:29:38, kalman wrote:
> I find this a bit funny to look at out of context. This is specifically for
the
> rule-based events right? Could you store these in a single object like
> "ruleFunctionSchemas" or something with keys "addRules", "getRules", and
> "removeRules"?
> 
> I find the comment on this quite hard to parse actually, maybe something like
> 
> // Schemas for the rule-style functions on the events API that
> // only need to be generated occasionally, so populate them lazily.

Done.

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:24: // Schema definition of Events
object.
On 2012/05/14 07:29:38, kalman wrote:
> Schemes -> Schemas?

Done.

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:115:
storeFunctionSchemes(GetExtensionAPIDefinition("events")[0]);
On 2012/05/14 07:29:38, kalman wrote:
> You really only need to load these right before calling sendRequest right? 
You
> could move all of this logic into the storeFunctionSchemes method; call it
like
> "ensureRuleSchemasLoaded" which early-exists if the ruleFunctionSchemas object
> hasn't been populated yet. Then call it right before calling sendRequest each
> time.

Done.

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/10392008/diff/6040/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:12: var events
= require('event_bindings');
On 2012/05/14 07:29:38, kalman wrote:
> Why this change? Doesn't look like it's being used?

Done.