Tim Nguyen :ntim Reporter
	Description • 7 years ago

Both of these don't seem used and could possibly be removed.

	:Paolo Amadini
	Comment 1 • 7 years ago

"toolbarbutton[type=menu].badged-button" is actually used once in the search bar popup. I'm however looking into simplifying the handling of the menu buttons for both badged and non-badged after bug 1434860 lands.

	:Paolo Amadini
	Comment 4 • 7 years ago

After part 2, the bindings only exist to set display="xul:menu", which I think is used to create the MenuBoxObject that handles displaying the menu. Do you have any thoughts on how to migrate away from this?

	:Paolo Amadini
	Comment 5 • 7 years ago

Also, we should do some Talos runs for part 2, but the extra elements hopefully won't show up in profiles, as we already have a number of unneeded elements that we create for each button.

	Brian Grinstead [:bgrins]
	Comment 6 • 7 years ago

(In reply to :Paolo Amadini from comment #4)
> After part 2, the bindings only exist to set display="xul:menu", which I
> think is used to create the MenuBoxObject that handles displaying the menu.
> Do you have any thoughts on how to migrate away from this?

Forwarding the question to Neil, but I suspect we could do a check on tag name + attribute somewhere instead.

	:Paolo Amadini
	Comment 7 • 7 years ago

I stated some Talos builds for comparing the performance after applying part 2:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4bd5f02590ee5a33cebfa23b2a80506cfb2061
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2a577ca8305a18a5beec49893b084666ee6d83

	:Paolo Amadini
	Comment 8 • 7 years ago

Most Talos results look alright, but there are two that stand out:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ef4bd5f02590ee5a33cebfa23b2a80506cfb2061&newProject=try&newRevision=3a2a577ca8305a18a5beec49893b084666ee6d83&framework=1&showOnlyConfident=1

Brian, you've probably looked at more Talos results than me recently. Are these regressions or expected noise? I've run six rebuilds, do we need more or would they be noisy anyways?

	Brian Grinstead [:bgrins]
	Comment 9 • 7 years ago

(In reply to :Paolo Amadini from comment #8)
> Most Talos results look alright, but there are two that stand out:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=ef4bd5f02590ee5a33cebfa23b2a8050
> 6cfb2061&newProject=try&newRevision=3a2a577ca8305a18a5beec49893b084666ee6d83&
> framework=1&showOnlyConfident=1
> 
> Brian, you've probably looked at more Talos results than me recently. Are
> these regressions or expected noise? I've run six rebuilds, do we need more
> or would they be noisy anyways?

Those aren't the typical ones I see a lot of noise in (although it's interesting this is only on Win64). 6 rebuilds is usually significant, but since that's only a medium confidence I'm not sure. I notice looking at the last 14 days on those two tests that they may have dropped overall recently:

- https://treeherder.mozilla.org/perf.html#/graphs?series=try,1536965,1,1&highlightedRevisions=ef4bd5f02590&highlightedRevisions=3a2a577ca830
- https://treeherder.mozilla.org/perf.html#/graphs?series=try,1536954,1,1&highlightedRevisions=ef4bd5f02590&highlightedRevisions=3a2a577ca830

I think rather than triggering rebuilds I'd just pull latest m-c then do two new try pushes and see if the same tests regress.

	:Paolo Amadini
	Comment 10 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to :Paolo Amadini from comment #4)
> > After part 2, the bindings only exist to set display="xul:menu", which I
> > think is used to create the MenuBoxObject that handles displaying the menu.
> > Do you have any thoughts on how to migrate away from this?
> 
> Forwarding the question to Neil, but I suspect we could do a check on tag
> name + attribute somewhere instead.

It's still unclear to me how "extends" and "display" are supposed to work, and how they relate to each other. They both seem related to inheritance:

https://dxr.mozilla.org/mozilla-central/rev/a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.cpp#1582-1659

I couldn't find any documentation either. Brian, can you clarify what the intent was? Do you think there could be another way to achieve the same behavior of opening the menu with different techniques?

Emilio, is the "display" attribute also related to the work in bug 1450652?

	Emilio Cobos Álvarez (:emilio)
	Comment 11 • 7 years ago

(In reply to :Paolo Amadini from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > (In reply to :Paolo Amadini from comment #4)
> > > After part 2, the bindings only exist to set display="xul:menu", which I
> > > think is used to create the MenuBoxObject that handles displaying the menu.
> > > Do you have any thoughts on how to migrate away from this?
> > 
> > Forwarding the question to Neil, but I suspect we could do a check on tag
> > name + attribute somewhere instead.
> 
> It's still unclear to me how "extends" and "display" are supposed to work,
> and how they relate to each other. They both seem related to inheritance:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.
> cpp#1582-1659
> 
> I couldn't find any documentation either. Brian, can you clarify what the
> intent was? Do you think there could be another way to achieve the same
> behavior of opening the menu with different techniques?
> 
> Emilio, is the "display" attribute also related to the work in bug 1450652?

It is. So, extends="xul:foo" / display="xul:foo" is used as a way to override the way a XUL element node name for layout. If the remaining consumers of this binding always have the same nodename, you can add a line here:

  https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/base/nsCSSFrameConstructor.cpp#4337

If there are multiple, the way to do it is adding a new `display` property value (`-moz-menu`?), and add the line here instead:

  https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/base/nsCSSFrameConstructor.cpp#4495

But I'd try to stick with the first solution if possible.

	Brian Grinstead [:bgrins]
	Comment 12 • 7 years ago

I believe there's at least one another place we check the node name (in nsIDocument::GetBoxObjectFor): https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6475-6476

	Brian Grinstead [:bgrins]
	Comment 13 • 7 years ago

(In reply to :Paolo Amadini from comment #10)
> It's still unclear to me how "extends" and "display" are supposed to work,
> and how they relate to each other. They both seem related to inheritance:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a1fb8ffae378963b128deaaf3a76eff9dbb6be21/dom/xbl/nsXBLPrototypeBinding.
> cpp#1582-1659
> 
> I couldn't find any documentation either. Brian, can you clarify what the
> intent was? Do you think there could be another way to achieve the same
> behavior of opening the menu with different techniques?
> 
> Emilio, is the "display" attribute also related to the work in bug 1450652?

We discussed this more at https://mozilla.logbot.info/content/20180404#c14560291. I do believe that burning down the list of display="xul:*" will be required to complete bug 1450652. Here are the remaining ones: https://searchfox.org/mozilla-central/search?q=display%3D%22xul%3A&path=xml.

	Brian Grinstead [:bgrins]
	Comment 14 • 7 years ago

As per https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton-badged-menu and https://bgrins.github.io/xbl-analysis/tree/#menu unfortunately these don't map cleanly to a tag name:

Selectors for the `toolbarbutton-badged-menu` binding: toolbarbutton.badged-button[type="menu"], toolbarbutton.badged-button[type="panel"]

Selectors for the `menu` binding: menu (content/xul.css), toolbarbutton[type="menu"], toolbarbutton[type="panel"] (content/xul.css), button[type="menu"], button[type="panel"]

	Brian Grinstead [:bgrins]
	Comment 15 • 7 years ago

Emilio's second idea (adding a new `display` property value) seems like it'd work for the layout side for these complex selectors, but I'm not sure if that value would be easily accessible from the callers of GetBoxObjectFor. We should go through the entire list of display="xul:" and see how many times we have these more complex selectors vs only tag names.

	Brian Grinstead [:bgrins]
	Comment 16 • 7 years ago

Here are the bindings with display="xul:*" - generated from https://bgrins.github.io/xbl-analysis/tree/#display with this code in the webconsole: `[...document.querySelectorAll("details[display]")].map(d=> { return "* `" + d.querySelector("summary span").textContent + "` " + d.querySelector("[highlight=display]").textContent + " " + d.querySelectorAll(".metadata em")[1].textContent }).join("\n")`

* `toolbarbutton` display (xul:button) Selectors: toolbarbutton (content/xul.css)
* `menu` display (xul:menu) Selectors: menu (content/xul.css), toolbarbutton[type=\"menu\"], toolbarbutton[type=\"panel\"] (content/xul.css), button[type=\"menu\"], button[type=\"panel\"] (content/xul.css)
* `toolbarbutton-badged-menu` display (xul:menu) Selectors: toolbarbutton.badged-button[type=\"menu\"], toolbarbutton.badged-button[type=\"panel\"] (content/xul.css)
* `button` display (xul:button) Selectors: button (content/xul.css)
* `button-repeat` display (xul:autorepeatbutton) Selectors: button[type=\"repeat\"] (content/xul.css)
* `menu-button` display (xul:menu) Selectors: button[type=\"menu-button\"] (content/xul.css)
* `tab` display (xul:button) Selectors: tab (content/xul.css)
* `tabbrowser-tab` display (xul:hbox) Selectors: .tabbrowser-tab (content/browser.css)
* `menulist` display (xul:menu) Selectors: menulist (content/xul.css)
* `menulist-popuponly` display (xul:menu) Selectors: menulist[popuponly=\"true\"] (content/xul.css)
* `columnpicker` display (xul:button) Selectors: treecolpicker (content/xul.css)
* `colorpicker-button` display (xul:menu) Selectors: colorpicker[type=\"button\"] (content/xul.css)
* `listheader` display (xul:button) Selectors: listheader (content/xul.css)"

Emilio, do extended bindings inherit the node name set from `display` / `extends`? That is, if `toolbarbutton` binding has display="xul:button" then will `toolbarbutton-badged` also get it? https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton

	Brian Grinstead [:bgrins]
	Comment 17 • 7 years ago

The only display="xul:*" values that need to worry about a box object are (taken from https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6474-6490):

menu, popup, menupopup, panel, tooltip, tree, listbox, scrollbox

So the only bindings we need to worry about regarding box objects appear to only be ones with display="xul:menu":

* `menu` display (xul:menu)
* `toolbarbutton-badged-menu` display (xul:menu) 
* `menu-button` display (xul:menu)
* `menulist` display (xul:menu)
* `menulist-popuponly` display (xul:menu)
* `colorpicker-button` display (xul:menu)

	:Paolo Amadini
	Comment 18 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Selectors for the `toolbarbutton-badged-menu` binding:
> toolbarbutton.badged-button[type="menu"],
> toolbarbutton.badged-button[type="panel"]
> 
> Selectors for the `menu` binding: menu (content/xul.css),
> toolbarbutton[type="menu"], toolbarbutton[type="panel"] (content/xul.css),
> button[type="menu"], button[type="panel"]

In the meantime I'll move part 1 to another bug to remove the type="panel" alias sooner.

	Brian Grinstead [:bgrins]
	Comment 19 • 7 years ago

Do we ever actually use 'toolbarbutton-badged-menu'? Just scanning search results here I don't see any places where the selector `toolbarbutton.badged-button[type="menu"]` would apply: https://searchfox.org/mozilla-central/search?q=badged-button&path=. If we could remove that binding entirely it would simplify the `display` handling.

	:Paolo Amadini
	Comment 20 • 7 years ago

It's still used in the search bar when a site asks to register more than one search engine. Anyways, I don't think it would simplify things much, because we still have to check for type="menu" regardless of the "badged-button" class.

	:Paolo Amadini
	Comment 21 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1b4b2402139e8b7f2c1cb2484720eede2e360c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9ae64ebcfde3a2b4478d030739f4784132f06f

	Brian Grinstead [:bgrins]
	Comment 22 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #12)
> I believe there's at least one another place we check the node name (in
> nsIDocument::GetBoxObjectFor):
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/dom/base/nsDocument.cpp#6475-6476

I think Neil is looking into ways to get rid of this in Bug 1446961, at least for PopupBoxObject

	Emilio Cobos Álvarez (:emilio)
	Comment 23 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Emilio, do extended bindings inherit the node name set from `display` /
> `extends`? That is, if `toolbarbutton` binding has display="xul:button" then
> will `toolbarbutton-badged` also get it?
> https://bgrins.github.io/xbl-analysis/tree/#toolbarbutton

Huh, thought I had replied to this yesterday, sorry. Yes it does, it looks it up through the whole base binding chain.

	:Paolo Amadini
	Comment 24 • 7 years ago

The latest Talos run with 10 rebuilds shows that there is no regression, and Windows 64 is just very noisy.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5b1b4b2402139e8b7f2c1cb2484720eede2e360c&newProject=try&newRevision=bf9ae64ebcfde3a2b4478d030739f4784132f06f&framework=1&showOnlyNoise=1

I'll move part 2 to another bug and leave this bug open to deal with the "display" attribute, since there's more information here on the topic.

	:Paolo Amadini
	Comment 25 • 7 years ago

Changed the subject to reflect the work that remains to be done, and unassigned myself for now since this may be completed as part of the work Emilio is doing, or may depend on it.

	ExE Boss
	Comment 26 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Emilio’s second idea (adding a new `display` property value) seems like it'd
> work for the layout side for these complex selectors, but I'm not sure if
> that value would be easily accessible from the callers of GetBoxObjectFor.

If we do that, then we’ll have to keep in mind bug 1288572.