Closed Bug 1272133 Opened 4 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P4)

defect

Tracking

(firefox48- wontfix, firefox49+ verified, firefox50+ verified, firefox51 verified)

VERIFIED FIXED
mozilla50
Tracking Status
firefox48 - wontfix
firefox49 + verified
firefox50 + verified
firefox51 --- verified

People

(Reporter: wbamberg, Assigned: mattw)

References

Details

(Whiteboard: [commands] triaged)

Attachments

(1 file)

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?
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.
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.
Whiteboard: [commands]
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
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.
[Tracking Requested - why for this release]: this should be working in 48 for chrome requests
chrome doesn't do this - but we'd like to do something better here
Priority: -- → P4
Whiteboard: [commands] → [commands] triaged
Track this as this is a improvement request.
Assignee: nobody → mwein
Blocks: 1289181
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
Attachment #8774455 - Flags: review?(kmaglione+bmo) → review+
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/
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47426821ace4
Fix runtime error thrown when no shortcut is found for the target platform. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47426821ace4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
too late for 48. Shell, is it something that you want in 49?
Flags: needinfo?(sescalante)
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?
Flags: needinfo?(sescalante) → needinfo?(amckay)
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.
Flags: needinfo?(amckay)
=] 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.
Flags: needinfo?(sledru)
Matt, could you fill the uplift request to beta? Thanks
Flags: needinfo?(sledru) → needinfo?(mwein)
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
Flags: needinfo?(mwein)
Attachment #8774455 - Flags: approval-mozilla-beta?
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.
Attachment #8774455 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.