1271777 - "MacCtr" typo in commands API

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Will Bamberg [:wbamberg]]

The regexp used to validate key shortcuts in the commands API specifies "MacCtr" instead of "MacCtrl".

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/schemas/commands.json#14

Comment 1

[Kris Maglione [:kmag]]

This is going to need uplift once it's fixed.

[Tracking Requested - why for this release]:

Firefox 48 is meant to be the first stable release of WebExtensions. This is a fairly trivial fix for a problem that could have a significant impact on Mac users/developers.

Comment 2

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

https://reviewboard.mozilla.org/r/51739/#review49566

Gah, sorry, I didn't notice this patch before now.

Looks good, but we should really add tests for all of the modifiers we expect to support.

Comment 5

[CosminB]

I received a different error in the browser console for the following webextension: https://addons-dev.allizom.org/en-US/firefox/addon/buzzsumo2/

1463729196840	addons.webextension.<unknown>	ERROR	Loading extension 'null': Reading manifest: Error processing content_scripts.0.exclude_matches: Array requires at least 1 items; you have 0   Log.jsm:753	

Is this the same issue or should I file another bug?

Comment 6

[Kris Maglione [:kmag]]

It's unrelated, but it's also not a bug. You just need to remove the `exclude_matches` property from your content script.

Comment 7

[CosminB]

(In reply to Kris Maglione [:kmag] from comment #6)
> It's unrelated, but it's also not a bug. You just need to remove the
> `exclude_matches` property from your content script.

Thanks for the info! Now it is working.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

At this point, I would like to only accept uplifts for critical recent regressions, severe stability, security, perf, mlk issues only. This is to minimize code churn and to ensure we ship a high-quality Fx47 in 2 weeks.

Given that we are a week away from entering RC mode for Fx47, this is now a wontfix for that version.

Comment 9

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

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

Comment 10

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

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

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

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

Comment 12

[Matthew Wein [:mattw]]

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

Comment 13

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Approval Request Comment
[Feature/regressing bug #]: 1242557
[User impact if declined]: 
This could have a significant impact on Mac users/developers.
[Describe test coverage new/current, TreeHerder]: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e56d2fd490

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/3-4/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

https://reviewboard.mozilla.org/r/51739/#review51990

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:96
(Diff revision 4)
> +
> +  EventUtils.synthesizeKey("VK_PAGE_UP", {accelKey: true});
> +  message = yield extension.awaitMessage("oncommand");
> +  is(message, "toggle-feature-using-control-page-up", "Expected onCommand listener to fire with correct message");
> +
> +  EventUtils.synthesizeKey("5", {ctrlKey: true});

We should only test this on on OS-X.

Comment 16

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/4-5/

Comment 17

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/5-6/

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/981fc919965e

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf7e08007f9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_ext_commands_onCommand.js failures. Please make sure the ESLint fix gets rolled into your updated patch as well.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cca36be27a57

https://treeherder.mozilla.org/logviewer.html#?job_id=28875439&repo=mozilla-inbound#L2640

Comment 21

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/6-7/

Comment 22

[Matthew Wein [:mattw]]

Sorry about that Ryan, the failure is fixed now.

Comment 23

[Matthew Wein [:mattw]]

Oh sorry, I need to also roll in the ESLint fix

Comment 24

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/7-8/

Comment 25

[Matthew Wein [:mattw]]

Ready.

Comment 26

[Pulsebot]

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

Comment 27

[Pulsebot]

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

Comment 28

[Carsten Book [:Tomcat]]

sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9626235&repo=fx-team

Comment 29

[Matthew Wein [:mattw]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51739/diff/8-9/

Comment 30

[Matthew Wein [:mattw]]

3rd try's a charm - let's hope it never takes this many attempts in the future :)

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

sorry need to backout again for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9647452&repo=fx-team

maybe we need a try run ?

Comment 33

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dc4d7f68030e
Backed out changeset ea4940dd52cc for test failures in browser_ext_commands_onCommand.js

Comment 34

[Matthew Wein [:mattw]]

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

Comment 35

[Andrew Overholt [:overholt]]

Given bug 1272323 is tracked as a regression in Firefox 47 but was marked as a dupe of this, can we consider uplifting this fix to beta?

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Andrew Overholt [:overholt] from comment #35)
> Given bug 1272323 is tracked as a regression in Firefox 47 but was marked as
> a dupe of this, can we consider uplifting this fix to beta?

Hi Andrew, I decided to wontfix this for Fx47 due to comment 1. We gtb RC1 in 2 hrs, and issues that are potential dot-release drivers or release blockers are making the cut. Based on that criteria, do you consider this safe and critical enough to uplift to RC1 47?

Comment 37

[Kris Maglione [:kmag]]

For what it's worth, the fix alone without the tests is a 1-character change to a regular expression. It should be extremely safe to uplift.

Comment 38

[Andrew Overholt [:overholt]]

Based on comment 37 I think we should uplift to 47.

Thanks for the risk assessment, Kris.

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Andrew Overholt [:overholt] from comment #38)
> Based on comment 37 I think we should uplift to 47.
> 
> Thanks for the risk assessment, Kris.

Sure. I gtb RC1 in less than an hour and this hasn't even landed in m-c. Hi Kris, Andrew, is it ready for uplift now or does it need to land in Nightly first? Please let me know.

Comment 40

[Kris Maglione [:kmag]]

If we're willing to uplift without tests (which I think is sensible in this case), then yes, it's ready.

Comment 41

[Kris Maglione [:kmag]]

Attachment: Patch without test changes

Comment 42

[Matthew Wein [:mattw]]

Thanks for taking care of this, Kris.  I think it makes sense to add the tests for this as a part of bug 1272198.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Why reason why this didn't land in 49 yet?

Comment 44

[Kris Maglione [:kmag]]

The first version got backed out for failing tests. I guess no-one landed the version without the tests after they got moved to another bug.

Comment 45

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/87207eee5e4785e29d405e7f3586d7bde08b91ce
Bug 1271777 - Fix the 'MacCtr' typo in the commands API. r=kmag

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87207eee5e47

Comment 47

[Vasilica Tamas, QA [:vtamas]]

Reproduced the initial issue on Firefox 48.0a2 (2016-05-31) under Windows 10 64-bit.

Verified fixed on Firefox 49.0a2 (2016-06-07) under Windows 10 64-bit and Mac OS X 10.11. The following webextension https://addons.allizom.org/en-US/firefox/addon/tab-resize-ica-test/ (from Bug 1272323) is successfully installed.

Any thoughts about uplifting this on Firefox 48?

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Switching approval flag for aurora to beta (for 48)

Comment 49

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8751001 [details]
MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag

Simple change, verified, taking it.
Should be in 48 beta 2.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff2241804c18