We want to make toolbar-menubar, TabsToolbar and PersonalToolbar to be customizable with the new Australis customization mode. An immediate difficult that I see with this is that toolbar-menubar and TabsToolbar both have toolbar bindings for things like autohiding and dragging...and the way we're currently doing things (creating our own toolbar binding) means that we either need to write our own (hopefully simplified) copies of those extra bindings, OR we change tack and have our toolbar binding subclass the toolkit toolbar bindings. Beyond these two, not sure there are other options. Blair - is there a good reason to stick with our own bindings instead of subclassing toolkit's toolbar bindings?

Another option, and one that I'm starting to prefer, is to drop our special toolbar XBL binding all-together, and just query the document for toolbars on browser window startup. Then register them, and set their customizationTarget properties. Any objection?

(In reply to Mike Conley (:mconley) from comment #1) > Another option, and one that I'm starting to prefer, is to drop our special > toolbar XBL binding all-together, and just query the document for toolbars > on browser window startup. Then register them, and set their > customizationTarget properties. > > Any objection? How would that work with 3rd-party toolbars? I would presume that we don't want those to become customization targets.

Or at the least, without an explicit opt-in by the 3rd party toolbar developers.

I should have been more precise; we should probably query for toolbars with a customizable attribute set to "true", a la https://mxr.mozilla.org/mozilla-central/search?string=customizable=%22true%22

Here's my first shot at this - Jared / Blair, see any holes with this approach?

Comment on attachment 740889 [details] [diff] [review] Patch v1 Review of attachment 740889 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +4053,5 @@ > + let toolbar = document.getElementById(toolbarId); > + if (toolbar) { > + let target = > + document.getElementById(toolbar.getAttribute("customizationtarget")); > + toolbar.customizationTarget = target ? target : toolbar; toolbar.customizationTarget = target || toolbar; ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +815,5 @@ > > LOG("Iterating the actual nodes of the window palette"); > for (let node of aWindowPalette.children) { > LOG("In palette children: " + node.id); > + if (node.id && !this.getPlacementOfWidget(node.id)) { If we're having issues where node's lack IDs, we shouldn't silently fail them. > Cu.reportError("Node missing id attribute in palette: " + (new XMLSerializer().serializeToString(node)));

(In reply to Jared Wein [:jaws] from comment #6) > If we're having issues where node's lack IDs, we shouldn't silently fail > them. > > Cu.reportError("Node missing id attribute in palette: " + (new XMLSerializer().serializeToString(node))); I didn't know this until now, but Fx11+ supports HTMLElement.outerHTML, so this can be simplified to: > Cu.reportError("Node missing id attribute in palette: " + node.outerHTML);

De-bitrotting.

Note that I still need to update the patch based on jaws' feedback.

Comment on attachment 741984 [details] [diff] [review] Patch v1.1 (feedback+'d by jaws) Review of attachment 741984 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +1468,5 @@ > get AREA_NAVBAR() "nav-bar", > + get AREA_MENUBAR() "toolbar-menubar", > + get AREA_TABSTRIP() "TabsToolbar", > + > + get builtInToolbars() [ This should be a Set. ::: browser/components/customizableui/src/CustomizeMode.jsm @@ +410,5 @@ > + let currentSetItems = currentSet.split(","); > + let targetItems = [node.id for (node of toolbar.customizationTarget.children)]; > + let targetIndex = currentSetItems.indexOf(toolbar.customizationTarget.id); > + currentSetItems.splice.apply(currentSetItems, > + [targetIndex, 1].concat(targetItems)); Why is this needed/wanted?

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #10) > Comment on attachment 741984 [details] [diff] [review] > Patch v1.1 (feedback+'d by jaws) > > Review of attachment 741984 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/customizableui/src/CustomizableUI.jsm > @@ +1468,5 @@ > > get AREA_NAVBAR() "nav-bar", > > + get AREA_MENUBAR() "toolbar-menubar", > > + get AREA_TABSTRIP() "TabsToolbar", > > + > > + get builtInToolbars() [ > > This should be a Set. > > ::: browser/components/customizableui/src/CustomizeMode.jsm > @@ +410,5 @@ > > + let currentSetItems = currentSet.split(","); > > + let targetItems = [node.id for (node of toolbar.customizationTarget.children)]; > > + let targetIndex = currentSetItems.indexOf(toolbar.customizationTarget.id); > > + currentSetItems.splice.apply(currentSetItems, > > + [targetIndex, 1].concat(targetItems)); > > Why is this needed/wanted? Because the customization target now stretches across the entire nav-bar, the toolkit toolbar binding assumes that nav-bar-customization-target is a toolbar item. I'm working around that by injecting the contents of the customization target into the currentset attribute of the toolbar in place of nav-bar-customization-target when customization is done. I think this addresses two issues: 1) Allows for at least some backwards compatibility, in the event that a user makes a customization change and then switches to an older build 2) Lets magical rules like this[1] one still work, since it assumes that the currentset attribute of the toolbar will reflect the list of toolbar items inside of it. I'm open to other suggestions though. I actually preferred the old method of having our own toolbar binding take care of all of this, but that means creating our own autohide binding for the menubar, and our own draggable binding for the TabsToolbar. Any suggestions? [1]: https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#21

Er, hmm. (In reply to Mike Conley (:mconley) from comment #11) > I actually preferred the old method of > having our own toolbar binding take care of all of this, but that means > creating our own autohide binding for the menubar, and our own draggable > binding for the TabsToolbar. Looking at those bindings, they're pretty small code-wise. I'm thinking it'd be best to just copy those bindings - given the alternative seems to be requiring ugly hacks (that are purposefully doing things wrong) to get around code we eventually want to replace. And that's nothing against your patch - I don't think there is a nice way to do what we want with the old bindings. (In reply to Mike Conley (:mconley) from comment #0) > Blair - is there a good reason to stick with our own bindings instead of > subclassing toolkit's toolbar bindings? One was just keeping it lightweight - the old toolkit bindings have a lot of extra cruft we no longer want, and APIs that will no longer work. The other was concerns like what this patch brings up - needing ugly hacks to get around old behaviour and old assumptions.

Ok, cool - I feel better going back to the binding approach. And you're right, the autohide and draggable bindings really aren't that bad in terms of straight-up forking. I also forgot to add the bookmarks toolbar as a target in my first patch - fixed that here. A before, you must apply the patches in bug 864355 and bug 863753 (in that order) before applying this one.

What's the long-term plan here? Forking within the same code base obviously increases the maintenance cost and implies an ongoing cost for future toolkit bindings. The different XBL base binding is also a burden for custom bindings for add-on toolbars.

(In reply to Dão Gottwald [:dao] from comment #14) > What's the long-term plan here? Forking within the same code base obviously > increases the maintenance cost and implies an ongoing cost for future > toolkit bindings. The different XBL base binding is also a burden for custom > bindings for add-on toolbars. Yea, I know. Long-term plan is to eventually replace (or at least deprecate) the toolkit customization code with the new customization code. So this code duplication is temporary, and lets us design the new code the way it should be instead of having to hack around the old code and re-do it to make it work properly later on. IMO, overall this ends up being a net win for maintenance costs.

Comment on attachment 743182 [details] [diff] [review] Patch v2 Review of attachment 743182 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/content/toolbar.xml @@ +111,5 @@ > > </implementation> > </binding> > > + <binding id="toolbar-menubar-autohide" Add a comment to these two bindings, stating they're basically copies of what's in toolkit? ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +686,5 @@ > let parent = node.parentNode; > while (parent && parent.localName != "toolbar") > parent = parent.parentNode; > > + if (parent && parent.toolbox == aToolbox) { This change doesn't seem to be needed any more.

(Also, this seems to work perfectly fine without bug 863753.)

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #15) > (In reply to Dão Gottwald [:dao] from comment #14) > > What's the long-term plan here? Forking within the same code base obviously > > increases the maintenance cost and implies an ongoing cost for future > > toolkit bindings. The different XBL base binding is also a burden for custom > > bindings for add-on toolbars. > > Yea, I know. Long-term plan is to eventually replace (or at least deprecate) > the toolkit customization code with the new customization code. So this code > duplication is temporary, and lets us design the new code the way it should > be instead of having to hack around the old code and re-do it to make it > work properly later on. IMO, overall this ends up being a net win for > maintenance costs. On the other hand, changing the base binding to a custom one and then back to toolkit are two add-ons-breaking API changes. Isolated hacks with limited lifetime might be nicer for external consumers.

(In reply to Dão Gottwald [:dao] from comment #18) > (In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from > comment #15) > > (In reply to Dão Gottwald [:dao] from comment #14) > > > What's the long-term plan here? Forking within the same code base obviously > > > increases the maintenance cost and implies an ongoing cost for future > > > toolkit bindings. The different XBL base binding is also a burden for custom > > > bindings for add-on toolbars. > > > > Yea, I know. Long-term plan is to eventually replace (or at least deprecate) > > the toolkit customization code with the new customization code. So this code > > duplication is temporary, and lets us design the new code the way it should > > be instead of having to hack around the old code and re-do it to make it > > work properly later on. IMO, overall this ends up being a net win for > > maintenance costs. > > On the other hand, changing the base binding to a custom one and then back > to toolkit are two add-ons-breaking API changes. Isolated hacks with limited > lifetime might be nicer for external consumers. In order not to block, I'm going to address Blair's comments and land this on Jamun. > Isolated hacks with limited lifetime might be nicer for external consumers. Possibly, but the necessary hacks are likely to be ugly and error-prone, as most hacks are. In the interest of correctness and simplicity, my personal position is to introduce the new binding, and move it back towards toolkit. I will admit, however, that I'm not familiar with how add-ons might be using the old bindings, and how much work would be required for them to transition (twice?) - so take my opinion here with a grain of salt.

Addressed review comments, and also removed customizable="true" from the add-on toolbar to prevent an "unrecognized customization area" exception from being thrown.

Landed on jamun as https://hg.mozilla.org/projects/jamun/rev/9768a0c8f016

Comment on attachment 743620 [details] [diff] [review] Patch v2.1 (r+'d by Unfocused) Clearly I was not able to get to this in a timely fashion. If there is cleanup/unforking work to be done here, we should follow up in new bugs.