Move per-element image animation state to ElementAnimations [chromium/src : main]
Seokho Song (Gerrit)
Seokho Song voted and added 4 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +1 |
4 comments
Seokho SongI'm not sure I understand all of indirection going on in this patch. I can see that when we have a value that is not normal, we need to track some information for that element - but the purpose of using ElementAnimations was to have element associated data - it seems like we're storing this data on the bitmap though with a possible risk of never erasing it.
the informations are moved to ElementAnimation.
if (RuntimeEnabledFeatures::CSSImageAnimationEnabled()) {Seokho SongThese calls to clear don't need to be flag guarded since we would not add any image animations if the flag wasn't enabled so the clear would be a no-op.
Done
image_animation_map_.find(image_node_animation_info->node_id);Seokho SongWhy is image_animation_map_ a map of NodeId to ImageAnimationData on BitmapImage rather than storing ImageAnimationData on ElementAnimations? It seems like if we stored the animation data on ElementAnimations then there would be no risk of the data persisting when the element is detached or memory leaks by not removing entries.
Ahh!! Yes. That's a good idea.
We don't need to store it in `bitmap_image`. We can do this by taking the paint id and sync sequence from the Painter.
image_animation_map_.erase(animation_data_it);Seokho SongIt seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?
Yes. Here is some context:
1. The first issue was raised on [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7596743/comment/fae52653_f5d32ce5/) where we decided to defer the erase logic.
2. And the discussions continued across WIP CLs. CLs ([First](https://chromium-review.googlesource.com/c/chromium/src/+/7692355) -> [Second](https://chromium-review.googlesource.com/c/chromium/src/+/7727509)).
So the original plan was to remove the entry data (similar with the first one) after landing this CL.
However, since this CL now changed the approach to store the data in `ElementAnimation` rather than `bitmap_image`, a further patch is no longer needed.
Related details
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 6 comments![]( "Open in Gerrit")
image_animation_data_;Much like we have wrapped up the css_animations_ into a separate CssAnimations class, would you mind creating a css_image_animations_ to encapsulate additional data members and functionality? I'm recommending css be in the name since it's driven by a css property and has the same lifetime expectations as css animations.
You can directly expose the inner class similar to how we do for CssAnimations():
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/element_animations.h;l=96 so that you don't need forwarding methods.
element_animations->ClearPreviousImageAnimation();We should clear everything about the CssImageAnimations - i.e. the entire map - not just the previous value.
image_animation);Rather than updating element animations here, i think it might be cleaner to provide The new CssImageAnimations class which could implement an interface in third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.
image_node_animation_info->previous_image_animation;I don't understand why we need to use the previous image animation value. We should be storing in the ImageAnimationData what the previous sequence was.
It seems like this should be sufficient to determine what we need to do here, e.g. something like this:
```c++
switch (image_animation) {
kNormal:
// Set stored sequence id to 0.
kRunning:
// If !has_previous_animation_data, then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
// Otherwise, it will continue to use the sequence that was set.
kPaused:
// If !has_previous_animation_data, or current sequence is 0,
// then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
kStopped:
// Remove from map
}
```
previous_image_animation.has_value();Rather than using the previous image animation value, it seems like we should use whether there was an ImageAnimationData in the map on ElementAnimations.
image_animation_map_.erase(animation_data_it);Seokho SongIt seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?
Yes. Here is some context:
1. The first issue was raised on [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7596743/comment/fae52653_f5d32ce5/) where we decided to defer the erase logic.
2. And the discussions continued across WIP CLs. CLs ([First](https://chromium-review.googlesource.com/c/chromium/src/+/7692355) -> [Second](https://chromium-review.googlesource.com/c/chromium/src/+/7727509)).So the original plan was to remove the entry data (similar with the first one) after landing this CL.
However, since this CL now changed the approach to store the data in `ElementAnimation` rather than `bitmap_image`, a further patch is no longer needed.
- Florin Malita
- Seokho Song