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)

defect
Not set
normal

Tracking

(firefox45 unaffected, firefox46 unaffected, firefox47 affected, firefox48 verified)

VERIFIED FIXED
mozilla48
Iteration:
48.3 - Apr 25
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.
Whiteboard: [commands]
Summary: Corrupted error message while installing Black Menu for Google webextension → White space should be accepted in commands API key names
Assignee: nobody → mwein
Iteration: --- → 48.3 - Apr 25
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 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.
I had to fix a couple eslint errors, but it should be ready for review now.
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/
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.
(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 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+
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
As much as breaking the world with a single commit would be interesting, I decided to stick with revision 4 :)

Thanks for the review.
Keywords: checkin-needed
Whiteboard: [commands] → [commands]triaged
https://hg.mozilla.org/mozilla-central/rev/bf3a35b355f0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwein)
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)
Depends on: 1273084
Depends on: 1273087
No longer depends on: 1273087
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.