940013 - Web developer toolbar addon broken with new Australis theme

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Pascal Chevrel:pascalc]

Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118094134 CSet: f2adb62d07eb

With the new Australis theme, the Web Developer toolbar becomes a blank toolbar.

https://addons.mozilla.org/fr/firefox/addon/web-developer/

1/ install web developer toolbar extension
2/ click on the web developer toolbar toggle button to display the toolbar

expected result:
Webdev toolbar displayed with all of its icons and menus

actual result:
The toolbar is blank

Comment 1

[:Gijs (he/him)]

I suspect this needs to call registerArea... but thinking about this... I wonder if we could

(a) make registerToolbarNode imply a registerArea call and take the options off the toolbar itself (ie defaultset attribute, type: toolbar, legacy: true)
(b) allow registerArea to be called again afterwards and override the existing properties (and rerun buildArea for all the registered nodes if necessary).

This way, AFAICT existing add-ons wouldn't normally need to do anything.

Jared, what do you think? Crazy or great idea? :-)

Comment 2

[:Gijs (he/him)]

Getting some more opinions on comment #1, I hope... I think this will reduce the headaches for add-on developers and will be even nicer than 'just' allowing registerToolbarNode to be called before registerArea. :-)

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hm, yea, that could work.

Comment 4

[:Gijs (he/him)]

Attachment: fix Australis' CustomizableUI to have better interaction between registerToolbarNode and registerArea,

This is fairly crazy, but surprisingly passes all the extant tests. Obviously, it needs tests itself, but I'm crashing so I'll write those tomorrow.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8340149 [details] [diff] [review]
fix Australis' CustomizableUI to have better interaction between registerToolbarNode and registerArea,

Blair, can you give this approach a quick look and tell me if it looks vaguely acceptable? Also fixes bug 940443 and bug 941561 - although I just realized I left out the legacy setting in registerArea itself. Meh, not sure how to do that because forcing everyone else to pass legacy: false seems dumb, too. The solution in the patch seems workable to me.

Comment 6

[:Gijs (he/him)]

Attachment: fix Australis' CustomizableUI to have better interaction between registerToolbarNode and registerArea,

Now with less brokenness, and a test.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8340374 [details] [diff] [review]
fix Australis' CustomizableUI to have better interaction between registerToolbarNode and registerArea,

Review of attachment 8340374 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +100,5 @@
> +/**
> + * gPendingToolbars is a map from area IDs to build nodes that are waiting
> + * for the area to be registered
> + */
> +let gPendingToolbars = new Map();

gPendingBuildAreas - much of the code dealing with this isn't specific to toolbars. The only time any code should explicitly refer to "toolbars" is in registerToolbarNode() and setting the default type in registerArea().

@@ +293,5 @@
>  
> +    // Sanity check type and default placements array:
> +    let allTypes = [CustomizableUI.TYPE_TOOLBAR, CustomizableUI.TYPE_MENU_PANEL];
> +    if (allTypes.indexOf(props.get("type")) == -1) {
> +      throw new Error("Invalid area type " + props.get("type"));

Move to just under where you set the default type, to potentially avoid a tiny amount of work (and clump the type/placements code together).

@@ +312,5 @@
> +      }
> +
> +      // If we have pending toolbar nodes, register all of them
> +      if (gPendingToolbars.has(aName)) {
> +        let toolbars = gPendingToolbars.get(aName);

s/toolbars/something else/

@@ +770,5 @@
>      }
> +
> +    for (let [area, toolbarMap] of gPendingToolbars) {
> +      let toDelete = [];
> +      for (let [toolbar, ] of toolbarMap) {

Ditto.

Comment 8

[:Gijs (he/him)]

With nits,

remote:   https://hg.mozilla.org/integration/fx-team/rev/78e12d1f3aa2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78e12d1f3aa2

Comment 11

[Pascal Chevrel:pascalc]

I can confirm that the Web developer toolbar is back to normal with today's build, thanks.

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Firefox Aurora (build ID: 20140226004001).