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

This issue is related to current ticket:
https://issues.adblockplus.org/ticket/1155

[saroyanm, 2014-07-28 20:04:05]

Wladimir can you please have look, when you have time.

[Wladimir Palant, 2014-07-30 07:45:39]

What if somebody already configured that feature in a previous version of
Customization (or in a version of Firefox without Australis)? They will be stuck
with a broken configuration. Please disable the feature as well, not only the
setting.

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
lib/main.js:67: }
No point to have a generic function here, we only have one element to hide. This
should do:

doc.getElementById("abpcustomization-icondisplay").collapsed = true;

Note that this is somewhat more complicated in Adblock Plus because different
options pages are possible there.

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
lib/main.js:72: if (CustomizableUI)
This check is pointless, if the module fails to load there will be an exception.

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
lib/main.js:86: QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
Ci.nsISupportsWeakReference])
Please import XPCOMUtils in this file. The fact that you can use XPCOMUtils from
bootstrap.js is a buildtools issue, `build.py build -m` will produce a build
with proper scope isolation.

[saroyanm, 2014-07-30 09:18:32]

On 2014/07/30 07:45:39, Wladimir Palant wrote:
> What if somebody already configured that feature in a previous version of
Customization (or in a version of Firefox without Australis)? They will be stuck
with a broken configuration. Please disable the feature as well, not only the
setting.

Yes, missed that. Thanks.

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
lib/main.js:67: }
On 2014/07/30 07:45:40, Wladimir Palant wrote:
> No point to have a generic function here, we only have one element to hide.
This
> should do:
> 
> doc.getElementById("abpcustomization-icondisplay").collapsed = true;
> 
> Note that this is somewhat more complicated in Adblock Plus because different
> options pages are possible there.

Will keep in mind.

http://codereview.adblockplus.org/4744516900749312/diff/5629499534213120/lib/...
lib/main.js:86: QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
Ci.nsISupportsWeakReference])
On 2014/07/30 07:45:40, Wladimir Palant wrote:
> Please import XPCOMUtils in this file. The fact that you can use XPCOMUtils
from
> bootstrap.js is a buildtools issue, `build.py build -m` will produce a build
> with proper scope isolation.

Done.

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

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:13: let CustomizableUI = null;
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.

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:222: } catch (e) {}
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) {}

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:228: enabled = true;
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]

Updated.

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:13: let CustomizableUI = null;
On 2014/07/30 09:38:55, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:222: } catch (e) {}
On 2014/07/30 09:38:55, Wladimir Palant wrote:
> 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) {}

Oops, too inattentive today!

http://codereview.adblockplus.org/4744516900749312/diff/5717271485874176/lib/...
lib/main.js:228: enabled = true;
On 2014/07/30 09:38:55, Wladimir Palant wrote:
> 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.

Awesome, great idea. How could I miss that.

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
lib/main.js:188: "remove-toolbar-icon-display-option":
RemoveToolbarIconDisplayOption,
Have little doubt regarding the naming, hope we will be Okey with namings.

[Wladimir Palant, 2014-07-30 11:34:08]

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/chro...
File chrome/content/noToolbarIconDisplay.css (right):

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/chro...
chrome/content/noToolbarIconDisplay.css:13: visibility: collapse;
Any reason why this shouldn't be display:none?

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
lib/main.js:188: "remove-toolbar-icon-display-option":
RemoveToolbarIconDisplayOption,
On 2014/07/30 10:56:23, saroyanm wrote:
> Have little doubt regarding the naming, hope we will be Okey with namings.

It's probably better to name it AddonPageAustralisStyles - not quite as long and
it describes the intention rather than the implementation. It would also make
sense to place it right after AddonPageStyles in the list here, given how
similar they are.

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
lib/main.js:201: enabled = !isAustralis;
I don't think we want to have |enabled = true| for every SeaMonkey user (no
Australis). Your previous attempt was correctly setting the enabled variable
only in the Australis scenario and following the default path (preferences)
otherwise.

[saroyanm, 2014-07-30 12:50:35]

Updated.

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/chro...
File chrome/content/noToolbarIconDisplay.css (right):

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/chro...
chrome/content/noToolbarIconDisplay.css:13: visibility: collapse;
On 2014/07/30 11:34:08, Wladimir Palant wrote:
> Any reason why this shouldn't be display:none?

I did it because, we were using element.collapsed to hide elements in code,
according to collapsed attribute documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/collapsed 
in CSS I used equivalent for the collapsed attribute thought that I should use
visibility attr to hide XUL elements, but I was wrong while the main purpose of
visibility property is different and in this case we should use display property
to hide XUL element.
Thanks for pointing.

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
File lib/main.js (right):

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
lib/main.js:188: "remove-toolbar-icon-display-option":
RemoveToolbarIconDisplayOption,
On 2014/07/30 11:34:08, Wladimir Palant wrote:
> On 2014/07/30 10:56:23, saroyanm wrote:
> > Have little doubt regarding the naming, hope we will be Okey with namings.
> 
> It's probably better to name it AddonPageAustralisStyles - not quite as long
and
> it describes the intention rather than the implementation. It would also make
> sense to place it right after AddonPageStyles in the list here, given how
> similar they are.

Done.

http://codereview.adblockplus.org/4744516900749312/diff/5700735861784576/lib/...
lib/main.js:201: enabled = !isAustralis;
On 2014/07/30 11:34:08, Wladimir Palant wrote:
> I don't think we want to have |enabled = true| for every SeaMonkey user (no
> Australis). Your previous attempt was correctly setting the enabled variable
> only in the Australis scenario and following the default path (preferences)
> otherwise.

Make, sense. Thanks.

[Wladimir Palant, 2014-07-30 13:30:09]

LGTM