Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882873002:
Deflake paint/invalidation/svg/image-animation-with-zoom.html (Closed)
DescriptionDeflake paint/invalidation/svg/image-animation-with-zoom.html
It had been flaky after https://codereview.chromium.org/2872423002/
which defined PaintInvalidationReason::kImage for invalidations
caused by image change. The test sometimes produced "full" and
sometimes produced "image" as the invalidation reason of the image.
The flakiness might be because of arbitrary timing of image animation.
- Add image onload handler to start the test;
- Advance image animation twice to avoid flakiness.
- Convert it into a ref test.
- Increase the size of the image to avoid the layout from being
affected by different font sizes on different platforms.
BUG=722131
Review-Url: https://codereview.chromium.org/2882873002
Cr-Commit-Position: refs/heads/master@{#472165}
Committed: https://chromium.googlesource.com/chromium/src/+/b6e599fe4496e4f1aefdff8e73d1697f2d9da6b0
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : - #Patch Set 4 : - #
Total comments: 2
Patch Set 5 : - #Messages
Total messages: 34 (21 generated)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Deflake paint/invalidation/svg/image-animation-with-zoom.html BUG=722131 ========== to ========== Deflake paint/invalidation/svg/image-animation-with-zoom.html It had been flaky after https://codereview.chromium.org/2872423002/ which defined PaintInvalidationReason::kImage for invalidations caused by image change. The test sometimes produced "full" and sometimes produced "image" as the invalidation reason of the image. The flakiness might be because of arbitrary timing of image animation. - Add image onload handler to start the test; - Advance image animation twice to avoid flakiness. - Convert it into a ref test. - Increase the size of the image to avoid the layout from being affected by different font sizes on different platforms. BUG=722131 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html (right): https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: internals.advanceImageAnimation(targetImage); Without the second advanceImageAnimation, the test sometimes fail on release builds, e.g. https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... I'm not sure how advanceImageAnimation works for animated svg images. Ran 1000 times of the test locally with a release build and all succeeded.
There are a few other tests that use advanceImageAnimation for svg images. The basic idea is to advance the SMIL (aka <set> or <animate> in SVG) timeline by 1 frame: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/I.... I wonder if we should fix advanceImageAnimation instead of working around it in this test? Are the other tests that use it flaky too?
On 2017/05/15 22:18:48, pdr. wrote: > There are a few other tests that use advanceImageAnimation for svg images. The > basic idea is to advance the SMIL (aka <set> or <animate> in SVG) timeline by 1 > frame: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/I.... > I wonder if we should fix advanceImageAnimation instead of working around it in > this test? Are the other tests that use it flaky too? The SVG has default fill: red 0% { fill: pink }, 1% { fill: green }, 100% { fill: green } In which color should the first frame be, red or pink? When the test failed with one advanceImageAnimation, the actual result is in pink. I guess the first frame is in red, and one advanceImageAnimation advances to pink. However, before the test finishes after another requestAnimationFrame, the frame may or may not be advanced to green. Do you think this guess is reasonable? If the guess is correct, the current patch can ensure the final state be in green.
https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html (right): https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: internals.advanceImageAnimation(targetImage); On 2017/05/15 at 22:13:20, Xianzhu wrote: > Without the second advanceImageAnimation, the test sometimes fail on release builds, e.g. https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... I'm not sure how advanceImageAnimation works for animated svg images. Ran 1000 times of the test locally with a release build and all succeeded. I think I found the bug: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... We don't actually advance the css animation timeline in svg images, despite what the comment above Internals::advanceImageAnimation says. Taking away both advanceImageAnimation lines in this patch has no effect. I think this test is getting rid of the flakiness by ensuring the SVG image's internal css timeline gets advanced twice. I think this is a good approach. WDYT of removing the advanceImageAnimation lines and adding the following comment: // Two paints are needed to ensure the CSS timeline advances because we do not have // control over the SVGImage's CSS timeline with internals.advanceImageAnimation, // see TODO in Internals.h. And then add the following in Internals.h above advanceImageAnimation: // Advances an animated image. For BitmapImage (e.g., animated gifs) this // will advance to the next frame. For SVGImage, this will trigger an // animation to advance the SMIL timeline by one frame. // TODO(pdr): SVGImage does not advance the CSS timer so CSS animations // will be difficult to test reliably.
On 2017/05/16 01:39:50, pdr. wrote: > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > (right): > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > internals.advanceImageAnimation(targetImage); > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > Without the second advanceImageAnimation, the test sometimes fail on release > builds, e.g. > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > I'm not sure how advanceImageAnimation works for animated svg images. Ran 1000 > times of the test locally with a release build and all succeeded. > > I think I found the bug: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > We don't actually advance the css animation timeline in svg images, despite what > the comment above Internals::advanceImageAnimation says. Taking away both > advanceImageAnimation lines in this patch has no effect. > > I think this test is getting rid of the flakiness by ensuring the SVG image's > internal css timeline gets advanced twice. I think this is a good approach. > > WDYT of removing the advanceImageAnimation lines and adding the following > comment: > // Two paints are needed to ensure the CSS timeline advances because we do not > have > // control over the SVGImage's CSS timeline with > internals.advanceImageAnimation, > // see TODO in Internals.h. > > And then add the following in Internals.h above advanceImageAnimation: > // Advances an animated image. For BitmapImage (e.g., animated gifs) this > // will advance to the next frame. For SVGImage, this will trigger an > // animation to advance the SMIL timeline by one frame. > // TODO(pdr): SVGImage does not advance the CSS timer so CSS animations > // will be difficult to test reliably. I tried that but it didn't work in my environment. Without the advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, the actual result is in pink. It seems that advanceImageAnimation still works for SVG images in some ways.
On 2017/05/16 at 16:16:57, wangxianzhu wrote: > On 2017/05/16 01:39:50, pdr. wrote: > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > File > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > (right): > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > > internals.advanceImageAnimation(targetImage); > > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > > Without the second advanceImageAnimation, the test sometimes fail on release > > builds, e.g. > > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > > I'm not sure how advanceImageAnimation works for animated svg images. Ran 1000 > > times of the test locally with a release build and all succeeded. > > > > I think I found the bug: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > > We don't actually advance the css animation timeline in svg images, despite what > > the comment above Internals::advanceImageAnimation says. Taking away both > > advanceImageAnimation lines in this patch has no effect. > > > > I think this test is getting rid of the flakiness by ensuring the SVG image's > > internal css timeline gets advanced twice. I think this is a good approach. > > > > WDYT of removing the advanceImageAnimation lines and adding the following > > comment: > > // Two paints are needed to ensure the CSS timeline advances because we do not > > have > > // control over the SVGImage's CSS timeline with > > internals.advanceImageAnimation, > > // see TODO in Internals.h. > > > > And then add the following in Internals.h above advanceImageAnimation: > > // Advances an animated image. For BitmapImage (e.g., animated gifs) this > > // will advance to the next frame. For SVGImage, this will trigger an > > // animation to advance the SMIL timeline by one frame. > > // TODO(pdr): SVGImage does not advance the CSS timer so CSS animations > > // will be difficult to test reliably. > > I tried that but it didn't work in my environment. Without the advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, the actual result is in pink. It seems that advanceImageAnimation still works for SVG images in some ways. Can you try again? I just double-checked on linux that removing the two advanceImageAnimation calls has no effect. I am using the following: $ rtd third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html --no-retry-failures --repeat-each=100 --num-retries=0 --run-singly
(to be clear: I patched in your patch then removed the two advanceImageAnimation calls)
On 2017/05/16 16:30:26, pdr. wrote: > On 2017/05/16 at 16:16:57, wangxianzhu wrote: > > On 2017/05/16 01:39:50, pdr. wrote: > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > File > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > (right): > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > > > internals.advanceImageAnimation(targetImage); > > > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > > > Without the second advanceImageAnimation, the test sometimes fail on > release > > > builds, e.g. > > > > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > > > I'm not sure how advanceImageAnimation works for animated svg images. Ran > 1000 > > > times of the test locally with a release build and all succeeded. > > > > > > I think I found the bug: > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > > > We don't actually advance the css animation timeline in svg images, despite > what > > > the comment above Internals::advanceImageAnimation says. Taking away both > > > advanceImageAnimation lines in this patch has no effect. > > > > > > I think this test is getting rid of the flakiness by ensuring the SVG > image's > > > internal css timeline gets advanced twice. I think this is a good approach. > > > > > > WDYT of removing the advanceImageAnimation lines and adding the following > > > comment: > > > // Two paints are needed to ensure the CSS timeline advances because we do > not > > > have > > > // control over the SVGImage's CSS timeline with > > > internals.advanceImageAnimation, > > > // see TODO in Internals.h. > > > > > > And then add the following in Internals.h above advanceImageAnimation: > > > // Advances an animated image. For BitmapImage (e.g., animated gifs) this > > > // will advance to the next frame. For SVGImage, this will trigger an > > > // animation to advance the SMIL timeline by one frame. > > > // TODO(pdr): SVGImage does not advance the CSS timer so CSS animations > > > // will be difficult to test reliably. > > > > I tried that but it didn't work in my environment. Without the > advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, the > actual result is in pink. It seems that advanceImageAnimation still works for > SVG images in some ways. > > Can you try again? I just double-checked on linux that removing the two > advanceImageAnimation calls has no effect. > > I am using the following: > $ rtd > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > --no-retry-failures --repeat-each=100 --num-retries=0 --run-singly I guess you used debug build which is slower and the timeline just advances naturally without advanceImageAnimation. Based on trybot results, the test with one advanceImageAnimation (instead of 2) passed on *_chromium_rel_ng bots (release builds with DCHECKs), but flakily failed on the bots for webkit-patch rebaseline-cl (release builds without DCHECKs).
On 2017/05/16 at 16:35:37, wangxianzhu wrote: > On 2017/05/16 16:30:26, pdr. wrote: > > On 2017/05/16 at 16:16:57, wangxianzhu wrote: > > > On 2017/05/16 01:39:50, pdr. wrote: > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > File > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > > (right): > > > > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > > > > internals.advanceImageAnimation(targetImage); > > > > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > > > > Without the second advanceImageAnimation, the test sometimes fail on > > release > > > > builds, e.g. > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > > > > I'm not sure how advanceImageAnimation works for animated svg images. Ran > > 1000 > > > > times of the test locally with a release build and all succeeded. > > > > > > > > I think I found the bug: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > > > > We don't actually advance the css animation timeline in svg images, despite > > what > > > > the comment above Internals::advanceImageAnimation says. Taking away both > > > > advanceImageAnimation lines in this patch has no effect. > > > > > > > > I think this test is getting rid of the flakiness by ensuring the SVG > > image's > > > > internal css timeline gets advanced twice. I think this is a good approach. > > > > > > > > WDYT of removing the advanceImageAnimation lines and adding the following > > > > comment: > > > > // Two paints are needed to ensure the CSS timeline advances because we do > > not > > > > have > > > > // control over the SVGImage's CSS timeline with > > > > internals.advanceImageAnimation, > > > > // see TODO in Internals.h. > > > > > > > > And then add the following in Internals.h above advanceImageAnimation: > > > > // Advances an animated image. For BitmapImage (e.g., animated gifs) this > > > > // will advance to the next frame. For SVGImage, this will trigger an > > > > // animation to advance the SMIL timeline by one frame. > > > > // TODO(pdr): SVGImage does not advance the CSS timer so CSS animations > > > > // will be difficult to test reliably. > > > > > > I tried that but it didn't work in my environment. Without the > > advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, the > > actual result is in pink. It seems that advanceImageAnimation still works for > > SVG images in some ways. > > > > Can you try again? I just double-checked on linux that removing the two > > advanceImageAnimation calls has no effect. > > > > I am using the following: > > $ rtd > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > --no-retry-failures --repeat-each=100 --num-retries=0 --run-singly > > I guess you used debug build which is slower and the timeline just advances naturally without advanceImageAnimation. Based on trybot results, the test with one advanceImageAnimation (instead of 2) passed on *_chromium_rel_ng bots (release builds with DCHECKs), but flakily failed on the bots for webkit-patch rebaseline-cl (release builds without DCHECKs). I wonder if advanceImageAnimation is just slowing the test a little bit in these cases? The code in advanceImageAnimation doesn't seem to actually advance the css timeline at all. Lets land this patch as-is and double-check that the flakiness dashboard doesn't show this test as being flaky. LGTM for this patch as-is.
On 2017/05/16 16:43:35, pdr. wrote: > On 2017/05/16 at 16:35:37, wangxianzhu wrote: > > On 2017/05/16 16:30:26, pdr. wrote: > > > On 2017/05/16 at 16:16:57, wangxianzhu wrote: > > > > On 2017/05/16 01:39:50, pdr. wrote: > > > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > > File > > > > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > > > > > internals.advanceImageAnimation(targetImage); > > > > > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > > > > > Without the second advanceImageAnimation, the test sometimes fail on > > > release > > > > > builds, e.g. > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > > > > > I'm not sure how advanceImageAnimation works for animated svg images. > Ran > > > 1000 > > > > > times of the test locally with a release build and all succeeded. > > > > > > > > > > I think I found the bug: > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > > > > > We don't actually advance the css animation timeline in svg images, > despite > > > what > > > > > the comment above Internals::advanceImageAnimation says. Taking away > both > > > > > advanceImageAnimation lines in this patch has no effect. > > > > > > > > > > I think this test is getting rid of the flakiness by ensuring the SVG > > > image's > > > > > internal css timeline gets advanced twice. I think this is a good > approach. > > > > > > > > > > WDYT of removing the advanceImageAnimation lines and adding the > following > > > > > comment: > > > > > // Two paints are needed to ensure the CSS timeline advances because we > do > > > not > > > > > have > > > > > // control over the SVGImage's CSS timeline with > > > > > internals.advanceImageAnimation, > > > > > // see TODO in Internals.h. > > > > > > > > > > And then add the following in Internals.h above advanceImageAnimation: > > > > > // Advances an animated image. For BitmapImage (e.g., animated gifs) > this > > > > > // will advance to the next frame. For SVGImage, this will trigger an > > > > > // animation to advance the SMIL timeline by one frame. > > > > > // TODO(pdr): SVGImage does not advance the CSS timer so CSS > animations > > > > > // will be difficult to test reliably. > > > > > > > > I tried that but it didn't work in my environment. Without the > > > advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, > the > > > actual result is in pink. It seems that advanceImageAnimation still works > for > > > SVG images in some ways. > > > > > > Can you try again? I just double-checked on linux that removing the two > > > advanceImageAnimation calls has no effect. > > > > > > I am using the following: > > > $ rtd > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > --no-retry-failures --repeat-each=100 --num-retries=0 --run-singly > > > > I guess you used debug build which is slower and the timeline just advances > naturally without advanceImageAnimation. Based on trybot results, the test with > one advanceImageAnimation (instead of 2) passed on *_chromium_rel_ng bots > (release builds with DCHECKs), but flakily failed on the bots for webkit-patch > rebaseline-cl (release builds without DCHECKs). > > I wonder if advanceImageAnimation is just slowing the test a little bit in these > cases? The code in advanceImageAnimation doesn't seem to actually advance the > css timeline at all. > Is it root_element->TimeContainer()->AdvanceFrameForTesting() (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph...) that makes advanceImageAnimation work for the test? It advances the timer for 0.025s which is bigger than the duration of the animation (0.01s). > Lets land this patch as-is and double-check that the flakiness dashboard doesn't > show this test as being flaky. LGTM for this patch as-is.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/16 at 16:58:31, wangxianzhu wrote: > On 2017/05/16 16:43:35, pdr. wrote: > > On 2017/05/16 at 16:35:37, wangxianzhu wrote: > > > On 2017/05/16 16:30:26, pdr. wrote: > > > > On 2017/05/16 at 16:16:57, wangxianzhu wrote: > > > > > On 2017/05/16 01:39:50, pdr. wrote: > > > > > > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > > > File > > > > > > > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > > > > (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2882873002/diff/60001/third_party/WebKit/Layo... > > > > > > > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html:26: > > > > > > internals.advanceImageAnimation(targetImage); > > > > > > On 2017/05/15 at 22:13:20, Xianzhu wrote: > > > > > > > Without the second advanceImageAnimation, the test sometimes fail on > > > > release > > > > > > builds, e.g. > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/win10_blink_rel/.... > > > > > > I'm not sure how advanceImageAnimation works for animated svg images. > > Ran > > > > 1000 > > > > > > times of the test locally with a release build and all succeeded. > > > > > > > > > > > > I think I found the bug: > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph... > > > > > > We don't actually advance the css animation timeline in svg images, > > despite > > > > what > > > > > > the comment above Internals::advanceImageAnimation says. Taking away > > both > > > > > > advanceImageAnimation lines in this patch has no effect. > > > > > > > > > > > > I think this test is getting rid of the flakiness by ensuring the SVG > > > > image's > > > > > > internal css timeline gets advanced twice. I think this is a good > > approach. > > > > > > > > > > > > WDYT of removing the advanceImageAnimation lines and adding the > > following > > > > > > comment: > > > > > > // Two paints are needed to ensure the CSS timeline advances because we > > do > > > > not > > > > > > have > > > > > > // control over the SVGImage's CSS timeline with > > > > > > internals.advanceImageAnimation, > > > > > > // see TODO in Internals.h. > > > > > > > > > > > > And then add the following in Internals.h above advanceImageAnimation: > > > > > > // Advances an animated image. For BitmapImage (e.g., animated gifs) > > this > > > > > > // will advance to the next frame. For SVGImage, this will trigger an > > > > > > // animation to advance the SMIL timeline by one frame. > > > > > > // TODO(pdr): SVGImage does not advance the CSS timer so CSS > > animations > > > > > > // will be difficult to test reliably. > > > > > > > > > > I tried that but it didn't work in my environment. Without the > > > > advanceImageAnimation, the test failed 90 times in 100 runs. When it failed, > > the > > > > actual result is in pink. It seems that advanceImageAnimation still works > > for > > > > SVG images in some ways. > > > > > > > > Can you try again? I just double-checked on linux that removing the two > > > > advanceImageAnimation calls has no effect. > > > > > > > > I am using the following: > > > > $ rtd > > > > > > third_party/WebKit/LayoutTests/paint/invalidation/svg/image-animation-with-zoom.html > > > > --no-retry-failures --repeat-each=100 --num-retries=0 --run-singly > > > > > > I guess you used debug build which is slower and the timeline just advances > > naturally without advanceImageAnimation. Based on trybot results, the test with > > one advanceImageAnimation (instead of 2) passed on *_chromium_rel_ng bots > > (release builds with DCHECKs), but flakily failed on the bots for webkit-patch > > rebaseline-cl (release builds without DCHECKs). > > > > I wonder if advanceImageAnimation is just slowing the test a little bit in these > > cases? The code in advanceImageAnimation doesn't seem to actually advance the > > css timeline at all. > > > > Is it root_element->TimeContainer()->AdvanceFrameForTesting() (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph...) that makes advanceImageAnimation work for the test? It advances the timer for 0.025s which is bigger than the duration of the animation (0.01s). > That's for the SMIL timeline only (aka <set> and <animate>) but not the CSS timeline.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494953916028280,
"parent_rev": "13ae67b02915295a69a6ceea4ba8c7552b4f29cc", "commit_rev":
"b6e599fe4496e4f1aefdff8e73d1697f2d9da6b0"}
Message was sent while issue was closed.
Description was changed from ========== Deflake paint/invalidation/svg/image-animation-with-zoom.html It had been flaky after https://codereview.chromium.org/2872423002/ which defined PaintInvalidationReason::kImage for invalidations caused by image change. The test sometimes produced "full" and sometimes produced "image" as the invalidation reason of the image. The flakiness might be because of arbitrary timing of image animation. - Add image onload handler to start the test; - Advance image animation twice to avoid flakiness. - Convert it into a ref test. - Increase the size of the image to avoid the layout from being affected by different font sizes on different platforms. BUG=722131 ========== to ========== Deflake paint/invalidation/svg/image-animation-with-zoom.html It had been flaky after https://codereview.chromium.org/2872423002/ which defined PaintInvalidationReason::kImage for invalidations caused by image change. The test sometimes produced "full" and sometimes produced "image" as the invalidation reason of the image. The flakiness might be because of arbitrary timing of image animation. - Add image onload handler to start the test; - Advance image animation twice to avoid flakiness. - Convert it into a ref test. - Increase the size of the image to avoid the layout from being affected by different font sizes on different platforms. BUG=722131 Review-Url: https://codereview.chromium.org/2882873002 Cr-Commit-Position: refs/heads/master@{#472165} Committed: https://chromium.googlesource.com/chromium/src/+/b6e599fe4496e4f1aefdff8e73d1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b6e599fe4496e4f1aefdff8e73d1... |
