I'm currently migrating my add-on, OmniSidebar, and trying to use the CustomizableUI methods as much as it can. The add-on provides its own toolbars in the sidebar's headers, and I've run into a problem. If you for some reason disable the add-on (common case: updating), there is no easy way to remove the toolbars without doing all the heavy work to keep things consistent yourself. The same way as there's a registerToolbarNode() method, that restores the buttons in the saved state to the toolbar when called, there should be a similar unregisterToolbarNode() method that does the opposite: - moves all child elements back to the palette - removes any buildAreas information relative to the toolbar and - does NOT remove placements information so when re-enabling the add-on we can call registerToolbarNode() on it again and restore everything to its proper saved state. Calling unregisterArea() isn't an option because that erases the placements information (if I'm reading the code correctly, please let me know if I'm wrong). As it is right now, if we want dynamically loaded toolbars like this, we need to do all these steps ourselves which would likely clash with the rest of the customization code if not done properly. IMO, this would be something best done in CustomizableUI itself. Or can this be done already and I just missed it?

I wasn't sure which meta bug I should mark as blocking, as this pertains to the customization routine itself, but would probably only be needed by add-ons using it. I'm blocking both australis-cust and australis-addons meta bugs. Please feel free to rectify if this is wrong.

But wouldn't you actually want to unregister *all* the build area nodes, not a specific window's? Sounds like it'd make more sense to have something that would do what unregisterArea does, but without deleting the placements. Needinfo'ing to be sure. :-) It'd make sense to have an API to do this and not nuke the placements info because it's possible to do the same with widgets (destroyWidget doesn't remove the placements info, IIRC, you have to manually call removeWidgetFromArea afterwards to remove that info - we could also employ that strategy here...). Separately, as a note to self/implementers, I actually looked at unregisterArea just now and I'm fairly sure its removal code is subtly broken, because it does this: let placements = gPlacements.get(aAreaId); placements.forEach(this.removeWidgetFromArea, this); But placements is by-reference, not cloned. And this.removeWidgetFromArea is going to remove items from it. I'm fairly sure forEach is one of the many APIs which don't DWIM when you remove items whilst iterating. I mean, we then remove the entire placements array wholesale, but I suspect this will have items inside the toolbars getting lost as a consequence. Not that anyone's noticed because we never use this API ourselves, and our tests for it probably don't check this with either multiple items or whilst checking the actual children of the build areas. :-\ (the worst is: I wrote that code over in bug 877178)

(In reply to :Gijs Kruitbosch from comment #2) > But wouldn't you actually want to unregister *all* the build area nodes, not > a specific window's? Sounds like it'd make more sense to have something that > would do what unregisterArea does, but without deleting the placements. That's what I was going to say originally actually, but I wasn't sure about the rest of unregisterArea's functionality (I'm still migrating code, learning what everything does in the process), so I wasn't sure about it. But yeah, the objective would be to "clean" from all windows of course. However, in the case of my add-on that's a minor issue, as it has to remove the toolbars from all windows, so it will/would call that method anyway; just like it appends the toolbar element individually to each window, each of which calls registerToolbarNode in its constructor. Subsequent calls to registerToolbarNode for the "same" toolbar in other windows are irrelevant as they're already initialized, if I'm following the code correctly. That's of course irrelevant to how the method should behave natively, you're right that it should be a global process, not a specific window's. > Separately, as a note to self/implementers, I actually looked at > unregisterArea just now and I'm fairly sure its removal code is subtly > broken, because it does this: > > let placements = gPlacements.get(aAreaId); > placements.forEach(this.removeWidgetFromArea, this); > > But placements is by-reference, not cloned. And this.removeWidgetFromArea is > going to remove items from it. I'm fairly sure forEach is one of the many > APIs which don't DWIM when you remove items whilst iterating. I mean, we > then remove the entire placements array wholesale, but I suspect this will > have items inside the toolbars getting lost as a consequence. Not that > anyone's noticed because we never use this API ourselves, and our tests for > it probably don't check this with either multiple items or whilst checking > the actual children of the build areas. :-\ > > (the worst is: I wrote that code over in bug 877178) Since you wrote this, I would like to ask you something. Why does unregisterArea remove the widgets from the area in the first place? (I don't mean physically, I mean the placements information). I don't see this method used in any native part of firefox, so I assume, from bug 877178, it was mostly for add-on use (bug 877178): (In reply to :Gijs Kruitbosch from bug 877178 comment 0) > What happens with the widgets in an area that are > unregistered? Do we keep placements in the area in the pref? If you > register, unregister, and the widgets end up in the palette, and the user > sticks the widget into some other area, and then you re-register the old > area, what happens with the widget (move/stay?). This should be re-considered I think. The point of unregisterArea seems to be to allow the removal of a toolbar (or any other customizable area of course). But as it is, when you remove it it forgets its placements altogether, essentially resetting it. Seems to me that just adding another method that does the same as unregisterArea except resetting its placements information would actually make unregisterArea useless, as I honestly can't see any scenario where unregisterArea would be used/preferred over the new method.

(In reply to Luís Miguel [:Quicksaver] from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > But wouldn't you actually want to unregister *all* the build area nodes, not > > a specific window's? Sounds like it'd make more sense to have something that > > would do what unregisterArea does, but without deleting the placements. > > That's what I was going to say originally actually, but I wasn't sure about > the rest of unregisterArea's functionality (I'm still migrating code, > learning what everything does in the process), so I wasn't sure about it. > > But yeah, the objective would be to "clean" from all windows of course. > However, in the case of my add-on that's a minor issue, as it has to remove > the toolbars from all windows, so it will/would call that method anyway; > just like it appends the toolbar element individually to each window, each > of which calls registerToolbarNode in its constructor. Subsequent calls to > registerToolbarNode for the "same" toolbar in other windows are irrelevant > as they're already initialized, if I'm following the code correctly. OOI, why call registerToolbarNode? If you add a (visible) <toolbar customizable="true"/> node inside a browser window, the XBL binding will make that call for you. IIRC we added a guard that makes the second call a no-op, but it's still more work than you need to do. :-) > > Separately, as a note to self/implementers, I actually looked at > > unregisterArea just now and I'm fairly sure its removal code is subtly > > broken, because it does this: > > > > let placements = gPlacements.get(aAreaId); > > placements.forEach(this.removeWidgetFromArea, this); > > > > But placements is by-reference, not cloned. And this.removeWidgetFromArea is > > going to remove items from it. I'm fairly sure forEach is one of the many > > APIs which don't DWIM when you remove items whilst iterating. I mean, we > > then remove the entire placements array wholesale, but I suspect this will > > have items inside the toolbars getting lost as a consequence. Not that > > anyone's noticed because we never use this API ourselves, and our tests for > > it probably don't check this with either multiple items or whilst checking > > the actual children of the build areas. :-\ > > > > (the worst is: I wrote that code over in bug 877178) > > Since you wrote this, I would like to ask you something. Why does > unregisterArea remove the widgets from the area in the first place? (I don't > mean physically, I mean the placements information). I don't see this method > used in any native part of firefox, so I assume, from bug 877178, it was > mostly for add-on use (bug 877178): Because removeWidgetFromArea also moves all the 'physical' widget nodes to the palette (the onWidgetRemoved notification that it fires triggers this inside CustomizableUI). What you're saying is you want it to do that, but keep the placements. This might actually be somewhat tricky to implement. In fact, my first thought is to have it do what it currently does, and then gPlacements.set(aAreaId, removedItemsArray). :-) > (In reply to :Gijs Kruitbosch from bug 877178 comment 0) > > What happens with the widgets in an area that are > > unregistered? Do we keep placements in the area in the pref? If you > > register, unregister, and the widgets end up in the palette, and the user > > sticks the widget into some other area, and then you re-register the old > > area, what happens with the widget (move/stay?). > > This should be re-considered I think. The point of unregisterArea seems to > be to allow the removal of a toolbar (or any other customizable area of > course). But as it is, when you remove it it forgets its placements > altogether, essentially resetting it. Seems to me that just adding another > method that does the same as unregisterArea except resetting its placements > information would actually make unregisterArea useless, as I honestly can't > see any scenario where unregisterArea would be used/preferred over the new > method. Add-on uninstall, I would have thought. And I would just add a flag argumente to the existing method to implement this, not add another method, that'd be overkill.

(In reply to :Gijs Kruitbosch from comment #4) > OOI, why call registerToolbarNode? If you add a (visible) <toolbar > customizable="true"/> node inside a browser window, the XBL binding will > make that call for you. IIRC we added a guard that makes the second call a > no-op, but it's still more work than you need to do. :-) Yeah, that's what I meant with "calls registerToolbarNode", I meant by just adding a toolbar to the window it triggers that method. I don't actually call them explicitly. :) > removeWidgetFromArea also moves all the 'physical' widget nodes to > the palette (the onWidgetRemoved notification that it fires triggers this > inside CustomizableUI). What you're saying is you want it to do that, but > keep the placements. Exactly. > This might actually be somewhat tricky to implement. In > fact, my first thought is to have it do what it currently does, and then > gPlacements.set(aAreaId, removedItemsArray). :-) So basically remove widgets and placements, saving the removed items in an array (or save them beforehand). Then add them back into placements? As long as it doesn't lose anything in the process, sounds fine to me. :) > > This should be re-considered I think. The point of unregisterArea seems to > > be to allow the removal of a toolbar (or any other customizable area of > > course). But as it is, when you remove it it forgets its placements > > altogether, essentially resetting it. Seems to me that just adding another > > method that does the same as unregisterArea except resetting its placements > > information would actually make unregisterArea useless, as I honestly can't > > see any scenario where unregisterArea would be used/preferred over the new > > method. > > Add-on uninstall, I would have thought. And I would just add a flag > argumente to the existing method to implement this, not add another method, > that'd be overkill. Flag sounds great! Might I recommend either not making the flag optional or have it save the placements by default?

This is pretty tricky, but the test makes sense to me and so I think we're alright here... even if it's a little weird to keep entire areas around just because they used to exist.

Comment on attachment 8338715 [details] [diff] [review] unregisterArea should keep placements by default so registering/unregistering doesn't lose data in Australis, Review of attachment 8338715 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +291,5 @@ > + // Only destroy placements when necessary: > + if (aDestroyPlacements) { > + gPlacements.delete(aName); > + } else { > + // Otherwise we need to re-set them, as removeFromArea will have emptied This kinda stinks that we have to clean up a mess we just made, but I don't have a better recommendation, just seems like a code smell.

remote: https://hg.mozilla.org/integration/fx-team/rev/43b8117acefc