1272133 - runtime error if commands/suggested_key isn't supplied for the target platform

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[Will Bamberg [:wbamberg]]

STR:

Include a key like this in manifest.json:

  "commands": {
    "toggle-feature": {
    "suggested_key": { "linux": "Ctrl+Shift+U" },
      "description": "Send a 'toggle-feature' event to the extension"
    }
  }

Then try to install the add-on on some other platform. You get a runtime error logged to the console:

    TypeError: shortcut is null: loadCommandsFromManifest@chrome://browser/content/ext-commands.js:80:9 ....

I'm not sure what the best thing to do here is. Should we make commands/suggested_key/default mandatory? Should we just check for commands/suggested_key/default in the linter? Should we print a warning in the console, or in about:debugging, when the developer tries to install an add-on like this?

Comment 1

[Andy McKay]

We should try and raise a useful message, which sounds similar to another bug on commands. But also I'd be tempted just make a default key mandatory to solve the problem.

Comment 2

[Kris Maglione [:kmag]]

In Chrome, the command would just have no shortcut on that platform, by default, but the user would be able to assign one.

We should probably fall back to accepting the command entry but not binding it to a key.

Comment 3

[Andy McKay]

For me loading from chrome://extensions it just fails:

https://www.dropbox.com/s/tmgkkwlg1pwdio4/Screenshot%202016-05-11%2015.50.02.png?dl=0

Comment 4

[Kris Maglione [:kmag]]

Ah. Interesting. Maybe we should do something similar. But, either way, Chrome allows commands without any suggested keys, and just doesn't bind them by default.

Comment 5

[:shell escalante]

[Tracking Requested - why for this release]: this should be working in 48 for chrome requests

Comment 6

[:shell escalante]

chrome doesn't do this - but we'd like to do something better here

Comment 7

[Gerry Chang [:gchang]]

Track this as this is a improvement request.

Comment 8

[Matthew Wein [:mattw]]

Attachment: Bug 1272133 - Fix runtime error thrown when no shortcut is found for the target platform.

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8774455 [details]
Bug 1272133 - Fix runtime error thrown when no shortcut is found for the target platform.

https://reviewboard.mozilla.org/r/66894/#review64304

Comment 10

[Matthew Wein [:mattw]]

Comment on attachment 8774455 [details]
Bug 1272133 - Fix runtime error thrown when no shortcut is found for the target platform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66894/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47426821ace4
Fix runtime error thrown when no shortcut is found for the target platform. r=kmag

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47426821ace4

Comment 13

[Sylvestre Ledru [:Sylvestre]]

too late for 48. Shell, is it something that you want in 49?

Comment 14

[:shell escalante]

changing need info to Andy for his call on where to land it.  we triaged it as a P4 - so it's unlikely we would want uplift to 49 - but 50?  or just ride the trains as en enhancement?

Comment 15

[Andy McKay]

If we can still get it in 49 that would be nice. Agreed its low priority, but it can be fatal to a WebExtension as it causes the runtime error. Should be low risk as its just in the WebExtensions code and has been baking for a while on 50.

Comment 16

[:shell escalante]

=] not sure if i should need info back with the answer.  From Andyms' comment 15, the answer is "yes, in Fx49 please".  low risk, but does hit webextensions and we're trying to greatly increase the usage/availability of those in 49.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Matt, could you fill the uplift request to beta? Thanks

Comment 18

[Matthew Wein [:mattw]]

Comment on attachment 8774455 [details]
Bug 1272133 - Fix runtime error thrown when no shortcut is found for the target platform.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Medium
[Describe test coverage new/current, TreeHerder]: N/A
[Risks and why]: Low risk, and prevents a fatal runtime error from occurring if an extension tries to register a shortcut without providing a valid key for the target OS.
[String/UUID change made/needed]: N/A

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8774455 [details]
Bug 1272133 - Fix runtime error thrown when no shortcut is found for the target platform.

This patch fixes a runtime error for extensions. Take it in 49 beta.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/878f4c530d61

Comment 21

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 48.0.2 under Windows 10 64-bit.

Verified fixed on Firefox 51.0a1 (2016-09-12), Firefox 50.0a2 (2016-09-12) and Firefox 49.0 (20160907153016) under Windows 10 64-bit. The error is no longer thrown in Browser Console.

Comment 22

[Igor]

This issue is still present if I don't set "suggested_key" at all, tested on Firefox 51.0.1 (stable) and Firefox 53.0a2 (developer edition).

Example extension
https://github.com/IgorCode/webextensions-examples/tree/master/commands