Closed Bug 1427001 Opened 6 years ago Closed 6 years ago

Unify slots between nsIContent instances.

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

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.
Attachment #8938798 - Flags: review?(bugs) → review-
Comment on attachment 8938799 [details]
Bug 1427001: Move SetXBLBinding and SetShadowRoot to Element.

https://reviewboard.mozilla.org/r/209328/#review215124
Attachment #8938799 - Flags: review?(bugs) → review+
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.
Attachment #8938798 - Flags: review?(bugs)
(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.
Attachment #8938798 - Flags: review?(bugs) → review-
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
Attachment #8938798 - Flags: review?(bugs) → review+
The Gecko bits didn't land, so the servo ones got backed out last night: https://hg.mozilla.org/integration/autoland/rev/57cf42b104ef
Flags: needinfo?(emilio)
Yup, relanding them in https://github.com/servo/servo/pull/19664, thanks!
Flags: needinfo?(emilio)
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
Blocks: 1427511
https://hg.mozilla.org/mozilla-central/rev/74fae496a739
https://hg.mozilla.org/mozilla-central/rev/894c4edfb0fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: