Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127313007:
SVG animate element's begin/end attribute set by js, should animate. (Closed)
|
Created:
5 years, 7 months ago by Shanmuga Pandi Modified:
5 years, 7 months ago CC:
blink-reviews, 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 element's begin/end attribute set by js, should animate.
When begin/end attribute changed, new conditions can be formed.
So it needs to connect the event base conditions again.
BUG=122846
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195296
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added testharness based layout test #Patch Set 3 : small nits #
Total comments: 4
Patch Set 4 : align with review comments #Patch Set 5 : Removed testRunner from Layout test #
Messages
Total messages: 17 (5 generated)
shanmuga.m@samsung.com changed reviewers: + ed@opera.com, fs@opera.com, pdr@chromium.org
Please review this change. Thanks
Please use js-test.js (or even testharness.js) instead of rolling your own. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... File LayoutTests/svg/animations/animation-begin-change-js.html (right): https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:3: <rect id="rect" x="0" width="100px" height="100px" fill='orange'> x="0" and fill='orange' doesn't appear to be needed. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:4: <animate id="anim" Can you use a <set> instead? https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:5: begin="infinite" indefinite? https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:11: <text y="150" x="20" id="console"/> Looks unnecessary. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:16: function Test() { Could just use an anonymous function() directly in setTimeout. Why is the setTimeout even needed though? https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:24: testRunner.displayAsyncThen(function() { Ought to be possible to run this on the 'begin' event instead.
Also I could not upload -expected.txt. Since I am getting below error * The following files are passing testharness results without console error messages, they should be removed: /home/shanmuga/Chrome-Desktop/src/third_party/WebKit/LayoutTests/svg/animations/animation-begin-change-js-expected.txt ERROR: found passing testharness results without console error messages. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... File LayoutTests/svg/animations/animation-begin-change-js.html (right): https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:3: <rect id="rect" x="0" width="100px" height="100px" fill='orange'> On 2015/05/11 15:41:53, fs wrote: > x="0" and fill='orange' doesn't appear to be needed. Done. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:4: <animate id="anim" On 2015/05/11 15:41:53, fs wrote: > Can you use a <set> instead? Done. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:5: begin="infinite" On 2015/05/11 15:41:53, fs wrote: > indefinite? Acknowledged. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:11: <text y="150" x="20" id="console"/> On 2015/05/11 15:41:53, fs wrote: > Looks unnecessary. Done. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:16: function Test() { On 2015/05/11 15:41:53, fs wrote: > Could just use an anonymous function() directly in setTimeout. Why is the > setTimeout even needed though? Done. https://codereview.chromium.org/1127313007/diff/1/LayoutTests/svg/animations/... LayoutTests/svg/animations/animation-begin-change-js.html:24: testRunner.displayAsyncThen(function() { On 2015/05/11 15:41:53, fs wrote: > Ought to be possible to run this on the 'begin' event instead. begin event will not be triggered with out this patch. So with out this patch, timeout occurs. So I keep it same
On 2015/05/12 09:43:14, Shanmuga Pandi wrote: > Also I could not upload -expected.txt. > Since I am getting below error > * The following files are passing testharness results without console error > messages, they should be removed: > > /home/shanmuga/Chrome-Desktop/src/third_party/WebKit/LayoutTests/svg/animations/animation-begin-change-js-expected.txt > ERROR: found passing testharness results without console error messages. Yes, that would be expected when using testharness.js I think, so it's fine. > > Ought to be possible to run this on the 'begin' event instead. > > begin event will not be triggered with out this patch. > So with out this patch, timeout occurs. So I keep it same That doesn't seem like a problem to me (rather the opposite.)
https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animation-begin-change-js.html (right): https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animation-begin-change-js.html:2: <title>Tests that animation works, if begin is set by js.</title> Try to make this describe the test better (it's very generic in its current form.) https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animation-begin-change-js.html:33: assert_not_equals(width, "100"); This is very atypical use of testharness I'd say. This would be better structured as an async_test. Using the 'begin' event I think it would look something like: var anim = ...; var t = async_test("Changing 'begin' to contain an event-base trigger."); anim.onbegin = t.step_func(function() { assert_equals(document.getElementById("rect").getAttribute("width"), 100); t.done(); }); anim.setAttribute(...); <dispatch click> No access to testRunner should be needed at all.
On 2015/05/12 10:17:53, fs wrote: > On 2015/05/12 09:43:14, Shanmuga Pandi wrote: > > Also I could not upload -expected.txt. > > Since I am getting below error > > * The following files are passing testharness results without console error > > messages, they should be removed: > > > > > /home/shanmuga/Chrome-Desktop/src/third_party/WebKit/LayoutTests/svg/animations/animation-begin-change-js-expected.txt > > ERROR: found passing testharness results without console error messages. > > Yes, that would be expected when using testharness.js I think, so it's fine. > > > > Ought to be possible to run this on the 'begin' event instead. > > > > begin event will not be triggered with out this patch. > > So with out this patch, timeout occurs. So I keep it same > > That doesn't seem like a problem to me (rather the opposite.) Done the change.
https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... File LayoutTests/svg/animations/animation-begin-change-js.html (right): https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animation-begin-change-js.html:2: <title>Tests that animation works, if begin is set by js.</title> On 2015/05/12 10:35:48, fs wrote: > Try to make this describe the test better (it's very generic in its current > form.) Done. https://codereview.chromium.org/1127313007/diff/40001/LayoutTests/svg/animati... LayoutTests/svg/animations/animation-begin-change-js.html:33: assert_not_equals(width, "100"); On 2015/05/12 10:35:48, fs wrote: > This is very atypical use of testharness I'd say. This would be better > structured as an async_test. Using the 'begin' event I think it would look > something like: > > var anim = ...; > var t = async_test("Changing 'begin' to contain an event-base trigger."); > anim.onbegin = t.step_func(function() { > assert_equals(document.getElementById("rect").getAttribute("width"), 100); > t.done(); > }); > > anim.setAttribute(...); > <dispatch click> > > No access to testRunner should be needed at all. Changed according to your suggestion. Thank you.
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/1127313007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/62491)
The CQ bit was unchecked by shanmuga.m@samsung.com
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/1127313007/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195296 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
