This patch also updates the bug ID for a FIXME leftover from bug 1840478
to bug 1869476, since the same FIXME is added in D190758.
Co-authored-by: Frederic Wang <[email protected]>
Depends on D191322
Details
Details
- Reviewers
fredw zsun emilio - Group Reviewers
Restricted Project - Commits
- rMOZILLACENTRAL6f62586a139b: Bug 1846516 - [css-properties-values-api] Animate custom properties in a…
- Bugzilla Bug ID
- 1846516
Diff Detail
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Build Status
Buildable 609332 Build 707498: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| servo/components/style/properties/helpers/animated_properties.mako.rs | ||
|---|---|---|
| 183 | I don't understand why you need this. The this_tag != other_tag should still be sound without this. If you have a single CustomAnimatedValue struct rather than two members in the Custom tag then you can reuse the common codepath. | |
| servo/ports/geckolib/glue.rs | ||
| 656 | Why only for logical properties? I don't understand this. | |
| servo/components/style/properties/helpers/animated_properties.mako.rs | ||
|---|---|---|
| 183 | Agreed, (Custom(..), _) | (_, Custom(..)) branch removed. | |
| servo/ports/geckolib/glue.rs | ||
| 656 | It was crashing on value_map.get(&property) on the next line for custom properties, because Gecko_AnimationGetBaseStyle was casting aBaseStyles to nsRefPtrHashtable<nsUint32HashKey, StyleAnimationValue>*. Fixed the crash by instead casting aBaseStyles to const nsRefPtrHashtable<nsGenericHashKey<AnimatedPropertyID>, StyleAnimationValue> in D191322. | |
Comment Actions
The analysis task source-test-mozlint-mingw-cap failed, but we could not detect any defect.
Please check this task manually.
If you see a problem in this automated review, please report it here.
Comment Actions
The analysis task source-test-clang-format failed, but we could not detect any defect.
Please check this task manually.
If you see a problem in this automated review, please report it here.
| dom/animation/KeyframeEffect.cpp | ||
|---|---|---|
| 702–703 | Let's split this assert probably: MOZ_ASSERT(aProperty != eCSSPropertyExtra_variable, "Can't animate variables on compositor"); ... Or so. | |
| servo/components/style/properties/declaration_block.rs | ||
| 184 | Let's do: if !self.longhands.contains_all(&properties.longhands) {
return false;
}
if properties.custom.len() > self.custom.len() {
return false;
}
properties.custom.iter().all(|item| self.custom.contains(item))To avoid iterating over a gazillion props unnecessarily in some cases. | |
| servo/components/style/properties/helpers/animated_properties.mako.rs | ||
| 183 | Can you file a follow-up to merge the Custom(name, value) into a single Custom(<struct>) variant, so that we can use the same code-path as everything else? | |
| dom/animation/KeyframeEffect.cpp | ||
|---|---|---|
| 702–703 | Assert split. | |
| servo/components/style/properties/declaration_block.rs | ||
| 184 | Split conditions and added the length check condition. | |
| servo/components/style/properties/helpers/animated_properties.mako.rs | ||
| 183 | Filed bug 1869472 and added a TODO for it. | |
Comment Actions
Code analysis found 2 defects in diff 798355:
- 2 build errors found by clang-tidy
IMPORTANT: Found 2 defects (error level) that must be fixed before landing.
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 798355.
Comment Actions
2 defects closed compared to the previous diff 798355.
If you see a problem in this automated review, please report it here.