This allows us to devirtualize some trivial getters / setters, and avoid duplicating stuff. Also makes it easier to add Servo support for some querySelector / querySelectorAll optimizations, given Servo already knows about nsIContent, but it doesn't know about nsGenericDOMDataNode and such.

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215106 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/base/FragmentOrElement.cpp:815 (Diff revision 1) > +nsIContent::nsExtendedContentSlots::nsExtendedContentSlots() > + : mBindingParent(nullptr) > +{ > +} > + > +nsIContent::nsExtendedContentSlots::~nsExtendedContentSlots() Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215122 ::: dom/base/Element.h:557 (Diff revision 1) > */ > inline CustomElementData* GetCustomElementData() const > { > - nsExtendedDOMSlots* slots = GetExistingExtendedDOMSlots(); > + const nsExtendedDOMSlots* slots = GetExistingExtendedDOMSlots(); > if (slots) { > - return slots->mCustomElementData; > + return slots->mCustomElementData.get(); Why is adding .get() needed? Is that because of adding const? If get() isn't needed, don't add it. ::: dom/base/FragmentOrElement.cpp:851 (Diff revision 1) > > if (!mExtendedSlots) { > return; > } > > + mExtendedSlots->nsExtendedContentSlots::Traverse(cb); All this Traverse/Unlink handling is now weird. Some stuff handled in nsExtendedContentSlots::Traverse/Unlink, some here. nsExtendedDOMSlots should override Traverse/Unlink.

Comment on attachment 8938799 [details] Bug 1427001: Move SetXBLBinding and SetShadowRoot to Element. https://reviewboard.mozilla.org/r/209328/#review215124

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215122 > Why is adding .get() needed? Is that because of adding const? > If get() isn't needed, don't add it. Yeah, get() is only needed for this pattern: `slots ? slots->mFoo.get() : nullptr;`, I shuffled a few to be consistent. > All this Traverse/Unlink handling is now weird. Some stuff handled in nsExtendedContentSlots::Traverse/Unlink, some here. > nsExtendedDOMSlots should override Traverse/Unlink. Yeah, I wasn't sure about how important was to avoid the virtual call there or what not, but under the assumption that the extended slots are rare, it shouldn't matter. I just did that.

(Still not quite right, I'll do the same setup as nsSlots / nsDOMSlots do now.

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215154 ::: dom/base/FragmentOrElement.cpp:1498 (Diff revision 4) > +nsIContent::Unlink(nsIContent* tmp) > +{ > nsINode::Unlink(tmp); > > + if (nsContentSlots* slots = tmp->GetExistingContentSlots()) { > + slots->Unlink(); Why is this right. We end up calling just the Unlink of ContentSlots, not DOMContentSlots. Same with Traverse. Those methods aren't virtual in this patch. And the setup is odd. First we unlink (nsSlots) in nsINode::Unlink and then access slots again here and unlink.

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215156 ::: dom/base/FragmentOrElement.cpp:1498 (Diff revision 4) > +nsIContent::Unlink(nsIContent* tmp) > +{ > nsINode::Unlink(tmp); > > + if (nsContentSlots* slots = tmp->GetExistingContentSlots()) { > + slots->Unlink(); Oh, I see you're then explicitly calling traverse/unlink on the DOMContentSlots and all that...

Wouldn't the setup be easier if nsSlots had a pointer to extended slots and Traverse/Unlink there were virtual and also extended slots had virtual Traverse/Unlink

(In reply to Olli Pettay [:smaug] from comment #16) > Wouldn't the setup be easier if nsSlots had a pointer to extended slots and > Traverse/Unlink there were virtual and also extended slots had virtual > Traverse/Unlink That works for me, I thought that the current setup was that way to avoid virtual calls and such (right now nsSlots::Traverse / Unlink aren't virtual, and instead subclasses need to do that, so I mimicked that setup). Happy to change it though, I do think it's cleaner.

Comment on attachment 8938798 [details] Bug 1427001: Stop duplicating slots. https://reviewboard.mozilla.org/r/209326/#review215240

Servo bits @ https://github.com/servo/servo/pull/19660

The Gecko bits didn't land, so the servo ones got backed out last night: https://hg.mozilla.org/integration/autoland/rev/57cf42b104ef

Yup, relanding them in https://github.com/servo/servo/pull/19664, thanks!

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/74fae496a739 Stop duplicating slots. r=smaug https://hg.mozilla.org/integration/autoland/rev/894c4edfb0fb Move SetXBLBinding and SetShadowRoot to Element. r=smaug

https://hg.mozilla.org/mozilla-central/rev/74fae496a739 https://hg.mozilla.org/mozilla-central/rev/894c4edfb0fb