Bug 216264 - Having an iframe as a descendent node shouldn't require ElementRareData
Summary: Having an iframe as a descendent node shouldn't require ElementRareData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 216208 216225
Blocks: 216305
  Show dependency treegraph
 
Reported: 2020-09-07 23:35 PDT by Ryosuke Niwa
Modified: 2020-09-08 23:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (22.25 KB, patch)
2020-09-08 00:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed tests (24.45 KB, patch)
2020-09-08 15:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed tests (22.38 KB, patch)
2020-09-08 15:59 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (22.53 KB, patch)
2020-09-08 19:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-09-07 23:35:10 PDT
ElementRareData is a pretty big data structure. Don't force its existence on every ancestor of an iframe.
Comment 1 Ryosuke Niwa 2020-09-08 00:21:42 PDT
Created attachment 408215 [details]
Patch
Comment 2 Ryosuke Niwa 2020-09-08 15:58:02 PDT
Created attachment 408278 [details]
Fixed tests
Comment 3 Ryosuke Niwa 2020-09-08 15:59:05 PDT
Created attachment 408279 [details]
Fixed tests
Comment 4 Darin Adler 2020-09-08 17:14:10 PDT
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?
Comment 5 Darin Adler 2020-09-08 17:14:32 PDT
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?
Comment 6 Ryosuke Niwa 2020-09-08 18:50:12 PDT
(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.
Comment 7 Ryosuke Niwa 2020-09-08 19:12:47 PDT
Created attachment 408302 [details]
Patch for landing
Comment 8 Ryosuke Niwa 2020-09-08 19:13:22 PDT
Comment on attachment 408302 [details]
Patch for landing

Wait for EWS.
Comment 9 Ryosuke Niwa 2020-09-08 20:32:13 PDT
Committed r266769: <https://trac.webkit.org/changeset/266769>
Comment 10 Radar WebKit Bug Importer 2020-09-08 20:33:23 PDT
<rdar://problem/68547479>