Fix toolbar icon customization in Australis

Multiple changes here: 

* Default icon placement is nav-bar again, no longer addon-bar

* Removed all add-on bar specific code, including checks for collapsed toolbars

* Our icon isn`t necessarily a direct child of the toolbar, account for that

* Use CustomizeUI API when available, fake it if it isn`t

* Make sure our menu is still initialized even if the icon is placed inside the Australis panel

* Silence a parameter redeclaration warning in _showNextNotification

* While at it, simplify a bunch of code passages by using for..of.

[Wladimir Palant, 2013-11-25 09:31:49]

Improved approach (custom button type), marked our custom API extensions

[Wladimir Palant, 2013-11-27 16:04:01]

http://codereview.adblockplus.org/5741004535627776/diff/5135032834326528/lib/...
File lib/customizableUI.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5135032834326528/lib/...
lib/customizableUI.js:253: return {area: toolbar.id, placement: index};
Given that we don't use the placement parameter in the current iteration of this
code, returning {area: toolbar.id} and removing unnecessary code is probably
better.

http://codereview.adblockplus.org/5741004535627776/diff/5135032834326528/lib/...
lib/customizableUI.js:265: let position =
toolbox.getAttribute(widget.positionAttribute);
This masks a parameter. We should probably simply remove area and position
parameters in order to indicate that these are ignored.

[Wladimir Palant, 2013-12-02 07:40:38]

These changes have been pushed so that nightly users can already test them:

https://hg.adblockplus.org/adblockplus/rev/2da8a4cb876a
https://hg.adblockplus.org/adblockplus/rev/adb40c833539
https://hg.adblockplus.org/adblockplus/rev/b5f4475b7dae
https://hg.adblockplus.org/adblockplus/rev/639f67e9f4d6

[Wladimir Palant, 2013-12-02 10:58:00]

I've added a 32x32 status icon now so that our 24x24 icon isn't being stretched
to fit any more. I've also fixed my own review comments. These changes have also
been pushed already:

https://hg.adblockplus.org/adblockplus/rev/f2ee6c86be79
https://hg.adblockplus.org/adblockplus/rev/c19eb7f3465d

[Wladimir Palant, 2013-12-02 15:14:17]

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/ui.js:435: onCommand: this.onIconCommand
These two lines don't actually do anything, they are ignored if type isn't
button. https://hg.adblockplus.org/adblockplus/rev/245083285e39 fixed this.

[tschuster, 2014-05-15 20:12:29]

I haven't really reviewed your shim for CustomizeUI yet, because I want to look
a bit more at the Firefox documentation to make sure the implementation makes
sense.

But I have some general comments already. Overall to me the new api seems really
nice.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
File chrome/content/ui/settings.xul (left):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
chrome/content/ui/settings.xul:35: <setting id="adblockplus-showinaddonbar"
type="bool" title="&showinaddonbar.label;"/>
I wonder if we should keep this for all the people, who use various "addon bar
for ever" extensions? I guess they can always just drag it in there now?

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
chrome/content/ui/settings.xul:37: <setting id="adblockplus-showinstatusbar"
pref="extensions.adblockplus.showinstatusbar" type="bool"
title="&showinstatusbar.label;"/>
Aside: Isn't the statusbar long gone?

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
File lib/appSupport.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/appSupport.js:279: parent: "nav-bar"
The new api calls this "area", maybe switch to that.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/ui.js:423: {
We can probably make these arrow functions when I land my patch first :)

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/ui.js:1007: let {defaultToolbarPosition} = require("appSupport");
From the rest of the code it looks like this can be null, but I can't say if it
could happen here.

[tschuster, 2014-05-19 11:21:29]

lgtm

The new API usage looks good, but I can't really verify your shim is totally
correct, but as it seems to reuse some of the old code, it should be fine.

[Felix Dahlke, 2014-07-08 17:33:45]

On 2014/05/19 11:21:29, tschuster wrote:
> lgtm
> 
> The new API usage looks good, but I can't really verify your shim is totally
> correct, but as it seems to reuse some of the old code, it should be fine.

Can you get back to Tom's comments Wladimir? Then I can do a quick review and we
can finally close this :D

[Wladimir Palant, 2014-07-11 07:15:54]

Sorry, looks like I forgot replying to Tom's comments. I don't think any of
these comments necessitate changes, see below.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
File chrome/content/ui/settings.xul (left):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
chrome/content/ui/settings.xul:35: <setting id="adblockplus-showinaddonbar"
type="bool" title="&showinaddonbar.label;"/>
On 2014/05/15 20:12:30, tschuster wrote:
> I wonder if we should keep this for all the people, who use various "addon bar
> for ever" extensions? I guess they can always just drag it in there now?

I don't think we can support the various customization extensions out there. The
assumption that they will work the same way Firefox used to work is a pretty
weak one. These extensions should still allow customizing the icons and if not -
this isn't really our concern.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/chro...
chrome/content/ui/settings.xul:37: <setting id="adblockplus-showinstatusbar"
pref="extensions.adblockplus.showinstatusbar" type="bool"
title="&showinstatusbar.label;"/>
On 2014/05/15 20:12:30, tschuster wrote:
> Aside: Isn't the statusbar long gone?

In Firefox - yes. This option is only being shown in Thunderbird and SeaMonkey.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
File lib/appSupport.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/appSupport.js:279: parent: "nav-bar"
On 2014/05/15 20:12:30, tschuster wrote:
> The new api calls this "area", maybe switch to that.

I don't really feel like renaming everything whenever the terminology in Firefox
changes :)

IMHO that would be an unnecessary risk. Also note that Thunderbird and SeaMonkey
don't call it "area."

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/ui.js:423: {
On 2014/05/15 20:12:30, tschuster wrote:
> We can probably make these arrow functions when I land my patch first :)

Don't think so, arrow functions are for trivial functions - here I'd prefer
having proper ones.

http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/...
lib/ui.js:1007: let {defaultToolbarPosition} = require("appSupport");
On 2014/05/15 20:12:30, tschuster wrote:
> From the rest of the code it looks like this can be null, but I can't say if
it
> could happen here.

No, it cannot - nothing would call toggleToolbarIcon() if the toolbar icon isn't
supported.

[Felix Dahlke, 2016-09-15 09:09:10]

Sorry for letting this sit here so long. It has landed pre-review a long time
ago, so there's little point in a thorough review from my side now. Instead I
did a very high level review to see if anything bothers me. Nothing does, so
LGTM :)