Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
802143002:
AnimationPolicy setting is applied to SVG animation. (Closed)
|
Created:
6 years ago by je_julie(Not used) Modified:
5 years, 11 months ago CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, rwlbuis, kouhei+svg_chromium.org, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, Steve Block, pdr+svgwatchlist_chromium.org, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAnimationPolicy setting is applied to SVG animation.
AnimationPolicy is set through setting and animations from web pages
should follow that policy.
This patch makes SVG animation also follows animation policy from
setting.
BUG=3690
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187775
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Move code to SMILTimeContainer and add LayoutTests #
Total comments: 20
Patch Set 4 : apply new concept #
Total comments: 16
Patch Set 5 : Update code #
Total comments: 38
Patch Set 6 : Use enum #
Total comments: 20
Patch Set 7 : Update TC and remove unnecessary code #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 25 (3 generated)
je_julie.kim@samsung.com changed reviewers: + dmazzoni@chromium.org, pdr@chromium.org
Hi pdr and Dominic, This patch makes SVG animation also follows animation policy from setting. @pdr, Let me explain about animation policy. If you look into crbug.com/3690, you can see history for this issue. Animation Policy is started from https://codereview.chromium.org/754813002/. Generally, I’m following FF for animation Policy. You can see how to handle animation control in FF at [1] and FF history for Animation handing at [2]. I discussed with Dominic about how to expose animation policy to user and we decided to make some extension API for accessibility. CL for animation policy extension is working on https://codereview.chromium.org/785723002/ [1]http://www.ghacks.net/2014/01/23/block-animated-gifs-firefox-playing-automatically/ [2]http://kb.mozillazine.org/Animated_images Could you take a look at this patch?
fs@opera.com changed reviewers: + fs@opera.com
This is a too low-level place to be applying a policy like this. Doing it at the time container level would seem more appropriate. Some additional questions/notes: * The enum is named ImageAnimationPolicy, which implies that it would apply only to images. As such I'd have thought that it could/would be applied (only) to SVGImage (which derives from Image). There it should be possible to explicitly control the timeline as needed by the chosen policy. If the policy should indeed apply to documents as well, then maybe the "Image" prefix should be dropped? (I'd also assume that CSS animations would also be blocked.) * Repeating behavior is not well-defined for SMIL/SVG animations, so just "truncating" repeatCount will not help. For any non-trivial SMIL animation a looping animation would likely involve a sync-base loop (or similar). Finding those seems unnecessarily complex though, so maybe we could define "once" as "play for X seconds" here instead? (Or something similar.) * Could we have some tests for this? (The referenced CL didn't seem to have any, which seemed a bit odd to me, consider that this feels like enough of a "fringe feature" to be highly susceptible to breakage.) * When policy=='none', should the animations not be applied at all, or be "frozen" at t=0? I'm assuming the latter since that would be consistent with the raster image behavior. If the policy is supposed to apply to documents as well, then there might be additional question marks.
On 2014/12/15 12:11:14, fs wrote: > This is a too low-level place to be applying a policy like this. Doing it at the > time container level would seem more appropriate. Thanks for suggestion. I'll check time container. > Some additional questions/notes: > > * The enum is named ImageAnimationPolicy, which implies that it would apply > only to images. As such I'd have thought that it could/would be applied (only) > to SVGImage (which derives from Image). There it should be possible to > explicitly control the timeline as needed by the chosen policy. If the policy > should indeed apply to documents as well, then maybe the "Image" prefix should > be dropped? (I'd also assume that CSS animations would also be blocked.) Yes. you're right. At the first time, I referred to FF working [1]. FF has 'normal', 'once' and 'none'. It describes "determines how multi-frame GIF images are animated". So, I started to check gif. After that, I started to be wondering how to handle other animations in FF. I checked that FF also applies that policy to SVG as well except 'once'. When I set policy to once on FF, SVG animation is just working like 'normal'. > * Repeating behavior is not well-defined for SMIL/SVG animations, so just > "truncating" repeatCount will not help. For any non-trivial SMIL animation a > looping animation would likely involve a sync-base loop (or similar). Finding > those seems unnecessarily complex though, so maybe we could define "once" as > "play for X seconds" here instead? (Or something similar.) I'll check. > * Could we have some tests for this? (The referenced CL didn't seem to have > any, which seemed a bit odd to me, consider that this feels like enough of a > "fringe feature" to be highly susceptible to breakage.) [2] is CL for extension of animation policy under review and [3] is simple test extension. We can see its working with them. > * When policy=='none', should the animations not be applied at all, or be > "frozen" at t=0? I'm assuming the latter since that would be consistent with the > raster image behavior. If the policy is supposed to apply to documents as well, > then there might be additional question marks. As you know, documents can have many kinds of animations. We can control dom timer or something similar because it's too general. That's why FF decides to just apply gif, from Pausing animated images in [1]. Even if I uploaded this patch, I'm still not sure how many types animation policy covers. As to this issue, I try to hear various opinions. I sent mail to Dominic about that. fs, do you have any opinion? [1] http://kb.mozillazine.org/Animated_images [2] https://codereview.chromium.org/785723002/ [3] https://drive.google.com/folderview?id=0BzK6cv0DIOPGSDBKVVVCUlloZjQ&usp=sharing
If this is specifically for when svg is treated as an image then SVGImage::hasAnimations might be a more appropriate place to enforce the policy.
On 2014/12/15 12:35:54, je_julie wrote: > On 2014/12/15 12:11:14, fs wrote: > > This is a too low-level place to be applying a policy like this. Doing it at > the > > time container level would seem more appropriate. > Thanks for suggestion. I'll check time container. > > > Some additional questions/notes: > > > > * The enum is named ImageAnimationPolicy, which implies that it would apply > > only to images. As such I'd have thought that it could/would be applied (only) > > to SVGImage (which derives from Image). There it should be possible to > > explicitly control the timeline as needed by the chosen policy. If the policy > > should indeed apply to documents as well, then maybe the "Image" prefix should > > be dropped? (I'd also assume that CSS animations would also be blocked.) > Yes. you're right. > At the first time, I referred to FF working [1]. > FF has 'normal', 'once' and 'none'. > It describes "determines how multi-frame GIF images are animated". > So, I started to check gif. > After that, I started to be wondering how to handle other animations in FF. > I checked that FF also applies that policy to SVG as well except 'once'. > When I set policy to once on FF, SVG animation is just working like 'normal'. > > > * Repeating behavior is not well-defined for SMIL/SVG animations, so just > > "truncating" repeatCount will not help. For any non-trivial SMIL animation a > > looping animation would likely involve a sync-base loop (or similar). Finding > > those seems unnecessarily complex though, so maybe we could define "once" as > > "play for X seconds" here instead? (Or something similar.) > I'll check. If FF doesn't implement the 'once' policy for SVG animations, then that's probably a reasonable option as well. Using a "play for X seconds" mechanism might be an improvement, but I suppose that depends on the use-cases. > > * Could we have some tests for this? (The referenced CL didn't seem to have > > any, which seemed a bit odd to me, consider that this feels like enough of a > > "fringe feature" to be highly susceptible to breakage.) > [2] is CL for extension of animation policy under review > and [3] is simple test extension. > We can see its working with them. I was thinking more in terms of LayoutTests (which I think would be good to have.) > > * When policy=='none', should the animations not be applied at all, or be > > "frozen" at t=0? I'm assuming the latter since that would be consistent with > the > > raster image behavior. If the policy is supposed to apply to documents as > well, > > then there might be additional question marks. > As you know, documents can have many kinds of animations. > We can control dom timer or something similar because it's too general. > That's why FF decides to just apply gif, from Pausing animated images in [1]. > > Even if I uploaded this patch, I'm still not sure how many types animation > policy covers. > As to this issue, I try to hear various opinions. I sent mail to Dominic about > that. I think I would leave "document animations" alone, because it's more difficult to know what they're used for. I suppose it ties in with what the use-case for the policy is to begin with. (It's worth noting that CSS animations can run in SVGs in an <img> context, but in that isolated case I think it can be managed by exerting control over the animation clock.)
On 2014/12/15 16:27:44, fs wrote:
> On 2014/12/15 12:35:54, je_julie wrote:
> > On 2014/12/15 12:11:14, fs wrote:
> > > This is a too low-level place to be applying a policy like this. Doing it
at
> > the
> > > time container level would seem more appropriate.
> > Thanks for suggestion. I'll check time container.
> >
> > > Some additional questions/notes:
> > >
> > > * The enum is named ImageAnimationPolicy, which implies that it would
apply
> > > only to images. As such I'd have thought that it could/would be applied
> (only)
> > > to SVGImage (which derives from Image). There it should be possible to
> > > explicitly control the timeline as needed by the chosen policy. If the
> policy
> > > should indeed apply to documents as well, then maybe the "Image" prefix
> should
> > > be dropped? (I'd also assume that CSS animations would also be blocked.)
> > Yes. you're right.
> > At the first time, I referred to FF working [1].
> > FF has 'normal', 'once' and 'none'.
> > It describes "determines how multi-frame GIF images are animated".
> > So, I started to check gif.
> > After that, I started to be wondering how to handle other animations in FF.
> > I checked that FF also applies that policy to SVG as well except 'once'.
> > When I set policy to once on FF, SVG animation is just working like
'normal'.
> >
> > > * Repeating behavior is not well-defined for SMIL/SVG animations, so just
> > > "truncating" repeatCount will not help. For any non-trivial SMIL animation
a
> > > looping animation would likely involve a sync-base loop (or similar).
> Finding
> > > those seems unnecessarily complex though, so maybe we could define "once"
as
> > > "play for X seconds" here instead? (Or something similar.)
> > I'll check.
>
> If FF doesn't implement the 'once' policy for SVG animations, then that's
> probably a reasonable option as well. Using a "play for X seconds" mechanism
> might be an improvement, but I suppose that depends on the use-cases.
Now I'm thinking how to handle 'once' with SVG but I'm not sure about "play for
X seconds".
If people see svg animation with 'once' works for a few second and it doesn't
finish a loop,
don't they feel that it's like a bug?
> > > * Could we have some tests for this? (The referenced CL didn't seem to
have
> > > any, which seemed a bit odd to me, consider that this feels like enough of
a
> > > "fringe feature" to be highly susceptible to breakage.)
> > [2] is CL for extension of animation policy under review
> > and [3] is simple test extension.
> > We can see its working with them.
>
> I was thinking more in terms of LayoutTests (which I think would be good to
> have.)
animation policy is set by settings.
Is there a way to control setting on web page, html and js?
> > > * When policy=='none', should the animations not be applied at all, or be
> > > "frozen" at t=0? I'm assuming the latter since that would be consistent
with
> > the
> > > raster image behavior. If the policy is supposed to apply to documents as
> > well,
> > > then there might be additional question marks.
> > As you know, documents can have many kinds of animations.
> > We can control dom timer or something similar because it's too general.
> > That's why FF decides to just apply gif, from Pausing animated images in
[1].
> >
> > Even if I uploaded this patch, I'm still not sure how many types animation
> > policy covers.
> > As to this issue, I try to hear various opinions. I sent mail to Dominic
about
> > that.
>
> I think I would leave "document animations" alone, because it's more difficult
> to know what they're used for. I suppose it ties in with what the use-case for
> the policy is to begin with. (It's worth noting that CSS animations can run in
> SVGs in an <img> context, but in that isolated case I think it can be managed
by
> exerting control over the animation clock.)
Fore your information, when I checked FF with animation mode,
normal once none comment
CSS animation : O O O it ignores animation
mode.
Web animation : X X X Web Animation is not
supported yet.
requestAnimationFrame: O O O it ignores animation mode.
settimeout: O O O it ignores animation
mode.
animated GIF: O once X it follows animation
mode.
SVG animation: O O X it ignores 'once'.
About coverage animation policy for blink, I'd like to get many feedback as
possible.
WDYT?
On 2014/12/16 07:11:23, je_julie wrote: > On 2014/12/15 16:27:44, fs wrote: > > On 2014/12/15 12:35:54, je_julie wrote: > > > On 2014/12/15 12:11:14, fs wrote: > > > > This is a too low-level place to be applying a policy like this. Doing it > at > > > the > > > > time container level would seem more appropriate. > > > Thanks for suggestion. I'll check time container. > > > > > > > Some additional questions/notes: > > > > > > > > * The enum is named ImageAnimationPolicy, which implies that it would > apply > > > > only to images. As such I'd have thought that it could/would be applied > > (only) > > > > to SVGImage (which derives from Image). There it should be possible to > > > > explicitly control the timeline as needed by the chosen policy. If the > > policy > > > > should indeed apply to documents as well, then maybe the "Image" prefix > > should > > > > be dropped? (I'd also assume that CSS animations would also be blocked.) > > > Yes. you're right. > > > At the first time, I referred to FF working [1]. > > > FF has 'normal', 'once' and 'none'. > > > It describes "determines how multi-frame GIF images are animated". > > > So, I started to check gif. > > > After that, I started to be wondering how to handle other animations in FF. > > > I checked that FF also applies that policy to SVG as well except 'once'. > > > When I set policy to once on FF, SVG animation is just working like > 'normal'. > > > > > > > * Repeating behavior is not well-defined for SMIL/SVG animations, so just > > > > "truncating" repeatCount will not help. For any non-trivial SMIL animation > a > > > > looping animation would likely involve a sync-base loop (or similar). > > Finding > > > > those seems unnecessarily complex though, so maybe we could define "once" > as > > > > "play for X seconds" here instead? (Or something similar.) > > > I'll check. > > > > If FF doesn't implement the 'once' policy for SVG animations, then that's > > probably a reasonable option as well. Using a "play for X seconds" mechanism > > might be an improvement, but I suppose that depends on the use-cases. > > Now I'm thinking how to handle 'once' with SVG but I'm not sure about "play for > X seconds". > If people see svg animation with 'once' works for a few second and it doesn't > finish a loop, > don't they feel that it's like a bug? Yes, that's certainly a possibility. Another possibility is that they feel: "Yes! it finally stopped playing!" ;-) > > > > * Could we have some tests for this? (The referenced CL didn't seem to > have > > > > any, which seemed a bit odd to me, consider that this feels like enough of > a > > > > "fringe feature" to be highly susceptible to breakage.) > > > [2] is CL for extension of animation policy under review > > > and [3] is simple test extension. > > > We can see its working with them. > > > > I was thinking more in terms of LayoutTests (which I think would be good to > > have.) > animation policy is set by settings. > Is there a way to control setting on web page, html and js? internals.settings.setSomeSetting(...); should work in layout tests. (See core/testing/InternalSettings.* - most of it is autogenerated though.) > > > > * When policy=='none', should the animations not be applied at all, or be > > > > "frozen" at t=0? I'm assuming the latter since that would be consistent > with > > > the > > > > raster image behavior. If the policy is supposed to apply to documents as > > > well, > > > > then there might be additional question marks. > > > As you know, documents can have many kinds of animations. > > > We can control dom timer or something similar because it's too general. > > > That's why FF decides to just apply gif, from Pausing animated images in > [1]. > > > > > > Even if I uploaded this patch, I'm still not sure how many types animation > > > policy covers. > > > As to this issue, I try to hear various opinions. I sent mail to Dominic > about > > > that. > > > > I think I would leave "document animations" alone, because it's more difficult > > to know what they're used for. I suppose it ties in with what the use-case for > > the policy is to begin with. (It's worth noting that CSS animations can run in > > SVGs in an <img> context, but in that isolated case I think it can be managed > by > > exerting control over the animation clock.) > > Fore your information, when I checked FF with animation mode, > normal once none comment > CSS animation : O O O it ignores animation mode. > Web animation : X X X Web Animation is not supported yet. > requestAnimationFrame: O O O it ignores animation mode. > settimeout: O O O it ignores animation mode. > animated GIF: O once X it follows animation mode. > SVG animation: O O X it ignores 'once'. > > About coverage animation policy for blink, I'd like to get many feedback as > possible. > WDYT? Thanks for the research! I'd suggest the above as a starting point. We can always improve upon it later if can come up with ideas how to do that.
@Erik, Thanks for your suggestion. I've looked into SVGImage and SMILTimeContainer and I decided to add it to SMILTimeContainer. Pleas take a look. @fs, Thanks to your comment, I realized that I can add LayoutTest for this. I added Layout Test for 'once' and 'none' because 'allowed' is default. I also moved code to SMILTimeContainer. PTAL.
I'm missing tests for SVG-in-<img> (not sure that will pick up the correct settings etc.). There's also some potential concern wrt DOM manipulation of the timeline. https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/resources/animation-policy.svg (right): https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:7: <animate id="an1" attributeName="y" values="0; 50; 100" dur="2s" repeatCount="indefinite"/> I think it's better to use an animation where the animated value at time 0 is different from the base-value (y=0 in this case.) You could probably also use a <set> animation here to avoid the interpolation. https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/svg-animation-policy-none.html (right): https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-none.html:3: <head> You can drop <html> and <head></head> (the <script>s will end up there anyway.) https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-none.html:19: shouldBeCloseEnough("rect.y.animVal.value", "0"); Shouldn't it be exactly '0'? https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/svg-animation-policy-once.html (right): https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:3: <head> You can drop <html> and <head></head>. https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:47: </body> One </body> is usually enough ;-) https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:171: ImageAnimationPolicy animationPolicy = document().settings()->imageAnimationPolicy(); I'd suggest you move these two into a helper function. https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:173: return; I'm a bit worried that this will leave us in an inconsistent state. We'll also need to guard ourselves from these DOM methods: void pauseAnimations(); void unpauseAnimations(); in this case. Should also test the behavior of the other (three IIRC) methods on SVGSVGElement (animationsPaused, getCurrentTime, setCurrentTime) to see they work in a reasonable way/return reasonable values. ). https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:177: m_animationPolicyOnceTimer.startOneShot(animationPolicyOnceDuration, FROM_HERE); I suppose this is one way to implement this. It has the downside though, that if the timeline is paused at this point it might still be possible to seek to 3 seconds. Not a big deal, and my other suggestions might help rectify that. https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:243: time = SMILTime(animationPolicyOnceDuration); Should be possible to simply do: time = std::min(time, SMILTime(animationPolicyOnceDuration)); here. https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:322: cancelAnimationFrame(); I don't think is enough (see above about DOM methods). I'd suggest adding a new method (freeze() maybe?) that pauses the timeline and then sets a flag to 'freeze' it. This flag would then be checked in appropriate places (and should probably also be set for the 'none' case.) BTW, to what degree/when can the policy value change? At any time?
@fs,
I considered how to handle animation policy with SVG for a few days
and also heard some opinions from my colleagues.
I decided that I suggest new policy different from the previous code.
Here is new policy I suggested.
"none" is not animated and "allowed" is a normal behaviour. These are simple.
As to "once",
basically whenever DOM APIs set animation,
Start animation, set timer for policy timer and stop animation if it's fired.
For instance,
if unpauseAnimations() is called,
the animation is restarted and set the policy timer again,
even if the policy timer is fired and the animation is frozen.
If setCurrentTime() is called,
the animation is restarted at the time and set policy timer again.
It's from the assumption that unpauseAnimations() and setCurrentTime()
includes content author's intention to show animation and
it's more reasonable to show animated SVG for a while again.
*Animation Policy for SVG
1) Allowed : Same as non-setting animation policy.
2) Once (*AT: AnimationPolicyTimer)
| before frozen | after frozen
------------------------------------------------------------------
pause | Pause & cancel AT | Pause
resume | Resume & start AT | Resume & start AT
isPaused | true if paused | true if paused or frozen
setElapsed|PausedState:setElapsed |PausedState:setElapsed
|UnpausedState:setElapsed|UnpausedState:setElapsed
| & start AT| & resume & start AT
elapsed | returns current time | returns current time
3) None
pause : not allowed.
resume : not allowed.
isPaused : always paused.
setElapsed: not allowed.
elapsed : always returns 0.
WDYT?
If you come back, please take a look.
Thanks.
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/resources/animation-policy.svg (right):
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/resources/animation-policy.svg:7: <animate id="an1"
attributeName="y" values="0; 50; 100" dur="2s" repeatCount="indefinite"/>
On 2014/12/18 16:45:17, fs (OoO) wrote:
> I think it's better to use an animation where the animated value at time 0 is
> different from the base-value (y=0 in this case.) You could probably also use
a
> <set> animation here to avoid the interpolation.
I updated TC. PTAL.
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/svg-animation-policy-none.html (right):
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-animation-policy-none.html:3: <head>
On 2014/12/18 16:45:17, fs (OoO) wrote:
> You can drop <html> and <head></head> (the <script>s will end up there
anyway.)
Done.
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-animation-policy-none.html:19:
shouldBeCloseEnough("rect.y.animVal.value", "0");
On 2014/12/18 16:45:17, fs (OoO) wrote:
> Shouldn't it be exactly '0'?
You're right. I changed it.
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/svg-animation-policy-once.html (right):
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-animation-policy-once.html:3: <head>
On 2014/12/18 16:45:18, fs (OoO) wrote:
> You can drop <html> and <head></head>.
Done.
https://codereview.chromium.org/802143002/diff/40001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-animation-policy-once.html:47: </body>
On 2014/12/18 16:45:17, fs (OoO) wrote:
> One </body> is usually enough ;-)
Woops. Sorry.
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:171: ImageAnimationPolicy
animationPolicy = document().settings()->imageAnimationPolicy();
On 2014/12/18 16:45:18, fs (OoO) wrote:
> I'd suggest you move these two into a helper function.
Done.
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:173: return;
On 2014/12/18 16:45:18, fs (OoO) wrote:
> I'm a bit worried that this will leave us in an inconsistent state. We'll also
> need to guard ourselves from these DOM methods:
>
> void pauseAnimations();
> void unpauseAnimations();
>
> in this case.
>
> Should also test the behavior of the other (three IIRC) methods on
SVGSVGElement
> (animationsPaused, getCurrentTime, setCurrentTime) to see they work in a
> reasonable way/return reasonable values.
> ).
I updated new concept with considering DOM APIs.
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:177:
m_animationPolicyOnceTimer.startOneShot(animationPolicyOnceDuration, FROM_HERE);
On 2014/12/18 16:45:18, fs (OoO) wrote:
> I suppose this is one way to implement this. It has the downside though, that
if
> the timeline is paused at this point it might still be possible to seek to 3
> seconds. Not a big deal, and my other suggestions might help rectify that.
Please check my new concept.
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:243: time =
SMILTime(animationPolicyOnceDuration);
On 2014/12/18 16:45:18, fs (OoO) wrote:
> Should be possible to simply do:
>
> time = std::min(time, SMILTime(animationPolicyOnceDuration));
>
> here.
Please check my new concept.
https://codereview.chromium.org/802143002/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:322: cancelAnimationFrame();
On 2014/12/18 16:45:18, fs (OoO) wrote:
> I don't think is enough (see above about DOM methods). I'd suggest adding a
new
> method (freeze() maybe?) that pauses the timeline and then sets a flag to
> 'freeze' it. This flag would then be checked in appropriate places (and should
> probably also be set for the 'none' case.)
>
> BTW, to what degree/when can the policy value change? At any time?
I added new function for freeze.
I haven't digested this fully, but below are some quick comments to hopefully help move this forward. https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:160: if (ImageAnimationPolicyNoAnimation == getAnimationPolicy()) I'd prefer if we could reuse existing state as much as possible. Also please don't write "yoda conditions". https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:181: ImageAnimationPolicy animationPolicy = settings->imageAnimationPolicy(); Nit: Return this directly. https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:187: pause(true); Could we structure this do a state-transition before/after calling pause() and not add this parameter to pause()? https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:208: return freezeTimeline(); This looks like a very complicated way to return doing nothing =) (freezeTimeline callas pause which returns based on this same conditional.) https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:213: startAnimationPolicyTimer(); Shouldn't this interact with pause in some way? (See m_pauseTime branch below.) https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:88: enum AnimatedState { I'd prefer to enumerate all the discrete states rather than using a bitfield. I'm also not sure if AnimatedState is the bet term for this - maybe TimelineState? https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:97: ImageAnimationPolicy getAnimationPolicy() const; animationPolicy https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:126: int m_animationState; With the discrete states this would then be the enum type instead.
@fs,
Thanks for your comment.
Your comment makes me find more reasonable solution.
I update the rule with more simple way.
From the previous patch,
I tried to keep state separately with Paused and Frozen.
That's why I needed bit flags.
But I realized that users and authors don't think them separately.
So, I updated the rule table below.
-Once (*AT: AnimationPolicyTimer)
| before frozen | after frozen
------------------------------------------------------------------
pause | Pause & cancel AT | already Paused
------------------------------------------------------------------
resume | Resume & start AT | Resume & start AT
------------------------------------------------------------------
isPaused | true if paused | true
------------------------------------------------------------------
setElapsed|PausedState:setElapsed | PausedState:setElapsed
|UnpausedState:setElapsed|(there is no unpaused state)
| & start AT|
------------------------------------------------------------------
elapsed | returns current time | returns current time
As to comment to previous patchset,
I removed state definition because I don't need to state check from the concept
above.
, kept pause() api and added 'paused' state check.
Could you take another look?
Happy Holidays!
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:160: if
(ImageAnimationPolicyNoAnimation == getAnimationPolicy())
On 2014/12/22 16:23:14, fs (OoO) wrote:
> I'd prefer if we could reuse existing state as much as possible.
>
> Also please don't write "yoda conditions".
Done.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:181: ImageAnimationPolicy
animationPolicy = settings->imageAnimationPolicy();
On 2014/12/22 16:23:14, fs (OoO) wrote:
> Nit: Return this directly.
done.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:187: pause(true);
On 2014/12/22 16:23:14, fs (OoO) wrote:
> Could we structure this do a state-transition before/after calling pause() and
> not add this parameter to pause()?
I removed it with improved concept.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:208: return freezeTimeline();
On 2014/12/22 16:23:15, fs (OoO) wrote:
> This looks like a very complicated way to return doing nothing =)
> (freezeTimeline callas pause which returns based on this same conditional.)
I removed it with improved concept.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:213:
startAnimationPolicyTimer();
On 2014/12/22 16:23:14, fs (OoO) wrote:
> Shouldn't this interact with pause in some way? (See m_pauseTime branch
below.)
I updated code with paused condition.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.h (right):
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.h:88: enum AnimatedState {
On 2014/12/22 16:23:15, fs (OoO) wrote:
> I'd prefer to enumerate all the discrete states rather than using a bitfield.
> I'm also not sure if AnimatedState is the bet term for this - maybe
> TimelineState?
I removed it with improved concept.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.h:97: ImageAnimationPolicy
getAnimationPolicy() const;
On 2014/12/22 16:23:15, fs (OoO) wrote:
> animationPolicy
done.
https://codereview.chromium.org/802143002/diff/60001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.h:126: int m_animationState;
On 2014/12/22 16:23:15, fs (OoO) wrote:
> With the discrete states this would then be the enum type instead.
I removed it with improved concept.
On 2014/12/24 03:50:52, je_julie wrote: > @fs, > Thanks for your comment. > Your comment makes me find more reasonable solution. > > I update the rule with more simple way. > From the previous patch, > I tried to keep state separately with Paused and Frozen. > That's why I needed bit flags. > But I realized that users and authors don't think them separately. > So, I updated the rule table below. > -Once (*AT: AnimationPolicyTimer) > | before frozen | after frozen > ------------------------------------------------------------------ > pause | Pause & cancel AT | already Paused > ------------------------------------------------------------------ > resume | Resume & start AT | Resume & start AT > ------------------------------------------------------------------ > isPaused | true if paused | true > ------------------------------------------------------------------ > setElapsed|PausedState:setElapsed | PausedState:setElapsed > |UnpausedState:setElapsed|(there is no unpaused state) > | & start AT| > ------------------------------------------------------------------ > elapsed | returns current time | returns current time > > As to comment to previous patchset, > I removed state definition because I don't need to state check from the concept > above. > , kept pause() api and added 'paused' state check. Yes, this certainly reduces the intrusiveness. I suppose the most interesting bit is if the 'forced pause' state should be observable or not - or rather how, because it will be in one way or the other. I think I'm fine with this approach. > Happy Holidays! Thank you =). Likewise (to the degree it applies). https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/resources/animation-policy.svg (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:2: <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd"> You can drop this. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:3: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> You don't need to declare the xlink namespace. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:5: <animate id="an1" attributeName="visibility" to="visible" begin="0s" dur="3s" repeatCount="indefinite"/> What case is this for? (AFAICS this will animate from 'visible' to 'visible'.) https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:8: begin="0s; bottom.end" dur="1s" fill="freeze"/> I don't see a need for the 'freeze's here. Could also just make this a discrete-mode animation w/ values="30;50;100" and a repeat. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:11: <set id="bottom" attributeName="y" to="100" Maybe align this with the other two <set>. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/script-tests/svg-animation-policy.js (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/script-tests/svg-animation-policy.js:8: ["an1", 0.0, sample1], I don't remember the details of this "framework", but using 'an1' as the id seems a bit odd TBH. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/svg-animation-policy-once.html (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:3: <script src="resources/SVGTestCase.js"></script> Are these really needed except for the "embedSVGTestCase", which would be trivial to replace (by explicitly adding a iframe/object.) https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:12: // It should be true because animation is freezed by animation policy. Nit: s/freezed/frozen/ You could probably also s/It should be true/True/ https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:48: setTimeout(timerFiredFirst, 3000); A 6+ seconds test - ouch =/ Could we at the very least split it to get it down to ~3s? https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:55: if (window.testRunner) { Might as well do this just after the setImageAnimationPolicy call. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:62: setTimeout(runTest(), 0); Drop the () - or the setTimeout? https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:162: return m_pauseTime; return m_oauseTime || animationPolicy() == ...; (Would it be possible to set pause-time early in the lifetime of the time container and get the same effect?) https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:174: if (!handleAnimationPolicy(true, false)) handleAnimationPolicy(RestartOnceTimerIfNotPaused) ? Possibly check for paused here and then pass RestartOnceTimer or IgnoreOnceTimer. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:208: if (!handleAnimationPolicy(false, true)) handleAnimationPolicy(CancelOnceTimer) https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:223: if (!handleAnimationPolicy(false, false)) handleAnimationPolicy(RestartOnceTimer) https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:241: if (!handleAnimationPolicy(true, false)) Same as in begin(). https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:354: if (passed) { This could've been !checkPausedState || !isPaused(). But maybe with an enum approach we can do: switch (onceAction) { case RestartOnceTimerIfNotPaused: if (isPaused()) break; /* fall through */ case RestartOnceTimer: schedule...; break; case CancelOnceTimer: cancel...; break; } https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:96: bool handleAnimationPolicy(bool cancelTimer, bool checkPausedState); These bools make for very hard to read code at the callsites. Let's try to use an (single enum for this instead. I'll add some suggestions on callsites. (I'll also note that it seems the names are reversed between here and the definition...)
Thanks for your nice suggestion. I updated the patchset. PTAL. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/resources/animation-policy.svg (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:2: <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd"> On 2014/12/25 17:32:07, fs (OoO) wrote: > You can drop this. Done. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:3: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> On 2014/12/25 17:32:07, fs (OoO) wrote: > You don't need to declare the xlink namespace. Done. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:5: <animate id="an1" attributeName="visibility" to="visible" begin="0s" dur="3s" repeatCount="indefinite"/> On 2014/12/25 17:32:07, fs (OoO) wrote: > What case is this for? (AFAICS this will animate from 'visible' to 'visible'.) I changed it with y, values="30;50;100" and discrete mode. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:8: begin="0s; bottom.end" dur="1s" fill="freeze"/> On 2014/12/25 17:32:07, fs (OoO) wrote: > I don't see a need for the 'freeze's here. Could also just make this a > discrete-mode animation w/ values="30;50;100" and a repeat. Removed freeze. I removed set tags and I changed animate tag with discrete. Do I need to keep set tags even if I use discrete mode. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:11: <set id="bottom" attributeName="y" to="100" On 2014/12/25 17:32:07, fs (OoO) wrote: > Maybe align this with the other two <set>. Removed set tags. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/script-tests/svg-animation-policy.js (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/script-tests/svg-animation-policy.js:8: ["an1", 0.0, sample1], On 2014/12/25 17:32:07, fs (OoO) wrote: > I don't remember the details of this "framework", but using 'an1' as the id > seems a bit odd TBH. I changed it to animation. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/svg-animation-policy-once.html (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:3: <script src="resources/SVGTestCase.js"></script> On 2014/12/25 17:32:07, fs (OoO) wrote: > Are these really needed except for the "embedSVGTestCase", which would be > trivial to replace (by explicitly adding a iframe/object.) I removed that script external link and I added APIs I need in this file. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:12: // It should be true because animation is freezed by animation policy. On 2014/12/25 17:32:07, fs (OoO) wrote: > Nit: s/freezed/frozen/ > > You could probably also s/It should be true/True/ Done. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:48: setTimeout(timerFiredFirst, 3000); On 2014/12/25 17:32:07, fs (OoO) wrote: > A 6+ seconds test - ouch =/ Could we at the very least split it to get it down > to ~3s? I updated test flow a little bit. I rearranged test cases before frozen state and after frozen state. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:55: if (window.testRunner) { On 2014/12/25 17:32:07, fs (OoO) wrote: > Might as well do this just after the setImageAnimationPolicy call. Done. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/svg-animation-policy-once.html:62: setTimeout(runTest(), 0); On 2014/12/25 17:32:07, fs (OoO) wrote: > Drop the () - or the setTimeout? Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:162: return m_pauseTime; On 2014/12/25 17:32:08, fs (OoO) wrote: > return m_oauseTime || animationPolicy() == ...; > > (Would it be possible to set pause-time early in the lifetime of the time > container and get the same effect?) Done. I assumed that it has the same effect whenever it's paused and policy is set with 'none'. Can you anticipate any problem with that assumption? https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:174: if (!handleAnimationPolicy(true, false)) On 2014/12/25 17:32:08, fs (OoO) wrote: > handleAnimationPolicy(RestartOnceTimerIfNotPaused) > > ? > > Possibly check for paused here and then pass RestartOnceTimer or > IgnoreOnceTimer. Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:208: if (!handleAnimationPolicy(false, true)) On 2014/12/25 17:32:08, fs (OoO) wrote: > handleAnimationPolicy(CancelOnceTimer) Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:223: if (!handleAnimationPolicy(false, false)) On 2014/12/25 17:32:08, fs (OoO) wrote: > handleAnimationPolicy(RestartOnceTimer) Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:241: if (!handleAnimationPolicy(true, false)) On 2014/12/25 17:32:08, fs (OoO) wrote: > Same as in begin(). Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:354: if (passed) { On 2014/12/25 17:32:08, fs (OoO) wrote: > This could've been !checkPausedState || !isPaused(). But maybe with an enum > approach we can do: > > switch (onceAction) { > case RestartOnceTimerIfNotPaused: > if (isPaused()) > break; > /* fall through */ > case RestartOnceTimer: > schedule...; > break; > case CancelOnceTimer: > cancel...; > break; > } Done. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:96: bool handleAnimationPolicy(bool cancelTimer, bool checkPausedState); On 2014/12/25 17:32:08, fs (OoO) wrote: > These bools make for very hard to read code at the callsites. Let's try to use > an (single enum for this instead. I'll add some suggestions on callsites. (I'll > also note that it seems the names are reversed between here and the > definition...) Thanks! I followed your suggestion.
Almost there now, mostly some testcase nits. https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... File LayoutTests/svg/animations/resources/animation-policy.svg (right): https://codereview.chromium.org/802143002/diff/80001/LayoutTests/svg/animatio... LayoutTests/svg/animations/resources/animation-policy.svg:8: begin="0s; bottom.end" dur="1s" fill="freeze"/> On 2014/12/27 07:08:53, je_julie wrote: > On 2014/12/25 17:32:07, fs (OoO) wrote: > > I don't see a need for the 'freeze's here. Could also just make this a > > discrete-mode animation w/ values="30;50;100" and a repeat. > > Removed freeze. > I removed set tags and I changed animate tag with discrete. > Do I need to keep set tags even if I use discrete mode. No, this should be fine. https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:162: return m_pauseTime; On 2014/12/27 07:08:54, je_julie wrote: > On 2014/12/25 17:32:08, fs (OoO) wrote: > > return m_oauseTime || animationPolicy() == ...; > > > > (Would it be possible to set pause-time early in the lifetime of the time > > container and get the same effect?) > > Done. > I assumed that it has the same effect whenever it's paused and policy is set > with 'none'. > Can you anticipate any problem with that assumption? That sounds fine. I suppose if the policy could change (from 'none' to something else) during the lifetime of the page it could matter. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... File LayoutTests/svg/animations/svg-animation-policy-once.html (right): https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html> https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:10: window.jsTestIsAsync = true; Probably better to just set this(once) outside of the if. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:17: window.jsTestIsAsync = true; No need to set this here (again). https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:24: bodyElement.insertBefore(iframeElement, document.getElementById("description")); This could be: <iframe src="resources/animation-policy.svg" style="..." onload="..."></iframe> directly within the <body>. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:28: rootSVGElement = iframeElement.getSVGDocument().rootElement; Use contentDocument.documentElement here instead of getSVGDocument().rootElement? https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:35: // It should be True because animation is frozen by animation policy. Drop the the "It should be" part (it doesn't really add much.) https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:49: shouldBeCloseEnough("rect.y.animVal.value", "100"); It should be possible to make all the shouldBeCloseEnough's shouldBe's, because the animation is discrete now. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:55: // SVG is not suspened. Nit: suspended (missing 'd') https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.cpp:361: default: Shouldn't need this (all values are handled.) https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.h:89: // Restart OnceTimer if it's not paused. "if it's" -> "if the timeline is"
@fs, Thanks for your time and review. I updated the patchset. PTAL. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... File LayoutTests/svg/animations/svg-animation-policy-once.html (right): https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2014/12/27 15:36:35, fs (OoO) wrote: > <!DOCTYPE html> Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:10: window.jsTestIsAsync = true; On 2014/12/27 15:36:35, fs (OoO) wrote: > Probably better to just set this(once) outside of the if. Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:17: window.jsTestIsAsync = true; On 2014/12/27 15:36:35, fs (OoO) wrote: > No need to set this here (again). Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:24: bodyElement.insertBefore(iframeElement, document.getElementById("description")); On 2014/12/27 15:36:35, fs (OoO) wrote: > This could be: > > <iframe src="resources/animation-policy.svg" style="..." onload="..."></iframe> > > directly within the <body>. Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:28: rootSVGElement = iframeElement.getSVGDocument().rootElement; On 2014/12/27 15:36:35, fs (OoO) wrote: > Use contentDocument.documentElement here instead of > getSVGDocument().rootElement? Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:35: // It should be True because animation is frozen by animation policy. On 2014/12/27 15:36:35, fs (OoO) wrote: > Drop the the "It should be" part (it doesn't really add much.) Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:49: shouldBeCloseEnough("rect.y.animVal.value", "100"); On 2014/12/27 15:36:35, fs (OoO) wrote: > It should be possible to make all the shouldBeCloseEnough's shouldBe's, because > the animation is discrete now. Done. https://codereview.chromium.org/802143002/diff/100001/LayoutTests/svg/animati... LayoutTests/svg/animations/svg-animation-policy-once.html:55: // SVG is not suspened. On 2014/12/27 15:36:35, fs (OoO) wrote: > Nit: suspended (missing 'd') Done. https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.cpp:361: default: On 2014/12/27 15:36:35, fs (OoO) wrote: > Shouldn't need this (all values are handled.) Done. https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/100001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.h:89: // Restart OnceTimer if it's not paused. On 2014/12/27 15:36:35, fs (OoO) wrote: > "if it's" -> "if the timeline is" Done.
LGTM, thanks! https://codereview.chromium.org/802143002/diff/120001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/120001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.h:97: Nit: Please drop one of the blank lines.
Thanks! I updated patchset to remove the blank line. Do I need to get a review from other reviewers? Otherwise, can I commit this? https://codereview.chromium.org/802143002/diff/120001/Source/core/svg/animati... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/802143002/diff/120001/Source/core/svg/animati... Source/core/svg/animation/SMILTimeContainer.h:97: On 2014/12/28 15:10:43, fs (OoO) wrote: > Nit: Please drop one of the blank lines. Done.
On 2014/12/29 01:37:49, je_julie wrote: > Thanks! > I updated patchset to remove the blank line. > Do I need to get a review from other reviewers? > Otherwise, can I commit this? I think that is up to you to decide. If additional changes are needed they could well be done incrementally.
On 2014/12/29 17:10:46, fs (OoO) wrote: > On 2014/12/29 01:37:49, je_julie wrote: > > Thanks! > > I updated patchset to remove the blank line. > > Do I need to get a review from other reviewers? > > Otherwise, can I commit this? > > I think that is up to you to decide. If additional changes are needed they could > well be done incrementally. I think it's done for SVG with this CL. If there is anything we need to update for SVG, I'll keep following that. @fs, Thanks a lot to keep reviewing this patch .
The CQ bit was checked by je_julie.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/802143002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187775 |
