LibWeb: Move table grid formation fully into Rust - #10927
Conversation
|
Hello! One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughTable span data now synchronizes into Rust-backed node state. Rust owns occupancy-aware table-grid calculation, while C++ TableGrid FFI usage is removed. Tree-builder table fixup and border conflict sorting use the updated implementations. ChangesTable layout migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DOM
participant NodeWithStyle
participant TableFormattingContext
participant TreeBuilderHost
DOM->>NodeWithStyle: change colspan, rowspan, or span
NodeWithStyle->>TableFormattingContext: synchronize table span data
TableFormattingContext->>TableFormattingContext: calculate_table_grid
TableFormattingContext-->>TreeBuilderHost: return occupancy-aware TableGrid
TreeBuilderHost->>TreeBuilderHost: fix missing table cells from occupancy
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Libraries/LibWeb/Rust/src/layout/table_formatting_context.rs (1)
624-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrid-growing algorithm looks correct; consider extracting
process_rowfrom a closure.Traced the occupancy/column-growth logic end-to-end (skip-ahead over occupied slots,
column_count/row_countgrowth, and the final span clipping againstrows.len()/column_count) — no off-by-one or out-of-bounds issues found; index bounds are always established before use.
process_rowis a 9-parameter closure that captures nothing from the enclosing scope — it could be a plain generic function (fn process_row<T: TableTree>(...)) instead, which would simplify the borrow patterns and make the algorithm easier to follow independent ofcalculate_table_grid's local state.♻️ Sketch of the refactor
- let process_row = |tree: &T, - row: Node, - row_group: Option<Node>, - cells: &mut Vec<TableCell>, - rows: &mut Vec<Row>, - occupancy: &mut HashSet<(usize, usize)>, - column_count: &mut usize, - row_count: &mut usize, - current_row: &mut usize| { + fn process_row<T: TableTree>( + tree: &T, + row: Node, + row_group: Option<Node>, + cells: &mut Vec<TableCell>, + rows: &mut Vec<Row>, + occupancy: &mut HashSet<(usize, usize)>, + column_count: &mut usize, + row_count: &mut usize, + current_row: &mut usize, + ) { // ...body unchanged... - }; + }🤖 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/table_formatting_context.rs` around lines 624 - 742, Extract the capture-free process_row closure inside calculate_table_grid into a local or module-level generic function named process_row<T: TableTree>, preserving its parameters, row/cell occupancy updates, span handling, and row creation behavior. Update both call sites in calculate_table_grid to invoke the function without changing the grid-growing algorithm.
🤖 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.
Inline comments:
In `@Libraries/LibWeb/Layout/Node.cpp`:
- Around line 1097-1110: Update NodeWithStyle::synchronize_table_span_data and
the table placement flow so a cell with rowSpan=0 retains its special “through
remaining rows” semantics instead of being normalized by the Rust grid builder
to a one-row span. Propagate an explicit representation or flag for zero row
spans, and have placement extend the cell through all remaining rows while
preserving existing behavior for positive row spans.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/table_formatting_context.rs`:
- Around line 624-742: Extract the capture-free process_row closure inside
calculate_table_grid into a local or module-level generic function named
process_row<T: TableTree>, preserving its parameters, row/cell occupancy
updates, span handling, and row creation behavior. Update both call sites in
calculate_table_grid to invoke the function without changing the grid-growing
algorithm.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a91efbc-4657-459e-b8c3-17b45eda01dc
📒 Files selected for processing (13)
Libraries/LibWeb/CMakeLists.txtLibraries/LibWeb/DOM/Element.cppLibraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Layout/TableGrid.cppLibraries/LibWeb/Layout/TableGrid.hLibraries/LibWeb/Layout/TreeBuilder.cppLibraries/LibWeb/Painting/TableBordersPainting.cppLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/node_data.rsLibraries/LibWeb/Rust/src/layout/table_formatting_context.rsLibraries/LibWeb/Rust/src/layout/tree_builder.rs
💤 Files with no reviewable changes (6)
- Libraries/LibWeb/CMakeLists.txt
- Libraries/LibWeb/Layout/TableGrid.cpp
- Libraries/LibWeb/Layout/TreeBuilder.cpp
- Libraries/LibWeb/Layout/TableGrid.h
- Libraries/LibWeb/Rust/src/layout/formatting_context.rs
- Libraries/LibWeb/Layout/LayoutRustBridge.cpp
71bced7 to
8b2b8e3
Compare
The Rust table formatting context already formed its own row and column grid, while layout tree fixup constructed a duplicate C++ TableGrid and queried every slot through an opaque FFI object. This kept two copies of the formation algorithm and made missing-cell fixup cross the bridge for each grid position. Share the Rust grid builder with layout tree fixup and mirror the bounded table spans into unused NodeData padding so grid formation needs no read callbacks. Keep the missing-cell mutation callback while C++ still owns layout boxes, and move the painting-only border comparator next to its caller. This removes the C++ TableGrid and its four bridge callbacks without changing NodeData's size.
8b2b8e3 to
d0e50a9
Compare
The Rust table formatting context already formed its own row and column grid, while layout tree fixup constructed a duplicate C++ TableGrid and queried every slot through an opaque FFI object. This kept two copies of the formation algorithm and made missing-cell fixup cross the bridge for each grid position.
Share the Rust grid builder with layout tree fixup and mirror the bounded table spans into unused NodeData padding so grid formation needs no read callbacks. Keep the missing-cell mutation callback while C++ still owns layout boxes, and move the painting-only border comparator next to its caller. This removes the C++ TableGrid and its four bridge callbacks without changing NodeData's size.