Closed Bug 1271777 Opened 7 years ago Closed 7 years ago

"MacCtr" typo in commands API

Categories

(WebExtensions :: Untriaged, defect, P1)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(firefox47- wontfix, firefox48+ fixed, firefox49 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox47 - wontfix
firefox48 + fixed
firefox49 --- verified

People

(Reporter: wbamberg, Assigned: mattw)

References

Details

(Whiteboard: [commands]triaged)

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1271777 - Fix the 'MacCtr' typo in the commands API r?kmag
58 bytes, text/x-review-board-request
kmag
: review+
Sylvestre
: approval-mozilla-beta+
Details
Patch without test changes
919 bytes, patch
Details | Diff | Splinter Review 
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
Assignee: nobody → mwein
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.
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
Whiteboard: [commands]triaged
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.
Attachment #8751001 - Flags: review?(kmaglione+bmo)
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?
Flags: needinfo?(mwein)
It's unrelated, but it's also not a bug. You just need to remove the `exclude_matches` property from your content script.
Flags: needinfo?(mwein)
(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.
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.
status-firefox47: affected → wontfix
tracking-firefox47: ? → -
Severity: normal → major
Priority: -- → P1
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 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/
Attachment #8756093 - Attachment is obsolete: true
Attachment #8756093 - Flags: review?(kmaglione+bmo)
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
Attachment #8751001 - Flags: approval-mozilla-aurora?
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/
Attachment #8751001 - Flags: review?(kmaglione+bmo) → review+
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 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 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/
Keywords: checkin-needed
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 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/
Sorry about that Ryan, the failure is fixed now.
Keywords: checkin-needed
Oh sorry, I need to also roll in the ESLint fix
Keywords: checkin-needed
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/
Ready.
Keywords: checkin-needed
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9626235&repo=fx-team
Flags: needinfo?(mwein)
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/
3rd try's a charm - let's hope it never takes this many attempts in the future :)
Flags: needinfo?(mwein)
Keywords: checkin-needed
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 ?
Flags: needinfo?(mwein)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dc4d7f68030e
Backed out changeset ea4940dd52cc for test failures in browser_ext_commands_onCommand.js
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?
Flags: needinfo?(rkothari)
(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?
Flags: needinfo?(rkothari)
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.
Based on comment 37 I think we should uplift to 47.

Thanks for the risk assessment, Kris.
Flags: needinfo?(rkothari)
(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.
Flags: needinfo?(rkothari)
Flags: needinfo?(overholt)
Flags: needinfo?(kmaglione+bmo)
If we're willing to uplift without tests (which I think is sensible in this case), then yes, it's ready.
Flags: needinfo?(kmaglione+bmo)
Thanks for taking care of this, Kris.  I think it makes sense to add the tests for this as a part of bug 1272198.
Flags: needinfo?(mwein)
Flags: needinfo?(overholt)
Why reason why this didn't land in 49 yet?
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.
https://hg.mozilla.org/mozilla-central/rev/87207eee5e47
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
Flags: needinfo?(kmaglione+bmo)
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)
Attachment #8751001 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #8751001 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.