1659871 - tests / code-cleanup for Alpenglow theme landing

Status: VERIFIED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Punam Dahiya [:pdahiya]]

Scope of this bug is to follow up on adding tests and performance improvement during first enable of Alpenglow theme during onboarding flow.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

it would be nice if as part of this we could also take care of some test-related followups that we did mention in the Bug 1643776 review, in particular:

• in browser_1007336_lwthemes_in_customize_mode.js it would be nice to tweak the "There should only be N themes" assertion to make it nicer and to derive the expected number of themes from the importantThemes set (which has to be exposed by CustomizeMode.jsm first), see phabricator comments 1 and comment 2

• in browser/base/content/test/static/browser_all_files_referenced.js it would be nice to adapt the part of the test that is parsing the manifest.json file so that we don't need an exception for "resource://app/modules/themes/alpenglow/images", see phabricator comment

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I've spun off the performance pieces to bug 1660568 (general theme enabling perf in about:welcome) and 1660569 (alpenglow perf in 81). So this bug remains to cover automated testing and code cleanup bits.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Right now, I'm poking at the "parse web-extension manifest.json" part of this bug (see comment 1).

:glandium, In bug 1660557, you wrote:

In any case, installing the theme files via EXTRA_JS_MODULES is a misuse of the variable.

All the themes in browser/themes/addons do this.

I don't know whether or not these two bugs effect each other in any interesting way, but I'm open to fixing up the theme file installation while I'm here. Given that this is an in-tree webextension, what is the best thing to do instead of using EXTRA_JS_MODULES? Use jar.mn? Or is there some way to get this for free just by having it in the manifest.json?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #3)

Or is there some way to get this for free just by having it in the manifest.json?

I'm writing the code that helps us get this for free, so you don't need to answer that one.

This I'm still up for:

I'm open to fixing up the theme file installation while I'm here. Given that this is an in-tree webextension, what is the best thing to do instead of using EXTRA_JS_MODULES? Use jar.mn?

Comment 5

[Mike Hommey [:glandium]]

Depends where you want to put them. If you want to put them in chrome, like the search plugins, you can use jar.mn (although technically, you can use FINAL_TARGET_FILES.chrome.whatever). If you want to put them in the features directory like the other bundled addons, you can use FINAL_TARGET_FILES.features.whatever (see, for example, browser/extensions/screenshots/moz.build). The latter would make them separate .xpis, though, so maybe that's not what you want.

I'm writing the code that helps us get this for free

Not sure what you mean here.

Comment 6

[Mike Hommey [:glandium]]

(I think it would be better to address bug 1660557 separately, fwiw, because you can't really move alpenglow without moving the others, and that's bug 1660557 territory)

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Bug 1659871 - Clean up tests + manifest parsing, add an Alpenglow test, r?pdahiya!,florian!

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

I didn't mean I was going to fix that whole bug as part of this, just that I would make alpenglow install more correctly than using EXTRA_JS_FILES, which I've done in the patch.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6eacadddf695918c550751fcaa54a9b55bc2d58

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e2abe718eb2
Clean up tests + manifest parsing, add an Alpenglow test, r=pdahiya,florian

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2e2abe718eb2

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

[Tracking Requested - why for this release]:

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 9173396 [details]
Bug 1659871 - Clean up tests + manifest parsing, add an Alpenglow test, r?pdahiya!,florian!

Beta/Release Uplift Approval Request

• User impact if declined: Likely none. The main reason to uplift is that it adds a test to make sure the Alpenglow theme is working, so if some other uplift were to break the Alpenglow theme, this test would catch it.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is mostly a testing change. There are minor tweaks of the code for testability, and a tweak of the way packaging works for correctness.
• String changes made/needed: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173396 [details]
Bug 1659871 - Clean up tests + manifest parsing, add an Alpenglow test, r?pdahiya!,florian!

Approved for 81.0b8.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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