Separate animation reset sequence for shared and own timeline [chromium/src : main]

0 views

Skip to first unread message

Florin Malita (Gerrit)

unread,

Jun 10, 2026, 2:58:51 PM (2 days ago) Jun 10

to Seokho Song, Olga Gerchikov, Menard, Alexis, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, Nate Chapin, android-bu...@system.gserviceaccount.com, blink-revie...@chromium.org, blink-revie...@chromium.org, kinuko...@chromium.org, gavinp...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, fserb...@chromium.org, drott+bl...@chromium.org, loading...@chromium.org

Attention needed from Seokho Song

Florin Malita voted and added 4 comments

Votes added by Florin Malita

Code-Review

4 comments

Patchset-level comments

File-level comment, Patchset 16 (Latest):

Florin Malita . resolved

LGTM with some minor suggestions.

File third_party/blink/renderer/core/layout/layout_image_resource.h

Line 60, Patchset 16 (Latest): void ResetAnimation(bool force = false);

Florin Malita . unresolved

nit: can we make this more explicit? Maybe add an `enum class Timeline { kAll, kSharedOnly }`.

File third_party/blink/renderer/platform/graphics/bitmap_image.cc

Line 437, Patchset 16 (Latest):


    has_image_animation_data = image_animation_data.has_value();

    if (has_image_animation_data) {

Florin Malita . unresolved

Looks like `has_image_animation_data` doesn't get much use. I think we could get rid of it and test `image_animation_data` directly:

```
  if (image_animation_data) {
    ...
```

File third_party/blink/web_tests/wpt_internal/css/css-image-animation/image-animation-shared-reset-preserves-own.html

Line 34, Patchset 16 (Latest): waitForFrameTime(120).then(() => {

Florin Malita . unresolved

I'm not that familiar with WPT, but synchronous waits are generally frowned upon in our web tests. I assume there are no better options for WPT?

Open in Gerrit

Related details

Attention is currently required from:

Seokho Song

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Seokho Song (Gerrit)

unread,

Jun 11, 2026, 1:19:10 AM (yesterday) Jun 11

to Florin Malita, Olga Gerchikov, Menard, Alexis, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, Nate Chapin, android-bu...@system.gserviceaccount.com, blink-revie...@chromium.org, blink-revie...@chromium.org, kinuko...@chromium.org, gavinp...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, fmalit...@chromium.org, zol...@webkit.org, fserb...@chromium.org, drott+bl...@chromium.org, loading...@chromium.org

Attention needed from Florin Malita

Seokho Song added 4 comments

Patchset-level comments

File-level comment, Patchset 17 (Latest):

Seokho Song . resolved

While on rebase, I resolve merge conflict and small changes made. PTAL!

File third_party/blink/renderer/core/layout/layout_image_resource.h

Line 60, Patchset 16: void ResetAnimation(bool force = false);

Florin Malita . resolved

nit: can we make this more explicit? Maybe add an `enum class Timeline { kAll, kSharedOnly }`.

Seokho Song

Great idea! Thanks. It seems `ResetAnimation()` in `LayoutImageResource` is likely to be removed after `SvgImageAnimationReset` landed. So I defined it in `ImageLoader` and reused it.

File third_party/blink/renderer/platform/graphics/bitmap_image.cc

Line 437, Patchset 16:


    has_image_animation_data = image_animation_data.has_value();

    if (has_image_animation_data) {

Florin Malita . resolved

Looks like `has_image_animation_data` doesn't get much use. I think we could get rid of it and test `image_animation_data` directly:
```
  if (image_animation_data) {
    ...
```

Seokho Song

Done

File third_party/blink/web_tests/wpt_internal/css/css-image-animation/image-animation-shared-reset-preserves-own.html

Line 34, Patchset 16: waitForFrameTime(120).then(() => {

Florin Malita . resolved

I'm not that familiar with WPT, but synchronous waits are generally frowned upon in our web tests. I assume there are no better options for WPT?

Seokho Song

Since we don't have there's no event or test hook that observes "the animation reached frame 2", I think there are no better option.

Open in Gerrit

Related details

Attention is currently required from:

Florin Malita

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages