1394975 - Remove the tabbrowser-tabbox xbl binding

Status: RESOLVED FIXED

(Firefox :: General, task, P5)

Description

[Brian Grinstead [:bgrins]]

The tabbrowser-tabbox binding extends tabbox and overrides a single property ("tabs"). There is only one instance of this binding, which is used in #tabbrowser.

Here's what it extends:

#tab-base (tabbox.xml)
   #tabbox (tabbox.xml)
      #tabbrowser-tabbox (tabbrowser.xml)

The binding itself and the CSS attaching it to .tabbrowser-tabbox could be removed relatively easily by allowing the #tabbox binding to receive a [tabcontainer] attribute and adding a conditional to its "tabs" property.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

This is used to change the tabs property on the tabbox, which can be done
on the tabbox binding directly by passing an attribute

Review commit: https://reviewboard.mozilla.org/r/174040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174040/

Comment 2

[Brian Grinstead [:bgrins]]

Not sure if this is something we'd want to land while we still have an option to enable legacy addons, as there are references to the binding in the addons repo: https://dxr.mozilla.org/addons/search?q=tabbrowser-tabbox&redirect=false

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Not sure if this is something we'd want to land while we still have an
> option to enable legacy addons, as there are references to the binding in
> the addons repo:
> https://dxr.mozilla.org/addons/search?q=tabbrowser-tabbox&redirect=false

My understanding is that the option to enable legacy add-ons will remain around for some time, but it's completely unsupported, and we don't need to maintain any compatibility just for their sake.

So, I believe you can ignore them here.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8902475 [details]
Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

For context, this isn't required to land for the prototype in Bug 1392352, it's just something I noticed when debugging that issue and changed as a refactor in that series. IMO this is an improvement since it removes one extra binding and a layer of inheritence by sending an already existing attribute down to the tabbox instead. I don't expect any noticeable perf change from this - the old code did a `document.getBindingParent(this)` and the new code does a `this.hasAttribute()` instead for the "tabs" getter. If you like this change I'm happy to land it, but we can also wontfix if you like the current implementation more.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8902475 [details]
Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

https://reviewboard.mozilla.org/r/174040/#review179744

Comment 6

[Brian Grinstead [:bgrins]]

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

Comment 7

[Brian Grinstead [:bgrins]]

A talos push [0] shows some small improvements in "tabpaint opt e10s stylo" and "tp5o_scroll opt e10s linux64-stylo" but I wouldn't be surprised if that was just noise. There's a concerning 90% regression in "bloom_basic opt e10s windows7-32". That test "ensures that we correctly fast-reject ancestor selectors that clearly don't match any ancestor elements" [1].  It does look like there's a fair bit of noise in that test in the last 14 days on m-c [2] and the 3.38 value in the push seems within the normal range there, so maybe it was just bad luck.

[0]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=210465f03b58ab8119c1c801188cb751fb55c708&newProject=try&newRevision=a1cb684659e666a355a03e704331808a8a88e2cd&framework=1&showOnlyImportant=0&showOnlyConfident=1
[1]: https://wiki.mozilla.org/Buildbot/Talos/Tests#bloom_basic
[2]: https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1498303,1,1

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8902475 [details]
Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174040/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58f4ecda0d4a
Remove the tabbrowser-tabbox xbl binding;r=mossop

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/58f4ecda0d4a