Bug 197960

Firefox 93 just released with support for `attachInternals` (https://hacks.mozilla.org/2021/10/lots-to-see-in-firefox-93/). Chromium already supports it. So webkit is the only one not yet supporting it.

Created attachment 444531 [details] Patch

It seems like you need to rebaseline the results for imported/w3c/web-platform-tests/html/dom/idlharness.https.html

Comment on attachment 444531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444531&action=review > Source/WebCore/dom/ElementInternals.h:52 > + HTMLElement& m_element; Please don't use a raw reference to a node like this in new code. Use WeakPtr<HTMLElement> instead. r- because of this. > Source/WebCore/dom/ElementInternals.idl:31 > + // Shadow root access This comment is useless. Please remove. > Source/WebCore/dom/Node.h:591 > + Precustomized = 4, Let's not waste an entire bit for having this value. The following condition should be used for an element to be precustomsized: !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() == CustomElementState::Failed r- because of this. > Source/WebCore/dom/Node.h:598 > + uint16_t areInternalsAttached : 1; Please don't waste a bitfield for this. Also, elementInternals is a singular noun object. r- because of this. > Source/WebCore/dom/Node.h:616 > + bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; } > + void setInternalsAttached(); > + This shouldn't be here. ElementInternals should be stored in ElementRareData. There is no need to avoid accessing when we need to check this condition since it never happens in a hot code path. > Source/WebCore/dom/ShadowRoot.h:78 > + bool availableToElementInternals() const { return m_availableToElementInternals; } > + Nit: should be named this instead: isAvailableToElementInternals https://webkit.org/code-style-guidelines/#names-verb > Source/WebCore/html/HTMLElement.cpp:1273 > + auto* elementInterface = registry->findInterface(*this); This is a very inefficient way of getting the element interface. Use Element::reactionQueue() which has a back reference to JSCustomElementInterface. > Source/WebCore/html/HTMLElement.cpp:1287 > + setInternalsAttached(); > + return ElementInternals::create(*this); We should store this in ElementRareData or CustomElementReactionQueue) (the latter is preferable to avoid increasing the size of ElementRareData by sizeof(void*)).

(In reply to Ryosuke Niwa from comment #4) Wow, thanks for such thorough review, Ryosuke! Much appreciated. > > > Source/WebCore/dom/ElementInternals.h:52 > > + HTMLElement& m_element; > > Please don't use a raw reference to a node like this in new code. > Use WeakPtr<HTMLElement> instead. r- because of this. Let's suppose a case of a detached & unreferenced HTML element, with only ElementInternals instance in userland JS code retaining a reference to it. Per spec, this reference should be strong and ElementInternals methods should work as expected. Can we make this work with WeakPtr? > > > Source/WebCore/dom/ElementInternals.idl:31 > > + // Shadow root access > > This comment is useless. Please remove. 👍🏻 > > > Source/WebCore/dom/Node.h:591 > > + Precustomized = 4, > > Let's not waste an entire bit for having this value. > The following condition should be used for an element to be precustomsized: > !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() == > CustomElementState::Failed > r- because of this. I don't like adding a bit to CustomElementState as well, yet proposed solution for "precustomized" state fails a lot of tests where custom element is synchronously created. Also, please note that "failed" => "precustomized" state transition happens only after _disable shadow_ is checked (step 8.2 of https://html.spec.whatwg.org/#upgrades); I'm not sure how we can get around with "failed" state only. > > > Source/WebCore/dom/Node.h:598 > > + uint16_t areInternalsAttached : 1; > > Please don't waste a bitfield for this. Also, elementInternals is a singular > noun object. > r- because of this. > > > Source/WebCore/dom/Node.h:616 > > + bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; } > > + void setInternalsAttached(); > > + > > This shouldn't be here. ElementInternals should be stored in ElementRareData. > There is no need to avoid accessing when we need to check this condition > since it never happens in a hot code path. It feels to me that CustomElementState should also be stored in ElementRareData, yet it's here in Node. I suppose the reason it's there is free bits in RareDataBitFields. This patch is neutral on sizeof(RareDataBitFields). How can we move it to ElementRareData without increasing its size? > > > Source/WebCore/dom/ShadowRoot.h:78 > > + bool availableToElementInternals() const { return m_availableToElementInternals; } > > + > > Nit: should be named this instead: isAvailableToElementInternals > https://webkit.org/code-style-guidelines/#names-verb 👍🏻, my bad. > > > Source/WebCore/html/HTMLElement.cpp:1273 > > + auto* elementInterface = registry->findInterface(*this); > > This is a very inefficient way of getting the element interface. > Use Element::reactionQueue() which has a back reference to > JSCustomElementInterface. Nice 👍🏻, this is a great simplification as well. > > > Source/WebCore/html/HTMLElement.cpp:1287 > > + setInternalsAttached(); > > + return ElementInternals::create(*this); > > We should store this in ElementRareData or CustomElementReactionQueue) > (the latter is preferable to avoid increasing the size of ElementRareData by > sizeof(void*)). I've checked the spec and seems like there is no necessity to store ElementInternals instance like we do for `classList` or `dataset`. It's a one-way API, from userland JS code to DOM, and it's not a [SameObject]. What am I missing?

(In reply to Alexey Shvayka from comment #5) > (In reply to Ryosuke Niwa from comment #4) > > Wow, thanks for such thorough review, Ryosuke! Much appreciated. > > > > > > Source/WebCore/dom/ElementInternals.h:52 > > > + HTMLElement& m_element; > > > > Please don't use a raw reference to a node like this in new code. > > Use WeakPtr<HTMLElement> instead. r- because of this. > > Let's suppose a case of a detached & unreferenced HTML element, with only > ElementInternals instance in userland JS code retaining a reference to it. > Per spec, this reference should be strong and ElementInternals methods > should work as expected. > > Can we make this work with WeakPtr? Yes. The way to keep HTML element alive in that case is to treat it as the opaque root of the ElementInternals. So, when GC visits ElementInternals, we need to be adding the associated element's opaque root to the slot visitors. Talk to Geoff / Chris if you have further questions. > > > Source/WebCore/dom/Node.h:591 > > > + Precustomized = 4, > > > > Let's not waste an entire bit for having this value. > > The following condition should be used for an element to be precustomsized: > > !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() == > > CustomElementState::Failed > > r- because of this. > > I don't like adding a bit to CustomElementState as well, yet proposed > solution for "precustomized" state fails a lot of tests where custom element > is synchronously created. > > Also, please note that "failed" => "precustomized" state transition happens > only after _disable shadow_ is checked (step 8.2 of > https://html.spec.whatwg.org/#upgrades); I'm not sure how we can get around > with "failed" state only. Hm... I see. Can we replace CustomElementState::Uncustomized with CustomElementState::Precustomized? Because we should be able to differentiate a custom element from the rest by asking if it's unknown or not (Node::isUnknownElement() which checks NodeFlag::IsUnknownElement). > > > Source/WebCore/dom/Node.h:616 > > > + bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; } > > > + void setInternalsAttached(); > > > + > > > > This shouldn't be here. ElementInternals should be stored in ElementRareData. > > There is no need to avoid accessing when we need to check this condition > > since it never happens in a hot code path. > > It feels to me that CustomElementState should also be stored in > ElementRareData, yet it's here in Node. That's for performance because we have to check it in various very hot code paths. > I suppose the reason it's there is free bits in RareDataBitFields. This patch is neutral on > sizeof(RareDataBitFields). We don't want to waste bits in RareDataBitFields because we may use it for other purposes. > How can we move it to ElementRareData without increasing its size? Use CompactUniquePtrTuple for CustomElementReactionQueue and store it there? > > > Source/WebCore/html/HTMLElement.cpp:1287 > > > + setInternalsAttached(); > > > + return ElementInternals::create(*this); > > > > We should store this in ElementRareData or CustomElementReactionQueue) > > (the latter is preferable to avoid increasing the size of ElementRareData by > > sizeof(void*)). > > I've checked the spec and seems like there is no necessity to store > ElementInternals instance like we do for `classList` or `dataset`. It's a > one-way API, from userland JS code to DOM, and it's not a [SameObject]. Ok, I see. Didn't realize that. That's pretty neat.

<rdar://problem/89989084>

(In reply to Ryosuke Niwa from comment #3) > It seems like you need to rebaseline the results for > imported/w3c/web-platform-tests/html/dom/idlharness.https.html Looks like a rebasline is still needed for this test for iPad queue's constant failure.

Pull request: https://github.com/WebKit/WebKit/pull/348

Committed r293183 (249861@main): <https://commits.webkit.org/249861@main> Reviewed commits have been landed. Closing PR #348 and removing active labels.

The commit that closed this a test gardening commit, not the actual implementation commit.

(In reply to Alexey Shvayka from comment #5) > (In reply to Ryosuke Niwa from comment #4) > > > > > Source/WebCore/dom/Node.h:591 > > > + Precustomized = 4, > > > > Let's not waste an entire bit for having this value. > > The following condition should be used for an element to be precustomsized: > > !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() == > > CustomElementState::Failed > > r- because of this. > > I don't like adding a bit to CustomElementState as well, yet proposed > solution for "precustomized" state fails a lot of tests where custom element > is synchronously created. > > Also, please note that "failed" => "precustomized" state transition happens > only after _disable shadow_ is checked (step 8.2 of > https://html.spec.whatwg.org/#upgrades); I'm not sure how we can get around > with "failed" state only. Blending "failed" and "precustomized" worked like charm, not sure why it didn't the first time. Pull request with all the comments applied: https://github.com/WebKit/WebKit/pull/2690. I will make sure to add GC / JSElementInternals::visitAdditionalChildren() test case before landing.

Committed 253303@main (c51d445d35e1): <https://commits.webkit.org/253303@main> Reviewed commits have been landed. Closing PR #2690 and removing active labels.