LibWeb: Move NodeFacts into node_facts.rs - #11056
Conversation
This pure code reorganization moves NodeFacts and its implementation from layout_state.rs to node_facts.rs, alongside the existing node fact helper functions.
📝 WalkthroughWalkthroughThe change moves the pass-scoped 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 (1)
Libraries/LibWeb/Rust/src/layout/node_facts.rs (1)
492-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the marker traversal.
list_item_markeris a hand-rolled descend-and-ascend walk with three exit paths. The existingnode_facts_testsmodule covers onlynode_can_have_children. Cover at least: an outside marker as a direct child, an inside marker below one anonymous wrapper, a nested authored list item that must not be entered, and a list item with no marker.The traversal needs a
NodeFactsinstance, so the tests need arena-backed fixtures rather than the plainNodeDatavalues the current tests use. Do you want me to draft those fixtures and 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/node_facts.rs` around lines 492 - 526, Add arena-backed fixtures in the node_facts_tests module to construct NodeFacts and exercise list_item_marker. Cover a direct outside marker, an inside marker beneath one anonymous wrapper, a nested authored list item that must not be traversed, and a list item without any marker; assert the returned node for each case, including Node::INVALID where appropriate.
🤖 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/node_facts.rs`:
- Around line 176-180: Make the fields of NodeFacts accessible to
LayoutState::node_facts by marking state, callbacks, and node pub(crate), or add
a pub(crate) NodeFacts::new constructor and update LayoutState::node_facts to
use it.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/node_facts.rs`:
- Around line 492-526: Add arena-backed fixtures in the node_facts_tests module
to construct NodeFacts and exercise list_item_marker. Cover a direct outside
marker, an inside marker beneath one anonymous wrapper, a nested authored list
item that must not be traversed, and a list item without any marker; assert the
returned node for each case, including Node::INVALID where appropriate.
🪄 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: 86100d83-aa1a-4e97-9d7b-64ab368802cd
📒 Files selected for processing (2)
Libraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/node_facts.rs
💤 Files with no reviewable changes (1)
- Libraries/LibWeb/Rust/src/layout/layout_state.rs
| pub(crate) struct NodeFacts<'pass> { | ||
| state: &'pass LayoutState, | ||
| callbacks: &'pass FfiLayoutFcCallbacks, | ||
| node: Node, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the module declarations and every NodeFacts construction site.
fd -t f 'mod.rs|layout.rs' Libraries/LibWeb/Rust/src --exec rg -n 'mod (node_facts|layout_state)' {} +
# Struct literal / constructor uses outside node_facts.rs
rg -nP --type=rust -C4 'NodeFacts\s*\{' Libraries/LibWeb/Rust/src
# Field declaration visibility in the new module
ast-grep run --pattern 'pub(crate) struct NodeFacts<$_> { $$$ }' --lang rust Libraries/LibWeb/Rust/src/layout/node_facts.rsRepository: LadybirdBrowser/ladybird
Length of output: 1329
🏁 Script executed:
#!/bin/bash
set -u
printf 'Module declarations:\n'
rg -n --type=rust '^\s*mod\s+(node_facts|layout_state)\s*;' Libraries/LibWeb/Rust/src || true
rg -n --type=rust '^\s*mod\s+(node_facts|layout_state)\s*;' Libraries/LibWeb/Rust/src/layout.rs 2>/dev/null || true
printf '\nNodeFacts construction/constructor usage:\n'
rg -n --type=rust -C3 'NodeFacts\s*\{|\bNodeFacts::|node_facts\s*:' Libraries/LibWeb/Rust/src || true
printf '\nRelevant file headers/imports:\n'
sed -n '1,80p' Libraries/LibWeb/Rust/src/layout/layout_state.rs
printf '\n---\n'
sed -n '160,185p' Libraries/LibWeb/Rust/src/layout/node_facts.rsRepository: LadybirdBrowser/ladybird
Length of output: 4590
🏁 Script executed:
#!/bin/bash
set -u
find Libraries/LibWeb/Rust/src -maxdepth 1 -type d -print | sort
printf '\nRoot Rust files:\n'
find Libraries/LibWeb/Rust/src -maxdepth 1 -type f -print | sort
printf '\nlayout/layout_state.rs module boundary around declarations:\n'
rg -n --type=rust '^(mod node_facts|mod layout_state|pub mod node_facts|pub mod layout_state)' Libraries/LibWeb/Rust/src/layout -g '*.rs' || true
printf '\nImports/usages of NodeFacts in layout_state.rs:\n'
rg -n --type=rust -C3 'NodeFacts|layout_state' Libraries/LibWeb/Rust/src/layout/layout_state.rs || true
printf '\nImports/usages of NodeFacts from node_facts module in layout state path:\n'
rg -n --type=rust -C3 'use .*node_facts|NodeFacts' Libraries/LibWeb/Rust/src/layout/layout_state.rs Libraries/LibWeb/Rust/src/layout/node_facts.rs | sed -n '1,220p'Repository: LadybirdBrowser/ladybird
Length of output: 2567
🌐 Web query:
Rust reference field visibility E0451 private fields struct literal outside defining module
💡 Result:
In Rust, the compiler error E0451 occurs when you attempt to instantiate a struct using struct literal syntax outside of the module where the struct is defined, if that struct contains any private fields [1][2]. This behavior is a fundamental part of Rust's encapsulation and privacy system [3][4]. By default, struct fields are private, meaning they are only accessible within the module that defines the struct (and its descendants) [3][4]. To use struct literal syntax (e.g., MyStruct { field1: val }) to create an instance, all fields of the struct must be visible in the scope where the instantiation occurs [3][5]. If even one field is private, the compiler prohibits direct construction via literals to prevent external code from bypassing the encapsulation intended by the module author [1][4]. To resolve this error, you generally have two options: 1. Make all fields public: If the struct is intended to be a simple data container, you can prefix the fields with the pub keyword (e.g., pub field1: T) [1][2][4]. 2. Provide a public constructor function: Keep the fields private to maintain encapsulation and define a public associated function (often named new) within an impl block for the struct to handle initialization [1][2][4]. Since private fields cannot be accessed or initialized externally, they are effectively hidden from outside code, ensuring that the struct's internal state can only be modified or viewed through public methods defined within the same module [3][4]. While there have been community discussions regarding potential future features—such as opt-in mechanisms to allow initialization of private fields without a constructor [6]—these are not currently part of the language. Top results: [1][3][4]
Citations:
- 1: https://doc.rust-lang.org/error_codes/E0451.html
- 2: https://dev-doc.rust-lang.org/beta/error_codes/E0451.html
- 3: https://doc.rust-lang.org/stable/reference/visibility-and-privacy.html
- 4: https://doc.rust-lang.org/rust-by-example/mod/struct_visibility.html
- 5: https://doc.rust-lang.org/reference/expressions/struct-expr.html
- 6: [Pre-RFC] Allow private fields to be set during struct initialization in external modules rust-lang/rfcs#3794
Make the NodeFacts fields reachable from layout_state.
LayoutState::node_facts constructs NodeFacts with fields in the sibling layout_state module. Since the fields are private to node_facts, NodeFacts { state: self, callbacks, node } is not accessible there after the move.
Add pub(crate) to the fields, or expose a NodeFacts::new(...) constructor from this module and call it from LayoutState::node_facts.
🤖 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/node_facts.rs` around lines 176 - 180, Make
the fields of NodeFacts accessible to LayoutState::node_facts by marking state,
callbacks, and node pub(crate), or add a pub(crate) NodeFacts::new constructor
and update LayoutState::node_facts to use it.
bbdccfb
into
LadybirdBrowser:master
1 participant
This pure code reorganization moves NodeFacts and its implementation from layout_state.rs to node_facts.rs, alongside the existing node fact helper functions.