1505734 - Remove CustomizableUI toolbar binding's constructor

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from bug 1473311 comment #4)
> (In reply to :Gijs (he/him) from bug 1473311 comment #3)
> > > and what to search for? all toolbar elements having @customizable="true"?
> > 
> > I don't understand this question. The constructors all live in the
> > toolbar.xml file I just cited.
> 
> I meant how to find a toolbar element in the document. For example, it could
> be document.querySelectorAll('[customizable]') and then run code from
> toolbar's constructor for each element in the list. Or, possibly there's
> only one element ID in the document that this code should be run for.

OK, here's how I would fix this:

- remove the constructor from the binding
- just before https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/browser/base/content/browser.js#1323 , add:

gNavToolbox.palette = document.getElementById("BrowserToolbarPalette");
gNavToolbox.palette.remove();
let areas = CustomizableUI.areas;
areas.splice(areas.indexOf(CustomizableUI.AREA_FIXED_OVERFLOW_PANEL), 1);
for (let area of areas) {
  let node = document.getElementById(area);
  // FIXME bug NNNNN to investigate whether we can stop passing the list of
  // child elements here. For now, keep passing it:
  let children = Array.from(this.children)
                      .filter(child => child.getAttribute("skipintoolbarset") != "true" && child.id)
                      .map(child => child.id);
  CustomizableUI.registerToolbarNode(node, children);
}


I'd suggest leaving the binding empty for now and do the actual removal of the binding in a separate bug as I wouldn't be surprised if we ended up with unfortunate side effects.

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #0)
>   let children = Array.from(this.children)

Clearly this should be `node.children` instead of `this.children`.

Comment 2

[Brian Grinstead [:bgrins]]

I put together a patch following Comment 0 and am seeing some test failures, I think due to stuff in the test helper functions that directly call create "customizable=true" toolbars and rely on the XBL logic (which has now moved into DOMContentLoaded) to run:

- Other instances of "customizable", "true": https://searchfox.org/mozilla-central/search?q=%22customizable%22&path=
- Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70f2776dac18dbbc5c8ec443d7fcc65af792e0ea&selectedJob=210921179

I guess we could update those callers to also run some of the same logic we are doing here. After looking at it, I think we could make CustomizableUI.registerToolbarNode not take in a list of IDs but rather read the DOM children from the node and do this filtering:

  let children = Array.from(this.children)
                      .filter(child => child.getAttribute("skipintoolbarset") != "true" && child.id)
                      .map(child => child.id);

Then we could do `CustomizableUI.registerToolbarNode(node)` instead of `CustomizableUI.registerToolbarNode(node, children)`, and then anywhere in a test that appends a "customizable=true" toolbar we could afterwards call `CustomizableUI.registerToolbarNode(node);`. It's not clear to me if the `gNavToolbox.palette = document.getElementById("BrowserToolbarPalette"); gNavToolbox.palette.remove();` bit should also be run when tests create one of these toolbars.

Does that sound good? Or do you have another idea for how to handle this?

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1505734 - Remove logic from CustomizableUI toolbar XBL constructor;r=Gijs

Comment 4

[Brian Grinstead [:bgrins]]

Pushed up a patch that does Comment 2, so it's easier to discuss

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> I put together a patch following Comment 0 and am seeing some test failures,
> I think due to stuff in the test helper functions that directly call create
> "customizable=true" toolbars and rely on the XBL logic (which has now moved
> into DOMContentLoaded) to run:
> 
> - Other instances of "customizable", "true":
> https://searchfox.org/mozilla-central/search?q=%22customizable%22&path=

Oh ugh. I forgot we'd still need to deal with these.

TBH, they all deal with bootstrapped scenarios. We don't support bootstrapped add-ons anymore, and we don't need to support toolbars that can't make their own calls into CUI.

So I agree with your suggestion that we just update all the tests to call registerToolbarNode for their respective toolbars themselves, for now.

> - Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=70f2776dac18dbbc5c8ec443d7fcc65af792e0ea&selectedJob=2
> 10921179
> 
> I guess we could update those callers to also run some of the same logic we
> are doing here. After looking at it, I think we could make
> CustomizableUI.registerToolbarNode not take in a list of IDs but rather read
> the DOM children from the node and do this filtering:
> 
>   let children = Array.from(this.children)
>                       .filter(child =>
> child.getAttribute("skipintoolbarset") != "true" && child.id)
>                       .map(child => child.id);
> 
> Then we could do `CustomizableUI.registerToolbarNode(node)` instead of
> `CustomizableUI.registerToolbarNode(node, children)`, and then anywhere in a
> test that appends a "customizable=true" toolbar we could afterwards call
> `CustomizableUI.registerToolbarNode(node);`.

Yep, that sounds fine.

> It's not clear to me if the
> `gNavToolbox.palette = document.getElementById("BrowserToolbarPalette");
> gNavToolbox.palette.remove();` bit should also be run when tests create one
> of these toolbars.

No, it doesn't need extra running - this is a one-time operation to remove the toolbox palette from the DOM. This is how that palette has worked since the Mozilla suite pre-Firefox days, and it's probably time to revisit that, but we don't need to do that here.

Does that work and/or did I miss any of the question? (Sorry for the delay, was at a conference today)

Comment 6

[Brian Grinstead [:bgrins]]

Yes, that works for me. I sent up a patch to phab that does this, with a slight change. I made it so `CustomizableUI.registerToolbarNode` is the thing that takes in just a node and creates the children list, and `CustomizableUIInternal.registerToolbarNode` remains unchanged.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/325bce8bcb07
Remove logic from CustomizableUI toolbar XBL constructor;r=Gijs

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/325bce8bcb07