1337457 - [commands] Add-on throws error on load when suggested_key is null / missing

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, defect)

Description

[Rob Wu [:robwu]]

An extension can declare a shortcut via the "commands" section in manifest.json.
In Chrome, the value can be ommitted. This is useful because then the add-on does not steal any shortcut by default, and users who want a shortcut can assign one via the UI.

In Firefox there is no UI yet, but I expect this to be possible, eventually (bug 1303384). So Firefox should not reject add-ons that omit "suggested_key".

Comment 1

[Rob Wu [:robwu]]

When I just tested with the latest stable (51), the add-on loads just fine.
However, the console still displays "command.suggested_key is null  ext-commands.js:79".
Maybe a manifest error no longer prevents the extension from being loaded?

background.js
console.log('Extension loaded!');

manifest.json
{
    "name": "commands without shortcut",
    "manifest_version": 2,
    "version": "1",
    "background": {
        "scripts": ["background.js"]
    },
    "commands": {
        "some-command-id": {}
    }
}

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1337457 - Handle missing commands[*].suggested_key

Defaulting to "" instead of null because Chrome too defaults to "".

Review commit: https://reviewboard.mozilla.org/r/110430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110430/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8834544 [details]
Bug 1337457 - Handle missing commands[*].suggested_key

https://reviewboard.mozilla.org/r/110430/#review113164

r=me with nits addressed. Thanks!

::: browser/components/extensions/ext-commands.js:79
(Diff revision 1)
>      // For Windows, chrome.runtime expects 'win' while chrome.commands
>      // expects 'windows'.  We can special case this for now.
>      let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;
> -    for (let name of Object.keys(manifest.commands)) {
> -      let command = manifest.commands[name];
> -      let shortcut = command.suggested_key[os] || command.suggested_key.default;
> +    for (let [name, command] of Object.entries(manifest.commands)) {
> +      let suggested_key = command.suggested_key || {};
> +      let shortcut = suggested_key[os] || suggested_key.default || "";

Please default to null rather than empty string.

::: browser/components/extensions/test/browser/browser_ext_commands_getAll.js:74
(Diff revision 1)
> +          errorMessage = "The description should match what is provided in the manifest";
> +          browser.test.assertEq("has no suggested_key", command.description, errorMessage);
> +
> +          errorMessage = "The shortcut should be an empty string if not provided";
> +          browser.test.assertEq("", command.shortcut, errorMessage);
> +
> +          command = commands.find(c => c.name == "without-suggested-key-nor-description");
> +
> +          errorMessage = "The description should be empty when it is not provided";
> +          browser.test.assertEq(null, command.description, errorMessage);
> +
> +          errorMessage = "The shortcut should be an empty string if not provided";
> +          browser.test.assertEq("", command.shortcut, errorMessage);

Please no `errorMessage` variables.

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8834544 [details]
Bug 1337457 - Handle missing commands[*].suggested_key

https://reviewboard.mozilla.org/r/110430/#review113164

> Please default to null rather than empty string.

I am defaulting to `''` to be consistent with Chrome's API (`chrome.commands.getAll` always has a string value in the `shortcut` field). Do you insist on using `null` regardlessly?

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8834544 [details]
Bug 1337457 - Handle missing commands[*].suggested_key

https://reviewboard.mozilla.org/r/110430/#review113164

> I am defaulting to `''` to be consistent with Chrome's API (`chrome.commands.getAll` always has a string value in the `shortcut` field). Do you insist on using `null` regardlessly?

I'd rather we default to null, yes.

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8834544 [details]
Bug 1337457 - Handle missing commands[*].suggested_key

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110430/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9e7efc94543
Handle missing commands[*].suggested_key r=kmag

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2c3aa1ebb12
ESlint fix a=bustage

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9e7efc94543
https://hg.mozilla.org/mozilla-central/rev/c2c3aa1ebb12