Closed Bug 1372694 Opened 2 years ago Closed 2 years ago

Remove heavy weight theme support

Categories

(Toolkit :: Add-ons Manager, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox57 --- unaffected
firefox61 --- verified
firefox62 --- verified

People

(Reporter: andy+bugzilla, Assigned: kmag)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

With Firefox 57 heavy weight themes will no longer be supported. This bug is to remove their support from the add-ons manager.
Severity: normal → enhancement
Priority: -- → P3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1448221
Bug 1448221 gets us most of the way, but there's still a bit more work we should do.

We still treat the default theme as a heavyweight theme, and have some funky code in XPIProvider to support it. We just don't allow installing any other legacy themes.

What we should really do is move it to omni.ja and register its manifest by default, and then make the default theme in the add-on manager a stub lightweight theme. That would massively simplify all sorts of stuff.
Status: RESOLVED → REOPENED
Depends on: 1448221
Resolution: DUPLICATE → ---
Attached patch dummy-default-theme.patch (obsolete) — Splinter Review
Attachment #8964135 - Attachment is obsolete: true
Assignee: nobody → ntim.bugs
Status: REOPENED → ASSIGNED
Is there anything I can do to make the transition easier for TB ?
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2)
> make the default theme in the add-on manager a stub
> lightweight theme.

I'm not sure this really makes sense, at least not the way Tim implemented it with nsBrowserGlue registering that stub. Wouldn't it be more straightforward if the addon manager itself had a default state with no lightweight theme selected?
(In reply to Tim Nguyen :ntim from comment #5)
> Is there anything I can do to make the transition easier for TB ?
File a bug and give us a patch ;-) (Just joking, no offence intended).
What are the consequences? Is there more to it than porting the browser bits? Or is our Default theme much different to the one FF has?
Flags: needinfo?(jorgk)
I'm no expert in this but I think we need to port FX's LightweightThemeManager for dark/light theme implementation to be able to register the theme as a lightweight theme. What would be needed for this?
Flags: needinfo?(richard.marti)
(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #2)
> > make the default theme in the add-on manager a stub
> > lightweight theme.
> 
> I'm not sure this really makes sense, at least not the way Tim implemented
> it with nsBrowserGlue registering that stub. Wouldn't it be more
> straightforward if the addon manager itself had a default state with no
> lightweight theme selected?

I agree that registering it in nsBrowserGlue doesn't make sense. It should be created by the add-on manager. But from the add-on manager perspective, it's much simpler if we have an actual add-on to represent the default theme, or it would need to be special-cased in a bunch of different parts of the UI.
Comment on attachment 8964139 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/232902/#review238348


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4981
(Diff revision 3)
> -          throw new Error("Cannot disable the default theme");
> -
> -        let defaultTheme = XPIDatabase.getVisibleAddonForInternalName(DEFAULT_SKIN);
> -        XPIProvider.updateAddonDisabledState(defaultTheme, false);
> -      }
>        if (!(theme && val) || isWebExtension(addon.type)) {

Error: 'theme' is not defined. [eslint: no-undef]

::: toolkit/mozapps/extensions/test/xpcshell/test_addonStartup.js:7
(Diff revision 3)
> -          }
> -        },
> -        "checkStartupModifications": true,
> -        "path": "c:\\Program Files\\Mozilla Firefox\\extensions",
> -      },
>        "app-profile": {

Error: Parsing error: unexpected token : [eslint]
(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #2)
> > make the default theme in the add-on manager a stub
> > lightweight theme.
> 
> I'm not sure this really makes sense, at least not the way Tim implemented
> it with nsBrowserGlue registering that stub. Wouldn't it be more
> straightforward if the addon manager itself had a default state with no
> lightweight theme selected?

The latest revision now registers the stub from LWTManager.jsm.

(In reply to Jorg K (GMT+1) from comment #8)
> (In reply to Tim Nguyen :ntim from comment #5)
> > Is there anything I can do to make the transition easier for TB ?
> File a bug and give us a patch ;-) (Just joking, no offence intended).
> What are the consequences? Is there more to it than porting the browser
> bits? Or is our Default theme much different to the one FF has?

The breaking changes I can think of is the removal of heavyweight themes internally, MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES, and of course the browser changes.
Comment on attachment 8964139 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/232902/#review238372

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:624
(Diff revision 4)
> +);
> +LightweightThemeManager.addBuiltInTheme({
> +  id: "default-theme@mozilla.org",
> +  name: extensionsBundle.GetStringFromName("defaultTheme.name"),
> +  description: extensionsBundle.GetStringFromName("defaultTheme.description"),
> +  iconURL: "chrome://browser/content/default-theme-icon.svg",

This should be a toolkit resource, maybe with browser overriding that resource or changing the iconURL or something.
Judging by the comments so far, I think Dão should review this.
Attachment #8964139 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (back Tuesday April 3rd) from comment #18)
> Judging by the comments so far, I think Dão should review this.

(FWIW, I moved the r? in reviewboard to Dão, but because Dão has already used reviewboard, apparently the result is that the r? just gets cleared...)
Comment on attachment 8964139 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/232902/#review238632

This is a good start, but there's a bunch of other stuff that can go too, now:

https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/mozapps/extensions/AddonManagerStartup.cpp#584-610
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/mozapps/extensions/AddonManagerStartup.cpp#511-535
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/mozapps/extensions/AddonManagerStartup.cpp#444-452
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/mozapps/extensions/AddonManagerStartup.h#41-49
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/mozapps/extensions/AddonManagerStartup.h#58-59

Thanks!

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:143
(Diff revision 6)
>  // Retired values:
>  // 32 = multipackage xpi file
>  // 8 = locale
>  const TYPES = {
>    extension: 2,
>    theme: 4,

This can go.

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:638
(Diff revision 6)
>    if (isTheme(addon.type)) {
> -    addon.userDisabled = !!LightweightThemeManager.currentTheme ||
> +    addon.userDisabled = !!LightweightThemeManager.currentTheme;

This whole branch can go away.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3593
(Diff revision 6)
>    addonChanged(aId, aType) {
>      // We only care about themes in this provider
>      if (!isTheme(aType))
>        return;
>  
>      let addons = XPIDatabase.getAddonsByType("theme", "webextension-theme");

"theme" can go.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3595
(Diff revision 6)
>      if (!isTheme(aType))
>        return;
>  
>      let addons = XPIDatabase.getAddonsByType("theme", "webextension-theme");
>      for (let theme of addons) {
>        if (isWebExtension(theme.type) && theme.visible && theme.id != aId)

The `isWebExtension check can go.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
(Diff revision 6)
> -    let defaultTheme = XPIDatabase.getVisibleAddonForInternalName(DEFAULT_SKIN);
> -    this.updateAddonDisabledState(defaultTheme, aId && aId != defaultTheme.id);

We still need something like this so we fall back to enabling the default theme when another theme is disabled.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:55
(Diff revision 6)
>  const KEY_APP_GLOBAL                  = "app-global";
>  const KEY_APP_TEMPORARY               = "app-temporary";
>  
>  // Properties to save in JSON file
>  const PROP_JSON_FIELDS = ["id", "syncGUID", "location", "version", "type",
>                            "internalName", "updateURL", "optionsURL",

"internalName" can go, both here and elsewhere.
Attachment #8964139 - Flags: review?(kmaglione+bmo)
Assignee: ntim.bugs → kmaglione+bmo
Attachment #8964139 - Attachment is obsolete: true
Comment on attachment 8969841 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/238684/#review244460

::: toolkit/mozapps/extensions/content/extensions.js:248
(Diff revision 2)
> -    legacy = !(addon.isWebExtension || addon.id.endsWith("@personas.mozilla.org"));
> +    legacy = !(addon.isWebExtension || addon.id.endsWith("@personas.mozilla.org") ||
> +               addon.id == "default-theme@mozilla.org");

The built-in dark/light themes are also LWT internally, shouldn't they be in this check ?
Comment on attachment 8969841 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/238684/#review244460

> The built-in dark/light themes are also LWT internally, shouldn't they be in this check ?

Nope. I special-cased the default theme ID so that it didn't get an @personas.mozilla.org suffix, which means it failed this check by default. The others still get that suffix, so they don't need another special case here.

All of this gunk will be able to go away when we migrate personas to WebExtension themes, though.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #26)
> Comment on attachment 8969841 [details]
> Bug 1372694 - Stop making the default theme a heavyweight theme.
> 
> https://reviewboard.mozilla.org/r/238684/#review244460
> 
> > The built-in dark/light themes are also LWT internally, shouldn't they be in this check ?
> 
> Nope. I special-cased the default theme ID so that it didn't get an
> @personas.mozilla.org suffix, which means it failed this check by default.
> The others still get that suffix, so they don't need another special case
> here.

The dark/light themes don't use the @mozilla.org as opposed to the @personas.mozilla.org suffix: https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#699,708
(In reply to Tim Nguyen :ntim from comment #27)
> The dark/light themes don't use the @mozilla.org as opposed to the
> @personas.mozilla.org suffix:
> https://searchfox.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#699,708

Urgh, meant the other way round.

The dark/light themes use the @mozilla.org as opposed to the
@personas.mozilla.org suffix*.
The LightweightThemeManager code adds "@personas.mozilla.org" to the ID that you give it for the theme. I special-cased the default theme, because I thought it would be too confusing and annoying for that case. I left the others as-is for the moment, though.
That doesn't seem like correct behaviour for the light/dark themes: `firefox-compact-light@mozilla.org@personas.mozilla.org` and `firefox-compact-dark@mozilla.org@personas.mozilla.org` seem like weird IDs to me.
Comment on attachment 8969841 [details]
Bug 1372694 - Stop making the default theme a heavyweight theme.

https://reviewboard.mozilla.org/r/238684/#review244654

This is great, thanks!

::: browser/components/customizableui/CustomizeMode.jsm
(Diff revision 2)
>        tbb.setAttribute("label", aTheme.name);
> -      if (aDefaultTheme == aTheme) {
> -        // The actual icon is set up so it looks nice in about:addons, but
> -        // we'd like the version that's correct for the OS we're on, so we set
> -        // an attribute that our styling will then use to display the icon.
> -        tbb.setAttribute("defaulttheme", "true");

it looks like there is a special case rule in browser/themes/shared/customizableui/customizeMode.inc.css for this that can go now
Attachment #8969841 - Flags: review?(aswan) → review+
Comment on attachment 8969842 [details]
Bug 1372694: Part 2 - Remove support for registering non-bootstrapped extension chrome.

https://reviewboard.mozilla.org/r/238686/#review244676

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
(Diff revision 2)
> -      if (!(isTheme(addon.type) && val) || isWebExtension(addon.type)) {
> -        // hidden and system add-ons should not be user disabled,
> +      // hidden and system add-ons should not be user disabled,
> -        // as there is no UI to re-enable them.
> +      // as there is no UI to re-enable them.
> -        if (this.hidden) {
> +      if (this.hidden) {
> -          throw new Error(`Cannot disable hidden add-on ${addon.id}`);
> +        throw new Error(`Cannot disable hidden add-on ${addon.id}`);
> -        }
> +      }
> -        XPIProvider.updateAddonDisabledState(addon, val);
> +      XPIProvider.updateAddonDisabledState(addon, val);
> -      }

should this be in part 1?
Attachment #8969842 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0819fc14fc57f1a10a9aaf179882a532ac2f6835
Bug 1372694 - Stop making the default theme a heavyweight theme. r=kmag,aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb7337970f19723f9ba34a682b4682c53863e5a
Bug 1372694: Part 2 - Remove support for registering non-bootstrapped extension chrome. r=aswan
https://hg.mozilla.org/mozilla-central/rev/0819fc14fc57
https://hg.mozilla.org/mozilla-central/rev/6bb7337970f1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Tim Nguyen :ntim from comment #30)
> That doesn't seem like correct behaviour for the light/dark themes:
> `firefox-compact-light@mozilla.org@personas.mozilla.org` and
> `firefox-compact-dark@mozilla.org@personas.mozilla.org` seem like weird IDs
> to me.

Agreed, it's gross, but it's another bug.
Depends on: 1418627
Retested and reproduced in Firefox 56.0a1 (20170629030206).
Retested and verified in Firefox 62.0a1 (20180508100105) on Windows 10 x64 and MacOS 10.13.3.
Status: RESOLVED → VERIFIED
Depends on: 1457078
Attachment #8969841 - Flags: review?(kmaglione+bmo)
Depends on: 1492519
You need to log in before you can comment on or make changes to this bug.