1263560 - White space should be accepted in commands API key names

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Vasilica Tamas, QA [:vtamas]]

[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.

Comment 1

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1263560 - Accept whitespace in command API key names. r=kmag

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

Comment 2

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cdb544f5767

Comment 3

[Matthew Wein [:mattw]]

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/

Comment 4

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4788a2c5a8b9

Comment 5

[Matthew Wein [:mattw]]

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/

Comment 6

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ad9ff6efdf

Comment 7

[Matthew Wein [:mattw]]

I had to fix a couple eslint errors, but it should be ready for review now.

Comment 8

[Matthew Wein [:mattw]]

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/

Comment 9

[Matthew Wein [:mattw]]

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

[Kris Maglione [:kmag]]

(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

[Kris Maglione [:kmag]]

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`.

Comment 12

[Matthew Wein [:mattw]]

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/

Comment 13

[Matthew Wein [:mattw]]

As much as breaking the world with a single commit would be interesting, I decided to stick with revision 4 :)

Thanks for the review.

Comment 14

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4ea8210c30d

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bf3a35b355f0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf3a35b355f0

Comment 17

[Vasilica Tamas, QA [:vtamas]]

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?

Comment 18

[Matthew Wein [:mattw]]

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.