Closed
Bug 1337457
Opened 7 years ago
Closed 7 years ago
[commands] Add-on throws error on load when suggested_key is null / missing
Categories
(WebExtensions :: Compatibility, defect)
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".
Assignee | ||
Comment 1•7 years ago
|
||
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": {} } }
Assignee | ||
Updated•7 years ago
|
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 hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
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?
Updated•7 years ago
|
Whiteboard: triaged
Comment 5•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/b9e7efc94543 Handle missing commands[*].suggested_key r=kmag
Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c2c3aa1ebb12 ESlint fix a=bustage
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b9e7efc94543 https://hg.mozilla.org/mozilla-central/rev/c2c3aa1ebb12
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•