[webkit-reviews] review denied: [Bug 197960] Implement ElementInternals : [Attachment 444531] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Alexey Shvayka
<ashvayka at apple.com>'s request for review:
Bug 197960: Implement ElementInternals
https://bugs.webkit.org/show_bug.cgi?id=197960

Attachment 444531: Patch

https://bugs.webkit.org/attachment.cgi?id=444531&action=review




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 444531
  --> https://bugs.webkit.org/attachment.cgi?id=444531
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*)).