Reduce tree builder FFI surface - #10938
Merged
kalenikaliaksandr merged 6 commits intoJul 30, 2026
Merged
Conversation
The tree builder crossed the FFI boundary for three queries the arena already answers. set_children_are_inline round-tripped through C++ only to write the ChildrenAreInline flag that lives in arena NodeData, so Rust now writes it through set_node_flag directly. rendered_legend is a scan for the first in-flow LegendBox child, and topmost_placement_node walks up through anonymous wrappers to find a direct viewport child; both are pure reads of arena topology, kind, and display bits, so they are now implemented in Rust and their callbacks are deleted. The C++ helpers keep their remaining DOM-side and paintable callers.
The tree builder asked C++ for a facts bundle (is_replaced_element plus six display predicates) after every principal box and pseudo-element content replacement, only to decide whether a replaced element's table display must be adjusted to block or inline. All inputs already sit in the arena mirror: IsReplacedElement is a constructor-stamped flag, and table_display plus the InlineOutside display bit cover the display predicates the adjustment reads. Rust now derives the adjustment from NodeData directly, and principal_layout_facts, the pseudo-element layout_facts, and the FfiPrincipalLayoutFacts struct are deleted. On the reuse path the mirror reflects a previously adjusted display, so Rust skips the redundant re-adjustment that C++ used to apply; the resulting display is identical and one computed-values clone is saved. The PrincipalNodeFrame display member only existed to feed the deleted facts callback, so it is gone too; the display adjustment callbacks now set the node display directly.
principal_descendant_facts marshalled three booleans the Rust side can already derive from NodeData: can_have_children and is_replaced_box_with_children follow from the node kind and the ReplacedBoxCanHaveChildren flag exactly as node_facts spells them out for the layout engine, and uses_button_layout is a constructor-stamped node flag whose consumer already holds the node data. The tree builder now computes all three locally and the struct fields are gone.
The tree builder tracked rebuild roots in the C++ bridge, which forced five callbacks whose only job was shuttling that state across the FFI: principal_placement_facts echoed two of Rust's own arguments back and bundled facts Rust either already held (entry facts, the current rebuild root) or could read from the arena (old-box attachment via the parent link once push_principal_frame returns the old box's slot), and start/restore/mark plus note_tree_restructuring existed purely because the escaped-roots flag and the rebuild-root pointer lived in C++. Rust now owns the current rebuild root slot, the rebuilt-root list, and the escaped flag; the escape test is an inclusive-ancestor walk over arena parent links. Rebuilt-root shell pointers are captured at the moment a rebuild root starts, matching the lifetime the bridge's list had. One new report_rebuild_outcome callback hands the results to C++ when the build finishes, replacing the layout_root read as well since the DocumentRoot placement already stores the root on the bridge. Removing an old ::backdrop box restructures the tree outside every rebuild root, so prepare_principal_element now reports that removal in its returned facts instead of setting the bridge flag directly.
The DOM callback table carried five stale-subtree callbacks that were all the same walk with different parameters: inclusive or descendants-only, and bounded to the walk root or not, where the bound decides whether SVG resource boxes attached inside the cleared root are cleared too. One clear_stale_subtree callback taking an explicit scope replaces them, and the top-layer detach table's two bounded inclusive variants collapse the same way. clear_update_flags and clear_dom_update_flags had identical bodies, so only the latter remains, and the two resolve_counters callbacks merge into one that takes the pseudo-element to resolve against, with a None value for the element itself.
The tree builder guarded against SVG resource reference cycles by asking C++ for the DOM node of every layout box on the ancestor stack and comparing pointers, one callback round trip per ancestor. A box registers itself on its element at construction time, before its subtree is built, so a resource element under construction is found by looking up its layout box slot once and checking stack membership. The per-ancestor layout_node_dom_node callback is replaced by an element_layout_node lookup matching the top-layer detach table.
|
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 (2)
📝 WalkthroughWalkthroughThe layout tree builder’s C++/Rust FFI contracts are reshaped around scoped stale cleanup, Rust-owned layout decisions, partial-rebuild tracking, and callback-based rebuild outcome reporting. Principal, pseudo-element, top-layer, insertion, and content-wrapping paths are updated accordingly. ChangesLayout tree builder refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LayoutTreeBuildBridge
participant rust_build_layout_tree
participant TreeBuilderState
participant LayoutArena
LayoutTreeBuildBridge->>rust_build_layout_tree: start layout tree construction
rust_build_layout_tree->>TreeBuilderState: track rebuild root and restructuring
TreeBuilderState->>LayoutArena: insert and update layout nodes
rust_build_layout_tree->>LayoutTreeBuildBridge: report rebuilt shells and escaped-root status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See commit descriptions