Web developer toolbar addon broken with new Australis theme

VERIFIED FIXED in Firefox 28

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: pascalc, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 28
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 939862, 872617
Component: Theme → Toolbars and Customization
(Assignee)

Updated

5 years ago
See Also: → bug 940443
Blocks: 942157
Whiteboard: [Australis:P3]
(Assignee)

Comment 1

5 years ago
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)
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 4

5 years ago
Created attachment 8340149 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Updated

5 years ago
Flags: needinfo?(jaws)
(Assignee)

Comment 6

5 years ago
Created attachment 8340374 [details] [diff] [review]
fix Australis' CustomizableUI to have better interaction between registerToolbarNode and registerArea,

Now with less brokenness, and a test.
Attachment #8340374 - Flags: review?(bmcbride)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 8

5 years ago
With nits,

remote:   https://hg.mozilla.org/integration/fx-team/rev/78e12d1f3aa2
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 941561
https://hg.mozilla.org/mozilla-central/rev/78e12d1f3aa2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 28
(Reporter)

Comment 11

5 years ago
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.