Own the formatting-context run root's record in the run - #11095
Conversation
A formatting-context run owns every used-values record it creates except its own root: the parent creates that record, keeps it registered in its own scope, and hands the same Rc into the run to mutate in place. Descendant records were already severed in "Own used-values records in their run scopes", so this shared root is the last aliasing between run scopes, and it makes run attempts irrevocable: everything a run writes to its root is immediately visible to the parent, so a run that is thrown away or replayed leaks partial state. This change is a preparation for future optimization that would rely on revoking layout run results. Give each run a private root record instead. The dispatch seam captures the state of the parent's record as a plain value, the run materializes and mutates its own record, and the completed run's root state is applied back onto the parent's record before any post-run consumer reads it. A discarded run now leaves no trace in its parent, and a cached run can be replayed from plain values with no record surgery.
📝 WalkthroughWalkthroughFormatting-context measurement now returns direct child layout results and borrows used values. Root used-value state is captured, materialized, restored, and absorbed through Sequence Diagram(s)sequenceDiagram
participant LayoutEntry
participant FormattingContext
participant UsedValues
participant FragmentCollection
LayoutEntry->>FormattingContext: run formatting context with root used values
FormattingContext->>UsedValues: capture and materialize root state
FormattingContext->>FragmentCollection: produce child result and fragments
FormattingContext-->>LayoutEntry: return direct result and root outcome
LayoutEntry->>UsedValues: absorb root state
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Libraries/LibWeb/Rust/src/layout/used_values.rs (1)
319-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
own_metrics_are_sealedusescontent_inline_sizeas a proxy for two seal operations.
content_inline_sizeis sealed by bothseal_own_metricsandseal_committed_box_metrics. The predicate therefore reportstruefor a record that was sealed through placement, not only for a record whose run sealed its own metrics.UsedValuesCellState::materialize_recordreaches exactly that state whenhas_content_offsetis set, soRunRootOutcomethen replaysseal_own_metricson the parent. The replay is currently harmless, because a placed box already sealed the wider set. Add a short comment so a future change to either seal list does not silently alter the outcome.♻️ Proposed comment
+ /// Reports whether this record's own metrics are sealed. + /// NB: `content_inline_size` stands in for the whole group. It is also sealed by + /// `seal_committed_box_metrics`, so a placed box reports `true` here as well. + /// Replaying `seal_own_metrics` for such a record is a no-op, because placement + /// already sealed a superset of these cells. pub(crate) fn own_metrics_are_sealed(&self) -> bool { self.content_inline_size.is_sealed() }🤖 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/used_values.rs` around lines 319 - 321, Add a concise comment above `own_metrics_are_sealed` documenting that `content_inline_size` is sealed by both `seal_own_metrics` and `seal_committed_box_metrics`, making it a proxy that also covers placement-sealed records and the harmless replay in `RunRootOutcome`.Libraries/LibWeb/Rust/src/layout/sizing_context.rs (1)
1885-1902: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
wrapper_outputsto match its new type.The binding now holds a
ChildLayoutResult, not the removedRunOutputs. Theoutputssuffix refers to a type that no longer reaches this call site. Rename it towrapper_resultfor consistency withwrapper_resultinlayout_replaced_with_children.♻️ Proposed rename
- let wrapper_outputs = measurement.run_with_layout_mode( + let wrapper_result = measurement.run_with_layout_mode( wrapper, &wrapper_used, LayoutMode::IntrinsicSizing, @@ - let table_used_block_size = wrapper_outputs + let table_used_block_size = wrapper_result .table_box_in_wrapper_border_box_block_size .expect("a table wrapper's measurement run lays out the table box inside it");🤖 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/sizing_context.rs` around lines 1885 - 1902, Rename the local binding `wrapper_outputs` to `wrapper_result` throughout this measurement flow, including the access to `table_box_in_wrapper_border_box_block_size`, to reflect its `ChildLayoutResult` type and match the naming used by `layout_replaced_with_children`.Libraries/LibWeb/Rust/src/layout/formatting_context.rs (1)
1935-1951: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the fragment pairing invariant in
absorb_run_outputs.The tuple match drops a returned
rootwhenparent_fragmentsisNone. Today that combination cannot occur, because a run builds fragments only whenlayout_mode == LayoutMode::Normaland the purpose is not measurement, and a child inherits the purpose from its parent. The invariant is implicit. If a future caller passesNonefragments for a commit-mode run, the unplaced root fragment disappears silently and the subtree never reaches the fragment tree.♻️ Proposed assertion
root_outcome.apply_to_record(parent_used); - if let (Some(fragments), Some(root)) = (parent_fragments, root) { - debug_assert!(root.node == child, "a child run returned a root for a different box"); - fragments.hold_unplaced_root(root); - } + match (parent_fragments, root) { + (Some(fragments), Some(root)) => { + debug_assert!(root.node == child, "a child run returned a root for a different box"); + fragments.hold_unplaced_root(root); + } + (None, Some(_)) => { + debug_assert!(false, "a fragment-building child run reported to a parent without a fragment builder"); + } + (_, None) => {} + } result🤖 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/formatting_context.rs` around lines 1935 - 1951, Update absorb_run_outputs to explicitly assert that a returned root is only present when parent_fragments is Some, preserving the existing child/node pairing assertion and hold_unplaced_root behavior. Ensure an unexpected root with None fragments fails loudly instead of being silently discarded.
🤖 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/Rust/src/layout/used_values.rs`:
- Around line 341-396: Prevent the duplicated UsedValuesCellState field list
from drifting from UsedValues by generating the relevant cell declarations and
state capture/apply fields from a shared macro invocation, or add a clear
synchronization comment at the UsedValues declaration as an interim safeguard.
Preserve the existing inequality check in UsedValuesCellState::apply_to_record
so identical values are not written to sealed cells.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/formatting_context.rs`:
- Around line 1935-1951: Update absorb_run_outputs to explicitly assert that a
returned root is only present when parent_fragments is Some, preserving the
existing child/node pairing assertion and hold_unplaced_root behavior. Ensure an
unexpected root with None fragments fails loudly instead of being silently
discarded.
In `@Libraries/LibWeb/Rust/src/layout/sizing_context.rs`:
- Around line 1885-1902: Rename the local binding `wrapper_outputs` to
`wrapper_result` throughout this measurement flow, including the access to
`table_box_in_wrapper_border_box_block_size`, to reflect its `ChildLayoutResult`
type and match the naming used by `layout_replaced_with_children`.
In `@Libraries/LibWeb/Rust/src/layout/used_values.rs`:
- Around line 319-321: Add a concise comment above `own_metrics_are_sealed`
documenting that `content_inline_size` is sealed by both `seal_own_metrics` and
`seal_committed_box_metrics`, making it a proxy that also covers
placement-sealed records and the harmless replay in `RunRootOutcome`.
🪄 Autofix
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: 43312f90-199a-4f85-9e63-1956d3b6b015
📒 Files selected for processing (6)
Libraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/replaced_with_children_formatting_context.rsLibraries/LibWeb/Rust/src/layout/sizing_context.rsLibraries/LibWeb/Rust/src/layout/table_formatting_context.rsLibraries/LibWeb/Rust/src/layout/used_values.rs
💤 Files with no reviewable changes (1)
- Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs
| macro_rules! used_values_cell_state { | ||
| ($($field:ident: $type:ty,)+) => { | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub(crate) struct UsedValuesCellState { | ||
| $(pub(crate) $field: $type,)+ | ||
| } | ||
|
|
||
| impl UsedValuesCellState { | ||
| pub(crate) fn capture(used: &UsedValues) -> Self { | ||
| Self { | ||
| $($field: used.$field.get(),)+ | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn apply_to_record(&self, used: &UsedValues) { | ||
| $( | ||
| if used.$field.get() != self.$field { | ||
| used.$field.set(self.$field); | ||
| } | ||
| )+ | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| used_values_cell_state! { | ||
| content_inline_size: CssPixels, | ||
| content_block_size: CssPixels, | ||
| margin_left: CssPixels, | ||
| margin_right: CssPixels, | ||
| margin_top: CssPixels, | ||
| margin_bottom: CssPixels, | ||
| border_left: CssPixels, | ||
| border_right: CssPixels, | ||
| border_top: CssPixels, | ||
| border_bottom: CssPixels, | ||
| padding_left: CssPixels, | ||
| padding_right: CssPixels, | ||
| padding_top: CssPixels, | ||
| padding_bottom: CssPixels, | ||
| inset_left: CssPixels, | ||
| inset_right: CssPixels, | ||
| inset_top: CssPixels, | ||
| inset_bottom: CssPixels, | ||
| has_definite_inline_size: bool, | ||
| has_definite_block_size: bool, | ||
| uses_collapsing_borders_model: bool, | ||
| inline_size_constraint: SizeConstraint, | ||
| block_size_constraint: SizeConstraint, | ||
| has_content_offset: bool, | ||
| content_offset: FfiCssPixelPoint, | ||
| has_first_baseline: bool, | ||
| first_baseline: CssPixels, | ||
| has_last_baseline: bool, | ||
| last_baseline: CssPixels, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Guard the hand-maintained field list against drift from UsedValues.
UsedValuesCellState now defines the full set of state that crosses a run boundary. The list is duplicated by hand. If someone adds a Cell or SealableCell field to UsedValues and does not add it here, the code still compiles, and the new field is silently dropped on every materialize_record and every apply_to_record. The failure appears as wrong layout geometry, not as a build error.
Consider generating the UsedValues cell fields from the same macro invocation, so one list feeds both declarations. A cheaper interim step is a comment on the UsedValues declaration that points here.
The inequality guard in apply_to_record is correct and load-bearing: it prevents SealableCell::set from asserting when identical state is replayed into a sealed parent record. Keep that behavior in any restructuring.
🤖 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/used_values.rs` around lines 341 - 396,
Prevent the duplicated UsedValuesCellState field list from drifting from
UsedValues by generating the relevant cell declarations and state capture/apply
fields from a shared macro invocation, or add a clear synchronization comment at
the UsedValues declaration as an interim safeguard. Preserve the existing
inequality check in UsedValuesCellState::apply_to_record so identical values are
not written to sealed cells.
c14fcaa
into
LadybirdBrowser:master
1 participant
A formatting-context run owns every used-values record it creates except its own root: the parent creates that record, keeps it registered in its own scope, and hands the same Rc into the run to mutate in place. Descendant records were already severed in "Own used-values records in their run scopes", so this shared root is the last aliasing between run scopes, and it makes run attempts irrevocable: everything a run writes to its root is immediately visible to the parent, so a run that is thrown away or replayed leaks partial state. This change is a preparation for future optimization that would rely on revoking layout run results.
Give each run a private root record instead. The dispatch seam captures the state of the parent's record as a plain value, the run materializes and mutates its own record, and the completed run's root state is applied back onto the parent's record before any post-run consumer reads it. A discarded run now leaves no trace in its parent, and a cached run can be replayed from plain values with no record surgery.