Reduce FFI surface in LayoutRustBridge - #10956
Merged
kalenikaliaksandr merged 2 commits intoAug 1, 2026
Merged
Conversation
FfiSvgElementFacts repeated concrete SVG box classifications already stamped into every layout node's NodeKind. Each facts snapshot therefore crossed the FFI with eleven redundant booleans, while the bridge also kept a DOM type switch solely to classify SVG container elements. Remove those fields and have Rust SVG layout derive exact box, graphics, container, and resource classifications directly from NodeKind. Child resource scans now read the arena kind instead of rebuilding SVG facts. Keep document state, SVGFitToViewBox membership, view-box attributes, transforms, unit selections, and pattern dimensions in the snapshot because these values can vary among nodes with the same NodeKind.
Grid track lists, placements, and areas were flattened through the FFI into a per-LayoutState cache, so every intrinsic measurement pass and every layout run rebuilt them from ComputedValues: a C++ snapshot arena was built, copied entry by entry into Rust vectors, and released again, with track-size calc values retained through a bridge-side handle map and grid line names through leaked fly-string references. The GridValues style group is now a Rust-native payload holding the flattened track lists, placements, areas, and name table directly, plus retained handles to the nine computed grid property values. The C++ population path flattens the computed values once per ComputedValues build and installs the payload through rust_build_grid_group; elements whose grid properties all hold their initial values share the immortal default payload through the existing descriptor fast path, which now skips its scratch allocation for constraint-only groups entirely. Layout reads the payload in place through the style reader: the grid formatting context borrows GridValues directly, track breadths decode their computed sizes on the fly with calc pointers borrowed from the payload, and the per-LayoutState grid facts store is gone, so repeated measurement passes no longer rebuild or copy anything. The computed-style-value reconstruction path returns the retained property values, falling back to the property initial values for the default payload. The table wrapper adoption copies and resets grid placements through payload operations that re-intern placement names and release the retained values.
📝 WalkthroughWalkthroughThe change moves CSS Grid computed values from C++ fields and layout snapshots to Rust-owned payloads. Grid layout reads these values directly. SVG layout now uses ChangesRust-native Grid computed values
NodeKind-based SVG classification
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Libraries/LibWeb/CSS/ComputedValues.cpp (1)
1136-1157: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider passing the parent grid payload for deduplication.
rust_build_grid_groupaccepts aparent_payloadand shares it when the built values compare equal, but this call passesnullptr. Every other group increate()forwardsinherit_parent->m_noninherited.<group>.operator->(). Grid values are non-inherited, yet the equality-based sharing still removes one payload allocation per element in subtrees that repeat the same grid declarations.If the omission is intentional, add a short comment stating why grid opts out of parent sharing.
♻️ Proposed change: thread the parent payload through
-static void* build_grid_group_payload(ComputedProperties const& computed_style) +static void* build_grid_group_payload(ComputedProperties const& computed_style, ComputedValues const* inherit_parent) {- return const_cast<void*>(ComputedValuesFFI::rust_build_grid_group(ComputedValues::GridValues::style_group_index, &values, nullptr)); + return const_cast<void*>(ComputedValuesFFI::rust_build_grid_group( + ComputedValues::GridValues::style_group_index, + &values, + inherit_parent ? static_cast<void const*>(inherit_parent->m_noninherited.grid.operator->()) : nullptr)); }🤖 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/CSS/ComputedValues.cpp` around lines 1136 - 1157, Pass the parent grid payload to rust_build_grid_group in the grid-group creation path, using inherit_parent->m_noninherited’s grid group payload consistently with the other groups, so equal descendant values can be shared. If grid values intentionally opt out, retain nullptr and add a brief comment documenting that decision.Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rs (1)
1179-1184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
'passfromgrid_style.
StyleReader::grid_values()carries the lifetime of itsFfiStylePayloadsinput, andcallbacks.style_payloads()already returns a pass-bounded reference. Use&'pass GridValueshere so the signature matches the actual payload lifetime and avoids the stronger'staticclaim.♻️ Proposed signature change
- fn grid_style(&self, node: Node) -> &'static GridValues { + fn grid_style(&self, node: Node) -> &'pass GridValues { StyleReader::new(self.callbacks.style_payloads(node)).grid_values() }🤖 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/grid_formatting_context.rs` around lines 1179 - 1184, Update the grid_style method to return a pass-bounded reference, changing its return type from &'static GridValues to &'pass GridValues. Preserve the existing StyleReader::new(self.callbacks.style_payloads(node)).grid_values() implementation and rely on the lifetime already provided by style_payloads.
🤖 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/CSS/ComputedProperties.cpp`:
- Around line 161-166: Update the grid_style_value_or_initial lambda to avoid
reinterpret_casting the ComputedStyleValueHandle reference; construct a
RustStyleValueHandle from handle.pointer or handle.data() and pass that object
to style_value_from_handle. Retain the existing initial-value fallback when no
style value is returned, and only rely on COMPUTED_STYLE_VALUE_HANDLE allocation
compatibility if it is explicitly verified.
In `@Libraries/LibWeb/CSS/ComputedValues.h`:
- Around line 3164-3176: Use empty style-value handles as the canonical
representation for auto grid placements: update reset_grid_placements_to_auto in
Libraries/LibWeb/CSS/ComputedValues.h lines 3164-3176 to require both auto kinds
and empty placement handles before returning, and update
build_grid_group_payload to omit handles for auto placements.
Libraries/LibWeb/Rust/src/css/computed_values.rs lines 336-395 requires no
direct change because GridValues::initial already uses empty handles; preserve
that convention so rust_build_grid_group can compare against the shared initial
value.
In `@Libraries/LibWeb/Rust/src/css/computed_value_types.rs`:
- Around line 328-351: Add a compile-time sizeof/alignof parity assertion for
the Rust GridValues type alongside the existing Rust-native computed-value group
checks, using the corresponding C++ ComputedValues::GridValues and
ComputedValuesFFI::GridValues symbols. Ensure the assertion validates the
inherited C++ grid layout against the Rust FFI layout.
In `@Libraries/LibWeb/Rust/src/css/computed_values.rs`:
- Around line 336-395: Update build_grid_group_payload to preserve empty
ComputedStyleValueHandle values instead of unconditionally retaining all nine
grid style-value handles, allowing the payload to equal GridValues::initial().
Apply the same empty-handle preservation to reset_grid_placements_to_auto, while
leaving non-empty handles retained normally.
---
Nitpick comments:
In `@Libraries/LibWeb/CSS/ComputedValues.cpp`:
- Around line 1136-1157: Pass the parent grid payload to rust_build_grid_group
in the grid-group creation path, using inherit_parent->m_noninherited’s grid
group payload consistently with the other groups, so equal descendant values can
be shared. If grid values intentionally opt out, retain nullptr and add a brief
comment documenting that decision.
In `@Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rs`:
- Around line 1179-1184: Update the grid_style method to return a pass-bounded
reference, changing its return type from &'static GridValues to &'pass
GridValues. Preserve the existing
StyleReader::new(self.callbacks.style_payloads(node)).grid_values()
implementation and rely on the lifetime already provided by style_payloads.
🪄 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: 74017b3d-133f-4e7e-82b9-2b51a2aa3171
📒 Files selected for processing (16)
Libraries/LibWeb/CSS/ComputedProperties.cppLibraries/LibWeb/CSS/ComputedProperties.hLibraries/LibWeb/CSS/ComputedValues.cppLibraries/LibWeb/CSS/ComputedValues.hLibraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Layout/LayoutRustBridge.hLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Rust/build.rsLibraries/LibWeb/Rust/src/css/computed_value_types.rsLibraries/LibWeb/Rust/src/css/computed_values.rsLibraries/LibWeb/Rust/src/css/retained_fly_string.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/grid_formatting_context.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/style_facts.rsLibraries/LibWeb/Rust/src/layout/svg_formatting_context.rs
💤 Files with no reviewable changes (3)
- Libraries/LibWeb/Rust/src/layout/formatting_context.rs
- Libraries/LibWeb/Rust/src/layout/layout_state.rs
- Libraries/LibWeb/Layout/LayoutRustBridge.h
Comment on lines
+161
to
+166
| auto grid_style_value_or_initial = [&](ComputedValuesFFI::ComputedStyleValueHandle const& handle) -> NonnullRefPtr<StyleValue const> { | ||
| static_assert(sizeof(RustStyleValueHandle) == sizeof(handle)); | ||
| if (auto value = style_value_from_handle(property_id, reinterpret_cast<RustStyleValueHandle const&>(handle))) | ||
| return value.release_nonnull(); | ||
| return property_initial_value(property_id); | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect RustStyleValueHandle and ComputedStyleValueHandle declarations.
set -euo pipefail
rg -n -C10 'RustStyleValueHandle' --glob '*.h' | head -80
rg -n -C6 'struct ComputedStyleValueHandle' --glob '*.h' --glob '*.rs'Repository: LadybirdBrowser/ladybird
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Changed files summary:"
git diff --stat || true
echo
echo "Target file context:"
sed -n '130,185p' Libraries/LibWeb/CSS/ComputedProperties.cpp
echo
echo "Find ComputedStyleValueHandle definitions/mentions:"
rg -n -C8 'ComputedStyleValueHandle|computedStyle.*Value|StyleValueData|RustStyleValueHandle|style_value_from_handle|property_initial_value' --glob '*.h' --glob '*.cpp' --glob '*.rs' | head -240Repository: LadybirdBrowser/ladybird
Length of output: 3563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files containing handles:"
rg -n -C8 'ComputedStyleValueHandle|RustStyleValueHandle|struct RustStyleValueHandle|struct ComputedStyleValueHandle|typedef.*StyleValueHandle|using.*StyleValueHandle|StyleValueData' Libraries --glob '*.{h,hpp,cpp,rs}' || true
echo
echo "List LibWeb FFI header files:"
fd -i 'Computed|Ffi|FFI|Style.*Value|StyleValue' Libraries/LibWeb -e h -e cpp -e rs | head -120
echo
echo "Search in all tracked files:"
rg -n -C8 'ComputedStyleValueHandle|RustStyleValueHandle|struct RustStyleValueHandle|struct ComputedStyleValueHandle|StyleValueData' . --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' || trueRepository: LadybirdBrowser/ladybird
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Definitions of ComputedStyleValueHandle:"
rg -n 'struct ComputedStyleValueHandle|class ComputedStyleValueHandle|typedef.*ComputedStyleValueHandle|using ComputedStyleValueHandle' --glob '*.h' --glob '*.hpp' --glob '*.hh' | head -80
echo
echo "Definitions of RustStyleValueHandle:"
rg -n 'struct RustStyleValueHandle|class RustStyleValueHandle|typedef.*RustStyleValueHandle|using RustStyleValueHandle' --glob '*.h' --glob '*.hpp' --glob '*.hh' --glob '*.rs' | head -80
echo
echo "StyleValueFFI namespaces and retain function definitions:"
rg -n -C8 'namespace StyleValueFFI|struct StyleValueHandle|struct RustStyleValueHandle|RetainedStyle|rust_style_value_retain|compute_style_value' --glob '*.h' --glob '*.hpp' --glob '*.hh' --glob '*.rs' Libraries Series | head -240Repository: LadybirdBrowser/ladybird
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Exact file:"
ls -l Libraries/LibWeb/CSS/ComputedProperties.cpp
echo
echo "Top includes:"
sed -n '1,60p' Libraries/LibWeb/CSS/ComputedProperties.cpp
echo
echo "Near target includes/line numbers around 161:"
sed -n '30,70p' Libraries/LibWeb/CSS/ComputedProperties.cpp
sed -n '240,290p' Libraries/LibWeb/CSS/ComputedProperties.cpp
echo
echo "Definitions in exact cpp include headers only:"
while IFS= read -r line; do
file=$(printf '%s' "$line" | sed 's#.*include <\([^>]*\)>.*/include <\([^>]*\)>\(.*\)/.*#\2#')
[ -n "$file" ] && echo "--- $file"
done < <(sed -n '1,70p' Libraries/LibWeb/CSS/ComputedProperties.cpp)
echo
echo "All definitions by exact name, no file limit:"
rg -n --fixed-strings 'ComputedStyleValueHandle' -g '*.h' -g '*.hpp' -g '*.hh' -g '*.rs' | head -200
rg -n --fixed-strings 'RustStyleValueHandle' -g '*.h' -g '*.hpp' -g '*.hh' -g '*.rs' | head -200Repository: LadybirdBrowser/ladybird
Length of output: 8886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Includes containing ComputedValuesFFI or StyleValue FFIs:"
rg -n --fixed-strings 'ComputedValuesFFI|StyleValuesFFI|rust_style_value_retain|struct RustStyleValueHandle|struct ComputedStyleValueHandle|class RustStyleValueHandle|class ComputedStyleValueHandle|StyleValueHandle' Libraries/LibWeb --glob '*.h' --glob '*.hpp' --glob '*.cpp' | head -240
echo
echo "Near struct definitions if present:"
rg -n -C6 --fixed-strings 'struct RustStyleValueHandle;|struct ComputedStyleValueHandle;' Libraries/LibWeb/Libraries/LibWeb --glob '*.h' --glob '*.hpp' --glob '*.cpp' || true
echo
echo "All structs in CSS-related headers with these names:"
rg -n -C6 '^(struct|class) .*Handle' Libraries/LibWeb/CSS --glob '*.h' --glob '*.hpp' | grep -E 'Handle|Computed|StyleValue|FFI' -C3 | head -260Repository: LadybirdBrowser/ladybird
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find files including ComputedProperties.h:"
rg -n --fixed-strings '<LibWeb/CSS/ComputedProperties.h>' --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' | head -80
echo
echo "Find ComputedProperties.h declaration:"
fd -i 'ComputedProperties.h' . -e h -e hpp | while read -r f; do echo "### $f"; sed -n '1,220p' "$f"; done
echo
echo "Any RustStyleValueHandle mentions with exact spacing:"
rg -n 'RustStyleValueHandle|ComputedStyleValueHandle' --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' . | head -200Repository: LadybirdBrowser/ladybird
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status:"
git status --short
echo
echo "Tracked files named ComputedProperties:*:"
git ls-files | grep -i 'ComputedProperties' || true
echo
echo "Target file with line numbers around callback:"
sed -n '150,170p' Libraries/LibWeb/CSS/ComputedProperties.cpp
echo
echo "Search all tracked files for exact handle names:"
git ls-files -z | xargs -0 rg -n --fixed-strings 'RustStyleValueHandle' --glob '*.{cpp,c,h,cc,cxx,hpp,rs}' | head -200
git ls-files -z | xargs -0 rg -n --fixed-strings 'ComputedStyleValueHandle' --glob '*.{cpp,c,h,cc,cxx,hpp,rs}' | head -200Repository: LadybirdBrowser/ladybird
Length of output: 6004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ComputedValues.h FFI declarations:"
sed -n '1350,1555p' Libraries/LibWeb/CSS/ComputedValues.h
echo
echo "StyleValue.h declarations:"
sed -n '28,38p' Libraries/LibWeb/CSS/StyleValues/StyleValue.h
sed -n '200,220p' Libraries/LibWeb/CSS/StyleValues/StyleValue.h
echo
echo "RustStyleValueHandle:"
cat -n Libraries/LibWeb/CSS/StyleValues/RustStyleValueHandle.h | sed -n '1,80p'
echo
echo "ComputedProperties.h declarations:"
sed -n '1,220p' Libraries/LibWeb/CSS/ComputedProperties.h
echo
echo "ComputedValues.cpp related static asserts/body:"
sed -n '680,705p' Libraries/LibWeb/CSS/ComputedValues.cpp
sed -n '80,105p' Libraries/LibWeb/CSS/ComputedProperties.cppRepository: LadybirdBrowser/ladybird
Length of output: 31158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find ComputedValuesFFI declarations:"
rg -n -C10 --fixed-strings 'struct ComputedValuesFFI|class ComputedValuesFFI|namespace ComputedValuesFFI|ComputedStyleValueHandle' --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' Libraries Series . --max-count 120 | head -320
echo
echo "Search all files for struct ComputedStyleValueHandle literal:"
rg -n --fixed-strings 'struct ComputedStyleValueHandle' . --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' || true
echo
echo "Show FFI header include candidates:"
rg -n '`#include` <LibWeb/.*Rust.*FFI|`#include` <.*Computed.*FFI|`#include` <.*Ffi|`#include` <.*FFI' --glob '*.{h,hpp,cpp,c,cc,cxx,rs}' Libraries/LibWeb | head -120Repository: LadybirdBrowser/ladybird
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find ComputedValuesFFI declarations:"
rg -n -C10 --fixed-strings 'struct ComputedValuesFFI|class ComputedValuesFFI|namespace ComputedValuesFFI|ComputedStyleValueHandle' --glob '*.h' --glob '*.hpp' --glob '*.hxx' --glob '*.hh' --glob '*.cpp' --glob '*.c' --glob '*.cc' --glob '*.cxx' --glob '*.rs' Libraries . | head -260
echo
echo "Search all files for struct ComputedStyleValueHandle literal:"
rg -n --fixed-strings 'struct ComputedStyleValueHandle' . --glob '*.h' --glob '*.hpp' --glob '*.hxx' --glob '*.hh' --glob '*.cpp' --glob '*.c' --glob '*.cc' --glob '*.cxx' --glob '*.rs' || true
echo
echo "Show FFI header include candidates:"
rg -n '`#include` <LibWeb/.*Rust.*FFI|`#include` <.*Computed.*FFI|`#include` <.*Ffi|`#include` <.*FFI' --glob '*.h' --glob '*.hpp' --glob '*.hxx' --glob '*.hh' --glob '*.cpp' --glob '*.c' --glob '*.cc' --glob '*.cxx' --glob '*.rs' Libraries/LibWeb | head -120Repository: LadybirdBrowser/ladybird
Length of output: 199
Avoid reinterpreting the FFI handle reference.
This code casts between two unrelated handle types based only on sizeof, while RustStyleValueHandle is a thin wrapper around StyleValueFFI::StyleValueData const*. Build a RustStyleValueHandle directly from handle.pointer/handle.data() before calling style_value_from_handle, or verify that COMPUTED_STYLE_VALUE_HANDLE is truly allocation-compatible.
🤖 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/CSS/ComputedProperties.cpp` around lines 161 - 166, Update
the grid_style_value_or_initial lambda to avoid reinterpret_casting the
ComputedStyleValueHandle reference; construct a RustStyleValueHandle from
handle.pointer or handle.data() and pass that object to style_value_from_handle.
Retain the existing initial-value fallback when no style value is returned, and
only rely on COMPUTED_STYLE_VALUE_HANDLE allocation compatibility if it is
explicitly verified.
Comment on lines
+3164
to
3176
| void reset_grid_placements_to_auto() | ||
| { | ||
| if (m_values.m_noninherited.grid->grid_column_end == value) | ||
| return; | ||
| m_values.m_noninherited.grid.access().grid_column_end = move(value); | ||
| } | ||
| void set_grid_column_start(GridTrackPlacement value) | ||
| { | ||
| if (m_values.m_noninherited.grid->grid_column_start == value) | ||
| return; | ||
| m_values.m_noninherited.grid.access().grid_column_start = move(value); | ||
| } | ||
| void set_grid_row_end(GridTrackPlacement value) | ||
| { | ||
| if (m_values.m_noninherited.grid->grid_row_end == value) | ||
| return; | ||
| m_values.m_noninherited.grid.access().grid_row_end = move(value); | ||
| } | ||
| void set_grid_row_start(GridTrackPlacement value) | ||
| { | ||
| if (m_values.m_noninherited.grid->grid_row_start == value) | ||
| // Every producer writes auto placements in canonical form, so a kind | ||
| // check alone detects the already-auto case without cloning. | ||
| auto placement_is_auto = [](ComputedValuesFFI::ComputedGridPlacement const& placement) { | ||
| return placement.kind == to_underlying(ComputedValuesFFI::ComputedGridPlacementKind::Auto); | ||
| }; | ||
| auto const& grid = *m_values.m_noninherited.grid; | ||
| if (placement_is_auto(grid.column_start) && placement_is_auto(grid.column_end) | ||
| && placement_is_auto(grid.row_start) && placement_is_auto(grid.row_end)) | ||
| return; | ||
| m_values.m_noninherited.grid.access().grid_row_start = move(value); | ||
| ComputedValuesFFI::rust_grid_values_reset_placements_to_auto(&m_values.m_noninherited.grid.access()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The grid payload has two competing conventions for the placement and template style-value handles. build_grid_group_payload in Libraries/LibWeb/CSS/ComputedValues.cpp (Lines 1136-1149) retains all nine grid_*_style_value handles unconditionally, while GridValues::initial and rust_grid_values_reset_placements_to_auto represent the same values with ComputedStyleValueHandle::empty(). ComputedStyleValueHandle equality treats an empty handle and a retained handle as unequal, so payloads that are semantically identical compare unequal.
Libraries/LibWeb/CSS/ComputedValues.h#L3164-L3176: extend the already-auto guard to also require empty placement handles, or make the producer omit the handle for anautoplacement.Libraries/LibWeb/Rust/src/css/computed_values.rs#L336-L395: aligninitial()with the producer so the shared-default comparison inrust_build_grid_group(Line 1952) can succeed, or document that the comparison is unreachable and theGROUP_FIELD_REQUIRE_INITIAL_VALUEdescriptors own the all-initial case.
📍 Affects 2 files
Libraries/LibWeb/CSS/ComputedValues.h#L3164-L3176(this comment)Libraries/LibWeb/Rust/src/css/computed_values.rs#L336-L395
🤖 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/CSS/ComputedValues.h` around lines 3164 - 3176, Use empty
style-value handles as the canonical representation for auto grid placements:
update reset_grid_placements_to_auto in Libraries/LibWeb/CSS/ComputedValues.h
lines 3164-3176 to require both auto kinds and empty placement handles before
returning, and update build_grid_group_payload to omit handles for auto
placements. Libraries/LibWeb/Rust/src/css/computed_values.rs lines 336-395
requires no direct change because GridValues::initial already uses empty
handles; preserve that convention so rust_build_grid_group can compare against
the shared initial value.
Comment on lines
+328
to
+351
| #[repr(C)] | ||
| pub struct GridValues { | ||
| pub names: crate::css::retained_fly_string::RetainedUtf16FlyStringList, | ||
| pub name_indices: RetainedGridNameIndexList, | ||
| pub entries: RetainedGridTrackEntryList, | ||
| pub areas: RetainedGridAreaList, | ||
| pub template_columns: ComputedGridTrackList, | ||
| pub template_rows: ComputedGridTrackList, | ||
| pub auto_columns: ComputedGridTrackList, | ||
| pub auto_rows: ComputedGridTrackList, | ||
| pub column_start: ComputedGridPlacement, | ||
| pub column_end: ComputedGridPlacement, | ||
| pub row_start: ComputedGridPlacement, | ||
| pub row_end: ComputedGridPlacement, | ||
| pub grid_template_columns_style_value: ComputedStyleValueHandle, | ||
| pub grid_template_rows_style_value: ComputedStyleValueHandle, | ||
| pub grid_auto_columns_style_value: ComputedStyleValueHandle, | ||
| pub grid_auto_rows_style_value: ComputedStyleValueHandle, | ||
| pub grid_template_areas_style_value: ComputedStyleValueHandle, | ||
| pub grid_column_start_style_value: ComputedStyleValueHandle, | ||
| pub grid_column_end_style_value: ComputedStyleValueHandle, | ||
| pub grid_row_start_style_value: ComputedStyleValueHandle, | ||
| pub grid_row_end_style_value: ComputedStyleValueHandle, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the generated C++ mirror of GridValues and the generation rules.
set -euo pipefail
fd -t f 'build.rs' Libraries/LibWeb/Rust --exec cat -n
rg -n -C6 'struct GridValues' --glob '*.h' --glob '*.hpp' --glob '*.toml' --glob '*.rs'
rg -n 'GridValues' Libraries/LibWeb/CSS/ComputedValues.cppRepository: LadybirdBrowser/ladybird
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate GridValues references and generated/computed value files =="
rg -n 'ComputedValuesFFI::GridValues|ComputeValues::GridValues|GridValues|struct GridValues' Libraries/Meta Libraries/LibWeb --glob '*.cpp' --glob '*.h' --glob '*.hpp' --glob '*.rs' --glob '*.toml' --glob 'Generated*.h' 2>/dev/null || true
echo
echo "== Build output locations =="
fd -t f 'ComputedValuesRustFFI.h|StyleStructRef.h|ComputedValues.h|ComputedValues.cpp' Libraries/LibWeb . 2>/dev/null || true
echo
echo "== ComputedValues.cpp relevant section =="
if [ -f Libraries/LibWeb/CSS/ComputedValues.cpp ]; then
rg -n -C 12 'GridValues|StyleStructRef|sizeof|alignof' Libraries/LibWeb/CSS/ComputedValues.cpp || true
fi
echo
echo "== Generated FFI header snippets if present =="
for f in $(fd -t f 'ComputedValuesRustFFI.h' . 2>/dev/null); do
echo "--- $f ---"
rg -n -C 4 'struct GridValues|struct StyleGroupVTable|GRID_NO_INDEX|ComputedGridTrackEntryKind' "$f" || true
doneRepository: LadybirdBrowser/ladybird
Length of output: 28427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ComputedValues.h GridValues and macro section =="
sed -n '880,925p' Libraries/LibWeb/CSS/ComputedValues.h
sed -n '1760,1800p' Libraries/LibWeb/CSS/ComputedValues.h
sed -n '3145,3170p' Libraries/LibWeb/CSS/ComputedValues.h
echo
echo "== GridValues declaration in Rust and lifecycle/assertions =="
sed -n '320,370p' Libraries/LibWeb/Rust/src/css/computed_value_types.rs
sed -n '500,565p' Libraries/LibWeb/Rust/src/css/computed_values.rs
echo
echo "== Static assertions around ComputedValuesFFI mirrors =="
sed -n '670,705p' Libraries/LibWeb/CSS/ComputedValues.cppRepository: LadybirdBrowser/ladybird
Length of output: 13499
Add parity checks for GridValues layout.
ComputedValues.h makes ComputedValues::GridValues inherit from ComputedValuesFFI::GridValues, and ComputedValues.cpp builds the grid payload with direct field names, but ComputedValues.cpp only compares sizeof/alignof for other Rust-native groups. Add a compile-assert for the grid group so any generated C++ ABI/literal layout mismatch fails the build.
🤖 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/css/computed_value_types.rs` around lines 328 -
351, Add a compile-time sizeof/alignof parity assertion for the Rust GridValues
type alongside the existing Rust-native computed-value group checks, using the
corresponding C++ ComputedValues::GridValues and ComputedValuesFFI::GridValues
symbols. Ensure the assertion validates the inherited C++ grid layout against
the Rust FFI layout.
Comment on lines
+336
to
+395
| impl GridValues { | ||
| fn auto_track_breadth() -> ComputedGridTrackBreadth { | ||
| ComputedGridTrackBreadth { | ||
| is_flex: false, | ||
| flex_factor: 0.0, | ||
| size: ComputedSize { | ||
| kind: ComputedSizeKind::Auto, | ||
| value: ComputedStyleValueHandle::empty(), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| fn auto_track_entry() -> ComputedGridTrackEntry { | ||
| ComputedGridTrackEntry { | ||
| kind: ComputedGridTrackEntryKind::TrackSize as u8, | ||
| next_sibling: GRID_NO_INDEX, | ||
| name_index_start: 0, | ||
| name_index_count: 0, | ||
| size: Self::auto_track_breadth(), | ||
| min_size: Self::auto_track_breadth(), | ||
| max_size: Self::auto_track_breadth(), | ||
| repeat_type: 0, | ||
| repeat_count: 0, | ||
| repeat_list: EMPTY_GRID_TRACK_LIST, | ||
| } | ||
| } | ||
|
|
||
| fn initial() -> Self { | ||
| Self { | ||
| names: RetainedUtf16FlyStringList::from_retained_strings(Vec::new()), | ||
| name_indices: RetainedGridNameIndexList::from_vec(Vec::new()), | ||
| entries: RetainedGridTrackEntryList::from_vec(vec![Self::auto_track_entry(), Self::auto_track_entry()]), | ||
| areas: RetainedGridAreaList::from_vec(Vec::new()), | ||
| template_columns: EMPTY_GRID_TRACK_LIST, | ||
| template_rows: EMPTY_GRID_TRACK_LIST, | ||
| auto_columns: ComputedGridTrackList { | ||
| is_subgrid: false, | ||
| preserves_line_name_sets: false, | ||
| first_entry: 0, | ||
| }, | ||
| auto_rows: ComputedGridTrackList { | ||
| is_subgrid: false, | ||
| preserves_line_name_sets: false, | ||
| first_entry: 1, | ||
| }, | ||
| column_start: AUTO_GRID_PLACEMENT, | ||
| column_end: AUTO_GRID_PLACEMENT, | ||
| row_start: AUTO_GRID_PLACEMENT, | ||
| row_end: AUTO_GRID_PLACEMENT, | ||
| grid_template_columns_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_template_rows_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_auto_columns_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_auto_rows_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_template_areas_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_column_start_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_column_end_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_row_start_style_value: ComputedStyleValueHandle::empty(), | ||
| grid_row_end_style_value: ComputedStyleValueHandle::empty(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
initial() leaves the nine style-value handles empty, so the default-payload comparison in rust_build_grid_group cannot succeed.
GridValues::initial sets every grid_*_style_value to ComputedStyleValueHandle::empty(). build_grid_group_payload in Libraries/LibWeb/CSS/ComputedValues.cpp (Lines 1136-1149) retains all nine handles unconditionally, including for initial values. ComputedStyleValueHandle's equality treats an empty handle and a retained handle as unequal, so built.eq(default_payload) at Line 1952 is always false and the shared-default branch is dead code.
The all-initial case is still covered by the GROUP_FIELD_REQUIRE_INITIAL_VALUE descriptors, so this is not a rendering defect. It shares one root cause with the reset_grid_placements_to_auto fast path in Libraries/LibWeb/CSS/ComputedValues.h; see the consolidated comment.
🤖 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/css/computed_values.rs` around lines 336 - 395,
Update build_grid_group_payload to preserve empty ComputedStyleValueHandle
values instead of unconditionally retaining all nine grid style-value handles,
allowing the payload to equal GridValues::initial(). Apply the same empty-handle
preservation to reset_grid_placements_to_auto, while leaving non-empty handles
retained normally.
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