Closed Bug 1246028 Opened 4 years ago Closed 4 years ago

[commands] Implement chrome.commands.getAll

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [commands]triaged)

Attachments

(2 files, 6 obsolete files)

No description provided.
Blocks: 1246029
No longer depends on: 1246024
No longer blocks: 1246029
Whiteboard: [commands]triaged
Assignee: nobody → mwein
Summary: Implement chrome.commands.getAll → [commands] Implement chrome.commands.getAll
Iteration: --- → 47.2 - Feb 22
Attached patch Implement chrome.commands.getAll (obsolete) — Splinter Review
Attachment #8720594 - Flags: review?(kmaglione+bmo)
Depends on: 1246031, 1242557
Comment on attachment 8720594 [details] [diff] [review]
Implement chrome.commands.getAll

Review of attachment 8720594 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-commands.js
@@ +8,5 @@
> +   PlatformInfo,
> +} = ExtensionUtils;
> +
> +// WeakMap[Extension -> Array[Command]]
> +var commandsMap = new WeakMap();

This should probably a map of command names to Command objects rather than an array.

@@ +26,5 @@
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if (commandsMap.has(extension)) {
> +    commandsMap.get(extension).shutdown();

This object has no `shutdown` method.

@@ +35,5 @@
> +
> +extensions.registerSchemaAPI("commands", null, (extension, context) => {
> +  return {
> +    commands: {
> +      getAll() {

Isn't this marked unsupported in the schema?

@@ +36,5 @@
> +extensions.registerSchemaAPI("commands", null, (extension, context) => {
> +  return {
> +    commands: {
> +      getAll() {
> +        return Promise.resolve(commandsMap.get(extension));

I think we should probably explicitly serialize the command objects into plain JS objects rather than expecting them to be cloned correctly.

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ +8,5 @@
> +      "name": "Commands Extension",
> +      "commands": {
> +        "with-desciption": {
> +          "suggested_key": {
> +            "default": "Ctrl+Shift+Y",

We should test handling of OS-specific keys.

@@ +26,5 @@
> +
> +      browser.commands.getAll((commands) => {
> +        browser.test.assertEq(commands.length, 2, "getAll should return an array of commands");
> +
> +        browser.test.assertEq("with-desciption", commands[0].name,

We shouldn't rely on the sorting order. You can sort the array by name before dong comparisons, though.
Attachment #8720594 - Flags: review?(kmaglione+bmo)
Attached patch Implement chrome.commands.getAll (obsolete) — Splinter Review
Attachment #8721152 - Flags: review?(kmaglione+bmo)
Attachment #8720594 - Attachment is obsolete: true
> > +// WeakMap[Extension -> Array[Command]]
> > +var commandsMap = new WeakMap();
> 
> This should probably a map of command names to Command objects rather than
> an array.
Hm, I'm not sure.. So the manifest is of the form {name1: {shortcut, description}, name2: {shortcut, description}, ...} while `getAll` returns an array of objects, like [{name, shortcut, description}, {name, shortcut, description}, ...] so I'll need to restructure it at some point unless we decide to change what `getAll` returns.

Making this change did simplify the code a bit, but it also makes `getAll` slightly slower. I went ahead with the change though because I don't think it will really affect performance much.  Let me know if you think this makes sense.

> > +    commandsMap.get(extension).shutdown();
> 
> This object has no `shutdown` method.
Oops! Good catch, sorry about that. I'll fix this in the next patch.

> > +      getAll() {
> 
> Isn't this marked unsupported in the schema?
The schema just landed in b/1242557 and I think I left it supported in that change by mistake. I was going to mark it as supported in this bug, but I won't need to now.  Also, just fyi, this bug still depends on b/1246031, which adds the utility for obtaining the platform information.

> @@ +36,5 @@
> > +extensions.registerSchemaAPI("commands", null, (extension, context) => {
> > +  return {
> > +    commands: {
> > +      getAll() {
> > +        return Promise.resolve(commandsMap.get(extension));
> 
> I think we should probably explicitly serialize the command objects into
> plain JS objects rather than expecting them to be cloned correctly.
With my change to the commandsMap, I'll be constructing the object when getAll is called so this won't be necessary.

> > +        browser.test.assertEq("with-desciption", commands[0].name,
> 
> We shouldn't rely on the sorting order. You can sort the array by name
> before dong comparisons, though.
That makes sense. I decided to use commands.find for each of them instead so I don't have to worry about the order. If you think this will be too slow I can switch to sorting them by name.
Comment on attachment 8721152 [details] [diff] [review]
Implement chrome.commands.getAll

Review of attachment 8721152 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Matthew Wein [:mattw] from comment #4)
> Hm, I'm not sure.. So the manifest is of the form {name1: {shortcut,
> description}, name2: {shortcut, description}, ...} while `getAll` returns an
> array of objects, like [{name, shortcut, description}, {name, shortcut,
> description}, ...] so I'll need to restructure it at some point unless we
> decide to change what `getAll` returns.
>
> Making this change did simplify the code a bit, but it also makes `getAll`
> slightly slower. I went ahead with the change though because I don't think
> it will really affect performance much.  Let me know if you think this makes
> sense.

I wouldn't worry much about performance in this case. I'd expect this method
to be called fairly infrequently, and the performance difference is pretty
trivial.

That said, I liked what you had before better. The only change I would have
made would be to store the Command objects in a Map rather than an array, and
explicitly create the objects returned by getAll, rather than just returning
the Command objects.

::: browser/components/extensions/ext-commands.js
@@ +16,5 @@
> +  commandsMap.set(extension, manifest.commands);
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if (commandsMap.has(extension)) {

This check isn't necessary. Calling `delete` on a nonexistent key is fine.

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ +36,5 @@
> +
> +    background: function() {
> +      browser.test.onMessage.addListener((message, additionalScope) => {
> +        browser.commands.getAll((commands) => {
> +          let command;

Always declare variables as close to their first use/definition as posible.

@@ +56,5 @@
> +
> +          let platform = additionalScope.platform;
> +          if (platform == "macosx") {
> +            platform = "mac";
> +          }

This isn't necessary. Just check for macosx below.

@@ +77,5 @@
> +              shortcut = "A";
> +              break;
> +            case "ios":
> +              shortcut = "I";
> +              break;

The "chromeos" and "ios" variants aren't necessary. This code will never run on either of those.

@@ +81,5 @@
> +              break;
> +            case "default":
> +              shortcut = "D";
> +              break;
> +          }

You'd be better off just defining an object and indexing it:

  let platformKeys = {
    macosx: "M",
    linux: "L",
    windows: "W",
    android: "A",
  };

  let shortcut = platformKeys[platform];
Attachment #8721152 - Flags: review?(kmaglione+bmo)
Attachment #8721152 - Attachment is obsolete: true
Attachment #8721721 - Attachment is obsolete: true
Attachment #8721721 - Flags: review?(kmaglione+bmo)
Blocks: 1246029
Comment on attachment 8721723 [details] [diff] [review]
Implement chrome.commands.getAll try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell

Review of attachment 8721723 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-commands.js
@@ +41,5 @@
> +            name: name,
> +            description: command.description,
> +            shortcut: command.shortcut,
> +          });
> +        });

let commands = Array.from(commandMap.get(extension), (name, command) => {
  return ({
    name,
    description: command.description,
    shortcut: command.shortcut,
  });
});

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ +53,5 @@
> +            macosx: "M",
> +            linux: "L",
> +            windows: "W",
> +            android: "A",
> +            default: "D",

We shouldn't need a "default". If we wind up with an unexpected platform, it's better that the tests fail.
Attachment #8721723 - Flags: review?(kmaglione+bmo) → review+
> let commands = Array.from(commandMap.get(extension), (name, command) => {
>   return ({
>     name,
>     description: command.description,
>     shortcut: command.shortcut,
>   });
> });

I don't think this works correctly, because it handles Maps weirdly. For each item in the map, it calls the map function with two arguments:

  mapFn(value, key): value: an array of the form [itemKey, itemValue];
                     key: the index of the item in the array it's creating

So, to make this work, it would need to be:

let commands = Array.from(commandMap.get(extension), (value, key) => {
  return ({
    name: value[0],
    description: value[1].description,
    shortcut: value[1].shortcut,
  });
});

Which I think is pretty confusing.  Perhaps there's a way to make it the way you suggested, but for now I'll keep it the way I had it.

> ::: browser/components/extensions/test/browser/browser_ext_commands.js
> @@ +53,5 @@
> > +            macosx: "M",
> > +            linux: "L",
> > +            windows: "W",
> > +            android: "A",
> > +            default: "D",
> 
> We shouldn't need a "default". If we wind up with an unexpected platform,
> it's better that the tests fail.
Sounds good.
Attachment #8722278 - Flags: review?(kmaglione+bmo) → review+
Attachment #8721723 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Matthew Wein [:mattw] from comment #12)
> So, to make this work, it would need to be:

Nope.

  let commands = Array.from(commandMap.get(extension), ([name, command]) => {
    return ({
      name,
      description: command.description,
      shortcut: command.shortcut,
    });
  });
Ah, thanks, yet another reason why the destructuring assignment is really nice.
Attached patch Implement chrome.commands.getAll (obsolete) — Splinter Review
Attachment #8722278 - Attachment is obsolete: true
Attachment #8722643 - Flags: review+
There's a bunch of test failures in the linked try push. Does this new patch fix them?
Nope, the patch looks broken. The platform utility returns "win" on Windows while the patch was expecting "windows".
Attachment #8722643 - Attachment is obsolete: true
Comment on attachment 8722761 [details] [diff] [review]
Implement chrome.commands.getAll try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell

Review of attachment 8722761 [details] [diff] [review]:
-----------------------------------------------------------------

Please make sure to remove the try: command from the commit message before checking in.

::: browser/components/extensions/ext-commands.js
@@ +19,5 @@
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_commands", (type, directive, extension, manifest) => {
> +  let commands = new Map();
> +  for (let name of Object.keys(manifest.commands)) {
> +    let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;

Ugh.

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ +58,5 @@
> +
> +          command = commands.find(c => c.name == "with-platform-info");
> +          let platformKey = platformKeys[additionalScope.platform];
> +          let shortcut = `Ctrl+Shift+${platformKey}`;
> +          browser.test.assertEq(shortcut, command.shortcut,

The first two arguments should be moved to the next line, or the subsequent arguments should be aligned just after the opening paren. Same for the above.
Attachment #8722761 - Flags: review?(kmaglione+bmo) → review+
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Backed out in https://hg.mozilla.org/integration/fx-team/rev/fdf9c9e0ea67 since mattw wasn't actually quite ready to land after all.
> Please make sure to remove the try: command from the commit message before
> checking in.
Done.

> > +    let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;
> 
> Ugh.
You can say that again :/

> > +          browser.test.assertEq(shortcut, command.shortcut,
> 
> The first two arguments should be moved to the next line, or the subsequent
> arguments should be aligned just after the opening paren. Same for the above.
I pulled out the messages into a variable to make all the assertEq's one-liners.
Keywords: checkin-needed
(In reply to Matthew Wein [:mattw] from comment #24)
> > > +    let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;
> > 
> > Ugh.
> You can say that again :/

I filed bug 1251147 to rename AppConstants.platform's "win" to "windows"
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> I filed bug 1251147 to rename AppConstants.platform's "win" to "windows"

I don't think that's going to help. We already remap AppConstants.platform to match Chrome conventions. The problem here is that the Chrome extensions API uses "win" as the platform identifier for Windows in other places, but in this API it uses "windows".

Hence "Ugh". We can't fix it :(
Maybe we can file a bug in Chrome to see if they will change it? I think it makes sense for this to be consistent.
(In reply to Matthew Wein [:mattw] from comment #27)
> Maybe we can file a bug in Chrome to see if they will change it? I think it
> makes sense for this to be consistent.

I suppose it's worth a try. It doesn't really help us in the short term, though. We'd just have to accept both "win" and "windows" until the end of the deprecation period.
https://hg.mozilla.org/mozilla-central/rev/3dfcbfa56b0e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.