Bug 1525511: Part 2c - Stop using LightweightThemeManager for customize mode themes menu. r=aswan
ClosedPublic
Actions

Authored by • kmag on Feb 6 2019, 5:36 AM.

Details

Reviewers

Commits

rMOZILLACENTRAL7262fc6d3c83: Bug 1525511: Part 2c - Stop using LightweightThemeManager for customize mode…
Restricted Diffusion Commit

Bugzilla Bug ID

1525511

Summary

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• kmag created this revision.Feb 6 2019, 5:36 AM

Herald added a project: secure-revision. · View Herald TranscriptFeb 6 2019, 5:36 AM

Harbormaster completed remote builds in B37866: Diff 58970.Feb 6 2019, 5:36 AM

• kmag added a child revision: D18785: Bug 1525511: Part 2d - Stop using LightweightThemeManager for in CustomizableUI. r=aswan.Feb 6 2019, 5:36 AM

phab-bot requested review of this revision.Feb 6 2019, 5:37 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

@kmag Since this is more-or-less adding support for static themes in the customization mode popup, could you add this test: https://hg.mozilla.org/mozreview/gecko/rev/5436d923b23de7b6d9066e8bbe37ba9a2f99c3a1#l4.3 ?

Also, there's some sporadic bug fixes for static themes in that same patch that you may want to integrate.

aswan added inline comments.Feb 6 2019, 9:01 PM

browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
69	would an AddonListener work here? (and below)

In D18784#482645, @ntim wrote:

@kmag Since this is more-or-less adding support for static themes in the customization mode popup, could you add this test

No, this will already handle testing static themes when we migrate the built-in themes. If you want to add a test in another bug, that's fine with me.

browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
69	No. There's async work that happens after the add-on listener fires.

In D18784#482700, @kmag wrote:

In D18784#482645, @ntim wrote:

@kmag Since this is more-or-less adding support for static themes in the customization mode popup, could you add this test

No, this will already handle testing static themes when we migrate the built-in themes. If you want to add a test in another bug, that's fine with me.

@kmag That's not going to happen anytime soon, not until this l10n issue is solved at least: https://bugzilla.mozilla.org/show_bug.cgi?id=1457865

In the meanwhile, it would be nice to have testing coverage for actual features we ship, which includes static theme support in the customization mode in this case.

In D18784#482727, @ntim wrote:

In D18784#482700, @kmag wrote:

In D18784#482645, @ntim wrote:

@kmag Since this is more-or-less adding support for static themes in the customization mode popup, could you add this test

No, this will already handle testing static themes when we migrate the built-in themes. If you want to add a test in another bug, that's fine with me.

@kmag That's not going to happen anytime soon, not until this l10n issue is solved at least: https://bugzilla.mozilla.org/show_bug.cgi?id=1457865

In the meanwhile, it would be nice to have testing coverage for actual features we ship, which includes static theme support in the customization mode in this case.

It is going to happen soon. In any case, supporting static themes isn't really my goal here. My goal is removing code that touches LightweightThemeManager

aswan added inline comments.Feb 11 2019, 9:58 PM