568701 - Support simultaneous personas and themes application

Status: RESOLVED FIXED

(Mozilla Labs Graveyard :: Personas Plus, defect)

Description

[Ryan Doherty (:rdoherty)]

Spec: http://docs.google.com/Doc?docid=0AZBbC8aKZE1mZGRzZzZkcmpfMThkeHhqbmdnbg&hl=en

Note: This is only for Firefox 3.6 or below. Firefox 4.0 will support this natively.

Comment 1

[Jose E. Bolanos]

Attachment: First patch attempt

This patch addresses all the tasks described here:
1) http://docs.google.com/Doc?docid=0AZBbC8aKZE1mZGRzZzZkcmpfMThkeHhqbmdnbg&hl=en
2) https://docs.google.com/document/edit?id=1EZsTe0bLEwLC15vC5DBXfHHOncbHah8_0xXZ9LdNGYc&hl=en&authkey=CO_Zm-AB&pli=1#
3) https://docs.google.com/document/edit?id=1sCvhV2IvHe9TgSFm3XEA0pZbrYSBUtKJ4Aa7l81-Usk&hl=en#

To accomplish this, both the nsExtensionManager component and Firefox's Add-on window were overriden/overlayed. Note that this patch only affects Firefox [3.6, 4.0[

The Add-ons window overlay adds a new view for Personas, thus separating regular themes with personas into two views. The added overlay scripts overrides methods from the original add-ons window to accommodate this view and this separation.

The nsExtensionManager component is overriden to allow personas to be applied over non-default themes. This is allowed when they have a "skinnable" property set to true in their install.rdf file, or when the "lightweightThemes.forceSkinning" preference is set to true (not added by the add-on).

Comment 2

[Ryan Doherty (:rdoherty)]

Comment on attachment 457986 [details] [diff] [review]
First patch attempt

Thanks Jose! Assigning the review to Myk.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 457986 [details] [diff] [review]
First patch attempt

Overall, this looks great, just a couple issues...


diff -r bdb75874cf45 client/chrome.manifest.in

-overlay chrome://browser/content/browser.xul      chrome://personas/content/personas.xul   application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
-overlay chrome://messenger/content/messenger.xul  chrome://personas/content/messenger.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6}
+overlay chrome://browser/content/browser.xul                chrome://personas/content/personas.xul    application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
+overlay chrome://mozapps/content/extensions/extensions.xul  chrome://personas/content/extensions.xul  application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}  appversion>=3.6

It looks like this patch is designed to work specifically with Firefox 3.6, and not on Firefox 4, where the addon manager has been completely rewritten, so ideally this manifest line would specify compatibility with only 3.6.*.  Unfortunately, it doesn't look like this is possible (except by doing "appversion=3.6 appversion=3.6.1 appversion=3.6.2 etc."), so you'll need to check the version in the overlay code itself, as you are already doing.

Fortunately, Firefox 4 opens the manager via about:addons, which doesn't actually trigger this overlay, so most of the time it won't get loaded.


diff -r bdb75874cf45 client/components/nsPersonasExtensionManager.js

+ * The Original Code is Personas.
+ *
+ * The Initial Developer of the Original Code is Mozilla.
+ * Portions created by the Initial Developer are Copyright (C) 2007
+ * the Initial Developer. All Rights Reserved.
+ *

This license block (and the others) should have a "Contributors" section, to which you should add your name!  Also, for new files, make the copyright date 2010.


+// LightweightThemeManager may not be not available (Firefox < 3.6 or Thunderbird)
+try { Cu.import("resource://gre/modules/LightweightThemeManager.jsm"); }
+catch (e) { LightweightThemeManager = null; }

Actually, Thunderbird 3.1 ships with LightweightThemeManager.jsm.  I'm not sure if it uses it, however.


+var gOS = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
+var gRDF = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
+var gPref = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2).
+  QueryInterface(Ci.nsIPrefService);

The Observers and Preferences modules would simplify access to the observer and preferences services.


+// Given two instances, copy in all properties from "base"
+// and create forwarding methods for all functions.
+function inheritCurrentInterface(self, base) {
+  for(let prop in base) {
+    if(typeof self[prop] === 'undefined')
+      if(typeof base[prop] === 'function') {
+        (function(prop) {
+          self[prop] = function() {
+            return base[prop].apply(base,arguments);
+          };
+        })(prop);
+      }
+      else
+        self[prop] = base[prop];
+  }
+}

This should define getters that retrieve the base instance's properties rather than merely copying their values to the inheriting instance, since that'll accommodate subsequent changes to those values, i.e. something like:

  self.__defineGetter__(prop, function() base[prop]);

Also, it looks like this function doesn't handle settable properties.  That's fine in this case, since the four interfaces the extension manager implements (nsIExtensionManager, nsITimerCallback, nsIObserver, nsIClassInfo) don't have any of those.  But it's worth at least noting that here, in case someone later tries to use this function on an object that does have settable properties.


+function getPref(func, preference, defaultValue) {
+  try {
+    return gPref[func](preference);
+  }
+  catch (e) {
+  }
+  return defaultValue;
+}

The Preferences module would handle this for you, and you wouldn't even have to know the datatype of the value you're getting, as it sorts that out while retrieving the value!


+/**
+ * Overriden Extension Manager object. Handles the lightweight theme observer
+ * notifications to allow personas to be applied over compatible themes.
+ */
+function PersonasExtensionManager() {
+  if (!LightweightThemeManager)
+    return;

If LightweightThemeManager is not defined, this is still going to return the new instance of PersonasExtensionManager, since returning "undefined" from a JS constructor causes it to return the newly created object; it just won't have been configured.  It would be better to simply not register the component if LightweightThemeManager is undefined, so the constructor never gets called, and this check becomes unnecessary.


+  // Load Persona strings, used in the restart notification.
+  this._strings =
+    Cc["@mozilla.org/intl/stringbundle;1"].
+      getService(Ci.nsIStringBundleService).
+        createBundle("chrome://personas/locale/personas.properties");

The StringBundle module would make accessing this string bundle easier.  It'd also be better to load this lazily via a memoizing unary getter, i.e.:

  get _strings() {
    Cu.import("resource://personas/modules/StringBundle.js", this);
    delete this._strings;
    return this._strings = new this.StringBundle("chrome://personas/locale/personas.properties");
  },


+PersonasExtensionManager.prototype = {
+  classDescription: "Personas Plus Extension Manager",
+  contractID: "@mozilla.org/extensions/manager;1",
+  classID: Components.ID("{DF74077B-EB16-47B0-8C37-12D577A7F1AE}"),
+
+  QueryInterface: function(aIID) {
+    gOldHandler.QueryInterface(aIID);
+    inheritCurrentInterface(this, gOldHandler);
+    return this;
+  },

Note that this is going to recopy all the interface elements from all interfaces previously requested, since QueryInterface is additive (i.e. QIing an object to nsIFoo and then nsIBar results in the object having both nsIFoo's and nsIBar's properties).  It would be better to retrieve the component anew and QI it to just the requested interface before copying its properties, so you only copy the properties of the requested interface.  It would also be useful to record which interfaces have already been queried and avoid recopying properties from previously requested interfaces.


+if (appInfo.ID == FIREFOX_ID &&
+    versionChecker.compare(appInfo.version, "3.6") >= 0 &&
+    versionChecker.compare(appInfo.version, "4.0") < 0) {
+  components = [PersonasExtensionManager];
+}

This'll correctly exclude Firefox 4.0 and all stable updates, but it'll include 4.0 beta releases (those with versions like 4.0b1, 4.0b2, etc.) and nightly builds, whose versions are "less than" 4.0.  That's ok for now, since client.mk doesn't specify compatibility with 4.0.  But presumably that'll change eventually, at which point, to exclude those beta releases (while continuing to include 3.6 stable updates), test for |3.6.* < 0| instead here and elsewhere (the asterisk represents infinity, so 3.6.anything will always be less than it).


diff -r bdb75874cf45 client/content/extensions.js

+  /**
+   * Overriden method from Firefox extensions.js. Enables cmd_useTheme for
+   * Persona entries.
+   */

It'd be handy to mark the parts of these functions that have been modified from their original versions, so if something changes in the core code, and they need to be updated, it's easier to do so.

Finally, is there a particular theme I can use to test the functionality?  I tried several on AMO that are apparently not compatible with personas, but I didn't find one that was.

Comment 4

[Ryan Doherty (:rdoherty)]

(In reply to comment #3)
> Finally, is there a particular theme I can use to test the functionality?  I
> tried several on AMO that are apparently not compatible with personas, but I
> didn't find one that was.

If you want to force a persona on a theme you can add it to about:config: lightweightThemes.forceSkinning = true

A good theme is Nautipolis https://addons.mozilla.org/en-US/firefox/addon/123/
You could also download the theme and edit its install.rdf, adding the value 'skinnable', set to true.

Comment 5

[Jose E. Bolanos]

Attachment: Second Patch Attempt

This patch addresses all the issues pointed out by Myk in comment #3, except for:

"This should define getters that retrieve the base instance's properties rather
than merely copying their values to the inheriting instance, since that'll
accommodate subsequent changes to those values, i.e. something like:

self.__defineGetter__(prop, function() base[prop]);"

We weren't able to use this code. Whenever we tried to set getters this way, Firefox would hang. We also tried other things with no luck. So we left it as it was. Any idea why this happens?

Plus:
- Custom personas were not being handled correctly in the previous patch. Now the LightweightThemeManager.setLocalTheme method is used for custom personas, both in the extensions.js and service.js files.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 460336 [details] [diff] [review]
Second Patch Attempt

(In reply to comment #5)

> self.__defineGetter__(prop, function() base[prop]);"
> 
> We weren't able to use this code. Whenever we tried to set getters this way,
> Firefox would hang. We also tried other things with no luck. So we left it as
> it was. Any idea why this happens?

Ah, I bet that's bug 449811, which I should have realized would create problems here, given that I was the one who filed it.

The workaround is to define the getter in a function, which then closes around each individual value of prop, i.e.:

function inheritCurrentInterface(self, base) {
  function defineGetter(prop) {
    self.__defineGetter__(prop, function() base[prop]);
  }

  for(let prop in base) {
    if(typeof self[prop] === 'undefined')
      if(typeof base[prop] === 'function') {
        (function(prop) {
          self[prop] = function() {
            return base[prop].apply(base,arguments);
          };
        })(prop);
      }
      else
        defineGetter(prop);
  }
}

Otherwise, this looks good, r=myk with that change!

Comment 7

[Jose E. Bolanos]

Thanks Myk! We've made this change and committed the changes in rev:
http://hg.mozilla.org/labs/personas/rev/aeee0b0961a9