Closed
Bug 1271777
Opened 9 years ago
Closed 9 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•9 years ago
|
Assignee: nobody → mwein
Comment 1•9 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•9 years ago
|
Whiteboard: [commands]triaged
Assignee | ||
Comment 2•9 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•9 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•9 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•9 years ago
|
Severity: normal → major
Priority: -- → P1
Assignee | ||
Comment 9•9 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•9 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•9 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•9 years ago
|
Attachment #8756093 -
Attachment is obsolete: true
Attachment #8756093 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 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•9 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•9 years ago
|
Attachment #8751001 -
Flags: review?(kmaglione+bmo) → review+
Comment 15•9 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•9 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•9 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•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Keywords: checkin-needed
Comment 19•9 years ago
|
||
Comment 20•9 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•9 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•9 years ago
|
||
Sorry about that Ryan, the failure is fixed now.
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
Oh sorry, I need to also roll in the ESLint fix
Keywords: checkin-needed
Assignee | ||
Comment 24•9 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•9 years ago
|
||
Keywords: checkin-needed
Comment 27•9 years ago
|
||
Comment 28•9 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•9 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•9 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•9 years ago
|
||
Keywords: checkin-needed
Comment 32•9 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•9 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•9 years ago
|
||
Comment 35•9 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•9 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•9 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•9 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•9 years ago
|
||
Assignee | ||
Comment 42•9 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•9 years ago
|
Flags: needinfo?(overholt)
Comment 43•9 years ago
|
||
Why reason why this didn't land in 49 yet?
Comment 44•9 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•9 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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 47•9 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 48•9 years ago
|
||
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•9 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•9 years ago
|
||
bugherder uplift |
Updated•9 years ago
|
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•