RulesRegistry for declarative APIs.

chrome/common/extensions/api/experimental.declarative.json

Index: chrome/common/extensions/api/experimental.declarative.json
diff --git a/chrome/common/extensions/api/experimental.declarative.json b/chrome/common/extensions/api/experimental.declarative.json
index 5f3abe28b3e04575e4e821f647d31b5ffcd7f66d..7c3f253909bf6d965dd2dc88fb0d6fb5bbfe1ea9 100644
--- a/chrome/common/extensions/api/experimental.declarative.json
+++ b/chrome/common/extensions/api/experimental.declarative.json
@@ -29,6 +29,34 @@
             "description": "Optional priority of this rule. Defaults to 100."
           }
         }

[not at google - send to devlin, 2012/02/02 11:59:16]

I don't think we can have these here as part of the API. Having nodoc=true hides
it from documentation*, but it doesn't hide it from extensions themselves, where
the namespace will still be polluted with these types and testEvent.

Any reason why you can't use the webRequest stuff for tests?

* (though it's a bit redundant here since the whole API is nodoc)

[Aaron Boodman, 2012/02/02 16:28:51]

It's a bigger problem that. The whole declarative API will also be exposed,
uselessly.

I guess there are two paths here:

1) Hack JSON schema and SchemaGeneratedBindings some more to prevent these names
from being exposed to extensions (but they still need to be exposed to other
bindings files).

2) (Sorry if this playing with dead snakes a bit) Bite the bullet and do this
with normal IPC. This will properly decouple the renderer and browser side of
extension bindings, the way it should have always been done.

Revisiting the previous thread about this (from
https://chromiumcodereview.appspot.com/9315010), Dominic's reasons for not using
real IPC were:

a. validation of arguments
b. asynchronous messaging with callbacks (addRules needs to "return" something
to the callback function)
c. association of messages to their extensions?
d. possibly some security features?

a) is not a valid argument. We will still do param validation because we will
still use JSON schema for the frontend of the API. It's just that instead of the
bindings calling the sendRequest() infrastructure, it will end up calling some
other native function, which will call a custom IPC. The native function does
not need to be careful with validation because it can assume the JSON schema
already did that.

b), c), and d) are somewhat legit, but I believe that code can be easily
refactored out of EFD.

I'm running out the door, but please see the recent thread on
chrome-extensions-team w/ subject "A proposed IDL syntax" for more context.

To be honest, I am not sure how much work there is in this approach. It needs a
little investigation. That is why originally, I backed off proposing this
because I didn't want to randomize Dominic too much. However, as we add more and
more hacks to SGB, I'm continually thinking that we're pushing it in a bad
direction. If it were me doing it, I would investigate the IPC route.

[battre, 2012/02/02 19:12:36]

I am considering removing the nodoc from the whole API once we have we have
reached a somewhat stable state.

The reason for having these TestConditions was to have clean testing. I don't
think that having these objects here is a major problem. They are invisible and
nobody could do anything with them. There won't be a RuleRegistry bound to
"declarative.testEvent", so all all calls bounce.

[battre, 2012/02/02 19:12:36]

I am still in favor of this approach. I don't see any advantage if we use the
same JSON schema. Did you see the new CL after incorporating Ben's suggestions
of using CustomBindings? It became really neat! See
http://codereview.chromium.org/9192029/

About the effort of doing something ourselves:
- We need a to bind the native functions in a V8Context
- We need to introduce new IPC messages
- We need to implement the association of requests and responses
- We need to implement the association of messages to individual renderers
- We need to implement security for permissions strings
- We need to implement error handling

This looks like a lot of unnecessary effort that is basically code duplication.

Can you tell me what advantages you see in approach 2?

[not at google - send to devlin, 2012/02/03 13:06:20]

Weighing in here -

I still don't think that these Test definitions belong here, or anywhere really.
 Like I said, the integration-style tests should use real data like the
WebRequest bindings.  Any definitions of fake data should be isolated to unit
tests.  If there's a problem injecting fake data with tests, hopefully we can
work to factor things a bit differently for that to not be an issue, rather than
exposing test infrastructure to the world.

As for the more general issue of whether this API should be using the SGB
infrastructure at all (as opposed to defining a schema elsewhere(?) and
validating that way(?) -- the ?s are because I'm not sure what you mean by
"using JSON schema at the frontend of the API")... the path of least resistance
is definitely to use it.  To stop "private" APIs from being exposed to normal
extensions though, we could do something like
(a) add a "private" property to APIs which tells bindings generators not to
generate into the "chrome" object but into the "chromeHidden.privateAPIs" object
or something, which only our own code would be able to access (since it's on
chromeHidden).  Like a more formal nodoc property.
(b) something similar, but without using annotation magic, i.e. by bringing the
separation up to a higher level (separate ExtensionAPI classes for example). 
But this sounds like more work.

At a philosophical level it's fine, IMO.

Oh, btw, Dominic -- why remove nodoc=true from the API?  It's an API who's
idiomatic use is through Event objects.  Seems nicer to continue to hide the
documentation.

[Aaron Boodman, 2012/02/03 18:04:51]

Caveat: I'm willing to let this go. Not because I agree with you :), but because
it's asking you to do significant infrastructure work which I don't feel is
fair. Anyway, you are just following a bad pattern we have already set down. Not
your fault.

I do think that we should add the 'internal' flag though, so that we don't
expose these APIs to extensions. We should add the internal flag to every
occurrence of this pattern - which should be a subset of the things marked
'nodoc'. Ben, can you do this?

But anyway...
Not sure what you mean by this. We already have the native function keyword and
numerous examples of using it. There is no work to do here.
This is a pretty common occurrence in Chrome development. Nothing to be afraid
of =P.
This is all pretty trivial plumbing code. The only thing I would really count as
'duplication' is the security checks. Of course those should be refactored into
some shared location.
The extension API system was originally designed to expose APIs from the browser
process into JS in renderers directly. All the steps between JS and
ExtensionFunction implementations were generalized/generated.

Over time, we realized that having the entire stack generalized was insufficient
and that we frequently needed to do custom things in the renderer.

Now, it has gotten to the point that we our renderer-side bindings are often
completely custom. The generalized bindings system itself is now not really used
and is just serving as an inefficient and difficult to use IPC system!

The disadvantages of abusing SGB system this way are:

1. Non-public bindings inadvertently exposed to extensions. Exposing non-public
things is just a bad engineering practice. It can lead to security issues,
brittleness in the system, etc.
  1a. In order to fix 1, we must add complexity to SGB by introducing 'internal'
or 'private' and an implementation of that.
2. JSON is a pain to send and consume in C++. Calamity's stuff is going to help
with that, but that introduces tons of complexity. Just using Chrome's built in
IPC would be so much simpler and cleaner.

These aren't huge problems, I just feel like it's the right way to do it. It's
going to happen one way or another, because this is also what we need to do to
properly support NaCL and Dart.

+      },
+      {
+        "nodoc": true,
+        "id": "TestCondition",
+        "type": "object",
+        "description": "Test condition for unit testing.",
+        "properties": {
+          "testParameter": {
+            "type": "string",
+            "description": "Optional test parameter",
+            "optional": true
+          },
+          "instanceType": { "type": "string", "enum": ["experimental.declarative.TestCondition"] }
+        }
+      },
+      {
+        "nodoc": true,
+        "id": "TestAction",
+        "type": "object",
+        "description": "Test action for unit testing.",
+        "properties": {
+          "testParameter": {
+            "type": "string",
+            "description": "Optional test parameter",
+            "optional": true
+          },
+          "instanceType": { "type": "string", "enum": ["experimental.declarative.TestAction"] }
+        }
       }
     ],
     "functions": [
@@ -120,6 +148,18 @@
           }
         ]
       }
+    ],
+    "events": [
+      {
+        "nodoc": true,
+        "name": "testEvent",
+        "options": {
+          "supportsListeners": false,
+          "supportsRules": true,
+          "conditions": ["TestCondition"],
+          "actions": ["TestAction"]
+        }
+      }
     ]
   }
 ]