Bug 216264 – Having an iframe as a descendent node shouldn't require ElementRareData

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

ElementRareData is a pretty big data structure. Don't force its existence on every ancestor of an iframe.

[Ryosuke Niwa]

Created attachment 408215 [details]
Patch

[Ryosuke Niwa]

Created attachment 408278 [details]
Fixed tests

[Ryosuke Niwa]

Created attachment 408279 [details]
Fixed tests

[Darin Adler]

Comment on attachment 408279 [details]
Fixed tests

View in context: https://bugs.webkit.org/attachment.cgi?id=408279&action=review

> Source/WebCore/ChangeLog:8
> +        Store the number of connected frames in the descendent nodes direclty in Node using CompactUniquePtrTuple

typo: "directly"

> Source/WebCore/dom/Node.h:540
> +        IsEditingText = 1 << 15, // Text
> +        HasFocusWithin = 1 << 16, // Element

Can’t these share a bit, since Text and Element are mutually exclusive?

> Source/WebCore/dom/Node.h:576
> +        uint16_t connectedSubframeCount : 10; // Must fit Page::maxNumberOfFrames.

Can we use a static_assert to make this more than just a comment?

> Source/WebCore/dom/NodeRareData.cpp:39
> +    uint32_t m_tabInex;

Typo here: m_tabIndex.

Maybe this "same size" struct should not bother using "m_" prefixes?

[Darin Adler]

Comment on attachment 408279 [details]
Fixed tests

View in context: https://bugs.webkit.org/attachment.cgi?id=408279&action=review

> Source/WebCore/ChangeLog:3
> +        Having an iframe as a descendent node shouldn't require ElementRareData

Should we make this directly testable using Internals?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #4)
> Comment on attachment 408279 [details]
> Fixed tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408279&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Store the number of connected frames in the descendent nodes direclty in Node using CompactUniquePtrTuple
> 
> typo: "directly"

Fixed.

> > Source/WebCore/dom/Node.h:540
> > +        IsEditingText = 1 << 15, // Text
> > +        HasFocusWithin = 1 << 16, // Element
> 
> Can’t these share a bit, since Text and Element are mutually exclusive?

In theory yes but this complicates the matter a bit because CSS JIT also checks this flag. We can consider in the future but we have plenty of bits available now so I don't think we have to worry about it for now.

> > Source/WebCore/dom/Node.h:576
> > +        uint16_t connectedSubframeCount : 10; // Must fit Page::maxNumberOfFrames.
> 
> Can we use a static_assert to make this more than just a comment?

Good point. Replaced this with static_assert in Node.cpp above Node::incrementConnectedSubframeCount:
static_assert(RareDataBitFields {Page::maxNumberOfFrames}.connectedSubframeCount == Page::maxNumberOfFrames, "connectedSubframeCount must fit Page::maxNumberOfFrames");

> > Source/WebCore/dom/NodeRareData.cpp:39
> > +    uint32_t m_tabInex;
> 
> Typo here: m_tabIndex.
> 
> Maybe this "same size" struct should not bother using "m_" prefixes?

Hm... I'd use m_* for now since that's what the other members do.

[Ryosuke Niwa]

Created attachment 408302 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 408302 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Committed r266769: <https://trac.webkit.org/changeset/266769>

[Radar WebKit Bug Importer]

<rdar://problem/68547479>