Summary: | Having an iframe as a descendent node shouldn't require ElementRareData | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, cmarcelo, darin, esprehn+autocc, ews-watchlist, kangil.han, koivisto, simon.fraser, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 216208, 216225 | ||||||||||||
Bug Blocks: | 216305 | ||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2020-09-07 23:35:10 PDT
Created attachment 408215 [details]
Patch
Created attachment 408278 [details]
Fixed tests
Created attachment 408279 [details]
Fixed tests
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 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? (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. Created attachment 408302 [details]
Patch for landing
Comment on attachment 408302 [details]
Patch for landing
Wait for EWS.
Committed r266769: <https://trac.webkit.org/changeset/266769> |