Closed
Bug 1271777
Opened 8 years ago
Closed 7 years ago
"MacCtr" typo in commands API
Categories
(WebExtensions :: Untriaged, defect, P1)
WebExtensions
Untriaged
Tracking
(firefox47- wontfix, firefox48+ fixed, firefox49 verified)
VERIFIED
FIXED
mozilla49
People
(Reporter: wbamberg, Assigned: mattw)
References
Details
(Whiteboard: [commands]triaged)
Attachments
(2 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
kmag
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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
Updated•8 years ago
|
Assignee: nobody → mwein
Comment 1•8 years ago
|
||
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:
--- → ?
Updated•8 years ago
|
Whiteboard: [commands]triaged
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51739/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51739/
Attachment #8751001 -
Flags: review?(kmaglione+bmo)
Comment 4•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
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.
Updated•7 years ago
|
Severity: normal → major
Priority: -- → P1
Assignee | ||
Comment 9•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54950/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54950/
Attachment #8756093 -
Flags: review?(kmaglione+bmo)
Attachment #8751001 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 10•7 years ago
|
||
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/
Assignee | ||
Comment 11•7 years ago
|
||
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/
Assignee | ||
Updated•7 years ago
|
Attachment #8756093 -
Attachment is obsolete: true
Attachment #8756093 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a6f4ddbb9e
Assignee | ||
Comment 13•7 years ago
|
||
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?
Assignee | ||
Comment 14•7 years ago
|
||
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/
Updated•7 years ago
|
Attachment #8751001 -
Flags: review?(kmaglione+bmo) → review+
Comment 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
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/
Assignee | ||
Comment 17•7 years ago
|
||
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/
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/981fc919965e
Keywords: checkin-needed
Comment 20•7 years ago
|
||
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
Assignee | ||
Comment 21•7 years ago
|
||
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/
Assignee | ||
Comment 22•7 years ago
|
||
Sorry about that Ryan, the failure is fixed now.
Keywords: checkin-needed
Assignee | ||
Comment 23•7 years ago
|
||
Oh sorry, I need to also roll in the ESLint fix
Keywords: checkin-needed
Assignee | ||
Comment 24•7 years ago
|
||
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 26•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cbeef9c81af6
Keywords: checkin-needed
Comment 27•7 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/a5b1bd969d19
Comment 28•7 years ago
|
||
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9626235&repo=fx-team
Flags: needinfo?(mwein)
Assignee | ||
Comment 29•7 years ago
|
||
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/
Assignee | ||
Comment 30•7 years ago
|
||
3rd try's a charm - let's hope it never takes this many attempts in the future :)
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 31•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ea4940dd52cc
Keywords: checkin-needed
Comment 32•7 years ago
|
||
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)
Comment 33•7 years ago
|
||
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
Assignee | ||
Comment 34•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2343fbe633ce
Comment 35•7 years ago
|
||
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)
Comment 37•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 40•7 years ago
|
||
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)
Comment 41•7 years ago
|
||
Assignee | ||
Comment 42•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(overholt)
Comment 43•7 years ago
|
||
Why reason why this didn't land in 49 yet?
Comment 44•7 years ago
|
||
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•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/87207eee5e4785e29d405e7f3586d7bde08b91ce Bug 1271777 - Fix the 'MacCtr' typo in the commands API. r=kmag
Comment 46•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87207eee5e47
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 47•7 years ago
|
||
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 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 49•7 years ago
|
||
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+
Comment 50•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ff2241804c18
Updated•7 years ago
|
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo)
Updated•5 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•