1359704 - ContextMenu context PAGE also appears in TAB

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[erosman [:erosman]]

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.

Comment 1

[Tomislav Jovanovic :zombie]

looking into

Comment 2

[Tomislav Jovanovic :zombie]

Yeah, oops

http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#331-337

Comment 3

[Tomislav Jovanovic :zombie]

Attachment: Bug 1359704 - "page" context items should not appear in "tab" context

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

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8862427 [details]
Bug 1359704 - "page" context items should not appear in "tab" context

https://reviewboard.mozilla.org/r/134354/#review137370

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd7a3a0608d7
"page" context items should not appear in "tab" context r=mixedpuppy

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd7a3a0608d7

Comment 7

[erosman [:erosman]]

Out of curiosity, how come the fixed wasn't applied to upcoming FF54?

Comment 8

[Tomislav Jovanovic :zombie]

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.

Comment 9

[erosman [:erosman]]

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? :)

Comment 10

[Tomislav Jovanovic :zombie]

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?

Comment 11

[Shane Caraveo (:mixedpuppy)]

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 12

[Tomislav Jovanovic :zombie]

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

Comment 14

[Gerry Chang [:gchang]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Andrei Vaida [:avaida]]

(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.