LibWeb: Declare each style groups in Properties.json - #11034
Conversation
📝 WalkthroughWalkthroughCSS property definitions now declare style groups. The generator validates this metadata and emits group APIs. ChangesCSS style-group metadata and computed values
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PropertiesJSON
participant CSSPropertyGenerator
participant ComputedValues
participant RustStyleGroups
PropertiesJSON->>CSSPropertyGenerator: provide style-group metadata
CSSPropertyGenerator->>ComputedValues: generate dependency masks and longhand spans
ComputedValues->>ComputedValues: cache and validate group properties
ComputedValues->>RustStyleGroups: gather dynamic computed-style group values
Possibly related PRs
Suggested reviewers: 🚥 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 |
edc01d8 to
c9cc7c9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Documentation/CSSGeneratedFiles.md (1)
38-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify the
style-groupdependency contract.For arrays, document that the first element is the storage group returned by
longhands_in_style_group(), that array order is insignificant, and that every listed group is included instyle_group_dependency_mask(). Clarify the logical-alias mask rule: aliases withoutinherited-boximplicitly addinherited-boxbecause the mapping context determines which physical longhand applies.🤖 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 `@Documentation/CSSGeneratedFiles.md` at line 38, Update the style-group documentation entry to specify that array order is insignificant, its first element is the storage group returned by longhands_in_style_group(), and every listed group contributes to style_group_dependency_mask(). Also document that logical aliases without inherited-box implicitly include inherited-box because the mapping context determines the applicable physical longhand.
🤖 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.
Nitpick comments:
In `@Documentation/CSSGeneratedFiles.md`:
- Line 38: Update the style-group documentation entry to specify that array
order is insignificant, its first element is the storage group returned by
longhands_in_style_group(), and every listed group contributes to
style_group_dependency_mask(). Also document that logical aliases without
inherited-box implicitly include inherited-box because the mapping context
determines the applicable physical longhand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5be64e0-f8b0-4be4-9585-3c1a5766fb6d
📒 Files selected for processing (1)
Documentation/CSSGeneratedFiles.md
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
Declare the group for every physical longhand in `Properties.json` and generate a dependency mask function and a per group longhand list from the declarations. The generator rejects a longhand without a declaration and a shorthand or alias with one. A logical alias derives its mask from the target property's group and the inherited box group that stores the mapping context.
c9cc7c9 to
97a6e2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Properties.json`:
- Around line 4426-4435: Update the style-group for the shape-rendering property
definition to inherited-svg instead of svg-reset, preserving its existing
inherited classification and other metadata.
🪄 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: 6af0dcfd-e4b9-4940-a75e-c5ecd158a1f3
📒 Files selected for processing (2)
Libraries/LibWeb/CSS/ComputedValues.cppLibraries/LibWeb/CSS/Properties.json
🚧 Files skipped from review as they are similar to previous changes (1)
- Libraries/LibWeb/CSS/ComputedValues.cpp
|
Superseded by #11088, which also does this. |
Declare the group for every physical longhand in
Properties.jsonand generate a dependency mask function and a per group longhand list from the declarations. The generator rejects a longhand without a declaration and a shorthand or alias with one. A logical alias derives its mask from the target property's group and the inherited box group that stores the mapping context.My intention is to use the newly-introduced
style_group_dependency_mask()function to speed up style diffing in certain cases.