Unify slots between nsIContent instances.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
a year ago
a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Attachment #8938798 - Flags: review?(bugs)
(Assignee)

Comment 9

a year ago
(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

a year 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

a year 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
(Assignee)

Comment 17

a year ago
(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

a year 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)
(Assignee)

Comment 24

a year ago
Yup, relanding them in https://github.com/servo/servo/pull/19664, thanks!
Flags: needinfo?(emilio)

Comment 25

a year 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

a year ago
Blocks: 1427511

Comment 26

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