LibWeb: Guarantee used values exist wherever layout reads them - #11042
Conversation
LayoutState::try_used_values let callers probe whether a box has per-pass used values, conflating "give me the used values" with "did this box participate in layout this pass". The Option-ness was C++ heritage from the retired engine's nullable LayoutState::get(); against pass-local state, every layout-time read site can know the answer statically. Restructure so used values are guaranteed to exist at every read, then drop try_used_values and the per-context try_used_pointer wrappers.
📝 WalkthroughWalkthroughThe layout engine replaces optional used-value lookups with direct access, adds shared node participation predicates, updates inline geometry state, passes content inline size into line-data emission, and centralizes table row-group classification and participation logic. ChangesLayout used-values and participation
Estimated code review effort: 4 (Complex) | ~45 minutes 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)
505-512: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
is_flow_layout_participantstill includes floating boxes.
is_flow_layout_participantexcludes absolutely positioned boxes, list-item markers, and SVG mask/clip boxes, but not floating boxes. Callers inderive_baselinesandautomatic_block_size_for_bfc_rootadd a separateis_floating()check next to this predicate. Add a short doc comment onis_flow_layout_participantthat states floats still pass this check, so future callers do not assume it covers floats.🤖 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 505 - 512, Add a short documentation comment above is_flow_layout_participant stating that floating boxes still satisfy this predicate and must be checked separately with is_floating(). Leave the predicate logic and callers such as derive_baselines and automatic_block_size_for_bfc_root unchanged.
🤖 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 505-512: Add a short documentation comment above
is_flow_layout_participant stating that floating boxes still satisfy this
predicate and must be checked separately with is_floating(). Leave the predicate
logic and callers such as derive_baselines and automatic_block_size_for_bfc_root
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7b7b22f-bba5-4e28-a9a1-c3ccd728834c
📒 Files selected for processing (9)
Libraries/LibWeb/Rust/src/css/display.rsLibraries/LibWeb/Rust/src/layout/abspos_engine.rsLibraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/grid_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_level_iterator.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/table_formatting_context.rs
f1ac946
into
LadybirdBrowser:master
LayoutState::try_used_values let callers probe whether a box has per-pass used values, conflating "give me the used values" with "did this box participate in layout this pass". The Option-ness was C++ heritage from the retired engine's nullable LayoutState::get(); against pass-local state, every layout-time read site can know the answer statically. Restructure so used values are guaranteed to exist at every read, then drop try_used_values and the per-context try_used_pointer wrappers.