Closed Bug 940013 Opened 7 years ago Closed 7 years ago

Web developer toolbar addon broken with new Australis theme

Categories

(Firefox :: Toolbars and Customization, defect)

All
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 28

People

(Reporter: pascalc, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 1 obsolete file)

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
Component: Theme → Toolbars and Customization
See Also: → 940443
Whiteboard: [Australis:P3]
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? :-)
Flags: needinfo?(jaws)
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. :-)
Blocks: 940443
Flags: needinfo?(bmcbride)
Hm, yea, that could work.
Flags: needinfo?(bmcbride)
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8340149 - Flags: feedback?(bmcbride)
Flags: needinfo?(jaws)
Now with less brokenness, and a test.
Attachment #8340374 - Flags: review?(bmcbride)
Attachment #8340149 - Attachment is obsolete: true
Attachment #8340149 - Flags: feedback?(bmcbride)
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.
Attachment #8340374 - Flags: review?(bmcbride) → review+
With nits,

remote:   https://hg.mozilla.org/integration/fx-team/rev/78e12d1f3aa2
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Duplicate of this bug: 941561
https://hg.mozilla.org/mozilla-central/rev/78e12d1f3aa2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 28
I can confirm that the Web developer toolbar is back to normal with today's build, thanks.
Verified as fixed on latest Firefox Aurora (build ID: 20140226004001).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.