Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1119723002:
SVG animate should respect attribute 'restart=never'. (Closed)
|
Created:
5 years, 7 months ago by Shanmuga Pandi Modified:
5 years, 7 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), darktears, gyuyoung2, 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. |
DescriptionSVG animate should respect attribute 'restart=never'.
SVG SMIL should consider restart=never when it is progressing for animation.
BUG=410918
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195170
Patch Set 1 #Patch Set 2 : Removed unwanted lines #
Total comments: 8
Patch Set 3 : Align with review comments #
Total comments: 26
Patch Set 4 : simplified layout test case #
Total comments: 3
Patch Set 5 : Rebased and aligned with code review #
Total comments: 3
Messages
Total messages: 34 (4 generated)
shanmuga.m@samsung.com changed reviewers: + ed@opera.com, fs@opera.com, pdr@chromium.org
Please review it. Thanks
https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:4: <script src="../../fast/repaint/resources/text-based-repaint.js"></script> This test looks really complicated, and why is it a repaint test? (The expected file looks like a reference.) I think you could just use beginElement(At) or something? https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:1134: m_nextProgressTime = SMILTime::unresolved(); This seems a bit "early" to handle this? Not entirely sure what is actually being fixed here...
https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:4: <script src="../../fast/repaint/resources/text-based-repaint.js"></script> On 2015/04/30 13:24:43, fs wrote: > This test looks really complicated, and why is it a repaint test? (The expected > file looks like a reference.) I think you could just use beginElement(At) or > something? Ok. I will check for reducing the complexity. https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:1134: m_nextProgressTime = SMILTime::unresolved(); On 2015/04/30 13:24:43, fs wrote: > This seems a bit "early" to handle this? Not entirely sure what is actually > being fixed here... Basically, I am trying to ignore the restart of animation if animation already ran and completed. So I just returned false. Anyway m_nextProgressTime = SMILTime::unresolved(); may not seems to be needed.
https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:1134: m_nextProgressTime = SMILTime::unresolved(); On 2015/04/30 13:42:57, Shanmuga Pandi wrote: > On 2015/04/30 13:24:43, fs wrote: > > This seems a bit "early" to handle this? Not entirely sure what is actually > > being fixed here... > > Basically, I am trying to ignore the restart of animation if animation already > ran and completed. Yes, I got that bit. =) > So I just returned false. Anyway m_nextProgressTime = > SMILTime::unresolved(); may not seems to be needed. I suspect the problem is really that when a new instance time is added, that a new interval is computed up front - and that interval should not be valid per 'restart'.
https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:1134: m_nextProgressTime = SMILTime::unresolved(); On 2015/04/30 13:53:42, fs wrote: > On 2015/04/30 13:42:57, Shanmuga Pandi wrote: > > On 2015/04/30 13:24:43, fs wrote: > > > This seems a bit "early" to handle this? Not entirely sure what is actually > > > being fixed here... > > > > Basically, I am trying to ignore the restart of animation if animation already > > ran and completed. > > Yes, I got that bit. =) > > > So I just returned false. Anyway m_nextProgressTime = > > SMILTime::unresolved(); may not seems to be needed. > > I suspect the problem is really that when a new instance time is added, that a > new interval is computed up front - and that interval should not be valid per > 'restart'. Sorry. Could you give some live example or jsfiddle for this scenario to understand it better?
https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/20001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:1134: m_nextProgressTime = SMILTime::unresolved(); On 2015/04/30 14:03:00, Shanmuga Pandi wrote: > On 2015/04/30 13:53:42, fs wrote: > > On 2015/04/30 13:42:57, Shanmuga Pandi wrote: > > > On 2015/04/30 13:24:43, fs wrote: > > > > This seems a bit "early" to handle this? Not entirely sure what is > actually > > > > being fixed here... > > > > > > Basically, I am trying to ignore the restart of animation if animation > already > > > ran and completed. > > > > Yes, I got that bit. =) > > > > > So I just returned false. Anyway m_nextProgressTime = > > > SMILTime::unresolved(); may not seems to be needed. > > > > I suspect the problem is really that when a new instance time is added, that a > > new interval is computed up front - and that interval should not be valid per > > 'restart'. > > Sorry. Could you give some live example or jsfiddle for this scenario to > understand it better? I think your testcase would should exactly that. I think the relevant code is in SVGSMILElement::beginListChanged.
Moved the logic into SVGSMILElement::beginListChanged. https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/20001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:4: <script src="../../fast/repaint/resources/text-based-repaint.js"></script> On 2015/04/30 13:24:43, fs wrote: > This test looks really complicated, and why is it a repaint test? (The expected > file looks like a reference.) I think you could just use beginElement(At) or > something? Changed logic !!
https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:2: <head> Not needed. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:13: rect.setAttribute("fill", "green"); I'm not quite sure why you'd need this. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:15: anim.setAttribute("to", "red"); The element already have these set? https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:16: anim.setAttribute("fill", "freeze"); Set this on the element in the content instead. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:27: function nextAction() { Just drop this whole mechanism - it's overkill for what you're trying to achieve (test) here. Something along the lines of: function animationEnded() { anim.beginElement(); testRunner.displayAsyncThen(function() { testRunner.notifyDone(); }); } ... <animate ... onend="animationEnded()"/> ought to work. (The dur="..." still worries me a bit though.) https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:38: <body onload="animateTest()"> Could set onload within <script> instead, and then you wouldn't need to define the <body>. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:41: <animate id="anim" xlink:href="#rect" Just make this element a child of the <rect> and drop the href. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:43: dur="0.01s" I get a suspicion that this will be flaky. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:44: attributeType="xml" This doesn't looked necessary. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:47: repeatCount="1" Nor this. https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:946: if (!m_isWaitingForFirstInterval && m_activeState != Active && this->restart() == RestartNever) { This will naturally fall into the else-branch below. Also, what if m_activeState == Active and predicate below (in the else-branch is true?)
https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:13: rect.setAttribute("fill", "green"); On 2015/05/04 14:46:51, fs wrote: > I'm not quite sure why you'd need this. As you said earlier, Green color should be used for expected result. So I have used this. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:15: anim.setAttribute("to", "red"); On 2015/05/04 14:46:51, fs wrote: > The element already have these set? I will remove this. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:16: anim.setAttribute("fill", "freeze"); On 2015/05/04 14:46:51, fs wrote: > Set this on the element in the content instead. If I added into this content, then it is really difficult to find out whether animation got completed or not. If we freeze there then rect always having "red" at the end. Also this test case will pass , even without this fix. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:27: function nextAction() { On 2015/05/04 14:46:51, fs wrote: > Just drop this whole mechanism - it's overkill for what you're trying to achieve > (test) here. > > Something along the lines of: > > function animationEnded() { > anim.beginElement(); > testRunner.displayAsyncThen(function() { > testRunner.notifyDone(); > }); > } > ... > <animate ... onend="animationEnded()"/> > > ought to work. (The dur="..." still worries me a bit though.) I will try this as you commented. https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:946: if (!m_isWaitingForFirstInterval && m_activeState != Active && this->restart() == RestartNever) { On 2015/05/04 14:46:52, fs wrote: > This will naturally fall into the else-branch below. Also, what if m_activeState > == Active and predicate below (in the else-branch is true?) Yes. If animation is currently active, m_nextProgressTime and m_interval should keep the current animation information. We need to reset back only if animation got over. Correct me if I wrong.
https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:13: rect.setAttribute("fill", "green"); On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > On 2015/05/04 14:46:51, fs wrote: > > I'm not quite sure why you'd need this. > > As you said earlier, Green color should be used for expected result. So I have > used this. Yes, but why not just set on the <rect> in the content below? https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:16: anim.setAttribute("fill", "freeze"); On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > On 2015/05/04 14:46:51, fs wrote: > > Set this on the element in the content instead. > > If I added into this content, then it is really difficult to find out whether > animation got completed or not. If we freeze there then rect always having "red" > at the end. Also this test case will pass , even without this fix. I'd say that either you need fill=freeze or you don't. If you need it, then just put it in the content rather than than mutate it like this. https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:946: if (!m_isWaitingForFirstInterval && m_activeState != Active && this->restart() == RestartNever) { On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > On 2015/05/04 14:46:52, fs wrote: > > This will naturally fall into the else-branch below. Also, what if > m_activeState > > == Active and predicate below (in the else-branch is true?) > > Yes. If animation is currently active, m_nextProgressTime and m_interval should > keep the current animation information. We need to reset back only if animation > got over. ...which will depend on what findInstanceTime() returns - no?
Modified test case https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:13: rect.setAttribute("fill", "green"); On 2015/05/04 15:37:11, fs wrote: > On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > > On 2015/05/04 14:46:51, fs wrote: > > > I'm not quite sure why you'd need this. > > > > As you said earlier, Green color should be used for expected result. So I have > > used this. > > Yes, but why not just set on the <rect> in the content below? Done. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:16: anim.setAttribute("fill", "freeze"); On 2015/05/04 15:37:11, fs wrote: > On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > > On 2015/05/04 14:46:51, fs wrote: > > > Set this on the element in the content instead. > > > > If I added into this content, then it is really difficult to find out whether > > animation got completed or not. If we freeze there then rect always having > "red" > > at the end. Also this test case will pass , even without this fix. > > I'd say that either you need fill=freeze or you don't. If you need it, then just > put it in the content rather than than mutate it like this. Acknowledged. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:41: <animate id="anim" xlink:href="#rect" On 2015/05/04 14:46:51, fs wrote: > Just make this element a child of the <rect> and drop the href. Done. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:43: dur="0.01s" On 2015/05/04 14:46:51, fs wrote: > I get a suspicion that this will be flaky. Acknowledged. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:44: attributeType="xml" On 2015/05/04 14:46:51, fs wrote: > This doesn't looked necessary. Done. https://codereview.chromium.org/1119723002/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:47: repeatCount="1" On 2015/05/04 14:46:51, fs wrote: > Nor this. Done. https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/40001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:946: if (!m_isWaitingForFirstInterval && m_activeState != Active && this->restart() == RestartNever) { On 2015/05/04 15:37:11, fs wrote: > On 2015/05/04 15:21:31, Shanmuga Pandi wrote: > > On 2015/05/04 14:46:52, fs wrote: > > > This will naturally fall into the else-branch below. Also, what if > > m_activeState > > > == Active and predicate below (in the else-branch is true?) > > > > Yes. If animation is currently active, m_nextProgressTime and m_interval > should > > keep the current animation information. We need to reset back only if > animation > > got over. > > ...which will depend on what findInstanceTime() returns - no? When I checked with sample test case, findInstanceTime returns same eventTime which is passed. And if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ interval.begin) never been true when Animation is active. Let me know if you want me to check some specific scenario which I missed?
On 2015/05/05 06:53:26, Shanmuga Pandi wrote:
...
> When I checked with sample test case, findInstanceTime returns same eventTime
> which is passed. And
> if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_
> interval.begin) never been true when Animation is active.
I guess we need to make sure the test is testing the correct thing then - maybe
beginElement() isn't good/similar enough. AFAICT it ought to involve
addBeginTime(...) in some way, because a simple test
(svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work.
Maybe some variant of:
<svg>
<rect width="100" height="100" fill="green">
<set attributeName="fill" to="red" begin="0s;foo.end" dur="500ms"/>
<set attributeName="width" to="200" dur="1s" id="foo"/>
</rect>
</svg>
https://codereview.chromium.org/1119723002/diff/60001/LayoutTests/svg/animati...
File LayoutTests/svg/animations/animate-restart-never.html (right):
https://codereview.chromium.org/1119723002/diff/60001/LayoutTests/svg/animati...
LayoutTests/svg/animations/animate-restart-never.html:9:
testRunner.displayAsyncThen(function() {
Always good to be able to run the test without the runner, so check for the
runner existing first.
https://codereview.chromium.org/1119723002/diff/60001/LayoutTests/svg/animati...
LayoutTests/svg/animations/animate-restart-never.html:10:
testRunner.notifyDone();
This still feels like it will be flaky - beginElement() is likely to end up
triggering the wake-up timer in the time container, and then one has to wait for
that first before the actual paint is triggered.
https://codereview.chromium.org/1119723002/diff/60001/LayoutTests/svg/animati...
LayoutTests/svg/animations/animate-restart-never.html:18: dur="0.2s"
Now this test takes 200+ms...
On 2015/05/05 14:39:20, fs wrote: > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > ... > > When I checked with sample test case, findInstanceTime returns same eventTime > > which is passed. And > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ > > interval.begin) never been true when Animation is active. > > I guess we need to make sure the test is testing the correct thing then - maybe > beginElement() isn't good/similar enough. AFAICT it ought to involve > addBeginTime(...) in some way, because a simple test > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > Maybe some variant of: > > <svg> > <rect width="100" height="100" fill="green"> > <set attributeName="fill" to="red" begin="0s;foo.end" dur="500ms"/> > <set attributeName="width" to="200" dur="1s" id="foo"/> > </rect> > </svg> Looks like we really do need something with an eventBase to trigger this, so should probably "restore" that part of the TC.
On 2015/05/05 16:24:22, fs wrote: > On 2015/05/05 14:39:20, fs wrote: > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > ... > > > When I checked with sample test case, findInstanceTime returns same > eventTime > > > which is passed. And > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ > > > interval.begin) never been true when Animation is active. > > > > I guess we need to make sure the test is testing the correct thing then - > maybe > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > addBeginTime(...) in some way, because a simple test > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > Maybe some variant of: > > > > <svg> > > <rect width="100" height="100" fill="green"> > > <set attributeName="fill" to="red" begin="0s;foo.end" dur="500ms"/> > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > </rect> > > </svg> I will check the mentioned sample svg and update you. > Looks like we really do need something with an eventBase to trigger this, so > should probably "restore" that part of the TC. Sorry for the late reply. Do I restore the initial TC in which I trigger "click" to start the animation ?
On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > On 2015/05/05 16:24:22, fs wrote: > > On 2015/05/05 14:39:20, fs wrote: > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > ... > > > > When I checked with sample test case, findInstanceTime returns same > > eventTime > > > > which is passed. And > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ > > > > interval.begin) never been true when Animation is active. > > > > > > I guess we need to make sure the test is testing the correct thing then - > > maybe > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > addBeginTime(...) in some way, because a simple test > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > Maybe some variant of: > > > > > > <svg> > > > <rect width="100" height="100" fill="green"> > > > <set attributeName="fill" to="red" begin="0s;foo.end" dur="500ms"/> > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > </rect> > > > </svg> > > I will check the mentioned sample svg and update you. > > > Looks like we really do need something with an eventBase to trigger this, so > > should probably "restore" that part of the TC. > > Sorry for the late reply. Do I restore the initial TC in which I trigger "click" > to start the animation ? some like begin="0s;foo.click" and then click on 'foo', yes.
On 2015/05/07 10:47:46, fs wrote: > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > On 2015/05/05 16:24:22, fs wrote: > > > On 2015/05/05 14:39:20, fs wrote: > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > ... > > > > > When I checked with sample test case, findInstanceTime returns same > > > eventTime > > > > > which is passed. And > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ > > > > > interval.begin) never been true when Animation is active. > > > > > > > > I guess we need to make sure the test is testing the correct thing then - > > > maybe > > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > > addBeginTime(...) in some way, because a simple test > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > Maybe some variant of: > > > > > > > > <svg> > > > > <rect width="100" height="100" fill="green"> > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > dur="500ms"/> > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > </rect> > > > > </svg> > > > > I will check the mentioned sample svg and update you. > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that place. I have created one below fiddle, http://jsfiddle.net/056z9u0q/ With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < m_ interval.begin)" when Animation is active, but that time it is not giving expected output. When animation works fine on jsfiddle test, then if condition never been true.
On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > On 2015/05/07 10:47:46, fs wrote: > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > On 2015/05/05 16:24:22, fs wrote: > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > ... > > > > > > When I checked with sample test case, findInstanceTime returns same > > > > eventTime > > > > > > which is passed. And > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < > m_ > > > > > > interval.begin) never been true when Animation is active. > > > > > > > > > > I guess we need to make sure the test is testing the correct thing then > - > > > > maybe > > > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > > > addBeginTime(...) in some way, because a simple test > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > Maybe some variant of: > > > > > > > > > > <svg> > > > > > <rect width="100" height="100" fill="green"> > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > dur="500ms"/> > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > </rect> > > > > > </svg> > > > > > > I will check the mentioned sample svg and update you. > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that place. > > I have created one below fiddle, > http://jsfiddle.net/056z9u0q/ > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= > eventTime || newBegin < m_ > interval.begin)" when Animation is active, but that time it is not giving > expected output. > When animation works fine on jsfiddle test, then if condition never been true. created bug also BUG : 485532
On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > On 2015/05/07 10:47:46, fs wrote: > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > On 2015/05/05 16:24:22, fs wrote: > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > ... > > > > > > When I checked with sample test case, findInstanceTime returns same > > > > eventTime > > > > > > which is passed. And > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin < > m_ > > > > > > interval.begin) never been true when Animation is active. > > > > > > > > > > I guess we need to make sure the test is testing the correct thing then > - > > > > maybe > > > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > > > addBeginTime(...) in some way, because a simple test > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > Maybe some variant of: > > > > > > > > > > <svg> > > > > > <rect width="100" height="100" fill="green"> > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > dur="500ms"/> > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > </rect> > > > > > </svg> > > > > > > I will check the mentioned sample svg and update you. > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that place. > > I have created one below fiddle, > http://jsfiddle.net/056z9u0q/ > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= > eventTime || newBegin < m_ > interval.begin)" when Animation is active, but that time it is not giving > expected output. > When animation works fine on jsfiddle test, then if condition never been true. Dependent sync-bases most likely is (very) buggy, but I don't think (hope) we need to go there to reproduce the bug your working on here. Not sure we care very much about filing new bugs either TBH, given the deprecated state of SMIL... there's likely existing bugs filed in that area already though...
On 2015/05/07 14:37:49, fs wrote: > On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > > On 2015/05/07 10:47:46, fs wrote: > > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > > On 2015/05/05 16:24:22, fs wrote: > > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > > ... > > > > > > > When I checked with sample test case, findInstanceTime returns same > > > > > eventTime > > > > > > > which is passed. And > > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || newBegin > < > > m_ > > > > > > > interval.begin) never been true when Animation is active. > > > > > > > > > > > > I guess we need to make sure the test is testing the correct thing > then > > - > > > > > maybe > > > > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > > > > addBeginTime(...) in some way, because a simple test > > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > > > Maybe some variant of: > > > > > > > > > > > > <svg> > > > > > > <rect width="100" height="100" fill="green"> > > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > > dur="500ms"/> > > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > > </rect> > > > > > > </svg> > > > > > > > > I will check the mentioned sample svg and update you. > > > > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that > place. > > > > I have created one below fiddle, > > http://jsfiddle.net/056z9u0q/ > > > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= > > eventTime || newBegin < m_ > > interval.begin)" when Animation is active, but that time it is not giving > > expected output. > > When animation works fine on jsfiddle test, then if condition never been true. > > Dependent sync-bases most likely is (very) buggy, but I don't think (hope) we > need to go there to reproduce the bug your working on here. Not sure we care > very much about filing new bugs either TBH, given the deprecated state of > SMIL... there's likely existing bugs filed in that area already though... Yes. Should I change/verify the changes in SVGSMILElement::beginListChanged? Or the change is ok ? And Will work on LayoutTest? I have tried to find out a test case which will enters into If statement when animation is active. But couldn't find any apart from that buggy test case.
On 2015/05/07 14:43:22, Shanmuga Pandi wrote: > On 2015/05/07 14:37:49, fs wrote: > > On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > > > On 2015/05/07 10:47:46, fs wrote: > > > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > > > On 2015/05/05 16:24:22, fs wrote: > > > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > > > ... > > > > > > > > When I checked with sample test case, findInstanceTime returns > same > > > > > > eventTime > > > > > > > > which is passed. And > > > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || > newBegin > > < > > > m_ > > > > > > > > interval.begin) never been true when Animation is active. > > > > > > > > > > > > > > I guess we need to make sure the test is testing the correct thing > > then > > > - > > > > > > maybe > > > > > > > beginElement() isn't good/similar enough. AFAICT it ought to involve > > > > > > > addBeginTime(...) in some way, because a simple test > > > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > > > > > Maybe some variant of: > > > > > > > > > > > > > > <svg> > > > > > > > <rect width="100" height="100" fill="green"> > > > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > > > dur="500ms"/> > > > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > > > </rect> > > > > > > > </svg> > > > > > > > > > > I will check the mentioned sample svg and update you. > > > > > > > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that > > place. > > > > > > I have created one below fiddle, > > > http://jsfiddle.net/056z9u0q/ > > > > > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= > > > eventTime || newBegin < m_ > > > interval.begin)" when Animation is active, but that time it is not giving > > > expected output. > > > When animation works fine on jsfiddle test, then if condition never been > true. > > > > Dependent sync-bases most likely is (very) buggy, but I don't think (hope) we > > need to go there to reproduce the bug your working on here. Not sure we care > > very much about filing new bugs either TBH, given the deprecated state of > > SMIL... there's likely existing bugs filed in that area already though... > > Yes. Should I change/verify the changes in SVGSMILElement::beginListChanged? Or > the change is ok ? And Will work on LayoutTest? As far as I'm concerned there are issues that has not been addressed (move code into else-branch), and I still think the fix looks like a tailored hack. If there no new interval computed, then there should be no need to do anything etc. > I have tried to find out a test case which will enters into If statement when > animation is active. But couldn't find any apart from that buggy test case. I could "enter the if" with (something like) the following: <svg> <rect width="100" height="100" fill="green" id="foo"> <set attributeName="fill" to="red" begin="0s;foo.click" dur="500ms"/> </rect> </svg> when clicking the rect after if had animated once.
On 2015/05/07 14:59:18, fs wrote: > On 2015/05/07 14:43:22, Shanmuga Pandi wrote: > > On 2015/05/07 14:37:49, fs wrote: > > > On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > > > > On 2015/05/07 10:47:46, fs wrote: > > > > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > > > > On 2015/05/05 16:24:22, fs wrote: > > > > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > > > > ... > > > > > > > > > When I checked with sample test case, findInstanceTime returns > > same > > > > > > > eventTime > > > > > > > > > which is passed. And > > > > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || > > newBegin > > > < > > > > m_ > > > > > > > > > interval.begin) never been true when Animation is active. > > > > > > > > > > > > > > > > I guess we need to make sure the test is testing the correct thing > > > then > > > > - > > > > > > > maybe > > > > > > > > beginElement() isn't good/similar enough. AFAICT it ought to > involve > > > > > > > > addBeginTime(...) in some way, because a simple test > > > > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > > > > > > > Maybe some variant of: > > > > > > > > > > > > > > > > <svg> > > > > > > > > <rect width="100" height="100" fill="green"> > > > > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > > > > dur="500ms"/> > > > > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > > > > </rect> > > > > > > > > </svg> > > > > > > > > > > > > I will check the mentioned sample svg and update you. > > > > > > > > > > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in that > > > place. > > > > > > > > I have created one below fiddle, > > > > http://jsfiddle.net/056z9u0q/ > > > > > > > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end <= > > > > eventTime || newBegin < m_ > > > > interval.begin)" when Animation is active, but that time it is not > giving > > > > expected output. > > > > When animation works fine on jsfiddle test, then if condition never been > > true. > > > > > > Dependent sync-bases most likely is (very) buggy, but I don't think (hope) > we > > > need to go there to reproduce the bug your working on here. Not sure we care > > > very much about filing new bugs either TBH, given the deprecated state of > > > SMIL... there's likely existing bugs filed in that area already though... > > > > Yes. Should I change/verify the changes in SVGSMILElement::beginListChanged? > Or > > the change is ok ? And Will work on LayoutTest? > > As far as I'm concerned there are issues that has not been addressed (move code > into else-branch), and I still think the fix looks like a tailored hack. If > there no new interval computed, then there should be no need to do anything etc. > > > I have tried to find out a test case which will enters into If statement when > > animation is active. But couldn't find any apart from that buggy test case. > > I could "enter the if" with (something like) the following: > > <svg> > <rect width="100" height="100" fill="green" id="foo"> > <set attributeName="fill" to="red" begin="0s;foo.click" dur="500ms"/> > </rect> > </svg> > > when clicking the rect after if had animated once. It will enter inside if condition, only when animation state is not active (either inactive or freeze) I can move the change into else and remove unwanted changes if any and upload.
On 2015/05/07 15:13:00, Shanmuga Pandi wrote: > On 2015/05/07 14:59:18, fs wrote: > > On 2015/05/07 14:43:22, Shanmuga Pandi wrote: > > > On 2015/05/07 14:37:49, fs wrote: > > > > On 2015/05/07 14:29:44, Shanmuga Pandi wrote: > > > > > On 2015/05/07 10:47:46, fs wrote: > > > > > > On 2015/05/07 05:58:33, Shanmuga Pandi wrote: > > > > > > > On 2015/05/05 16:24:22, fs wrote: > > > > > > > > On 2015/05/05 14:39:20, fs wrote: > > > > > > > > > On 2015/05/05 06:53:26, Shanmuga Pandi wrote: > > > > > > > > > ... > > > > > > > > > > When I checked with sample test case, findInstanceTime returns > > > same > > > > > > > > eventTime > > > > > > > > > > which is passed. And > > > > > > > > > > if (newBegin.isFinite() && (m_interval.end <= eventTime || > > > newBegin > > > > < > > > > > m_ > > > > > > > > > > interval.begin) never been true when Animation is > active. > > > > > > > > > > > > > > > > > > I guess we need to make sure the test is testing the correct > thing > > > > then > > > > > - > > > > > > > > maybe > > > > > > > > > beginElement() isn't good/similar enough. AFAICT it ought to > > involve > > > > > > > > > addBeginTime(...) in some way, because a simple test > > > > > > > > > (svg/W3C-SVG-1.1/animate-elem-67-t.svg) seems to work. > > > > > > > > > > > > > > > > > > Maybe some variant of: > > > > > > > > > > > > > > > > > > <svg> > > > > > > > > > <rect width="100" height="100" fill="green"> > > > > > > > > > <set attributeName="fill" to="red" begin="0s;foo.end" > > > > > > dur="500ms"/> > > > > > > > > > <set attributeName="width" to="200" dur="1s" id="foo"/> > > > > > > > > > </rect> > > > > > > > > > </svg> > > > > > > > > > > > > > > I will check the mentioned sample svg and update you. > > > > > > > > > > > > > > > > > I had checked svg/W3C-SVG-1.1/animate-elem-67-t.svg, it never hit in > that > > > > place. > > > > > > > > > > I have created one below fiddle, > > > > > http://jsfiddle.net/056z9u0q/ > > > > > > > > > > With that, It comes inside "if (newBegin.isFinite() && (m_interval.end > <= > > > > > eventTime || newBegin < m_ > > > > > interval.begin)" when Animation is active, but that time it is not > > giving > > > > > expected output. > > > > > When animation works fine on jsfiddle test, then if condition never been > > > true. > > > > > > > > Dependent sync-bases most likely is (very) buggy, but I don't think (hope) > > we > > > > need to go there to reproduce the bug your working on here. Not sure we > care > > > > very much about filing new bugs either TBH, given the deprecated state of > > > > SMIL... there's likely existing bugs filed in that area already though... > > > > > > Yes. Should I change/verify the changes in SVGSMILElement::beginListChanged? > > Or > > > the change is ok ? And Will work on LayoutTest? > > > > As far as I'm concerned there are issues that has not been addressed (move > code > > into else-branch), and I still think the fix looks like a tailored hack. If > > there no new interval computed, then there should be no need to do anything > etc. > > > > > I have tried to find out a test case which will enters into If statement > when > > > animation is active. But couldn't find any apart from that buggy test case. > > > > I could "enter the if" with (something like) the following: > > > > <svg> > > <rect width="100" height="100" fill="green" id="foo"> > > <set attributeName="fill" to="red" begin="0s;foo.click" dur="500ms"/> > > </rect> > > </svg> > > > > when clicking the rect after if had animated once. > > It will enter inside if condition, only when animation state is not active > (either inactive or freeze) > > I can move the change into else and remove unwanted changes if any and upload. Done the changes , Please review it. Thanks
https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:27: dur="0.05s" Please verify that this test fails reliably (in the test-runner) without the fix. (There still appear to be a risk of timer vs. frame race here.) https://codereview.chromium.org/1119723002/diff/80001/Source/core/svg/animati... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/1119723002/diff/80001/Source/core/svg/animati... Source/core/svg/animation/SVGSMILElement.cpp:948: } else if (this->restart() != RestartNever) { I still think this should be checked later, but I'll accept this as-is given the state of things.
https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animate-restart-never.html (right): https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... LayoutTests/svg/animations/animate-restart-never.html:27: dur="0.05s" On 2015/05/08 10:28:31, fs wrote: > Please verify that this test fails reliably (in the test-runner) without the > fix. (There still appear to be a risk of timer vs. frame race here.) I have verified it , with out this fix, the test case fails. Ran the test 100 times (Getting intermediate color between red and green) If we get any color other than green as output, then test will fail. Also I don't mind to increase the "dur" little bit more (may be 0.09s)
On 2015/05/08 11:30:56, Shanmuga Pandi wrote: > https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... > File LayoutTests/svg/animations/animate-restart-never.html (right): > > https://codereview.chromium.org/1119723002/diff/80001/LayoutTests/svg/animati... > LayoutTests/svg/animations/animate-restart-never.html:27: dur="0.05s" > On 2015/05/08 10:28:31, fs wrote: > > Please verify that this test fails reliably (in the test-runner) without the > > fix. (There still appear to be a risk of timer vs. frame race here.) > > I have verified it , with out this fix, the test case fails. Ran the test 100 > times (Getting intermediate color between red and green) Ok, good. > If we get any color other than green as output, then test will fail. > Also I don't mind to increase the "dur" little bit more (may be 0.09s) We should keep 'dur' as short as possible since it's the main contributor to the runtime of the test. LGTM.
The CQ bit was checked by shanmuga.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119723002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
On 2015/05/08 13:32:17, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...) webkit_tests webkit_tests failures: virtual/slimmingpaint/fast/forms/textarea/textarea-placeholder-paint-order-2.html Looks to be this test case is flawky in Linux.
The CQ bit was checked by shanmuga.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119723002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195170 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
