Closed Bug 1337457 Opened 4 years ago Closed 4 years ago

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

Categories

(WebExtensions :: Compatibility, defect)

52 Branch
defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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".
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": {}
    }
}
Summary: [commands] Add-on refuses to load if suggested_key is null / missing → [commands] Add-on throws error on load when suggested_key is null / missing
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.
Attachment #8834544 - Flags: review?(kmaglione+bmo) → review+
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?
Whiteboard: triaged
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.
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/b9e7efc94543
Handle missing commands[*].suggested_key r=kmag
https://hg.mozilla.org/mozilla-central/rev/b9e7efc94543
https://hg.mozilla.org/mozilla-central/rev/c2c3aa1ebb12
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.