issue #1155 - Don't allow displaying icon text with Australis theme

lib/main.js

 /*
  * This Source Code is subject to the terms of the Mozilla Public License
  * version 2.0 (the "License"). You can obtain a copy of the License at
  * http://mozilla.org/MPL/2.0/.
  */
 
 Cu.import("resource://gre/modules/Services.jsm");
-let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", null);
 
 let {Prefs} = require("prefs");
 let {WindowObserver} = require("windowObserver");
 
-let CustomizableUI = null;

[Wladimir Palant, 2014/07/30 09:38:55]

No point keeping a global reference to CustomizableUI around - a flag like
isAustralis will do as well, and it will be more obvious what this is about.

[saroyanm, 2014/07/30 10:56:23]

Done.

+var isAustralis = false;
+try
+{
+  Cu.import("resource:///modules/CustomizableUI.jsm", null);
+  isAustralis = true;
+} catch(e) {}
 
 var WindowFeature =
 {
   observer: null,
 
   init: function()
   {
     if (!this.observer)
       this.observer = new WindowObserver(this, "ready");
   },
@@ skipping 12 matching lines @@
     this._applyToWindow(window);
   },
 
   removeFromWindow: function(window)
   {
     if (window.location.href != this.windowUrl)
       return;
     this._removeFromWindow(window);
   }
 }
-
-/**
- * Object initializing add-on options, observes add-on manager notifications
- * about add-on options being opened.
- * @type nsIObserver
- */
-let optionsObserver =
-{
-  init: function()
-  {
-    Services.obs.addObserver(this, "addon-options-displayed", true);
-    onShutdown.add(function()
-    {
-      Services.obs.removeObserver(this, "addon-options-displayed");
-    }.bind(this));
-  },
-
-  initOptionsDoc: function(/**Document*/ doc)
-  {
-    if (CustomizableUI)
-      doc.getElementById("abpcustomization-icondisplay").collapsed = true;
-  },
-
-  observe: function(subject, topic, data)
-  {
-    let {addonID} = require("info")
-    if (data != addonID)
-      return;
-
-    this.initOptionsDoc(subject.QueryInterface(Ci.nsIDOMDocument));
-  },
-
-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference])
-};
-optionsObserver.init();
 
 var VerticalPreferencesLayout =
 {
   __proto__: WindowFeature,
   windowUrl: "chrome://adblockplus/content/ui/filters.xul",
 
   _applyToWindow: function(window)
   {
     let content = window.document.getElementById("content");
     let splitter = window.document.getElementById("filtersSplitter");
@@ skipping 64 matching lines @@
   __proto__: StylesheetFeature,
   stylesheet: "chrome://abpcustomization/content/oneLineSubscriptions.css"
 };
 
 var RemoveActionsButton =
 {
   __proto__: StylesheetFeature,
   stylesheet: "chrome://abpcustomization/content/noActionButton.css"
 };
 
+var AddonPageAustralisStyles =
+{
+  __proto__: StylesheetFeature,
+  stylesheet: "chrome://abpcustomization/content/addonPageAustralisStyles.css"
+};
+
 var ToolbarIconDisplay =
 {
   __proto__: StylesheetFeature,
   get template()
   {
     let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
     request.open("GET", "chrome://abpcustomization/content/toolbarIconDisplay.css", false);
     request.overrideMimeType("text/plain");
     request.send();
     let result = request.responseText;
@@ skipping 28 matching lines @@
   stylesheet: "chrome://abpcustomization/content/hideMenus.css"
 };
 
 let features =
 {
   "addon-page-styles": AddonPageStyles,
   "vertical-preferences-layout": VerticalPreferencesLayout,
   "preferences-one-line-subscriptions": OneLineSubscriptions,
   "preferences-remove-actions-button": RemoveActionsButton,
   "toolbar-icon-display": ToolbarIconDisplay,
+  "addon-page-australis-styles": AddonPageAustralisStyles,
   "green-icon": GreenIcon,
   "remove-menus": RemoveMenus
 };
 
 function updateFeature(name)
 {
-  try
-  {
-    ({CustomizableUI}) = Cu.import("resource:///modules/CustomizableUI.jsm", null);
-  } catch (e) {}

[Wladimir Palant, 2014/07/30 09:38:55]

Why run this for each feature? It should be best to have this at the top, where
the variable is declared:

var isAustralis = false;
try
{
  Cu.import("resource:///modules/CustomizableUI.jsm", null);
  isAustralis = true;
} catch(e) {}

[saroyanm, 2014/07/30 10:56:23]

Oops, too inattentive today!

-
   if (name in features)
   {
     let enabled;
     if (name == "addon-page-styles")
       enabled = true;

[Wladimir Palant, 2014/07/30 09:38:55]

Actually, seeing this - it's probably cleaner to implement hiding of the
preference the same way, without any observer. We can add a new
StylesheetFeature similar to AddonPageStyles and have |enabled = isAustralis|
here so that the stylesheet will only be applied if Australis theme is active.

[saroyanm, 2014/07/30 10:56:23]

Awesome, great idea. How could I miss that.

-    else if (name == "toolbar-icon-display" && CustomizableUI)
+    else if (name == "toolbar-icon-display" && isAustralis)
       enabled = false;
+    else if (name == "addon-page-australis-styles")
+      enabled = isAustralis;
     else
       enabled = Prefs[name];
 
     if (enabled)
       features[name].init();
     else
       features[name].shutdown();
   }
 }
 
 // Initialize features and make sure to update them on changes
 for (let feature in features)
   updateFeature(feature);
 
 Prefs.addListener(updateFeature);