1538583 - Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Kris Maglione [:kmag]]

Static themes are always allowed in private windows, and cannot run any code which should make that a concern. Showing this loud warning for all of them serves no purpose, and is decidedly ugly.

Comment 1

[Trishul]

If this is confirmed, can I work on this?

Comment 2

[Kris Maglione [:kmag]]

(In reply to Trishul from comment #1)

If this is confirmed, can I work on this?

Sure. There's nothing to confirm. Note that it will probably need a test, though.

Comment 3

[Shane Caraveo (:mixedpuppy)]

This should probably be done on top of D24720 in bug 1538546, or maybe even D24990 in bug 1536459 if it touches the same test code. And if we can reproduce this in beta, we'll want to uplift it.

Comment 4

[Trishul]

Thanks Shane, I am on it now.

Comment 5

[Trishul]

Attachment: Bug 1538583: Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92cfc564dbff
Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice r=kmag

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9054503 [details]
Bug 1538583: Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1380809
• User impact if declined: In about:addons list view, loud purple badges on themes when they are not applicable to themes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: load some themes and verify there are not purple badges
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple fix. Thanks Trishul!
• String changes made/needed: none

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/92cfc564dbff

Comment 9

[Pascal Chevrel:pascalc]

Just landed, I would like to see it verified on Nightly before we uplift it to Beta, thanks.

Comment 10

[CosminB]

Attachment: Bug1538583.gif

I tested on Firefox 68.0a1 (20190331215222) and on 68.0a1 (20190323094805) under Win 7 64-bit and the bug is not reproducing on the fixed build, also I can’t reproduce the bug on the affected build.

Could you please give me some STR to see how this bug is reproducing on the affected build?

Thanks!

Comment 11

[Pascal Chevrel:pascalc]

Trishul, could you get back to Cosmin please? (see comment #10) Thanks.

Comment 12

[Trishul]

Hello everyone, apologies for late reply.
For this specific bug, I didn't have STRs from UI, I had to use flags in my code to reproduce it and write tests accordingly.
Maybe Kris can help us with this.

Comment 13

[Kris Maglione [:kmag]]

You should just need to install a static WebExtension theme and look at the themes section of about:addons

Comment 14

[Shane Caraveo (:mixedpuppy)]

:Trishul, could you do a followup patch where you add the same check in Extension.jsm? That is the actual point where the builtin themes are gaining the permission.

https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/toolkit/components/extensions/Extension.jsm#1932

QE: you wont be able to reproduce because a patch on nightly that kmag landed caused the issue, and the patch here, which has landed, hides the issue.

This is not reproducible on beta, but my request to Trishul above should go onto beta to prevent giving the permission to non-extensions on upgrade. If you undo the patch here, you'll be able to see the badge for any builtin theme that has been enabled.

Comment 15

[Shane Caraveo (:mixedpuppy)]

patch on nightly that kmag landed caused the issue

exposed the issue.

Comment 16

[Trishul]

Sure, on it.

Comment 17

[Trishul]

Attachment: Bug 1538583

Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9054503 [details]
Bug 1538583: Themes should not show [ALLOWED IN PRIVATE WINDOWS] notice

Uplift approved for 67 beta 10, thanks.

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44282db3178b

Comment 20

[CosminB]

Attachment: Bug1538583.png

This issue is verified as fixed on Firefox 69.0a1 (20190522152821), Firefox 68.0b3 (20190521110747) and Firefox 67.0 (20190516215225) under Win 7 64-bit and Mac OS X 10.14.1.

There are no purple badges displayed for the installed themes.

Please see the attached screenshot.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Trishul, was D26298 needed? If so, update it and get it landed, otherwise abandon the change.

Comment 22

[Trishul]

Hi Shane, I believe its an overkill and is already handled by the previous patch D25411, so I will abandon this change.