The commands API is described by a schema JSON file: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/api/commands.json which is currently missing and should be imported in one of the current schemas directories and registered accordingly: - "toolkit"-level schemas schemas directory: toolkit/components/extensions/schemas/ schemas registration: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#78 - "browser"-level schemas: schemas directory: browser/components/extensions/schemas/ schemas registration: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#562

After looking at https://wiki.mozilla.org/WebExtensions/Hacking#Code_layout, I'm still not 100% sure if the schema file should be in toolkit or browser. Since I think the commands API should work on iOS and Android (using a connected keyboard), I put the schema in /toolkit since /browser is desktop specific.

(In reply to Matthew Wein [:mattw] from comment #2) > After looking at https://wiki.mozilla.org/WebExtensions/Hacking#Code_layout, > I'm still not 100% sure if the schema file should be in toolkit or browser. > Since I think the commands API should work on iOS and Android (using a > connected keyboard), I put the schema in /toolkit since /browser is desktop > specific. Yep, it exactly what I was thinking about. I assumed that the schema files (and the API implementations) are registered at toolkit or browser level based on their implementation details: - "toolkit", if it's implementation can be shared to the other products build from mozilla-central - "browser", if it's implementation is product specific (e.g. it needs to be implemented differently on Firefox for Android, e.g. like the tabs API) But I can be wrong, I have not yet registered any new schema file. I'm adding a needinfo request assigned to Bill to ask him for advices.

Comment on attachment 8715049 [details] [diff] [review] Import missing commands API schema file Review of attachment 8715049 [details] [diff] [review]: ----------------------------------------------------------------- The new schema files need to be added to the jar files during the build. To instruct the build process to do so, this patch needs to add the path of the new schema file to the file "jar.mn" which is located in the "schemas" dir at the same level of the schema file, e.g.: - https://dxr.mozilla.org/mozilla-central/search?q=path%3Aschemas%2Fjar.mn&case=true&=mozilla-central&redirect=true

Ah, thanks, this patch adds the file path of the commands schema to the "jar.mn"

(In reply to Matthew Wein [:mattw] from comment #2) > After looking at https://wiki.mozilla.org/WebExtensions/Hacking#Code_layout, > I'm still not 100% sure if the schema file should be in toolkit or browser. > Since I think the commands API should work on iOS and Android (using a > connected keyboard), I put the schema in /toolkit since /browser is desktop > specific. The schema can go in /toolkit, but the implementation and the schema import need to go under /browser. It's probably better to also put the schema in /browser until we add support for other platforms. iOS isn't really relevant, since it doesn't use Gecko and won't be able to use the toolkit code in any case.

Comment on attachment 8715985 [details] [diff] [review] Import missing commands API schema file Review of attachment 8715985 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Matthew, we're almost there ;-) Here are the main changes needed: - let's move this schema at browser level (because, as Kris confirmed, this has to be implemented at browser level): - move the schema registering to nsBrowserGlue.js[1] - move the schema file to "browser/components/extensions/schemas/" (and at the schema file path to the corresponding jar.mn file) - in the schema all the API methods (the onCommand event and the getAll function) should be marked as unsupported - the manifest.json schema files[2] needs to be tweaked to recognize the new "commands" manifest field, but we should probably just mark it as unsupported for now and complete it in a followup, e.g.: "commands": { "type": "object", "optional": true, "unsupported": true }, And a couple of more things that could make this patch even nicer: - includes a new "ext-commands.js" file (just the skeleton, ready to be filled with the API implementation over the time) - includes a preliminary test case for the new API, just a small test case which: - checks that the expected namespace has been created (even if it will be currently empty because all the methods will be marked as unsupported) - checks that the "commands" field of the extension manifest is correctly recognized (currently an manifest field explicitly marked as unsupported raise an exception with the error message: 'Reading manifest: Property "commands" is unsupported by Firefox') [1]: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js [2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json

Thanks for the feedback! > - let's move this schema at browser level (because, as Kris confirmed, this > has to be implemented at browser level): > - move the schema registering to nsBrowserGlue.js[1] > - move the schema file to "browser/components/extensions/schemas/" (and at > the schema file path to the corresponding jar.mn file) Done. > - in the schema all the API methods (the onCommand event and the getAll > function) should be marked as unsupported Done. > - the manifest.json schema files[2] needs to be tweaked to recognize the new > "commands" manifest field, but we should probably just mark it as > unsupported for now and complete it in a followup, e.g.: > > "commands": { > "type": "object", > "optional": true, > "unsupported": true > }, Done. > And a couple of more things that could make this patch even nicer: > > - includes a new "ext-commands.js" file (just the skeleton, ready to be > filled with the API implementation over the time) Done. > - includes a preliminary test case for the new API I'm not sure how to test this when it is marked as unsupported in the manifest. Could you help me with that? Also, if I mark the API supported in the manifest and try to run any of the tests, I get the following error: "Uncaught exception - startup failed". Do you have an idea of what is going on there?

Comment on attachment 8717301 [details] [diff] [review] Import missing commands API schema file and add preliminary boilerplate code Review of attachment 8717301 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Matthew, looks good! all the pieces seems to be in their place, ready to be implemented in the follow ups. By importing the patch on a recent fx-team there are a couple of conflict to resolve, in the following files (nothing to worry about, the patch just need to be rebased): - browser/components/extensions/jar.mn.rej - browser/components/extensions/test/browser/browser.ini.rej - browser/components/nsBrowserGlue.js.rej If you want you can mark the getAll method as async in the schema before landing, or we can do it in the patch which implements the API method as well. Follows a couple of side notes and more info about the "async" API methods: ::: browser/components/extensions/ext-commands.js @@ +10,5 @@ > + > +extensions.registerSchemaAPI("commands", null, (extension, context) => { > + return { > + commands: { > + getAll() { throw new Error("Not Implemented"); }, Nit: this is not needed because, at least currently, all properties marked as unsupported in the schema are not injected in the webextension context. In my opinion, it would be helpful to inject methods which explicitly log a more helpful error message than "chrome.commands.getAll is undefined", but it would be better to solve it for all the unsupported API methods in once (e.g. by implementing the desidered behavior in the webextensions schema internals). As side notes (not really important until there is a real implementation of this method): - the above Error raised from the privileged code (the API method implementation) code cannot be accessed from the add-on code (e.g. by trying to access the error.message will raise a security error), to raise an error which can be accessed from the add-on code we have to use |context.cloneScope.Error|, e.g.: getAll() { throw new context.cloneScope.Error("..."); }, - when an API method as a callback parameter, most of the time is supposed to fail by setting the chrome.runtime.lastError and calling the callback (more details about this in the following comments on the commands.json file) ::: browser/components/extensions/schemas/commands.json @@ +47,5 @@ > + { > + "name": "getAll", > + "unsupported": true, > + "type": "function", > + "description": "Returns all the registered extension commands for this extension and their shortcut (if active).", this method has a callback parameter and it should be marked as async in the schema (this feature has been recently introduced by Bug 1234020), e.g.: "functions": [ { "name": "getAll", "unsupported": true, "type": "function", "async": "callback", "description": "Returns all the registered extension commands for this extension and their shortcut (if active).", "parameters": [ { "type": "function", "name": "callback", This way, the API method supports both the "callback"-based API (through the |chrome| APIs object) and the "Promise"-based API (through the |browser| APIs object). This is helpful even in the API method implementation, e.g. we can warn the add-on developer about an error condition by returning a rejected Promise (which will set the "chrome.runtime.lastError" as well), or return a promise to resolve it asynchronously once the result is ready: getAll() { if (/* something is wrong */) { return Promise.reject({ message: "something is wrong" }); } return new Promise((resolve, reject) => { /* do something (maybe asynchronous) which collect all the defined commands */ ... resolve(allCommandsDetails); /* or reject({ message: "something else was wrong" }); */ }); }

Please don't bother writing tests until you have an actual implementation. There are better uses of your time.

Thanks for the feedback, Luca!

Comment on attachment 8718950 [details] [diff] [review] Import missing commands API schema file and add preliminary boilerplate code Review of attachment 8718950 [details] [diff] [review]: ----------------------------------------------------------------- Most of this is fine, but let's land it in one of the bugs that contains some of the implementation. I'd rather not load schema definitions and API scripts before they actually do anything. ::: browser/components/extensions/ext-commands.js @@ +11,5 @@ > +extensions.registerSchemaAPI("commands", null, (extension, context) => { > + return { > + commands: { > + getAll() { throw new Error("Not Implemented"); }, > + onCommand: ignoreEvent(context, "commands.onCommand"), These are both marked as unsupported, so they won't actually be used. ::: browser/components/extensions/schemas/commands.json @@ +47,5 @@ > + { > + "name": "getAll", > + "unsupported": true, > + "type": "function", > + "description": "Returns all the registered extension commands for this extension and their shortcut (if active).", Please add `"async": "callback"` ::: browser/components/extensions/test/browser/browser_ext_commands.js @@ +16,5 @@ > + > + yield extension.startup(); > + yield extension.awaitFinish("commands"); > + yield extension.unload(); > +}); I'd rather not add a test that doesn't actually test anything. ::: toolkit/components/extensions/schemas/manifest.json @@ +48,5 @@ > > + "commands": { > + "type": "object", > + "optional": true, > + "unsupported": true This should go in commands.json. See browser_action.json for an example. We should also fill in the type details here. They should look something like: "extraProperties": { "type": "object", "properties": { "suggested_key": { "extraProperties": { "$ref": "KeyName" } }, "description": { "type": "string", "optional": true } } } And a KeyName type something like: { "id": "KeyName", "choices": [ { "type": "string", "pattern": "^(Alt|Ctrl|Command|Command|MacCtrl)\\+(Shift\\+)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)$" }, { "type": "string", "pattern": "^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$" } ] }

Luca, thanks for the help. I think I agree with Kris that the implementation and test files should land in one of the bugs that contains some of the implementation, while this bug will specifically be for importing the schema file. Kris, Thanks for the help. I made some modifications to the KeyName property you suggested: I added the `Search` modifier for Chrome OS since it's specified in the documentation, and explicitly listed the platform types (mac, chromeos, linux, etc). Let me know if you agree with this.

Comment on attachment 8719652 [details] [diff] [review] Import missing commands API schema file and add preliminary boilerplate code Review of attachment 8719652 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Matthew Wein [:mattw] from comment #17) > I made some modifications to the KeyName property you suggested: I added > the `Search` modifier for Chrome OS since it's specified in the > documentation, and explicitly listed the platform types (mac, chromeos, > linux, etc). Let me know if you agree with this. I'm not sure. We definitely shouldn't make unknown platforms a hard error, since we or Chrome may add platforms in the future. We can make them a warning, but we should at least accept "android" and "ios" without warnings. As for ChromeOS and unknown platforms, I don't think we should typecheck the value, especially since we're never going to support ChromeOS, and that would allow us to remove the Search key. Aside from that, it looks good. ::: browser/components/extensions/schemas/commands.json @@ +26,5 @@ > + "type": "object", > + "extraProperties": { > + "type": "object", > + "properties": { > + "suggested_key": { This should be optional. @@ +47,5 @@ > + }, > + "windows": { > + "$ref": "KeyName", > + "optional": true > + } I think we should make a few changes here: 1) Remove the value checking for "chromeos", and just make it a string. 2) Add "android" and "ios", also without value checking for now. 3) Add "extraProperties" to the attribute, with a value something like: { "type": "string", "deprecated": "Unknown platform name" }