Port ChunkIterator to Rust - #10907
Merged
kalenikaliaksandr merged 6 commits intoJul 28, 2026
Merged
Conversation
compute_inline_containing_block_rect() decided which fragments and boxes belong to a position:relative inline by calling back into C++ to test DOM inclusive ancestry, which kept a DOM dependency in the Rust engine and cost an FFI round trip per visited fragment. The test is expressible with data already in the layout node arena. Nodes without a DOM node are exactly the nodes with NodeFlag::Anonymous, and for non-anonymous nodes layout-tree ancestry mirrors DOM ancestry: the tree builder keeps interrupting blocks inside their inline parent, and the only boxes it hoists out of inlines are anonymous ones (such as a block-level ::before box) whose subtrees are entirely anonymous and were rejected by the old check anyway. Ownership is now a pure arena parent walk.
This shrinks the snapshot to the text, the chunk array, and the empty-editable bit, in preparation for removing the build_text_facts and release_text_facts callbacks entirely.
The arena now owns a per-slot text content table on the Rust side, holding the rendered code units and a precomputed may-require-bidi-processing bit. C++ pushes into it only when something changed: text nodes enroll themselves in a per-arena dirty list on construction, on invalidate_text_for_rendering(), and whenever the text-dependent cache rebuilds under a changed key; set_computed_values additionally enrolls text children since their rendered text derives from the parent style. The three layout entry points drain the list before entering Rust, so a layout pass always reads settled text. A drained node that is alive but detached keeps its enrollment, because it cannot resolve style-dependent text without a parent and may be reinserted by a later tree update without another trigger. Layout-time consumers (item generation, whitespace queries, fragment building) now read the arena table directly, and the text_may_require_bidi_processing FC callback is gone since the flag is computed at push time. The per-pass snapshot shrinks to the chunk array plus the empty-editable bit.
Preparation before porting ChunkIterator to Rust.
|
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 skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe change adds font and Unicode C APIs, synchronizes text content through ChangesText layout pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LayoutRustBridge
participant NodeArena
participant LayoutState
participant TextChunker
participant FontCascadeListRef
LayoutRustBridge->>NodeArena: synchronize enrolled text content
LayoutState->>NodeArena: request cached text chunks
NodeArena->>TextChunker: compute chunks on cache miss
TextChunker->>FontCascadeListRef: resolve fonts and emoji presentation
TextChunker-->>NodeArena: return TextChunk values
NodeArena-->>LayoutState: return cached chunks
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (2)
Libraries/LibWeb/Rust/src/layout/text_chunker.rs (1)
132-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the pure UTF-16/whitespace helpers.
code_point_at,previous_code_point_at,code_unit_length_for_code_point, and the ASCIIGraphemeSegmenterpath need no FFI and encode the surrogate/lone-surrogate semantics this port depends on. A handful of cases (lone high surrogate at end of buffer, lone low surrogate, valid pair,previous_code_point_atover a pair) would lock in parity cheaply.Want me to draft those tests?
🤖 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/text_chunker.rs` around lines 132 - 179, The pure UTF-16 helpers lack unit coverage for their surrogate semantics. Add focused tests for code_point_at covering a trailing high surrogate, lone low surrogate, and valid pair; test previous_code_point_at stepping over a valid pair, and cover code_unit_length_for_code_point plus the ASCII GraphemeSegmenter path using existing test conventions and symbols.Libraries/LibWeb/Layout/NodeArena.cpp (1)
41-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider swapping the enrollment list out before iterating.
sync_text_content_to_arena()runs arbitrary text-cache computation while we iteratem_text_nodes_enrolled_for_content_sync. Today the per-nodem_enrolled_for_arena_text_content_syncflag prevents re-entrantappend()on the vector being iterated, but any future enrollment reached from that call path would reallocate the vector mid-loop and invalidateweak_text_node. Moving the list into a local first makes the loop reentrancy-safe and also removes the separatestill_detached_text_nodesbookkeeping.♻️ Proposed refactor
- Vector<WeakPtr<TextNode>> still_detached_text_nodes; - for (auto& weak_text_node : m_text_nodes_enrolled_for_content_sync) { + auto enrolled_text_nodes = move(m_text_nodes_enrolled_for_content_sync); + m_text_nodes_enrolled_for_content_sync.clear(); + for (auto& weak_text_node : enrolled_text_nodes) { auto const* text_node = weak_text_node.ptr(); if (!text_node) continue; if (!text_node->parent()) { - still_detached_text_nodes.append(move(weak_text_node)); + m_text_nodes_enrolled_for_content_sync.append(move(weak_text_node)); continue; } text_node->sync_text_content_to_arena(); } - m_text_nodes_enrolled_for_content_sync = move(still_detached_text_nodes);🤖 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/Layout/NodeArena.cpp` around lines 41 - 60, Update NodeArena::sync_enrolled_text_node_content to move m_text_nodes_enrolled_for_content_sync into a local vector before iterating. Process that local snapshot, retain alive detached nodes by appending them back to the arena’s enrollment list, and allow any enrollments triggered during sync_text_content_to_arena() to accumulate safely without invalidating the iteration.
🤖 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/HTML/HTMLElement.cpp`:
- Around line 355-371: Update the whitespace collapsing logic in the shown
text-processing path so CSS::WhiteSpaceCollapse::PreserveBreaks retains newline
code units even when preceded by collapsible spaces. Ensure runs such as a space
followed by “\n” preserve the newline, while ordinary collapsing behavior
remains unchanged for CSS::WhiteSpaceCollapse::Collapse.
In `@Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs`:
- Around line 573-618: Add a shared layout-pass epoch to enforce validity of
laundered text references: in
Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs:573-618, update
text_chunks to retain replaced TextChunkCacheEntry values until the current
epoch ends or assert that an existing entry’s key remains stable; in
Libraries/LibWeb/Rust/src/layout/formatting_context.rs:4795-4803, have
text_content() record and debug-assert its synchronization epoch before
laundering the TextContent reference; in
Libraries/LibWeb/Rust/src/layout/line_builder.rs:112-113, store the epoch or
owning slot ID with the raw text pointer and make LineBoxFragmentData::text()
validate that the backing Vec<u16> has not been replaced.
---
Nitpick comments:
In `@Libraries/LibWeb/Layout/NodeArena.cpp`:
- Around line 41-60: Update NodeArena::sync_enrolled_text_node_content to move
m_text_nodes_enrolled_for_content_sync into a local vector before iterating.
Process that local snapshot, retain alive detached nodes by appending them back
to the arena’s enrollment list, and allow any enrollments triggered during
sync_text_content_to_arena() to accumulate safely without invalidating the
iteration.
In `@Libraries/LibWeb/Rust/src/layout/text_chunker.rs`:
- Around line 132-179: The pure UTF-16 helpers lack unit coverage for their
surrogate semantics. Add focused tests for code_point_at covering a trailing
high surrogate, lone low surrogate, and valid pair; test previous_code_point_at
stepping over a valid pair, and cover code_unit_length_for_code_point plus the
ASCII GraphemeSegmenter path using existing test conventions and symbols.
🪄 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: 1f3b630d-f46b-4a99-92a1-be575740aa86
📒 Files selected for processing (22)
Libraries/LibGfx/Font/Font.cppLibraries/LibGfx/FontCascadeList.cppLibraries/LibGfx/Rust/src/font.rsLibraries/LibUnicode/Segmenter.cppLibraries/LibWeb/HTML/HTMLElement.cppLibraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Layout/LayoutRustBridge.hLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Layout/NodeArena.cppLibraries/LibWeb/Layout/NodeArena.hLibraries/LibWeb/Layout/TextNode.cppLibraries/LibWeb/Layout/TextNode.hLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_level_iterator.rsLibraries/LibWeb/Rust/src/layout/layout_node_arena.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/line_builder.rsLibraries/LibWeb/Rust/src/layout/mod.rsLibraries/LibWeb/Rust/src/layout/style_facts.rsLibraries/LibWeb/Rust/src/layout/text_chunker.rs
Comment on lines
+573
to
+618
| pub(crate) fn text_chunks( | ||
| &self, | ||
| id: NodeSlotId, | ||
| key: TextChunkCacheKey, | ||
| compute: impl FnOnce() -> Vec<crate::layout::TextChunk>, | ||
| ) -> &'static [crate::layout::TextChunk] { | ||
| // data() validates that id names a live slot with a matching generation. | ||
| self.data(id); | ||
| let index = id.slot_index() as usize; | ||
|
|
||
| // SAFETY (for both laundered returns below): an entry is only replaced | ||
| // when its key changes or its slot is freed, and every key input is | ||
| // fixed for a given node within one layout pass while the arena | ||
| // itself outlives the pass, so a slice handed out during a pass stays | ||
| // valid for that pass. | ||
| { | ||
| let slots = self.text_chunk_caches.borrow(); | ||
| if let Some(slot) = slots.get(index) | ||
| && slot.generation == id.generation() | ||
| && let Some(entry) = slot.entry.as_deref() | ||
| && entry.key == key | ||
| { | ||
| return unsafe { std::slice::from_raw_parts(entry.chunks.as_ptr(), entry.chunks.len()) }; | ||
| } | ||
| } | ||
|
|
||
| let chunks = compute(); | ||
| let mut slots = self.text_chunk_caches.borrow_mut(); | ||
| if slots.len() <= index { | ||
| slots.resize_with(index + 1, TextChunkCacheSlot::default); | ||
| } | ||
| slots[index] = TextChunkCacheSlot { | ||
| generation: id.generation(), | ||
| entry: Some(Box::new(TextChunkCacheEntry { | ||
| key, | ||
| // SAFETY: The caller derives the key's cascade-list pointer | ||
| // from a live style snapshot. | ||
| _retained_font_cascade_list: unsafe { | ||
| libgfx_rust::font::RetainedFontCascadeList::retain(key.font_cascade_list) | ||
| }, | ||
| chunks, | ||
| })), | ||
| }; | ||
| let entry = slots[index].entry.as_deref().expect("entry was just stored"); | ||
| unsafe { std::slice::from_raw_parts(entry.chunks.as_ptr(), entry.chunks.len()) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Arena text side tables hand out 'static borrows and raw pointers whose validity rests on an unenforced invariant. text_contents and text_chunk_caches own boxed data that is laundered to 'static (or to a bare pointer) and then outlives the &self arena borrow. Correctness depends entirely on "text is only mutated between passes" and "a slot's chunk key never changes during a pass" — neither is checked, and a violation is silent UB rather than a panic. Worth one enforcement mechanism shared by all three consumers, e.g. a pass epoch on the arena that set_text_content bumps and the laundering accessors debug-assert against, plus retaining replaced boxes until the epoch ends.
Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L573-L618: stop dropping the previousTextChunkCacheEntryin place — keep replaced entries alive for the current pass (or debug-assert the key is stable while an entry exists) before returning a laundered slice.Libraries/LibWeb/Rust/src/layout/formatting_context.rs#L4795-L4803: havetext_content()assert the pass epoch it was synced in before laundering theTextContentreference to'static.Libraries/LibWeb/Rust/src/layout/line_builder.rs#L112-L113: the rawtext.as_ptr()stored inLineBoxFragmentDatapersists into commit — record the epoch alongside it (or the owning slot ID) soLineBoxFragmentData::text()can assert the backingVec<u16>was not replaced.
📍 Affects 3 files
Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L573-L618(this comment)Libraries/LibWeb/Rust/src/layout/formatting_context.rs#L4795-L4803Libraries/LibWeb/Rust/src/layout/line_builder.rs#L112-L113
🤖 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_node_arena.rs` around lines 573 -
618, Add a shared layout-pass epoch to enforce validity of laundered text
references: in Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs:573-618,
update text_chunks to retain replaced TextChunkCacheEntry values until the
current epoch ends or assert that an existing entry’s key remains stable; in
Libraries/LibWeb/Rust/src/layout/formatting_context.rs:4795-4803, have
text_content() record and debug-assert its synchronization epoch before
laundering the TextContent reference; in
Libraries/LibWeb/Rust/src/layout/line_builder.rs:112-113, store the epoch or
owning slot ID with the raw text pointer and make LineBoxFragmentData::text()
validate that the backing Vec<u16> has not been replaced.
Text chunking was the last piece of inline layout still computed in C++ behind the build_text_facts callback: every pass re-entered the host, copied the chunk array into a leaked heap arena, and released it through a stored function pointer when the pass-scoped store dropped. The chunker itself is layout logic whose only layout-time consumer is the Rust inline formatting context, so it now lives in the layout crate as a faithful port of TextNode::ChunkIterator, consuming fonts and Unicode segmentation as libraries: per-code-point cascade resolution and emoji presentation through the LibGfx externs, boundary queries through the LibUnicode segmenter handles (with a native Rust fast path for ASCII grapheme stepping), and the direction/line-break-class policy lookups through new flat externs on the layout bridge. word-break and font-variant-emoji join the registered style schema so the chunk inputs read natively. Chunk lists are cached per arena slot across passes, mirroring the cross-pass chunk cache the C++ TextNode kept. The key covers the wrap/linebreak flags, the per-IFC direction mode, and the style inputs including the font cascade list compared by pointer identity; the entry retains the cascade list, which both keeps every font the chunks reference alive and makes pointer-identity keying ABA-safe. Text pushes invalidate the cached chunks since chunk offsets index the rendered text. The one genuinely DOM-dependent leftover, the empty-editable check for caret-height fragments, becomes a boolean predicate callback beside document_cursor_is_on_node. build_text_facts, release_text_facts, the snapshot arena, and the pass-scoped facts store with its Drop plumbing are gone.
With inline layout chunking text in the Rust layout crate, the C++ ChunkIterator had exactly one caller left: get-the-text-steps used it to apply whitespace collapsing for innerText, dragging the whole font-cascade machinery along even though font-change chunk boundaries are meaningless for text extraction. That caller now performs the one transformation the iterator contributed — removing every code point after the first in each collapsible whitespace run — with a direct scan over the rendered text. This lets TextNode shed the ChunkIterator, the chunk list types, the cross-pass chunk cache (superseded by the arena-side chunk cache), and the cached line segmenter that only chunking used. The grapheme segmenter stays for editing, hit testing, and fragment painting, and text_type_for_code_point stays as the direction-classification lookup behind the layout bridge extern.
kalenikaliaksandr
force-pushed
the
drop-dom-ancestry-layout-callback
branch
from
July 28, 2026 21:05
ad84302 to
dd0cf62
Compare
kalenikaliaksandr
merged commit Jul 28, 2026
d01e1cf
into
LadybirdBrowser:master
14 of 16 checks passed
This was referenced Jul 29, 2026
This was referenced Aug 5, 2026
Merged
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.
This allows us to drop FFI to ask C++ ChunkIterator for text chunks from layout code (at cost of introducing FFI to talk to LibUnicode and LibGfx)