Reset image animation when off from layout and prevent store normal [chromium/src : main]
Seokho Song (Gerrit)
Seokho Song voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita added 3 comments![]( "Open in Gerrit")
builder.SetPreviousImageAnimation(state.OldStyle()->ImageAnimation());Would this be problematic if style recalc is triggered by other properties?
E.g.
initial: normal, update: running -> prev normal, current running
(some other property update) -> prev running, current running
If we only care about state transitions, we should prolly check that the old style value is different before updating.
Also should we guard this on the runtime feature?
bool is_image_animation_shared_timeline_ = false;
bool has_previous_image_animation_ = false;I would prefer to keep the struct simple, and have the shared timeline deduction code close to the other image-animation logic in PaintImageForCurrentFrameWithInfo. Maybe just store a `std::optional<ImageAnimationEnum> previous_image_animation here`?
if (prev != image_animation) {
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal ||
(image_animation == ImageAnimationEnum::kRunning &&
prev == ImageAnimationEnum::kNormal);
} else {
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal;
}I think we can simplify this to just
```
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal ||
(image_animation == ImageAnimationEnum::kRunning &&
prev == ImageAnimationEnum::kNormal);
```
Related details
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 3 comments![]( "Open in Gerrit")
builder.SetPreviousImageAnimation(state.OldStyle()->ImageAnimation());Would this be problematic if style recalc is triggered by other properties?
E.g.
initial: normal, update: running -> prev normal, current running
(some other property update) -> prev running, current runningIf we only care about state transitions, we should prolly check that the old style value is different before updating.
Also should we guard this on the runtime feature?
First, the previous implementation was incorrect because this function is never called when cached. Therefore, I moved it to `RunUncacheableStyleAdjustment`.
Second, while setting the value itself might not be problematic from a behavioral perspective, I agree that we should add an explicit check.
I've also added the flag!
bool is_image_animation_shared_timeline_ = false;
bool has_previous_image_animation_ = false;I would prefer to keep the struct simple, and have the shared timeline deduction code close to the other image-animation logic in PaintImageForCurrentFrameWithInfo. Maybe just store a `std::optional<ImageAnimationEnum> previous_image_animation here`?
Ah yes. That's good idea.
if (prev != image_animation) {
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal ||
(image_animation == ImageAnimationEnum::kRunning &&
prev == ImageAnimationEnum::kNormal);
} else {
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal;
}I think we can simplify this to just
```
is_image_animation_shared_timeline_ =
image_animation == ImageAnimationEnum::kNormal ||
(image_animation == ImageAnimationEnum::kRunning &&
prev == ImageAnimationEnum::kNormal);
```
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
- Anders Hartvoll Ruud
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.It don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Related details
- Florin Malita
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.It don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Related details
- Florin Malita
- Rune Lillesveen
Florin Malita (Gerrit)
Florin Malita added 1 comment![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.Seokho SongIt don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Storing in `ElementRareData` makes sense to me.
I think Ian also suggested something similar at some point, and one of the issues I ran into was I could not easily locate a good place to update the value (no `StyleDidChange` equivalent for `Element`/`Node`). But I didn't look too hard, I'm sure there's a way.
Related details
- Rune Lillesveen
- Seokho Song
Rune Lillesveen (Gerrit)
Rune Lillesveen added 2 comments![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.Seokho SongIt don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment?
There will be an old style, but there might be intermediate style changes which are never passed on to the paint code. I've suggested a way this can be observed in the test you're adding.
img1.style.imageAnimation = 'paused';I think you would see the multiple style-change events issue with storing the old value on ComputedStyle if you add these two lines here:
```
getComputedStyle(img1).color;
img1.style.color = 'red';
```
which should set both the new and old value to 'paused' for the next paint.
Related details
- Seokho Song
Seokho Song (Gerrit)
Seokho Song voted and added 3 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +0 |
3 comments
PTAL at the high-level approach. I'll polish the details tomorrow...
`BitmapImage` via the `ImageNodeAnimationInfo` struct.Seokho SongIt don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Rune LillesveenAlso, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment?
There will be an old style, but there might be intermediate style changes which are never passed on to the paint code. I've suggested a way this can be observed in the test you're adding.
Ah got it. Thanks! I've changed the approach to update/store previous image animation.
img1.style.imageAnimation = 'paused';I think you would see the multiple style-change events issue with storing the old value on ComputedStyle if you add these two lines here:
```
getComputedStyle(img1).color;
img1.style.color = 'red';
```which should set both the new and old value to 'paused' for the next paint.
- Florin Malita
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |