Unify slots between nsIContent instances.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

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 hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
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 4

2 years ago
mozreview-review
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 5

2 years ago
mozreview-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+
Assignee

Comment 6

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8938798 - Flags: review?(bugs)
(Still not quite right, I'll do the same setup as nsSlots / nsDOMSlots do now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
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 15

2 years ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
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)

Comment 25

2 years ago
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
Assignee

Updated

2 years ago
Blocks: 1427511

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74fae496a739
https://hg.mozilla.org/mozilla-central/rev/894c4edfb0fb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.