Closed Bug 1659871 Opened 1 year ago Closed 1 year ago

tests / code-cleanup for Alpenglow theme landing

Categories

(Firefox :: Messaging System, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
82 Branch
Iteration:
82.1 - Aug 24 - Sep 6
Tracking Status
firefox81 --- fixed
firefox82 --- fixed

People

(Reporter: pdahiya, Assigned: dmose)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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

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

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.

Assignee: nobody → dmose
Summary: Add tests and performance followup fixes for Alpenglow theme → tests / code-cleanup for Alpenglow theme landing
Iteration: --- → 82.1 - Aug 24 - Sep 6
Priority: -- → P1

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?

Flags: needinfo?(mh+mozilla)

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

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.

Flags: needinfo?(mh+mozilla)

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

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.

Pushed by dmosedale@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e2abe718eb2
Clean up tests + manifest parsing, add an Alpenglow test, r=pdahiya,florian
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

[Tracking Requested - why for this release]:

Status: RESOLVED → VERIFIED

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
Attachment #9173396 - Flags: approval-mozilla-beta?

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

Approved for 81.0b8.

Attachment #9173396 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.