Skip to content

LibWeb: Return slot ids instead of shells from layout FC callbacks - #10903

Merged
kalenikaliaksandr merged 1 commit into
LadybirdBrowser:masterfrom
kalenikaliaksandr:layout-slot-id-callbacks
Jul 28, 2026
Merged

LibWeb: Return slot ids instead of shells from layout FC callbacks#10903
kalenikaliaksandr merged 1 commit into
LadybirdBrowser:masterfrom
kalenikaliaksandr:layout-slot-id-callbacks

Conversation

@kalenikaliaksandr

Copy link
Copy Markdown
Member

The formatting-context callbacks handed raw Layout::Node pointers back into Rust in three places (static position containing block, anchor lookup, and the list item marker in the list item facts), forcing the Rust side to reconstruct arena slot ids by reading the NodeData pointer at a byte displacement inside the C++ object. That displacement had to be computed at runtime and defended by a vtable-sampling verification walk over the layout tree, all resting on the assumption that every layout node class keeps its NodeData member at one process-wide offset.

Slot ids are already the canonical cross-boundary node reference: the topology synchronization writes them into NodeData, and Node::slot_id() is a single field read carrying the packed index and generation. Return slot ids from the three callbacks directly, which lets the displacement field, the verification walk, and the Rust-side unsafe pointer arithmetic all be deleted with no replacement. The anchor lookup's bool-plus-out-pointer shape collapses into returning an invalid slot id for "not found", and the marker link in the list item facts still reads the C++ WeakPtr at fetch time, so a detached marker stays absent rather than becoming a stale slot reference.

The formatting-context callbacks handed raw Layout::Node pointers back
into Rust in three places (static position containing block, anchor
lookup, and the list item marker in the list item facts), forcing the
Rust side to reconstruct arena slot ids by reading the NodeData pointer
at a byte displacement inside the C++ object. That displacement had to
be computed at runtime and defended by a vtable-sampling verification
walk over the layout tree, all resting on the assumption that every
layout node class keeps its NodeData member at one process-wide offset.

Slot ids are already the canonical cross-boundary node reference: the
topology synchronization writes them into NodeData, and Node::slot_id()
is a single field read carrying the packed index and generation. Return
slot ids from the three callbacks directly, which lets the displacement
field, the verification walk, and the Rust-side unsafe pointer
arithmetic all be deleted with no replacement. The anchor lookup's
bool-plus-out-pointer shape collapses into returning an invalid slot id
for "not found", and the marker link in the list item facts still reads
the C++ WeakPtr at fetch time, so a detached marker stays absent rather
than becoming a stale slot reference.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b3701bf6-676a-474c-9bdc-9667b1e6c5e6

📥 Commits

Reviewing files that changed from the base of the PR and between 22e1013 and d17dfb6.

📒 Files selected for processing (8)
  • Libraries/LibWeb/Layout/LayoutRustBridge.cpp
  • Libraries/LibWeb/Layout/LayoutRustBridge.h
  • Libraries/LibWeb/Layout/Node.cpp
  • Libraries/LibWeb/Layout/Node.h
  • Libraries/LibWeb/Rust/src/layout/formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs
  • Libraries/LibWeb/Rust/src/layout/layout_state.rs
  • Libraries/LibWeb/Rust/src/layout/node_facts.rs
💤 Files with no reviewable changes (3)
  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs
  • Libraries/LibWeb/Layout/Node.cpp
  • Libraries/LibWeb/Layout/Node.h

📝 Walkthrough

Walkthrough

Formatting-context callbacks and list-item marker facts now use arena-backed NodeSlotId values across the C++/Rust layout FFI. Obsolete pointer conversion and node displacement helpers were removed.

Changes

NodeSlotId FFI migration

Layer / File(s) Summary
Arena-backed node addressing
Libraries/LibWeb/Layout/Node.*, Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs, Libraries/LibWeb/Rust/src/layout/formatting_context.rs
Node slot identifiers and arena slot indexes replace displacement and data-reference slot helpers; Rust accesses NodeData directly through arena nodes.
Formatting callback contract
Libraries/LibWeb/Layout/LayoutRustBridge.*, Libraries/LibWeb/Rust/src/layout/formatting_context.rs
Formatting callbacks no longer accept a subtree root, and static-position and anchor lookup callbacks return NodeSlotId values with INVALID for failure.
List-item marker slots
Libraries/LibWeb/Layout/LayoutRustBridge.cpp, Libraries/LibWeb/Rust/src/layout/node_facts.rs, Libraries/LibWeb/Rust/src/layout/layout_state.rs
List-item marker facts store typed NodeSlotId values, default to INVALID, and return the stored marker node directly.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LayoutRustBridge
  participant FfiLayoutFcCallbacks
  participant AbsposEngine
  participant LayoutNodeArena
  LayoutRustBridge->>FfiLayoutFcCallbacks: provide NodeSlotId callbacks
  AbsposEngine->>FfiLayoutFcCallbacks: request static containing block
  FfiLayoutFcCallbacks-->>AbsposEngine: return NodeSlotId
  AbsposEngine->>FfiLayoutFcCallbacks: perform anchor lookup
  FfiLayoutFcCallbacks->>LayoutNodeArena: resolve NodeData by Node
  FfiLayoutFcCallbacks-->>AbsposEngine: return NodeSlotId or INVALID
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description accurately matches the changes: it describes the slot-id callback migration, anchor lookup API change, and removal of unsafe displacement logic.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant