Closed
Bug 1372694
Opened 7 years ago
Closed 6 years ago
Remove heavy weight theme support
Categories
(Toolkit :: Add-ons Manager, enhancement, P3)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox57 | --- | unaffected |
firefox61 | --- | verified |
firefox62 | --- | verified |
People
(Reporter: andy+bugzilla, Assigned: kmag)
References
(Depends on 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.
Reporter | ||
Updated•7 years ago
|
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 2•6 years ago
|
||
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.
Comment 3•6 years ago
|
||
Updated•6 years ago
|
Attachment #8964135 -
Attachment is obsolete: true
Updated•6 years ago
|
Assignee: nobody → ntim.bugs
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
Is there anything I can do to make the transition easier for TB ?
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
(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?
Comment 8•6 years ago
|
||
(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)
Comment 9•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
(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 12•6 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
(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 15•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
Judging by the comments so far, I think Dão should review this.
Updated•6 years ago
|
Attachment #8964139 -
Flags: review?(gijskruitbosch+bugs)
Comment 19•6 years ago
|
||
(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...)
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Assignee: ntim.bugs → kmaglione+bmo
Updated•6 years ago
|
Attachment #8964139 -
Attachment is obsolete: true
Comment 25•6 years ago
|
||
mozreview-review |
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 ?
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
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.
Comment 27•6 years ago
|
||
(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
Comment 28•6 years ago
|
||
(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*.
Assignee | ||
Comment 29•6 years ago
|
||
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.
Comment 30•6 years ago
|
||
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 31•6 years ago
|
||
mozreview-review |
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 32•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 33•6 years ago
|
||
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
Comment 34•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0819fc14fc57 https://hg.mozilla.org/mozilla-central/rev/6bb7337970f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 35•6 years ago
|
||
(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.
Comment 36•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Attachment #8969841 -
Flags: review?(kmaglione+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•