LibWeb: Derive node display facts from style payloads on demand - #10942
Conversation
Every style application re-derived four facts from the fresh computed values and stamped them into the arena NodeData record: the two table display classifications, a bitset of display predicates, and the own-style block-formatting-context flag. The stamps duplicated data the node's mirrored group payloads already carry, could drift from them if any write path forgot to restamp, and kept C++ copies of the block-formatting-context predicate and the table display classification alive purely to feed the mirror. Tree building, formatting-context classification, and table grid construction now read these facts straight from the box group payload: StyleReader classifies the display value into FfiTableDisplay and evaluates the computed-style half of the block-formatting-context predicate, and styleless nodes answer every predicate with the same defaults the zeroed stamps used to produce. The mirror shrinks to storing the style pointer and the payload array, NodeData drops the three stamp bytes, and the NodeDisplayFlag enum, the OwnStyleEstablishesBlockFormattingContext flag, and the C++ predicate and classification helpers are deleted. The NodeData layout tests follow the new field offsets, and the stamp-based composite test for the block-formatting-context predicate goes away with the bits it exercised.
📝 WalkthroughWalkthroughLayout display, table roles, positioning, containment, and block-formatting-context classification now use live Rust ChangesStyle contracts and computed-style facts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LayoutTreeBuilder
participant TreeBuilderHost
participant StyleReader
participant TableFormattingContext
LayoutTreeBuilder->>TreeBuilderHost: request node classification
TreeBuilderHost->>StyleReader: read computed display values
StyleReader-->>TreeBuilderHost: return display and table roles
TreeBuilderHost-->>LayoutTreeBuilder: return classification
TableFormattingContext->>TreeBuilderHost: request table_display(node)
TreeBuilderHost-->>TableFormattingContext: return FfiTableDisplay
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Libraries/LibWeb/Rust/src/layout/layout_state.rs (1)
622-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the triplicated "replaced-element table-display adjustment" predicate. The exact same condition —
IsReplacedElementflag set andstyle.table_display_before() != FfiTableDisplay::Other— is independently implemented in three places. Consolidating into a single shared(data, style)free function (alongsidenode_is_out_of_flow/node_creates_block_formatting_contextinnode_facts.rs) would remove the risk of the three copies drifting apart.
Libraries/LibWeb/Rust/src/layout/layout_state.rs#L622-L627: replace the body ofhas_replaced_element_table_display_adjustmentwith a call to the new shared helper.Libraries/LibWeb/Rust/src/layout/formatting_context.rs#L3098-L3100: call the shared helper instead of inlining the check informatting_context_type_created_by_node_data.Libraries/LibWeb/Rust/src/layout/tree_builder.rs#L2123-L2128: replacenode_has_replaced_element_table_display_adjustment's body with a call to the shared helper (or delete it in favor of calling the helper directly at call sites).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/layout/layout_state.rs` around lines 622 - 627, Extract the shared replaced-element table-display adjustment predicate into a free function in node_facts.rs alongside node_is_out_of_flow and node_creates_block_formatting_context, accepting the node data and style and checking IsReplacedElement plus table_display_before() != FfiTableDisplay::Other. Update Libraries/LibWeb/Rust/src/layout/layout_state.rs:622-627, Libraries/LibWeb/Rust/src/layout/formatting_context.rs:3098-3100, and Libraries/LibWeb/Rust/src/layout/tree_builder.rs:2123-2128 to call the helper, removing their duplicated checks while preserving existing APIs or deleting the redundant wrapper where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/layout_state.rs`:
- Around line 622-627: Extract the shared replaced-element table-display
adjustment predicate into a free function in node_facts.rs alongside
node_is_out_of_flow and node_creates_block_formatting_context, accepting the
node data and style and checking IsReplacedElement plus table_display_before()
!= FfiTableDisplay::Other. Update
Libraries/LibWeb/Rust/src/layout/layout_state.rs:622-627,
Libraries/LibWeb/Rust/src/layout/formatting_context.rs:3098-3100, and
Libraries/LibWeb/Rust/src/layout/tree_builder.rs:2123-2128 to call the helper,
removing their duplicated checks while preserving existing APIs or deleting the
redundant wrapper where appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f570851b-91c3-4d1d-b684-83f64ba0fe63
📒 Files selected for processing (11)
Libraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Rust/build.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/mod.rsLibraries/LibWeb/Rust/src/layout/node_data.rsLibraries/LibWeb/Rust/src/layout/node_facts.rsLibraries/LibWeb/Rust/src/layout/style_facts.rsLibraries/LibWeb/Rust/src/layout/table_formatting_context.rsLibraries/LibWeb/Rust/src/layout/tree_builder.rs
💤 Files with no reviewable changes (3)
- Libraries/LibWeb/Rust/build.rs
- Libraries/LibWeb/Rust/src/layout/mod.rs
- Libraries/LibWeb/Layout/Node.cpp
Every style application re-derived four facts from the fresh computed values and stamped them into the arena NodeData record: the two table display classifications, a bitset of display predicates, and the own-style block-formatting-context flag. The stamps duplicated data the node's mirrored group payloads already carry, could drift from them if any write path forgot to restamp, and kept C++ copies of the block-formatting-context predicate and the table display classification alive purely to feed the mirror.
Tree building, formatting-context classification, and table grid construction now read these facts straight from the box group payload: StyleReader classifies the display value into FfiTableDisplay and evaluates the computed-style half of the block-formatting-context predicate, and styleless nodes answer every predicate with the same defaults the zeroed stamps used to produce. The mirror shrinks to storing the style pointer and the payload array, NodeData drops the three stamp bytes, and the NodeDisplayFlag enum, the OwnStyleEstablishesBlockFormattingContext flag, and the C++ predicate and classification helpers are deleted. The NodeData layout tests follow the new field offsets, and the stamp-based composite test for the block-formatting-context predicate goes away with the bits it exercised.