Closed Bug 1359704 Opened 3 years ago Closed 3 years ago
Menu context PAGE also appears in TAB
59 bytes, text/x-review-board-request
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170421122919 Steps to reproduce: I noticed that setting contexts: ['page'] in contextMenus.create also causes the contextmenu to appear in Tab Contextmenu. While this is not a problem, it seems semantically incorrect. N.B. Only tested on FF54. I am not aware if it was present in FF53.
Assignee: nobody → tomica
Yeah, oops http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#331-337
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8862427 - Flags: review?(mixedpuppy)
Comment on attachment 8862427 [details] Bug 1359704 - "page" context items should not appear in "tab" context https://reviewboard.mozilla.org/r/134354/#review137370
Attachment #8862427 - Flags: review?(mixedpuppy) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/dd7a3a0608d7 "page" context items should not appear in "tab" context r=mixedpuppy
Out of curiosity, how come the fixed wasn't applied to upcoming FF54?
By default, patches land in Nightly (which is currently 55), and ride the train to release, unless it's a security / high-impact breaking bug, in which case one could ask for approval to uplift directly to beta/release. While this bug is unfortunate, it doesn't seem like it would break any extensions, so I didn't ask for approval. Feel free to disagree, and give your reasons why.
As mentioned, I was curious. I was under the impression that any yet-unreleased version is still under development and therefore fixes are applied until the day the version is released to public. I am currently on 54.0b2 and there are already nearly 20 minor updates to FF54 since I downloaded that version. http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32-add-on-devel/ I also thought that security updates will be applied regardless of the release/beta/nightly procedure ie as security update with minor version increments. Firefox 54 is due to be released 2017-06-13, 1.5 months away. Leaving bugs (even minor one like this) for a release 3.5 months away (FF55) seems rather strange. As you have said, it will not break any extension, but why wait to correct an error? :)
Every change, no matter how minor, can be risky, and you have to stop changing things at one point in order to test/stabilize before release. We don't actually "stop", but limit to high-impact / low risk changes, or some favorable combination of those factors. This is rather low-impact, but also admittedly very low risk, and I'm not actually sure what's the policy in cases like this. Shane, do you think we wanna ask for uplift here?
I think this change is low risk, but not very high gain. However, given it's a very small patch that fixes broken behavior, and it's relatively early in beta, I'd be ok to go with the risk of uplift. Otherwise its over 4 months before the fix hits release.
Comment on attachment 8862427 [details] Bug 1359704 - "page" context items should not appear in "tab" context Approval Request Comment >[Feature/Bug causing the regression]: bug 1316020 >[User impact if declined]: Extension context menu items appear in the wrong context. >[Is this code covered by automated tests?]: Yes >[Has the fix been verified in Nightly?]: Yes >[Needs manual test from QE? If yes, steps to reproduce]: No >[List of other uplifts needed for the feature/fix]: None >[Is the change risky?]: No >[Why is the change risky/not risky?]: Very small change, good test coverage. >[String changes made/needed]: None
Attachment #8862427 - Flags: approval-mozilla-beta?
Comment on attachment 8862427 [details] Bug 1359704 - "page" context items should not appear in "tab" context Fix an extension context menu item issue. Beta54+. Should be in 54 beta 4.
Attachment #8862427 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Tomislav Jovanovic :zombie from comment #12) > >[Is this code covered by automated tests?]: > Yes > > >[Has the fix been verified in Nightly?]: > Yes > > >[Needs manual test from QE? If yes, steps to reproduce]: > No Setting qe-verify- based on Tomislav's assessment on manual testing needs and the fact that this fix has automated coverage.
You need to log in before you can comment on or make changes to this bug.