Closed
Bug 1263560
Opened 8 years ago
Closed 8 years ago
White space should be accepted in commands API key names
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox45 unaffected, firefox46 unaffected, firefox47 affected, firefox48 verified)
Tracking | Status | |
---|---|---|
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | --- | affected |
firefox48 | --- | verified |
People
(Reporter: vtamas, Assigned: mattw)
References
Details
(Whiteboard: [commands]triaged)
Attachments
(1 file)
[Note] This is a follow-up bug for Bug 1253374 [Affected versions]: Firefox 48.0a1 (2016-04-10) Firefox 47.0a2 (2016-04-10) [Affected platforms]: Windows 10 64-bit Ubuntu 14.04 32-bit Mac OS X 10.11 [Steps to reproduce]: 1.Launch Firefox with clean profile. 2.Create the xpinstall.signatures.dev-root pref in about:config and set it to true. 3.Install the following webextension: https://addons.allizom.org/en-US/firefox/addon/black-menu-for-google/ [Expected Results]: The webextesnion is successfully installed. [Actual Results]: - The webextension could not be installed because it appears to be corrupted. - The following error is thrown in Browser Console: 1460362386009 addons.webextension.<unknown> ERROR Loading extension 'null': Reading manifest: Error processing commands._execute_browser_action.suggested_key.default: Error processing commands._execute_browser_action.suggested_key.default: String "Alt + B" must match /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/ Log.jsm:753 [Regression range]: Last good revision: 584870f1cbc5d060a57e147ce249f736956e2b62 First bad revision: b2a3dc4b161fdfac6db78cc5bbfb1f53a383b65a Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=584870f1cbc5d060a57e147ce249f736956e2b62&tochange=b2a3dc4b161fdfac6db78cc5bbfb1f53a383b65a - This error have started to appear since Bug 1225715 had landed. [Additional notes]: - Sounds like it's related to chrome.commands parsing of media keys.
Updated•8 years ago
|
Whiteboard: [commands]
Updated•8 years ago
|
Summary: Corrupted error message while installing Black Menu for Google webextension → White space should be accepted in commands API key names
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 48.3 - Apr 25
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46625/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46625/
Attachment #8741602 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cdb544f5767
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8741602 [details] MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46625/diff/1-2/
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4788a2c5a8b9
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8741602 [details] MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46625/diff/2-3/
Attachment #8741602 -
Attachment description: MozReview Request: Bug 1263560 - Modify Schema.jsm to allow values to contain whitespace. r?kmag → MozReview Request: Bug 1263560 - Modify Schema.jsm to allow values to contain whitespace.
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ad9ff6efdf
Assignee | ||
Comment 7•8 years ago
|
||
I had to fix a couple eslint errors, but it should be ready for review now.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8741602 [details] MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46625/diff/3-4/
Assignee | ||
Comment 9•8 years ago
|
||
Kris, I'm curious to hear what you think about the differences between revision 3 and 4, and which one you think is better. At this point, I think revision 3 is cleaner, but I don't know if it makes sense in other contexts.
Comment 10•8 years ago
|
||
(In reply to Matthew Wein [:mattw] from comment #9) > Kris, I'm curious to hear what you think about the differences between > revision 3 and 4, and which one you think is better. At this point, I think > revision 3 is cleaner, but I don't know if it makes sense in other contexts. Revision 3 will break the world, so revision 4 is definitely better.
Comment 11•8 years ago
|
||
Comment on attachment 8741602 [details] MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag https://reviewboard.mozilla.org/r/46625/#review43429 Please update the commit summary to reflect this version of the patch. ::: browser/components/extensions/ext-commands.js:76 (Diff revision 4) > let commands = new Map(); > // 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 c = manifest.commands[name]; Please avoid one-letter variable names. ::: browser/components/extensions/ext-commands.js:81 (Diff revision 4) > - let command = manifest.commands[name]; > - commands.set(name, { > - description: command.description, > - shortcut: command.suggested_key[os] || command.suggested_key.default, > - }); > + let c = manifest.commands[name]; > + let description = c.description; > + let shortcut = c.suggested_key[os] ? c.suggested_key[os] : c.suggested_key.default; > + > + // Strip whitespace from the shortcut name. > + shortcut = shortcut.replace(/\s/g, ""); `/\s+/g` please. ::: browser/components/extensions/ext-commands.js:82 (Diff revision 4) > - commands.set(name, { > - description: command.description, > - shortcut: command.suggested_key[os] || command.suggested_key.default, > - }); > + let description = c.description; > + let shortcut = c.suggested_key[os] ? c.suggested_key[os] : c.suggested_key.default; > + > + // Strip whitespace from the shortcut name. > + shortcut = shortcut.replace(/\s/g, ""); > + commands.set(name, {description, shortcut}); I preferred the old style, I think. Maybe something like this? let command = manifest.commands[name]; let shortcut = command.suggested_key[os] || command.suggested_key.default; commands.set(name, { description: command.description, shortcut: shortcut.replace(/\s+/g, ""), }); ::: browser/components/extensions/schemas/commands.json:14 (Diff revision 4) > { > "id": "KeyName", > "choices": [ > { > "type": "string", > - "pattern": "^(Alt|Ctrl|Command|MacCtr)\\+(Shift\\+)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)$" > + "pattern": "^(\\s)*(Alt|Ctrl|Command|MacCtr)(\\s)*\\+(\\s)*(Shift(\\s)*\\+(\\s)*)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)(\\s)*$" No need for the extra parens. `\\s*` will do. ::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:27 (Diff revision 4) > }, > "unrecognized_property": "with-a-random-value", > }, > + "toggle-feature-with-whitespace-in-suggested-key": { > + "suggested_key": { > + "default": " \t \n Alt \n\t +\t \nShift+ 2\n", I wouldn't worry about `\t` or `\n`.
Attachment #8741602 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8741602 [details] MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46625/diff/4-5/
Attachment #8741602 -
Attachment description: MozReview Request: Bug 1263560 - Modify Schema.jsm to allow values to contain whitespace. → MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag
Assignee | ||
Comment 13•8 years ago
|
||
As much as breaking the world with a single commit would be interesting, I decided to stick with revision 4 :) Thanks for the review.
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4ea8210c30d
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Whiteboard: [commands] → [commands]triaged
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/bf3a35b355f0
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf3a35b355f0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Blocks: webext-port-blackmenu
Reporter | ||
Comment 17•8 years ago
|
||
I was able to reproduce the initial issue on Firefox 49.0a1 (2016-04-11) under Windows 10 64-bit. The webextension mentioned in Description is successfully installed on Firefox 49.0a1 (2016-05-13) and Firefox 48.0a2 (2016-05-13) under Windows 10 64-bit, Ubuntu 16-04 32-bit and Mac OS X 10.10.4, but the following errors are thrown in Browser Console: http://pastebin.com/ekapGyV6. Are these errors already tracked or should I file separate bug?
Assignee | ||
Comment 18•8 years ago
|
||
Thanks for noticing this, I think it would be good to file bugs for both of these issues and then we can try to figure out what's going on.
Flags: needinfo?(mwein)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•