Closed Bug 1094821 Opened 10 years ago Closed 9 years ago

Add ability to modify a list of built in themes for the Lightweight Theme Manager

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: jwalker, Assigned: bgrins)

References

Details

(Keywords: addon-compat)

Attachments

(2 files, 15 obsolete files)

18.86 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
10.15 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
The problems:

* We've got 4 UI controls, in 3 totally different parts of the UI.
  * about:addons#Appearance (is this still used?)
  * Developer Tools / Options / Theme chooser
  * Customize mode, Themes menu
  * Customize mode, DevEdition theme button

* It's not totally obvious that about:preferences isn't the place to look.

* Without the light-devtools theme, if you're using the light theme in devtools, then you'll get Australis even when you ask for Devtools in the customize menu.

Worth noting: Firebug3 adds a 3rd theme to the devtools theme selector.
Blocks: 1094356
>   * about:addons#Appearance (is this still used?)

Yes, it's actually the only thing in shipping with the browser that's used until the customize themes menu ships by default.  There is also the ability to switch directly from the addons.mozilla site.

>   * Customize mode, DevEdition theme button

Matt and I had a discussion about making the devedition theme a 'special' lightweight theme.  It would be preinstalled (similar to how the Default Theme is).  Then we would have a listener for when the theme and add the extra stylesheet.  That would get rid of this UI control and also make sure the theme was listed in places 1 and 2.

I'm not positive how hard it would be to do this (it *seemed* easy), and how we would deal with multiple channels.  For instance, it would be weird to users of stable if they discovered it since the default devtools theme there is light.  So we may want to hide it from the menu in that case.  But then what if a user from a devedition profile and the theme applied has opened up stable / nightly with the same profile - would it be applied and added to the menu then, or would it 'cancel' out their theme selection only when on this other channel?
(In reply to Brian Grinstead [:bgrins] from comment #1)
> >   * about:addons#Appearance (is this still used?)
> 
> Yes, it's actually the only thing in shipping with the browser that's used
> until the customize themes menu ships by default.

Actually, the customize themes menu in the Customize page is already shipping by default with respect to Aurora/DevEdition users (Firefox 34).
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Actually, the customize themes menu in the Customize page is already
> shipping by default with respect to Aurora/DevEdition users (Firefox 34).

Bug 1007336 was responsible for the lightweight theme chooser in Customize mode.
Attached patch devedition-lwtheme-1.patch (obsolete) — Splinter Review
This is a new approach to the theme application.  Here, I install a stub lightweight theme for dev edition and allow the normal addon management to work.  If it's a non-DE browser then I uninstall it at startup.  I'd like some initial feeback on this approach before spending more time on this.

This is great because we can move the theme selection into the place where everyone expects it to be.  And also delete a bunch of code (incoming in the next patch).

It gets a little tricky given different channel-switching scenarios.  Note that the current pref-based approach doesn't do a great job with these scenarios, and sometimes I think intended behavior is ambiguous:

1) Brand new user A opens devedition for the first time (theme is installed and applied).  Then they switch to Release (theme is *not* uninstalled because it is still applied).  Then they switch back (theme is still applied).
2) Brand new user B opens release for the first time (theme is not installed).  Then they switch to devedition (theme is installed and applied).  Then they switch back to release (theme is *not* uninstalled because it is still applied).
3) Existing user C has been using devedition and the theme.  They open the new version and nothing appears different (theme is installed and applied).
4) Existing user D has been using devedition but disabled the theme.  They open the new version (theme is installed and applied).  This isn't right - still need to handle this case.

I'm not sure the intended behavior for case 1 / 2.  We could uninstall the theme on a non-DE channel regardless of whether it's applied or not - this could make re-applying it on dev edition harder to track, but it should be possible with prefs.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8570763 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8570763 - Flags: feedback?(dao)
Attached patch devedition-lwtheme-2.patch (obsolete) — Splinter Review
Deleting customize mode and devtools options panel code that was dealing with theme management.
Summary: Post DevEdition, theme switching should be improved → Convert dev edition theme to a lightweight theme
Comment on attachment 8570763 [details] [diff] [review]
devedition-lwtheme-1.patch

Review of attachment 8570763 [details] [diff] [review]:
-----------------------------------------------------------------

Just taking my own advice and moving this to :Unfocused's queue. (Blair, if you're overloaded and have reading list things you want me to look at in return, please do. :-) )

::: browser/base/content/browser-devedition.js
@@ +5,5 @@
> +// XXX TODO:
> +// * Audit lw theme styles to make sure new -moz-lw-theme rules don't apply and interfere with the theme
> +//   (or old ones that were important are no longer applying).
> +// * Is there some way to limit FOUC?
> +//   Like moving this code to somewhere that runs before browser.js, maybe nsBrowserGlue?

This sounds good. The add-ons manager should be available before the first window is up, so this should be doable.

@@ +10,5 @@
> +// * Make the resources a separate patch that can be uplifted to prevent visual when running
> +//   this in an old build that doesn't have these listeners.
> +// * Have a one-time migration for users that currently have the theme applied with
> +//   browser.devedition.theme.enabled (and no other themes applied),
> +//   so that the new lw theme is selected by default.

You can do this in nsBrowserGlue.js, see the _migrate stuff there.

@@ +36,5 @@
> +  },
> +
> +  get isThemeCurrentlyApplied() {
> +    let theme = LightweightThemeManager.currentTheme;
> +    return theme && theme.id == "devedition";

I expect we'd need to liaise with AMO to avoid other people uploading themes with this ID.

@@ +55,3 @@
>      Services.prefs.addObserver(this._devtoolsThemePrefName, this, false);
>      Services.obs.addObserver(this, "lightweight-theme-styling-update", false);
> +    this._updateDevtoolsThemeAttribute();

Nit: leave this in its previous place to preserve blame

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +190,5 @@
> +        var oldWrapper = theme ? new AddonWrapper(theme) : null;
> +        var wrapper = new AddonWrapper(aData);
> +        AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
> +                                                 wrapper, oldWrapper, false);
> +        AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);

This looks kind of hacky, I would hope there'd be a better way to do this... - can you request review from :Unfocused instead? I think his addon manager expertise as well as australis expertise here make him a more suitable reviewer.
Attachment #8570763 - Flags: feedback?(gijskruitbosch+bugs) → feedback?(bmcbride)
Comment on attachment 8570763 [details] [diff] [review]
devedition-lwtheme-1.patch

Review of attachment 8570763 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think installing a theme like this fits well here, as evidenced by the numerous edge cases and hacks needed.

A lot of edge cases would disappear if LightwieghtThemeManager allowed the application to define a static list of themes, separate from those in the lightweightThemes.usedThemes pref. Currently lightweightThemes.usedThemes awkwardly serves two purposes - as a store for themes and as a way to determine which theme is currently used (the first item in the array). If we remove lightweightThemes.isThemeSelected, and add a lightweightThemes.selectedThemeID pref, it shouldn't be too much trouble to add support for a second application defined list of themes. Should also be able to then make such app-supplied themes non-removable.

That way, when switching branches, LightweightThemeManager won't find the theme with the specified ID and automatically fall back to no theme.
Attachment #8570763 - Flags: feedback?(bmcbride) → feedback-
Comment on attachment 8570763 [details] [diff] [review]
devedition-lwtheme-1.patch

I agree the install / firstRun logic doesn't feel quite right, and having the theme non-removable would probably be preferable.
Attachment #8570763 - Flags: feedback?(dao) → feedback-
Attached patch lwtheme-storage-change.patch (obsolete) — Splinter Review
Yes, this makes things simpler.  I'm breaking it out into two patches, one for the changes in lw theme storage (and sync), and another to add the ability for an application to specify a list of extra themes.

I noticed that we will need some changes for sync to work since we are changing some of the prefs here.  Asking for a preliminary review for the changes in LightweightThemeManager and looking for some feebback about how to tackle the sync changes.
Attachment #8570763 - Attachment is obsolete: true
Attachment #8570764 - Attachment is obsolete: true
Attachment #8572436 - Flags: feedback?(bmcbride)
Changes to be able to load application specific lightweight themes separately from the usedThemes pref, and removing a bunch of devedition specific code.
Comment on attachment 8572436 [details] [diff] [review]
lwtheme-storage-change.patch

Review of attachment 8572436 [details] [diff] [review]:
-----------------------------------------------------------------

Yea, that works pretty nicely :)

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +85,5 @@
> +      themes = JSON.parse(_prefs.getComplexValue("usedThemes",
> +                                                 Ci.nsISupportsString).data);
> +    } catch (e) { }
> +
> +    if (themes[0]) {

Will want an Array.isArray() and a .length>0 check here.
Attachment #8572436 - Flags: feedback?(bmcbride) → feedback+
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #12)
> Comment on attachment 8572436 [details] [diff] [review]
> lwtheme-storage-change.patch
> 
> Review of attachment 8572436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yea, that works pretty nicely :)

For the sync code, should I just remove all references to isThemeSelected and replace them with selectedThemeID?  I'm guessing that would mean the selected lw theme wouldn't sync properly between old and new channels.  I'm not sure of all the different scenarios that need to be supported for sync, and don't have a good idea of how much work it would be to get the cross-channel functionality working well.
Flags: needinfo?(bmcbride)
Hmm... I don't think it would work well regardless of what we do there. But I don't think that's a big deal (we've never had strong guarantees of things transitioning nicely when switching to older branches). So I'd recommend just removing isThemeSelected, and just syncing the new pref.
Flags: needinfo?(bmcbride)
Attached patch lwtheme-storage-change.patch (obsolete) — Splinter Review
I haven't been able to this check on an Android build yet, but all the other changes should be ready for review.  I've tested syncing between two profiles on the same build and it works as expected.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60fcb9d8c77a
Attachment #8573707 - Flags: review?(bmcbride)
Blocks: 1141326
Comment on attachment 8573707 [details] [diff] [review]
lwtheme-storage-change.patch

Review of attachment 8573707 [details] [diff] [review]:
-----------------------------------------------------------------

I'm swamped, and I think we have the approach nailed down now, so going to see if Gijs feels like doing the final review here.
Attachment #8573707 - Flags: review?(bmcbride) → review?(gijskruitbosch+bugs)
Comment on attachment 8573707 [details] [diff] [review]
lwtheme-storage-change.patch

Review of attachment 8573707 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming try is green and you manually checked this works on Android.

::: mobile/android/chrome/content/browser.js
@@ +7700,5 @@
>        } catch (e) { /* ignore bad prefs and move on */ }
>      }
>  
>      // Apply a lightweight theme if necessary
> +    // XXX: Need to check this on android

Have you? :-)

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +91,5 @@
> +    }
> +  }
> +};
> +
> +migrateToNewStorageFormat();

Nit:

(function migrateToNewStorageFormat() {
...
})();

works just fine. :-)
Attachment #8573707 - Flags: review?(gijskruitbosch+bugs) → review+
(oh, and don't forget to get rid of the XXX comment once you have checked it on Android)
Attachment #8572436 - Attachment is obsolete: true
Attached patch lwtheme-storage-change.patch (obsolete) — Splinter Review
Built and tested on Android, lightweight theme applying and switching is working and confirmed that the new pref was migrated to in about:config.

Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f548f8b660a4
Attachment #8573707 - Attachment is obsolete: true
Attachment #8575581 - Flags: review+
Attached patch lwtheme-appspecificthemes.patch (obsolete) — Splinter Review
This patch adds a new Array called 'appSpecificThemes' to the LightweightThemeManager as described in Comment 8.  Any themes in this array will always show up at the front of the list and do not appear as removable in the Addons Manager.  Not sure about the property name, but the code is ready for review.

Blair, I know you are swamped so please redirect the review to someone appropriate if needed.
Attachment #8576255 - Flags: review?(bmcbride)
Comment on attachment 8576255 [details] [diff] [review]
lwtheme-appspecificthemes.patch

Review of attachment 8576255 [details] [diff] [review]:
-----------------------------------------------------------------

Yea, sorry, over to Gijs for full review.

Looks good on the surface, without thinking too deeply about it.

One UX-related thought: I think app themes should show up last in the list. Themes that have been installed are the result of a person making a choice to have them there, and that should have priority in the UI over anything that's there by default.
Attachment #8576255 - Flags: review?(gijskruitbosch+bugs)
Attachment #8576255 - Flags: review?(bmcbride)
Attachment #8576255 - Flags: feedback+
Comment on attachment 8576255 [details] [diff] [review]
lwtheme-appspecificthemes.patch

Review of attachment 8576255 [details] [diff] [review]:
-----------------------------------------------------------------

f+ pending questions :-)

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +96,5 @@
>    get name() "LightweightThemeManager",
>  
> +  // Themes that can be added for an application.  They can't be removed, and
> +  // will always show up at the top of the list.
> +  appSpecificThemes: [],

So, how are these added to?

@@ +725,5 @@
> +  LightweightThemeManager.appSpecificThemes.forEach(appSpecificTheme=> {
> +    aList = aList.filter(theme=> {
> +      return theme.id != appSpecificTheme.id;
> +    })
> +  });

let appThemeIds = new Set(LightweightThemeManager.appSpecificThemes.map(t => t.id));
aList = aList.filter(theme => !appThemeIds.has(theme.id));

is prettier and slightly more cpu-efficient (assuming an efficient implementation of Set).

... but this could be even simpler if we just stick a property on the theme objects. Is that not an option?
Attachment #8576255 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #22) 
> ::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
> @@ +96,5 @@
> >    get name() "LightweightThemeManager",
> >  
> > +  // Themes that can be added for an application.  They can't be removed, and
> > +  // will always show up at the top of the list.
> > +  appSpecificThemes: [],
> 
> So, how are these added to?

Attaching the relevant patch that is to be applied in sequence.  You can see appSpecificThemes being set in nsBrowserGlue in this patch.

This patch converts the devedition theme into a lw theme and removes a bunch of code in customize mode / devtools toolbox / browser-devedition.js.
Attachment #8572438 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #22)
> @@ +725,5 @@
> > +  LightweightThemeManager.appSpecificThemes.forEach(appSpecificTheme=> {
> > +    aList = aList.filter(theme=> {
> > +      return theme.id != appSpecificTheme.id;
> > +    })
> > +  });
> 
> let appThemeIds = new Set(LightweightThemeManager.appSpecificThemes.map(t =>
> t.id));
> aList = aList.filter(theme => !appThemeIds.has(theme.id));
> 
> is prettier and slightly more cpu-efficient (assuming an efficient
> implementation of Set).
> 
> ... but this could be even simpler if we just stick a property on the theme
> objects. Is that not an option?

I hadn't thought about doing it that way, mostly because I was focused on loading the theme only in certain channels.  I couldn't think of a good way to modify usedThemes directly without keeping a separate list, since the usedThemes getter always pulls a fresh copy from the pref.

I guess we could have a method called addAppSpecificTheme that takes in a theme object, modifies it with a property, and then pushes it into a private _appSpecificThemes array and then any checks could look at that property directly?  Or maybe we could add a getter that returns these themes in a Map keyed on the ID and always refer to that within the file?
Attached patch lwtheme-appspecificthemes.patch (obsolete) — Splinter Review
Implements the shorter version of the app theme pruning as suggested, disallows calls to forgetUsedTheme with app specific themes, and puts the app specific themes at the end of the list as suggested in Comment 21.

Do you think we should change the storage mechanism for these things (dumping them into a Map keyed on ID or setting a property on the theme object, for example)?  Alternatively, we could keep storing them as an array but add a get appSpecificThemesMap/Set() to do quick lookups by ID.  Let me know whatever you think is best.
Attachment #8576255 - Attachment is obsolete: true
Attachment #8576324 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8576324 [details] [diff] [review]
lwtheme-appspecificthemes.patch

>+    if (!theme || LightweightThemeManager.appSpecificThemes.find(t=>t.id==theme.id))

drive-by nit: please add spaces around => and ==
Comment on attachment 8576324 [details] [diff] [review]
lwtheme-appspecificthemes.patch

Review of attachment 8576324 [details] [diff] [review]:
-----------------------------------------------------------------

Notes:
1) I think code that sets this should be able to append rather than replace. If multiple files / consumers want to add/remove stuff here, it's currently pretty difficult. Modifying a public property like this also seems... slightly evil? Maybe I've worked with too much java and am scarred, or something. So yes, I would prefer a method to add/remove items.
2) This has the benefit that the storage mechanism is something we can change later
3) OTOH, changing the usedTheme storage thing is likely to break add-ons (Personas Plus, maybe others). But then, the earlier patch already would. Then again, the current storage isn't super ideal. Then again, this is also premature optimizations in the sense that it's unlikely that usedThemes becomes so large that the big-o characteristics of operations on it start to seriously matter.

I think in the end, I'm less bothered about the storage mechanism here than about how we access/modify/test for the app-specific themes.

Out of the suggestions, I guess if the app-specific thing was a map from id => theme, that would likely be the easiest? You could concat with appSpecificThemes.values() instead of just the list, and you could test by checking if Map.has(id). You could also set/delete in pretty trivial methods. We could do this only for appSpecificThemes and avoid the additional add-on pain by not touching usedThemes, I guess?

If you think I'm giving you to much make-work and would prefer to land as-is, please re-flag.

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +158,5 @@
>    },
>  
>    forgetUsedTheme: function LightweightThemeManager_forgetUsedTheme(aId) {
>      let theme = this.getUsedTheme(aId);
> +    if (!theme || LightweightThemeManager.appSpecificThemes.find(t=>t.id==theme.id))

As Dão noted, spaces please.

Also, Uber-nit (if we're not changing storage): s/find/some/ -- which returns a bool rather than the object, and would be enough here. Same in the permissions getter below.
Attachment #8576324 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch lwtheme-appspecificthemes.patch (obsolete) — Splinter Review
> Out of the suggestions, I guess if the app-specific thing was a map from id
> => theme, that would likely be the easiest? You could concat with
> appSpecificThemes.values() instead of just the list, and you could test by
> checking if Map.has(id). You could also set/delete in pretty trivial
> methods.

Updated the patch to keep them in a map and implemented add/forget/clear methods.  I think this is a much better approach.

> We could do this only for appSpecificThemes and avoid the
> additional add-on pain by not touching usedThemes, I guess?

I'm not sure I understand about not modifying usedThemes.  If you are talking about the pref, then absolutely, that's what makes this approach work without accidentally leaking the devedition theme to a release channel.

But if if you are talking about the getter on LightweightThemeManager, I'm not sure.  Right now, as far as addon compat is concerned, changing the getter to include app specific themes causes these inconsistencies:

1) forgetUsedTheme will be a noop if you call it with an app specific theme id
2) App specific themes aren't stored in the pref, so if you manually wrote to or read from that pref string it wouldn't be consistent with LWTM.usedThemes.

To change this we would need to change the rest of the LW theme manager (and any calling code) to concat appSpecificThemes onto usedThemes.  Maybe not modifying the getter would be better?  In that case we could add a new getter `LWTM.allThemes` that returned the concat-ed object and just change any other code in chrome to use that (like Customize Mode).
Attachment #8576324 - Attachment is obsolete: true
Attachment #8576919 - Flags: review?(gijskruitbosch+bugs)
(In reply to Brian Grinstead [:bgrins] from comment #28)
> Created attachment 8576919 [details] [diff] [review]
> lwtheme-appspecificthemes.patch
> 
> > Out of the suggestions, I guess if the app-specific thing was a map from id
> > => theme, that would likely be the easiest? You could concat with
> > appSpecificThemes.values() instead of just the list, and you could test by
> > checking if Map.has(id). You could also set/delete in pretty trivial
> > methods.
> 
> Updated the patch to keep them in a map and implemented add/forget/clear
> methods.  I think this is a much better approach.
> 
> > We could do this only for appSpecificThemes and avoid the
> > additional add-on pain by not touching usedThemes, I guess?
> 
> I'm not sure I understand about not modifying usedThemes.  If you are
> talking about the pref, then absolutely, that's what makes this approach
> work without accidentally leaking the devedition theme to a release channel.

Yeah, I just meant the storage method we use for the usedThemes stuff - because that's still an array, fetching non-app themes will still be inefficient/cumbersome/annoying. We can live with that.
Comment on attachment 8576919 [details] [diff] [review]
lwtheme-appspecificthemes.patch

Review of attachment 8576919 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a test for the forgetAppSpecificTheme case where it's already applied.

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +185,5 @@
> +  forgetAppSpecificTheme: function LightweightThemeManager_forgetAppSpecificTheme(id) {
> +    if (!this._appSpecificThemes.has(id)) {
> +      let currentTheme = this.currentTheme;
> +      if (currentTheme && currentTheme.id == id) {
> +        this.currentTheme = null;

Oh, interesting. You don't test this behaviour, AFAICT? Can you add a test for it?

@@ +192,5 @@
> +    return this._appSpecificThemes.delete(id);
> +  },
> +
> +  clearAppSpecificThemes: function LightweightThemeManager_clearAppSpecificThemes() {
> +    for (let id of this._appSpecificThemes.keys()) {

Could also do let [id, ] of this._appSpecificThemes. Dunno which is better, up to you.
Attachment #8576919 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8576919 [details] [diff] [review]
lwtheme-appspecificthemes.patch

Review of attachment 8576919 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/test_LightweightThemeManager.js
@@ +574,5 @@
> +      ltm.currentTheme = ltm.getUsedTheme("appSpecificTheme0");
> +
> +      Assert.throws(() => { ltm.addAppSpecificTheme("appSpecificTheme0") },
> +        "Exception is thrown adding duplicate theme");
> +      do_check_true(ltm.forgetAppSpecificTheme("appSpecificTheme0"));

I'm checking the forgetAppSpecificTheme functionality on a currentTheme here
Organizes the test assertions for forgetAppSpecificTheme / addAppSpecificTheme to make it more explicit
Attachment #8576919 - Attachment is obsolete: true
Attachment #8577367 - Flags: review+
Attached patch lwtheme-devedition-changes.patch (obsolete) — Splinter Review
I believe everything in the patch is ready for review except for the changes to nsBrowserGlue.js.  Luckily a lot of this patch is just deleting code, but some of the migration pieces do get a little complicated.

I was wanting some advise on this file regarding the definition of the theme, and the migration steps.  My current thinking is that for Developer Edition we set a default pref of selectedThemeID to "devedition" and handle a one time migration for people who had opted out of the browser.devedition.theme.enabled pref.  I'm not sure if I'm missing any cases here - since you did some of the work with the customize mode button your feedback would be valuable.
Attachment #8576272 - Attachment is obsolete: true
Attachment #8578345 - Flags: feedback?(gijskruitbosch+bugs)
Keywords: dev-doc-needed
Comment on attachment 8578345 [details] [diff] [review]
lwtheme-devedition-changes.patch

Review of attachment 8578345 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +724,5 @@
> +    //   given that the name is kind of reusing the brand name and the author is a name.
> +    // * Coordinate with marketplace to make sure no theme can be added with this ID.
> +    LightweightThemeManager.addAppSpecificTheme({
> +      "id": "devedition",
> +      "name": "Developer Edition",

This should be localized, but with an l10n note that it would normally need to match the brand name for aurora (which are in l10n repos).

@@ +727,5 @@
> +      "id": "devedition",
> +      "name": "Developer Edition",
> +      "headerURL": "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> +      "iconURL": "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> +      "author": "Mozilla",

This is probably already available as the vendor name via brand as well (vendorShortName).

@@ +2004,5 @@
> +      // If we are on the devedition channel, the devedition theme is on by default.  But we need
> +      // to handle the case where they didn't want it applied, and unapply the theme.
> +      let userChoseToNotUseDeveditionTheme = !deveditionThemeEnabled ||
> +                                             !defaultThemeSelected ||
> +                                             lightweightThemeSelected;

If this ever runs a second time for a profile or whatever (shouldn't happen, but hey), lightweightThemeSelected is going to be true even if the lwt is the devedition theme, which seems wrong.

@@ +2007,5 @@
> +                                             !defaultThemeSelected ||
> +                                             lightweightThemeSelected;
> +
> +      if (userChoseToNotUseDeveditionTheme && selectedThemeID == "devedition") {
> +          Services.prefs.setCharPref("lightweightThemes.selectedThemeID", "");

Nit: indenting

::: browser/devtools/framework/toolbox-options.js
@@ +9,5 @@
>  const promise = require("promise");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "gDevTools", "resource:///modules/devtools/gDevTools.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
> +                                  "resource://gre/modules/LightweightThemeManager.jsm");

Why does this still need the lwtheme manager?
Attachment #8578345 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8578345 [details] [diff] [review]
lwtheme-devedition-changes.patch

>+#ifndef RELEASE_BUILD
>+    // XXX:
>+    // * Where should these strings go?  I'm not sure if these *should* be localized,
>+    //   given that the name is kind of reusing the brand name and the author is a name.
>+    // * Coordinate with marketplace to make sure no theme can be added with this ID.
>+    LightweightThemeManager.addAppSpecificTheme({
>+      "id": "devedition",

If you use "firefox-devedition@mozilla.org" as the ID, it's highly unlikely that some other author will want to reuse that, so I don't think addons.mozilla.org (not the marketplace) needs to know about this. Just make sure LightweightThemeManager lets themes added via this API win any hypothetical conflict.

I would also suggest renaming addAppSpecificTheme to addBuiltInTheme.

>+      "name": "Developer Edition",
>+      "headerURL": "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
>+      "iconURL": "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
>+      "author": "Mozilla",
>+      "version":"0",

The quotes around the property names aren't needed.

browser/base/content/defaultthemes/ is misleading; our default themes are in browser/themes/ and devedition isn't one of them. If you don't have a better idea, you could just put the images directly in browser/base/content/ as devedition-header.png and devedition-icon.png.
Had to do a lot of tracking down to figure out what was causing leaks in Windows debug bc tests in this patch, and it turned out to be the references to http://lwttest.invalid/a.png and b.png in the browser_devedition.  Switching these to a resource:///chrome URI fixed the problem.  I have no idea why this was a problem - the old URLs were copied over from some non-bc tests.  Anyway, it's a minor test-only change, so copying over r+
Attachment #8575581 - Attachment is obsolete: true
Attachment #8581053 - Flags: review+
(In reply to Brian Grinstead [:bgrins] from comment #36)
> Created attachment 8581053 [details] [diff] [review]
> lwtheme-storage-change.patch
> 
> Had to do a lot of tracking down to figure out what was causing leaks in
> Windows debug bc tests in this patch, and it turned out to be the references
> to http://lwttest.invalid/a.png and b.png in the browser_devedition. 
> Switching these to a resource:///chrome URI fixed the problem.  I have no
> idea why this was a problem - the old URLs were copied over from some non-bc
> tests.  Anyway, it's a minor test-only change, so copying over r+

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef956c5fa3b6
> I would also suggest renaming addAppSpecificTheme to addBuiltInTheme.

I like the builtInTheme name better than appSpecificTheme.. Going to update the second and third patches to match.
Marking addon-compat since any extensions directly modifying lightweightThemes.usedThemes and lightweightThemes.isThemeSelected as a way of trying to change the current theme may fail.

It used to be that in the case that if lightweightThemes.usedThemes was set to [{id: theme1}, {id: theme2}, {id: theme3}] and lightweightThemes.isThemeSelected was set to true, then theme1 would implicitly be the selected theme (because it was the first in the list).

With these changes, isThemeSelected is not used anymore and there is a new pref called lightweightThemes.selectedThemeID so that the selected theme separated from the order of the usedThemes list.

Anyway, instead of fiddling with these prefs an addon could be using the LightweightThemeManager.currentTheme setter / getter and the LightweightThemeManager.usedThemes getter.  This is less work and is much less likely to break than relying on the implementation details for these properties.
Keywords: addon-compat
(In reply to Brian Grinstead [:bgrins] from comment #39)
> Marking addon-compat since any extensions directly modifying
> lightweightThemes.usedThemes and lightweightThemes.isThemeSelected as a way
> of trying to change the current theme may fail.

And by 'fail', I mean that it would update the used themes list, but no lightweight theme would be selected until the user went in and activated it from about:addons or customize mode.
Just renamed appSpecificThemes to builtInThemes
Attachment #8577367 - Attachment is obsolete: true
Attachment #8582018 - Flags: review+
Try push of the first two parts is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6744caa34d7.  Planning to land these in the meantime while I'm finishing up the devedition-specific patch.
(In reply to Dão Gottwald [:dao] from comment #35)
> browser/base/content/defaultthemes/ is misleading; our default themes are in
> browser/themes/ and devedition isn't one of them. If you don't have a better
> idea, you could just put the images directly in browser/base/content/ as
> devedition-header.png and devedition-icon.png.

This folder is already there and is not for the browser's default themes but rather the default recommended lw themes from customize mode: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/defaultthemes/.  This isn't *exactly* the same thing, but close enough that it seemed better to put the icons here instead of directly in base/content.
Attached patch lwtheme-devedition-changes.patch (obsolete) — Splinter Review
Addresses Comments 34/35 and I believe ready for review
Attachment #8578345 - Attachment is obsolete: true
Attachment #8582736 - Flags: review?(gijskruitbosch+bugs)
I don't suppose you have an interdiff?
Flags: needinfo?(bgrinstead)
(In reply to :Gijs Kruitbosch from comment #47)
> I don't suppose you have an interdiff?

Here it is
Flags: needinfo?(bgrinstead)
Comment on attachment 8582736 [details] [diff] [review]
lwtheme-devedition-changes.patch

Review of attachment 8582736 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: browser/components/nsBrowserGlue.js
@@ +719,5 @@
>  #endif
>  
> +#ifndef RELEASE_BUILD
> +    let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");

Can you file a followup bug to refactor this and the other uses in nsBrowserGlue.js to just define some lazy getters at the top of the file and reuse them in the 10+ places that use these bundles? It can probably be marked good first bug with me as a mentor.

@@ +2004,5 @@
> +      // applied, and unapply the theme.
> +      let userChoseToNotUseDeveditionTheme =
> +        !deveditionThemeEnabled ||
> +        !defaultThemeSelected ||
> +        (lightweightThemeSelected && selectedThemeID != "devedition");

This needs to be updated, right?
Attachment #8582736 - Flags: review?(gijskruitbosch+bugs) → review+
> @@ +2004,5 @@
> > +      // applied, and unapply the theme.
> > +      let userChoseToNotUseDeveditionTheme =
> > +        !deveditionThemeEnabled ||
> > +        !defaultThemeSelected ||
> > +        (lightweightThemeSelected && selectedThemeID != "devedition");
> 
> This needs to be updated, right?

Good catch
Attachment #8582736 - Attachment is obsolete: true
Attachment #8582825 - Attachment is obsolete: true
Attachment #8582867 - Flags: review+
Blocks: 1147398
(In reply to :Gijs Kruitbosch from comment #49)
> Can you file a followup bug to refactor this and the other uses in
> nsBrowserGlue.js to just define some lazy getters at the top of the file and
> reuse them in the 10+ places that use these bundles?

Filed Bug 1147398
https://hg.mozilla.org/mozilla-central/rev/01482cdccd72
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Depends on: 1148761
So there are test failures with the third part of this bug when applied against Aurora (Bug 1148761).  Planning to back out this part since it needs to be fixed by tomorrow and there will need to be some reviews of the necessary changes (see https://bugzilla.mozilla.org/show_bug.cgi?id=1148761#c5).

Because of the addon-compat implications of the LightweightThemeManger portions of this bug I'm suggesting we just file a new one to reland the devedition specific bits, so it's obvious in this bug that the LWTM parts were fixed in 39.
Keywords: dev-doc-needed
Summary: Convert dev edition theme to a lightweight theme → Add ability to modify a list of built in themes for the Lightweight Theme Manager
Blocks: 1148996
Comment on attachment 8582867 [details] [diff] [review]
lwtheme-devedition-changes-r=Gijs.patch

This patch had to be backed out because of bug 1148761
Attachment #8582867 - Attachment is obsolete: true
Re-opening as per comment 56, so this doesn't get lost.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #57)
> Re-opening as per comment 56, so this doesn't get lost.

My plan (in Comment 55) is to keep this fixed, so the lightweight theme changes can stand on their own (and have a clear milestone of 39).  And then handle relanding the devedition bits in Bug 1148996.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to Brian Grinstead [:bgrins] from comment #58)
> My plan (in Comment 55) is to keep this fixed, so the lightweight theme
> changes can stand on their own (and have a clear milestone of 39).  And then
> handle relanding the devedition bits in Bug 1148996.

FWIW I should have probably used bug 1148761 (not this one) in the commit message for: https://hg.mozilla.org/mozilla-central/rev/1b6bf6612c0f
I had a bug "related" to this one when updated from dev edition 39 to 40 with the "Default theme".

Default Theme was revert to Dark Theme and setting it to "Default Theme" was working TILL I restart Firefox... So every time I restarted Firefox it was with Dark Theme instead of the default theme. 

English is not my native language but here it the fix I've found ---> http://forums.mozillazine.org/viewtopic.php?p=14158607#p14158607

Read my 3 messages especially the second message which include the fix of the bug.

Hoping it will help to fix it !

Regards
See Also: → 1260596
Depends on: 1327560
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: