510909 - Integrate most recently used lightweight themes into the themes list in the add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Dave Townsend [:mossop]]

This bug will track integrating the most recently used personas into the themes list in the add-ons manager. It depends on the personas backend being implemented and providing a means to list recently used personas, see which is the current persona and hear when the recently used list changes and when a new persona is selected (presumably by install from website).

Basic implementation will mean adding the personas list to a dummy RDF datasource that the template builder builds the UI from. Switching to a persona will require ensuring the default theme is selected (a restart will be required if not) and then activating the persona through the backend. Switching to any old-style theme will deactivate personas and switch requesting a restart.

Comment 1

[Pardal Freudenthal (:ShareBird)]

I can't see any reason to place Personas in the themes list. IMHO Personas is something totally different from themes at all... This will maybe make things confusing for users.

I also don't see a necessity to force a restart if the used theme isn't the default. This will be a PITA for users wanting to use a third-party theme and a persona. Just as an example, I can, using my theme, change persona without needing to restart the browser. Am I missing something here?

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> I can't see any reason to place Personas in the themes list. IMHO Personas is
> something totally different from themes at all... This will maybe make things
> confusing for users.

I disagree, both achieve essentially the same effect, changing the look of Firefox.

> I also don't see a necessity to force a restart if the used theme isn't the
> default. This will be a PITA for users wanting to use a third-party theme and a
> persona. Just as an example, I can, using my theme, change persona without
> needing to restart the browser. Am I missing something here?

Switching the lightweight theme does not require a restart, they are easily replaceable. Switching to a different theme however does, always has and until bug 226791 is fixed always will.

We are not, for Firefox 3.6, going to support the notion of an active lightweight theme on top of a non-default theme.

Comment 3

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #2)
> Switching the lightweight theme does not require a restart, they are easily
> replaceable. Switching to a different theme however does, always has and until
> bug 226791 is fixed always will.

I'm not talking about switching themes without a restart. I very know that bug. 
I'm talking about switching to a persona and having to restart if the default theme is not selected.. Or I misunderstood this:
> Switching to a persona will require ensuring the default theme is selected
>(a restart will be required if not) and then activating the persona 
> through the backend. Switching to any old-style theme will deactivate 
> personas and switch requesting a restart.


> I disagree, both achieve essentially the same effect, changing the look of
> Firefox.

There are hundred of extensions that "changes the look" of Firefox. Should they also be listed as themes?
We used to say about themes that they change the "look and feel" of the browser. Actually a theme can do much, much more then Personas. And technically a theme is totally different from Personas...

Personas is a fine thing. I made my efforts to make my theme fully compatible with it, but Personas can not substitute a theme. Don't you think?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (In reply to comment #2)
> > Switching the lightweight theme does not require a restart, they are easily
> > replaceable. Switching to a different theme however does, always has and until
> > bug 226791 is fixed always will.
> 
> I'm not talking about switching themes without a restart. I very know that bug. 
> I'm talking about switching to a persona and having to restart if the default
> theme is not selected.. Or I misunderstood this:
> > Switching to a persona will require ensuring the default theme is selected
> >(a restart will be required if not) and then activating the persona 
> > through the backend. Switching to any old-style theme will deactivate 
> > personas and switch requesting a restart.

Since we will only apply a persona on top of the default theme in 3.6. If the user has a non-default theme selected and they select a persona, then we must activate the default theme, which requires the restart.

> > I disagree, both achieve essentially the same effect, changing the look of
> > Firefox.
> 
> There are hundred of extensions that "changes the look" of Firefox. Should they
> also be listed as themes?
> We used to say about themes that they change the "look and feel" of the
> browser. Actually a theme can do much, much more then Personas. And technically
> a theme is totally different from Personas...

They have more capabilities sure, but as far as users are concerned they do essentially the same thing.

> Personas is a fine thing. I made my efforts to make my theme fully compatible
> with it, but Personas can not substitute a theme. Don't you think?

I agree, that is why we offer both.

Comment 5

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #4)
> Since we will only apply a persona on top of the default theme in 3.6. If the
> user has a non-default theme selected and they select a persona, then we must
> activate the default theme, which requires the restart.
> 
Does it mean Personas will not work with third-party themes?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (In reply to comment #4)
> > Since we will only apply a persona on top of the default theme in 3.6. If the
> > user has a non-default theme selected and they select a persona, then we must
> > activate the default theme, which requires the restart.
> > 
> Does it mean Personas will not work with third-party themes?

As I said earlier:

> We are not, for Firefox 3.6, going to support the notion of an active
> lightweight theme on top of a non-default theme.

The Personas extension may investigate and support this notion and it may come to a later version of Firefox based on the results of that.

Comment 7

[Dave Townsend [:mossop]]

Attachment: wip

Work in progress patch, not actually tested but should be close to about right.

Comment 8

[Tony Chung [:tchung]]

hey dave, testcase to follow?   i'd also like to test this out from the UI, if you have a test theme i can play with to show this fix.

Comment 9

[Dave Townsend [:mossop]]

Attachment: wip 2

Mostly working, still need to remove the getpersonas specific bits from it.

Comment 10

[Dão Gottwald [:dao]]

How is this going to interact with bug 511771? More precisely, how am I expected to switch the theme? I assume setting LightweightThemeManager.currentTheme won't bring up the EM and ask for a restart if the default theme isn't selected? Should we add another observer topic, e.g. lightweight-theme-changing, so that the EM could cancel the theme change?

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 398935 [details] [diff] [review]
wip 2

>         skin/classic/mozapps/extensions/themeGeneric.png           (extensions/themeGeneric.png)
>         skin/classic/mozapps/extensions/viewButtons.png            (extensions/viewButtons.png)
>         skin/classic/mozapps/extensions/ratings.png                (extensions/ratings.png)
>         skin/classic/mozapps/extensions/extensionIcons.png         (extensions/extensionIcons.png)
>+        skin/classic/mozapps/extensions/lwtheme.png                (extensions/lwtheme.png)

Should we just use themeGeneric.png for this?

Comment 12

[Dave Townsend [:mossop]]

Attachment: wip 3

Yeah that seems like a reasonable idea. Still not quite done I think but this adds a lightweight-theme-change-requested which LightweightThemeManager.currentTheme dispatches and the EM cancels if the default skin isn't currently selected, it handles making that change happen on restart. Doesn't open the UI yet which we probably want there, we probably also want to stop previewing from working in some way when the default skin isn't on.

This version of the patch doesn't use anything getpersonas specific, jsut assumes all the right info might be resent in the JSON. Just realised though that by taking an "author" field out of the JSON we're probably going to need a string localised to word it properly.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> we probably also
> want to stop previewing from working in some way when the default skin isn't
> on.

Add lightweight-theme-preview-requested and cancel that, i.e. just silently do nothing?

Comment 14

[Axel Hecht [:Pike]]

If we can get that string in by tomorrow, that should still be in time with the fennec bookmarks. Probably fine for 1.9.2 still.

Comment 15

[Dave Townsend [:mossop]]

Attachment: strings

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 399180 [details] [diff] [review]
strings

Can I haz a localization note with that? Breaks coding style in that file in a good way :-)

I hope the string isn't space-restricted, I guess a few languages need to work around gender here.

Comment 17

[Dave Townsend [:mossop]]

Attachment: strings 2 [checked in]

With localization note

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 399182 [details] [diff] [review]
strings 2 [checked in]

nit ;-), 
# LOCALIZATION NOTE (lightweightThemeDescription): %S is the theme designer, either a person or organisation

... just adding the key reference, IIRC some tools actually use that.

Noting my r+ here purely from an l10n pov.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 399182 [details] [diff] [review]
strings 2 [checked in]

uir+a192=beltzner

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 399182 [details] [diff] [review]
strings 2 [checked in]

Rob can you do a quick r+ on this so we can land the string today. It's just so we can localize the persona author into the add-on description if we decide to go that route.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 399182 [details] [diff] [review]
strings 2 [checked in]

Landed the string:

http://hg.mozilla.org/mozilla-central/rev/4c68cfe051fe
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/db433a0f3f42

Comment 22

[Dave Townsend [:mossop]]

Attachment: wip 4

Need to write some tests but this should be pretty much ready. Aim to have it up for review tomorrow.

Comment 23

[Dave Townsend [:mossop]]

Attachment: LWTM changes rev 1

These are the small changes to allow cancelling lightweight theme previews and changes.

Comment 24

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is the add-ons manager portion. Basically the same deal as plugins we define a new datasource populated with the lightweight themes. The extension manager itself controls cancelling changing lightweight themes if a non-default theme is in use, in this case we cache the ID of the lightweight theme to select in a pref to be applied during the next startup and select the default theme. Hopefully the rest is mostly self explanatory.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 400004 [details] [diff] [review]
LWTM changes rev 1

>+    let cancel = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
>+    cancel.data = false;
>+    _observerService.notifyObservers(cancel, "lightweight-theme-change-requested",
>+                                     JSON.stringify(aData));
>+
>     if (aData) {
>       let usedThemes = _usedThemesExceptId(aData.id);
>       usedThemes.unshift(aData);
>       _updateUsedThemes(usedThemes);

We shouldn't get to this state with the EM code, but if the change has been canceled, I think it would make sense to check whether a theme is already selected and insert the new one at the second position in that case. Otherwise the currentTheme getter would return the wrong theme.

Comment 26

[Dave Townsend [:mossop]]

Attachment: LWTM changes rev 2

Yeah that makes sense, updated.

Comment 27

[Robert Strong (they/them - no direct email)]

Comment on attachment 400005 [details] [diff] [review]
patch rev 1

>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in
>@@ -403,30 +403,30 @@ def fillCertificateDB(profileDir, certPa
> 
>   os.unlink(pwfilePath)
>   return 0
> 
> def environment(env = None, xrePath = DIST_BIN, crashreporter = True):
>   if env == None:
>     env = dict(os.environ)
> 
>-  ldLibraryPath = os.path.abspath(os.path.join(SCRIPT_DIR, xrePath))
>-  if UNIXISH or IS_MAC:
>-    envVar = "LD_LIBRARY_PATH"
>-    if IS_MAC:
>-      envVar = "DYLD_LIBRARY_PATH"
>-    if envVar in env:
>-      ldLibraryPath = ldLibraryPath + ":" + env[envVar]
>-    env[envVar] = ldLibraryPath
>-  elif IS_WIN32:
>-    env["PATH"] = env["PATH"] + ";" + ldLibraryPath
>-
>-  if crashreporter:
>-    env['MOZ_CRASHREPORTER_NO_REPORT'] = '1'
>-    env['MOZ_CRASHREPORTER'] = '1'
>+#  ldLibraryPath = os.path.abspath(os.path.join(SCRIPT_DIR, xrePath))
>+#  if UNIXISH or IS_MAC:
>+#    envVar = "LD_LIBRARY_PATH"
>+#    if IS_MAC:
>+#      envVar = "DYLD_LIBRARY_PATH"
>+#    if envVar in env:
>+#      ldLibraryPath = ldLibraryPath + ":" + env[envVar]
>+#    env[envVar] = ldLibraryPath
>+#  elif IS_WIN32:
>+#    env["PATH"] = env["PATH"] + ";" + ldLibraryPath
>+#
>+#  if crashreporter:
>+#    env['MOZ_CRASHREPORTER_NO_REPORT'] = '1'
>+#    env['MOZ_CRASHREPORTER'] = '1'
> 
>   return env
> 
> 
> ###############
> # RUN THE APP #
> ###############
> 
>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>@@ -259,18 +259,18 @@ class MochitestServer:
>   def start(self):
>     "Run the Mochitest server, returning the process ID of the server."
>     
>     env = dict(os.environ)
>     if automation.UNIXISH:
>       env["LD_LIBRARY_PATH"] = self._xrePath
>       env["MOZILLA_FIVE_HOME"] = self._xrePath
>       env["XPCOM_DEBUG_BREAK"] = "warn"
>-    elif automation.IS_MAC:
>-      env["DYLD_LIBRARY_PATH"] = self._xrePath
>+#    elif automation.IS_MAC:
>+#      env["DYLD_LIBRARY_PATH"] = self._xrePath
>     elif automation.IS_WIN32:
>       env["PATH"] = env["PATH"] + ";" + self._xrePath
> 
>     args = ["-g", self._xrePath,
>             "-v", "170",
>             "-f", "./" + "httpd.js",
>             "-f", "./" + "server.js"]
> 
Unrelated changes for this bug. Have you run tests without these changes?

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 400516 [details] [diff] [review]
LWTM changes rev 2

I just realized you shouldn't reset _previewTimer if you're not changing the theme.

Comment 29

[Dave Townsend [:mossop]]

(In reply to comment #27)
> Unrelated changes for this bug. Have you run tests without these changes?

Sorry, ignore the changes in automation.py.in and runtests.py.in. Tests don't work on snow leopard without them right now.

Comment 30

[Robert Strong (they/them - no direct email)]

>diff --git a/toolkit/mozapps/extensions/content/about.js b/toolkit/mozapps/extensions/content/about.js
>--- a/toolkit/mozapps/extensions/content/about.js
>+++ b/toolkit/mozapps/extensions/content/about.js
>...
>@@ -91,17 +95,20 @@ function init()
>   var creator = gExtensionDB.GetTarget(extension, creatorArc, true);
>   if (creator)
>     creator = creator.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
>   
>   document.title = extensionsStrings.getFormattedString("aboutWindowTitle", [name]);
>   var extensionName = document.getElementById("extensionName");
>   extensionName.setAttribute("value", name);
>   var extensionVersion = document.getElementById("extensionVersion");
>-  extensionVersion.setAttribute("value", extensionsStrings.getFormattedString("aboutWindowVersionString", [version]));
>+  if (version)
>+    extensionVersion.setAttribute("value", extensionsStrings.getFormattedString("aboutWindowVersionString", [version]));
>+  else
>+    extensionVersion.hidden = true;
Can you do the same for description especially since several add-ons don't provide one?

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -47,32 +47,38 @@ const nsIIOService           = Component
> const nsIFileProtocolHandler = Components.interfaces.nsIFileProtocolHandler;
> const nsIURL                 = Components.interfaces.nsIURL;
> const nsIAppStartup          = Components.interfaces.nsIAppStartup;
> 
> var gView             = null;
> var gExtensionManager = null;
> var gExtensionsView   = null;
> var gExtensionStrings = null;
>+// The theme to switch to after restart or the currently displaying theme
>+var gSelectedTheme    = "classic/1.0";
Is there a reason to initialize this here?

>+// The currently displaying theme
> var gCurrentTheme     = "classic/1.0";
The names gSelectedTheme and gCurrentTheme don't differentiate the two well... I've found myself having to refer to these comments several times during the review to remember what the difference between them are. Would you give them better names? Also, if I'm not mistaken "The currently displaying theme" really means the current theme is use which I think is much clearer. Can you also make it clear that these don't ever refer to lightweight themes?

>+// The default theme for the app
> var gDefaultTheme     = "classic/1.0";
>+var gSelectedLWTheme  = null;
nit: since you have comments for the other theme vars it would be good to add one here as well.

>@@ -994,19 +1100,30 @@ function Startup()
> {
>   gExtensionStrings = document.getElementById("extensionsStrings");
>   gPref = Components.classes["@mozilla.org/preferences-service;1"]
>                     .getService(Components.interfaces.nsIPrefBranch2);
>   var defaultPref = gPref.QueryInterface(Components.interfaces.nsIPrefService)
>                          .getDefaultBranch(null);
>   try {
>     gCurrentTheme = gPref.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN);
>+    gSelectedTheme = gCurrentTheme;
nit: I could go either way with this but these two lines could just be one.

>     gDefaultTheme = defaultPref.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN);
>     if (gPref.getBoolPref(PREF_EXTENSIONS_DSS_SWITCHPENDING))
>-      gCurrentTheme = gPref.getCharPref(PREF_DSS_SKIN_TO_SELECT);
>+      gSelectedTheme = gPref.getCharPref(PREF_DSS_SKIN_TO_SELECT);
>+    if (gPref.prefHasUserValue(PREF_LWTHEME_TO_SELECT)) {
>+      var id = gPref.getCharPref(PREF_LWTHEME_TO_SELECT);
>+      if (id)
>+        gSelectedLWTheme = LightweightThemeManager.getUsedTheme(id);
>+      else
>+        gSelectedLWTheme = null;
>+    }
>+    else {
>+      gSelectedLWTheme = LightweightThemeManager.currentTheme;
>+    }
>   }
>   catch (e) { }
Extending this try statement to cover all of what it is covering seems to me to be a tad much. Can you clean this up or explain what you are protecting against?

Comment 31

[Robert Strong (they/them - no direct email)]

(In reply to comment #30)
> better names? Also, if I'm not mistaken "The currently displaying theme" really
> means the current theme is use which I think is much clearer. Can you also make
> it clear that these don't ever refer to lightweight themes?
s/means the current theme is use/means the current theme in use/

Comment 32

[Robert Strong (they/them - no direct email)]

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>...
>@@ -863,16 +875,110 @@ function initSearchDS() {
>...
>+function rebuildLWThemeDS() {
>+  var rdfCU = Components.classes["@mozilla.org/rdf/container-utils;1"]
>+                        .getService(Components.interfaces.nsIRDFContainerUtils);
>+  var rootctr = rdfCU.MakeSeq(gLWThemeDS, gRDF.GetResource(RDFURI_ITEM_ROOT));
>+  var themes = LightweightThemeManager.usedThemes;
For my own benefit could you define what usedThemes are (specifically what used means in the context of a leightweight theme)?

>...
>+    if (theme.author) {
>+      gLWThemeDS.Assert(themeNode,
>+                        gRDF.GetResource(PREFIX_NS_EM + "description"),
>+                        gRDF.GetLiteral(getExtensionString("lightweightThemeDescription",
>+                                                           [theme.author])),
>+                        true);
>+      gLWThemeDS.Assert(themeNode,
>+                        gRDF.GetResource(PREFIX_NS_EM + "creator"),
>+                        gRDF.GetLiteral(theme.author),
>+                        true);
>+    }
>+    if (theme.description) {
>+      gLWThemeDS.Assert(themeNode,
>+                        gRDF.GetResource(PREFIX_NS_EM + "lwdescription"),
>+                        gRDF.GetLiteral(theme.description),
>+                        true);
>+    }
I'm not knocking it but could you explain to me why description uses "Created by %S." and theme's that have a description put it in lwdescription which is only displayed on the about page or point me to a wiki that explains it?

>@@ -2439,32 +2581,39 @@ var gExtensionsViewController = {
>         return true;
>       return false;
>     }
>     switch (aCommand) {
>     case "cmd_installSearchResult":
>       return selectedItem.getAttribute("action") == "" ||
>              selectedItem.getAttribute("action") == "failed";
>     case "cmd_useTheme":
>+      if (selectedItem.hasAttribute("lwtheme"))
>+        return !gSelectedLWTheme ||
>+               selectedItem.getAttribute("addonID") != gSelectedLWTheme.id;
>+      if (gSelectedLWTheme)
>+        return true;
I'm having a bit of trouble parsing this. It seems to me that if there was a selected lightweight theme and the selected richlistitem is a theme that is disabled this would return true.

Comment 33

[Robert Strong (they/them - no direct email)]

Comment on attachment 400005 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>...
>@@ -2569,37 +2718,55 @@ var gExtensionsViewController = {
> 
>     cmd_close: function (aSelectedItem)
>     {
>       closeWindow(true);
>     },
> 
>     cmd_useTheme: function (aSelectedItem)
>     {
>-      gCurrentTheme = aSelectedItem.getAttribute("internalName");
>+      if (aSelectedItem.hasAttribute("lwtheme")) {
>+        let newTheme = LightweightThemeManager.getUsedTheme(aSelectedItem.getAttribute("addonID"));
>+        LightweightThemeManager.currentTheme = newTheme;
>+        gSelectedLWTheme = newTheme;
nit: the above two lines could be combined into one as you've done elsewhere but I'm ok with it as is though I prefer consistency.

>-      // If choosing the current skin just reset the pending change
>-      if (gCurrentTheme == gPref.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN)) {
>-        gPref.clearUserPref(PREF_EXTENSIONS_DSS_SWITCHPENDING);
>-        gPref.clearUserPref(PREF_DSS_SKIN_TO_SELECT);
>-        clearRestartMessage();
>+        if (gPref.prefHasUserValue(PREF_LWTHEME_TO_SELECT)) {
>+          clearRestartMessage();
>+          setRestartMessage(aSelectedItem);
>+        }
>       }
>       else {
>-        if (gPref.getBoolPref(PREF_EXTENSIONS_DSS_ENABLED)) {
>-          gPref.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, gCurrentTheme);
>+        gSelectedTheme = aSelectedItem.getAttribute("internalName");
>+
>+        // If choosing the current skin just reset the pending change
>+        if (gSelectedTheme == gCurrentTheme) {
>+          gPref.clearUserPref(PREF_EXTENSIONS_DSS_SWITCHPENDING);
>+          gPref.clearUserPref(PREF_DSS_SKIN_TO_SELECT);
>+          gSelectedLWTheme = LightweightThemeManager.currentTheme = null;
>+          clearRestartMessage();
>         }

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>@@ -2271,16 +2279,69 @@ ExtensionManager.prototype = {
>       this._profileSelected();
>       break;
>     case "quit-application-requested":
>       this._confirmCancelDownloadsOnQuit(subject);
>       break;
>     case "offline-requested":
>       this._confirmCancelDownloadsOnOffline(subject);
>       break;
>+    case "lightweight-theme-preview-requested":
>+      if (gPref.prefHasUserValue(PREF_GENERAL_SKINS_SELECTEDSKIN)) {
>+        let cancel = subject.QueryInterface(Ci.nsISupportsPRBool);
>+        cancel.data = true;
>+      }
>+      break;
>+    case "lightweight-theme-change-requested":
>+      let theme = JSON.parse(data);
>+      if (!theme)
>+        return;
What conditions would theme not be set? Is it worth adding a warn logging statement here when it is?

>+
>+      if (gPref.prefHasUserValue(PREF_GENERAL_SKINS_SELECTEDSKIN)) {
>+        if (getPref("getBoolPref", PREF_EM_DSS_ENABLED, false)) {
>+          gPref.clearUserPref(PREF_GENERAL_SKINS_SELECTEDSKIN);
>+          return;
>+        }
>+
>+        let cancel = subject.QueryInterface(Ci.nsISupportsPRBool);
>+        cancel.data = true;
>+        gPref.setBoolPref(PREF_DSS_SWITCHPENDING, true);
>+        gPref.setCharPref(PREF_DSS_SKIN_TO_SELECT, gDefaultTheme);
>+        gPref.setCharPref(PREF_LWTHEME_TO_SELECT, theme.id);
>+
>+        // Open the UI so the user can see that a restart is necessary
>+        var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                 getService(Ci.nsIWindowMediator);
>+        var win = wm.getMostRecentWindow("Extension:Manager");
>+
>+        if (win) {
>+          win.showView("themes");
>+          return;
>+        }
>+
>+        const EMURL = "chrome://mozapps/content/extensions/extensions.xul";
nit: this is already used in two other places... might as well make it global

Can I test this with existing personas and if not can you provide details as to how I can test this?

Comment 34

[Dave Townsend [:mossop]]

Updated patch on the way and I'll provide a way to test this more.

(In reply to comment #32)
.getService(Components.interfaces.nsIRDFContainerUtils);
> >+  var rootctr = rdfCU.MakeSeq(gLWThemeDS, gRDF.GetResource(RDFURI_ITEM_ROOT));
> >+  var themes = LightweightThemeManager.usedThemes;
> For my own benefit could you define what usedThemes are (specifically what used
> means in the context of a leightweight theme)?

The LightweightThemeManager remembers the last 8 lightweight themes that the user used. usedThemes is just an array of them

> >+    if (theme.description) {
> >+      gLWThemeDS.Assert(themeNode,
> >+                        gRDF.GetResource(PREFIX_NS_EM + "lwdescription"),
> >+                        gRDF.GetLiteral(theme.description),
> >+                        true);
> >+    }
> I'm not knocking it but could you explain to me why description uses "Created
> by %S." and theme's that have a description put it in lwdescription which is
> only displayed on the about page or point me to a wiki that explains it?

Very few lightweight themes have real descriptions set, and none of them are localised. It seemed best to just display who created the lightweight theme in the add-ons list, but I figured it would be worthwhile still showing the real description in the about dialog if it was available rather than showing the creator twice.

(In reply to comment #33)
> >     cmd_useTheme: function (aSelectedItem)
> >     {
> >-      gCurrentTheme = aSelectedItem.getAttribute("internalName");
> >+      if (aSelectedItem.hasAttribute("lwtheme")) {
> >+        let newTheme = LightweightThemeManager.getUsedTheme(aSelectedItem.getAttribute("addonID"));
> >+        LightweightThemeManager.currentTheme = newTheme;
> >+        gSelectedLWTheme = newTheme;
> nit: the above two lines could be combined into one as you've done elsewhere
> but I'm ok with it as is though I prefer consistency.

Actually they can't. The extension manager may veto the change to the new lightweight theme (if the non-default heavyweight theme is selected) and mark it to be enabled on the next restart. In which case we still want gSelectedLWTheme to be newTheme, but LightweightThemeManager.currentTheme will give null.

> >+    case "lightweight-theme-change-requested":
> >+      let theme = JSON.parse(data);
> >+      if (!theme)
> >+        return;
> What conditions would theme not be set? Is it worth adding a warn logging
> statement here when it is?

This notification is sent when turning off the lightweight theme as well which is when we get null. I don't think it is worth logging as it's an expected case.

Comment 35

[Robert Strong (they/them - no direct email)]

Thanks for the explanations.

(In reply to comment #34)
>...
> Very few lightweight themes have real descriptions set, and none of them are
> localised. It seemed best to just display who created the lightweight theme in
> the add-ons list, but I figured it would be worthwhile still showing the real
> description in the about dialog if it was available rather than showing the
> creator twice.
Just seems strange that you wouldn't use it in the add-ons mgr when it was available.

> (In reply to comment #33)
> > >     cmd_useTheme: function (aSelectedItem)
> > >     {
> > >-      gCurrentTheme = aSelectedItem.getAttribute("internalName");
> > >+      if (aSelectedItem.hasAttribute("lwtheme")) {
> > >+        let newTheme = LightweightThemeManager.getUsedTheme(aSelectedItem.getAttribute("addonID"));
> > >+        LightweightThemeManager.currentTheme = newTheme;
> > >+        gSelectedLWTheme = newTheme;
> > nit: the above two lines could be combined into one as you've done elsewhere
> > but I'm ok with it as is though I prefer consistency.
> 
> Actually they can't. The extension manager may veto the change to the new
> lightweight theme (if the non-default heavyweight theme is selected) and mark
> it to be enabled on the next restart. In which case we still want
> gSelectedLWTheme to be newTheme, but LightweightThemeManager.currentTheme will
> give null.
I would think the following would work
LightweightThemeManager.currentTheme = gSelectedLWTheme = newTheme;

but I'm fine with it the way it is even if it does.

Comment 36

[Dave Townsend [:mossop]]

(In reply to comment #35)
> Thanks for the explanations.
> 
> (In reply to comment #34)
> >...
> > Very few lightweight themes have real descriptions set, and none of them are
> > localised. It seemed best to just display who created the lightweight theme in
> > the add-ons list, but I figured it would be worthwhile still showing the real
> > description in the about dialog if it was available rather than showing the
> > creator twice.
> Just seems strange that you wouldn't use it in the add-ons mgr when it was
> available.

Perhaps yeah. We can consider switching it, doing so isn't much work, but I'd prefer to get this functionality landed rather than hold on for a decision.

> > (In reply to comment #33)
> > > >     cmd_useTheme: function (aSelectedItem)
> > > >     {
> > > >-      gCurrentTheme = aSelectedItem.getAttribute("internalName");
> > > >+      if (aSelectedItem.hasAttribute("lwtheme")) {
> > > >+        let newTheme = LightweightThemeManager.getUsedTheme(aSelectedItem.getAttribute("addonID"));
> > > >+        LightweightThemeManager.currentTheme = newTheme;
> > > >+        gSelectedLWTheme = newTheme;
> > > nit: the above two lines could be combined into one as you've done elsewhere
> > > but I'm ok with it as is though I prefer consistency.
> > 
> > Actually they can't. The extension manager may veto the change to the new
> > lightweight theme (if the non-default heavyweight theme is selected) and mark
> > it to be enabled on the next restart. In which case we still want
> > gSelectedLWTheme to be newTheme, but LightweightThemeManager.currentTheme will
> > give null.
> I would think the following would work
> LightweightThemeManager.currentTheme = gSelectedLWTheme = newTheme;
> 
> but I'm fine with it the way it is even if it does.

Uhh yeah of course that'd work.

Comment 37

[Dave Townsend [:mossop]]

Attachment: LWTM changes rev 3

Comment 38

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This addresses all the other comments that I haven't answered. Will supply testing info in a moment.

Comment 39

[Dave Townsend [:mossop]]

Attachment: test theme preference

If you set the value of lightweightThemes.usedThemes to the text in this file it will give you 8 installed lightweight themes to play with. http://design-noir.de/log/2009/09/basic-support-for-lightweight-theming-landed/ describes the js required to install a new lightweight theme. Or you can probably just try with bug 511771 which adds install support and should work on getpersonas.com

Comment 40

[Dão Gottwald [:dao]]

getpersonas.com won't work with bug 511771 yet, since the events are slightly different.

Comment 41

[Robert Strong (they/them - no direct email)]

Comment on attachment 400983 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>...
>@@ -2439,32 +2588,37 @@ var gExtensionsViewController = {
>         return true;
>       return false;
>     }
>     switch (aCommand) {
>     case "cmd_installSearchResult":
>       return selectedItem.getAttribute("action") == "" ||
>              selectedItem.getAttribute("action") == "failed";
>     case "cmd_useTheme":
>+      if (selectedItem.hasAttribute("lwtheme"))
>+        return !gLWThemeToSelect ||
>+               selectedItem.getAttribute("addonID") != gLWThemeToSelect.id;
>       return selectedItem.type == nsIUpdateItem.TYPE_THEME &&
>              !selectedItem.isDisabled &&
>              selectedItem.opType != OP_NEEDS_UNINSTALL &&
>-             gCurrentTheme != selectedItem.getAttribute("internalName");
>+             (gLWThemeToSelect || gThemeToSelect != selectedItem.getAttribute("internalName"));
nit: to keep it under 80
               (gLWThemeToSelect ||
                gThemeToSelect != selectedItem.getAttribute("internalName"));

Comment 42

[Robert Strong (they/them - no direct email)]

Comment on attachment 400983 [details] [diff] [review]
patch rev 2

Everything works well. r=me

I believe the following errors that I found while testing this is unrelated to these changes but they should be looked into.

This first one was reported multiple times... the rest only once

Error: document.getElementById(id) is null
Source File: chrome://mozapps/content/extensions/extensions.xml
Line: 415

image.onload = function() {
>>  document.getElementById(id)._imageLoaded(image);
};


While installing a theme

Error: dialog is null
Source File: chrome://global/content/bindings/dialog.xml
Line: 356

      <method name="_fireButtonEvent">
        <parameter name="aDlgType"/>
        <body>
        <![CDATA[
          var event = document.createEvent("Events");
          event.initEvent("dialog"+aDlgType, true, true);
          
          // handle dom event handlers
          var noCancel = this.dispatchEvent(event);
          
          // handle any xml attribute event handlers
          var handler = this.getAttribute("ondialog"+aDlgType);
          if (handler != "") {
>>            var fn = new Function("event", handler);
            var returned = fn(event);
            if (returned == false)
              noCancel = false;
          }
          
          return noCancel;
        ]]>


While installing a theme

Warning: reference to undefined property this._progressData[id].progress
Source File: file:///C:/moz/_1_mozilla-central/ff-debug/dist/bin/components/nsExtensionManager.js
Line: 7910

  _rdfGet_progress: function EMDS__rdfGet_progress(item, property) {
    var id = item.Value;
    if (id in this._progressData)
>>      return EM_I(this._progressData[id].progress);
    return null;
  },

Comment 43

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/c41850fecc96

We have some automated tests but we'll need some litmus tests to cover the interactions with other third party themes as we don't have an easy way to automate that. I'll follow up with some test cases in a bit.

Comment 44

[Dave Townsend [:mossop]]

(In reply to comment #42)
> This first one was reported multiple times... the rest only once
> 
> Error: document.getElementById(id) is null
> Source File: chrome://mozapps/content/extensions/extensions.xml
> Line: 415

Yeah I've known about this for a while but never quite got around to correcting it, filed and patched in bug 517176

> While installing a theme

Not seen these before, filed bug 517177 to track them.

Comment 45

[Dave Townsend [:mossop]]

Landed on branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e5a3d46e380b

Comment 46

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre.  8 MRUs shown.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #43)
> We have some automated tests but we'll need some litmus tests to cover the
> interactions with other third party themes as we don't have an easy way to
> automate that. I'll follow up with some test cases in a bit.

I know that we have added a lot of automated tests in the last couple of months. Has something be changed in scope of the proposed litmus tests? Are those still needed, and which specific areas are lacking test coverage?

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Manual test covered by:
https://litmus.mozilla.org/show_test.cgi?id=11412