Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1720403002:
Alternative syntax for element.animate list of keyframes (Closed)
|
Created:
4 years, 10 months ago by suzyh_UTC10 (ex-contributor) Modified:
4 years, 9 months ago CC:
alancutter (OOO until 2018), darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright Base URL:
https://chromium.googlesource.com/chromium/src.git@animations-keyframeeffect-api Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlternative syntax for element.animate list of keyframes
This patch implements and adds tests for the object-form for specifying
a list of keyframes as an argument to element.animate()
(http://w3c.github.io/web-animations/#processing-a-frames-argument).
BUG=589701
Committed: https://crrev.com/c0846df098b6a1ba6487d7c351d1f3bb0d6f7690
Cr-Commit-Position: refs/heads/master@{#378170}
Patch Set 1 #Patch Set 2 : Refactor #Patch Set 3 : Fill in error message strings #
Total comments: 32
Patch Set 4 : Response to review comments #Patch Set 5 : Rebase #
Total comments: 6
Patch Set 6 : Fix mistake in rebase #
Total comments: 3
Patch Set 7 : Change to error types and how offset/composite are handled, plus minor fixes #
Total comments: 6
Patch Set 8 : Response to review nits, added tests #Patch Set 9 : Additional (mismatched-length-list) test; ensure offset is initialized #Messages
Total messages: 51 (23 generated)
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== WIP: Alternative keyframe list syntax for element.animate BUG= ========== to ========== Alternative syntax for element.animate list of keyframes This patch implements and adds tests for the object-form for specifying a list of keyframes as an argument to element.animate() (http://w3c.github.io/web-animations/#processing-a-frames-argument). BUG= ==========
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
suzyh@chromium.org changed reviewers: + shans@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
This is a first pass review, haven't assessed the tests in great detail yet. Please add a bug number for this patch. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:19: var durationValue = 1; Why not inline this if it never changes? It's common in animation tests to use 0-1 time fractions when testing effect output. It would also make the expectations easier to read. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:25: return target; It's strange to see the name target used here and no where else. I would name this element or name every other reference to it target for consistency. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:28: var assertEquivalentFramesSyntax = function(listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList) { Javascript style nit: Consider using destructuring so the call sites don't have singe use variables hanging around. function assertEquivalentFramesSyntax({listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList}) { https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:33: var options = { duration: durationArg, fill: "forwards" }; No need for fill forwards if you never seek past 1. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:53: return (a->offset() < b->offset()); No need for the outer brackets. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:55: } // namespace Blank lines around the namespace boundaries. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:69: bool EffectInput::setKeyframeValue(Element* element, StringKeyframe* keyframe, const String& property, const String& value) Blink style dictates the element and keyframe parameters should be a mutable references. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:92: element->document().updateLayoutTreeForNodeIfNeeded(element); This code always needs to be run before the call to forceConversionsToAnimatableValues() in createEffectModelFromKeyframes() so it should just be inlined there in that function. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:95: EffectModel* EffectInput::createEffectModelFromKeyframes(Element* element, const StringKeyframeVector& keyframes, ExceptionState& exceptionState) element should be a reference. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:183: if (isList) No need for single use bool variable. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:184: exceptionState.throwDOMException(InvalidModificationError, "Lists of values not permitted in array-form list of keyframes"); Is there a test for this exception? Shouldn't we return nullptr here? https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:187: DictionaryHelper::get(keyframeDictionary, property, value); Can this return false if the user provides their own toString() method? Example: element.animate({color: {toString() {}}}) If so we should probably throw a TypeError and have a test case or if not add an ASSERT. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.h (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.h:32: static bool setKeyframeValue(Element*, StringKeyframe*, const String& property, const String& value); Add comment for what the return value means.
Description was changed from ========== Alternative syntax for element.animate list of keyframes This patch implements and adds tests for the object-form for specifying a list of keyframes as an argument to element.animate() (http://w3c.github.io/web-animations/#processing-a-frames-argument). BUG= ========== to ========== Alternative syntax for element.animate list of keyframes This patch implements and adds tests for the object-form for specifying a list of keyframes as an argument to element.animate() (http://w3c.github.io/web-animations/#processing-a-frames-argument). BUG=589701 ==========
> Please add a bug number for this patch. Done. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:19: var durationValue = 1; On 2016/02/24 at 08:50:02, alancutter wrote: > Why not inline this if it never changes? It's common in animation tests to use 0-1 time fractions when testing effect output. > It would also make the expectations easier to read. This was in the original code. I'd prefer to err on the side of keeping it than replacing it with magic numbers. It looks like the variable was added in response to a comment on the patch where this test was first added: https://codereview.chromium.org/96283002/patch/130001/90003 https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:25: return target; On 2016/02/24 at 08:50:02, alancutter wrote: > It's strange to see the name target used here and no where else. I would name this element or name every other reference to it target for consistency. Done. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:28: var assertEquivalentFramesSyntax = function(listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList) { On 2016/02/24 at 08:50:02, alancutter wrote: > Javascript style nit: Consider using destructuring so the call sites don't have singe use variables hanging around. > > function assertEquivalentFramesSyntax({listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList}) { Done. I get a syntax error when trying to use var assertEquivalentFramesSyntax = function({listOfKeyframes ... etc so I'm passing it in as a single argument testParams. Let me know if you'd prefer I do it a different way. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:33: var options = { duration: durationArg, fill: "forwards" }; On 2016/02/24 at 08:50:02, alancutter wrote: > No need for fill forwards if you never seek past 1. The tests fail if I seek to 1, not just past 1. Does that indicate something else is wrong? https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:53: return (a->offset() < b->offset()); On 2016/02/24 at 08:50:02, alancutter wrote: > No need for the outer brackets. Done. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:55: } // namespace On 2016/02/24 at 08:50:02, alancutter wrote: > Blank lines around the namespace boundaries. Done. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:69: bool EffectInput::setKeyframeValue(Element* element, StringKeyframe* keyframe, const String& property, const String& value) On 2016/02/24 at 08:50:02, alancutter wrote: > Blink style dictates the element and keyframe parameters should be a mutable references. I'm happy to make the keyframe parameter a StringKeyframe&, but the Element* parameter is an existing parameter from before I touched this file. Should all the Element* parameters be changed (in which case all the call sites of these functions will need to be updated) or is this an acceptable exception? https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:92: element->document().updateLayoutTreeForNodeIfNeeded(element); On 2016/02/24 at 08:50:02, alancutter wrote: > This code always needs to be run before the call to forceConversionsToAnimatableValues() in createEffectModelFromKeyframes() so it should just be inlined there in that function. Done. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:183: if (isList) On 2016/02/24 at 08:50:02, alancutter wrote: > No need for single use bool variable. Done. I used this here to be consistent with the case lower down, although I guess it is clearer not to use it here. For the case below, I think the code is clearer with the explicit bool, even if it is single-use. What do you think? https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:184: exceptionState.throwDOMException(InvalidModificationError, "Lists of values not permitted in array-form list of keyframes"); On 2016/02/24 at 08:50:02, alancutter wrote: > Is there a test for this exception? > Shouldn't we return nullptr here? Test: Done. Return nullptr: Whoops, yes. Fixed. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:187: DictionaryHelper::get(keyframeDictionary, property, value); On 2016/02/24 at 08:50:02, alancutter wrote: > Can this return false if the user provides their own toString() method? > Example: element.animate({color: {toString() {}}}) > If so we should probably throw a TypeError and have a test case or if not add an ASSERT. Hm, I have no idea. The existing code used DictionaryHelper::get without checking its return value. Can you file a bug to look into this later? https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.h (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.h:32: static bool setKeyframeValue(Element*, StringKeyframe*, const String& property, const String& value); On 2016/02/24 at 08:50:02, alancutter wrote: > Add comment for what the return value means. Done.
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/80001
The CQ bit was unchecked by suzyh@chromium.org
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:19: var durationValue = 1; On 2016/02/25 at 02:42:35, suzyh wrote: > On 2016/02/24 at 08:50:02, alancutter wrote: > > Why not inline this if it never changes? It's common in animation tests to use 0-1 time fractions when testing effect output. > > It would also make the expectations easier to read. > > This was in the original code. I'd prefer to err on the side of keeping it than replacing it with magic numbers. It looks like the variable was added in response to a comment on the patch where this test was first added: https://codereview.chromium.org/96283002/patch/130001/90003 Looking back at when it was added it was because the duration for element.animate() was hard coded in C++ to be 1 second and the variable was a way of matching the C++ behaviour at a single point. Now that the test is capable of setting the duration itself the variable's purpose no longer remains. I don't mind if it stays since it was already part of the test but I would be in favour of culling it off. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:28: var assertEquivalentFramesSyntax = function(listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList) { On 2016/02/25 at 02:42:35, suzyh wrote: > On 2016/02/24 at 08:50:02, alancutter wrote: > > Javascript style nit: Consider using destructuring so the call sites don't have singe use variables hanging around. > > > > function assertEquivalentFramesSyntax({listOfKeyframes, keyframeWithListOfValues, durationArg, expectationList}) { > > Done. I get a syntax error when trying to use > var assertEquivalentFramesSyntax = function({listOfKeyframes ... etc > so I'm passing it in as a single argument testParams. > Let me know if you'd prefer I do it a different way. Strange, I don't get a syntax error when I change the line to be: function assertEquivalentFramesSyntax({listOfKeyframes, keyframeWithListOfValues, duration, expectationList}) { https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:33: var options = { duration: durationArg, fill: "forwards" }; On 2016/02/25 at 02:42:35, suzyh wrote: > On 2016/02/24 at 08:50:02, alancutter wrote: > > No need for fill forwards if you never seek past 1. > > The tests fail if I seek to 1, not just past 1. Does that indicate something else is wrong? Ah my bad, fill forwards is needed for t=1 you're right. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:69: bool EffectInput::setKeyframeValue(Element* element, StringKeyframe* keyframe, const String& property, const String& value) On 2016/02/25 at 02:42:35, suzyh wrote: > On 2016/02/24 at 08:50:02, alancutter wrote: > > Blink style dictates the element and keyframe parameters should be a mutable references. > > I'm happy to make the keyframe parameter a StringKeyframe&, but the Element* parameter is an existing parameter from before I touched this file. Should all the Element* parameters be changed (in which case all the call sites of these functions will need to be updated) or is this an acceptable exception? If a function dereferences it without performing a null check then it ought to be a reference, this is the case for setKeyframeValue() and createEffectModelFromKeyframes(). https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:183: if (isList) On 2016/02/25 at 02:42:36, suzyh wrote: > On 2016/02/24 at 08:50:02, alancutter wrote: > > No need for single use bool variable. > > Done. I used this here to be consistent with the case lower down, although I guess it is clearer not to use it here. For the case below, I think the code is clearer with the explicit bool, even if it is single-use. What do you think? True, the name does add to code clarity. Feel free to retain it though personally I think the error message in the body is enough indication. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:209: bool frameHasComposite = DictionaryHelper::get(keyframeDictionary, "composite", compositeString); I think the spec wants us to apply the composite value to all keyframes just like easing despite it missing from the dictionary definition. The later parts of the spec treat it in the same way as easing. Will need to clarify with spec editor on this. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:214: } I don't think this behaviour matches spec, I interpret the spec to ignore the offset in single keyframes. I suspect this is a change in behaviour and would result in some test cases failing though. Will need to clarify with spec editor on this. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.h (right): https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.h:40: static EffectModel* createEffectModelFromKeyframes(Element*, const Vector<RefPtr<StringKeyframe>>& keyframes, bool encounteredCompositableProperty, ExceptionState&); No need for private static methods, these can live in an anonymous namespace in the cpp file. This will probably reduce the number of includes and forward declarations in the header file. https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:186: exceptionState.throwDOMException(InvalidModificationError, exceptionMessage); This is definitely not a modification error, I'd go with SyntaxError or TypeError here.
LGTM Would be nice to see some tests with more than 2 values per list. https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:186: exceptionState.throwDOMException(InvalidModificationError, exceptionMessage); On 2016/02/25 at 06:52:27, alancutter wrote: > This is definitely not a modification error, I'd go with SyntaxError or TypeError here. Spec says TypeError.
PTAL. Probably still need to add tests for offset/composite handling. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:19: var durationValue = 1; On 2016/02/25 at 06:52:27, alancutter wrote: > On 2016/02/25 at 02:42:35, suzyh wrote: > > On 2016/02/24 at 08:50:02, alancutter wrote: > > > Why not inline this if it never changes? It's common in animation tests to use 0-1 time fractions when testing effect output. > > > It would also make the expectations easier to read. > > > > This was in the original code. I'd prefer to err on the side of keeping it than replacing it with magic numbers. It looks like the variable was added in response to a comment on the patch where this test was first added: https://codereview.chromium.org/96283002/patch/130001/90003 > > Looking back at when it was added it was because the duration for element.animate() was hard coded in C++ to be 1 second and the variable was a way of matching the C++ behaviour at a single point. Now that the test is capable of setting the duration itself the variable's purpose no longer remains. > > I don't mind if it stays since it was already part of the test but I would be in favour of culling it off. Oh, I see. Removed. https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:69: bool EffectInput::setKeyframeValue(Element* element, StringKeyframe* keyframe, const String& property, const String& value) On 2016/02/25 at 06:52:27, alancutter wrote: > On 2016/02/25 at 02:42:35, suzyh wrote: > > On 2016/02/24 at 08:50:02, alancutter wrote: > > > Blink style dictates the element and keyframe parameters should be a mutable references. > > > > I'm happy to make the keyframe parameter a StringKeyframe&, but the Element* parameter is an existing parameter from before I touched this file. Should all the Element* parameters be changed (in which case all the call sites of these functions will need to be updated) or is this an acceptable exception? > > If a function dereferences it without performing a null check then it ought to be a reference, this is the case for setKeyframeValue() and createEffectModelFromKeyframes(). Done. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:209: bool frameHasComposite = DictionaryHelper::get(keyframeDictionary, "composite", compositeString); On 2016/02/25 at 06:52:27, alancutter wrote: > I think the spec wants us to apply the composite value to all keyframes just like easing despite it missing from the dictionary definition. The later parts of the spec treat it in the same way as easing. Will need to clarify with spec editor on this. Changed as discussed offline. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.cpp:214: } On 2016/02/25 at 06:52:27, alancutter wrote: > I don't think this behaviour matches spec, I interpret the spec to ignore the offset in single keyframes. I suspect this is a change in behaviour and would result in some test cases failing though. Will need to clarify with spec editor on this. Changed as discussed offline. https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/EffectInput.h (right): https://codereview.chromium.org/1720403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/EffectInput.h:40: static EffectModel* createEffectModelFromKeyframes(Element*, const Vector<RefPtr<StringKeyframe>>& keyframes, bool encounteredCompositableProperty, ExceptionState&); On 2016/02/25 at 06:52:27, alancutter wrote: > No need for private static methods, these can live in an anonymous namespace in the cpp file. This will probably reduce the number of includes and forward declarations in the header file. Done. https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:186: exceptionState.throwDOMException(InvalidModificationError, exceptionMessage); On 2016/02/25 at 22:30:28, shans wrote: > On 2016/02/25 at 06:52:27, alancutter wrote: > > This is definitely not a modification error, I'd go with SyntaxError or TypeError here. > > Spec says TypeError. As discussed offline, I've made all these InvalidModificationErrors into TypeErrors, and updated tests accordingly.
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:77: { at: 1, is: { left: '150px', opacity: '1' } } We should maintain a consistent style within the same file, other literal objects don't have spaces padding the inside of their braces. https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:257: } I think we can avoid having the offsets vector and these loops entirely if we inline this logic in the below for-loop. https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:264: const String& value = values[i]; No need for this local variable.
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/140001
On 2016/02/28 at 23:56:53, commit-bot wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1720403002/140001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1720403002/140001 New processing of offset, easing and composite LGTM. Could you add a test with: element.animate({width: ['0px', '100px'], opacity: [0, 0.5, 1]}, ..) or similar? Just to demonstrate property lists with mismatched lengths.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/160001
On 2016/02/29 at 00:22:52, shans wrote: > On 2016/02/28 at 23:56:53, commit-bot wrote: > > Dry run: CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1720403002/140001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1720403002/140001 > > New processing of offset, easing and composite LGTM. > > Could you add a test with: > element.animate({width: ['0px', '100px'], opacity: [0, 0.5, 1]}, ..) > > or similar? Just to demonstrate property lists with mismatched lengths. Done.
https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:77: { at: 1, is: { left: '150px', opacity: '1' } } On 2016/02/28 at 22:47:35, alancutter wrote: > We should maintain a consistent style within the same file, other literal objects don't have spaces padding the inside of their braces. Done. https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/animation/EffectInput.cpp (right): https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:257: } On 2016/02/28 at 22:47:35, alancutter wrote: > I think we can avoid having the offsets vector and these loops entirely if we inline this logic in the below for-loop. Good point. Done. https://codereview.chromium.org/1720403002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/animation/EffectInput.cpp:264: const String& value = values[i]; On 2016/02/28 at 22:47:35, alancutter wrote: > No need for this local variable. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by suzyh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org, shans@chromium.org Link to the patchset: https://codereview.chromium.org/1720403002/#ps160001 (title: "Additional (mismatched-length-list) test; ensure offset is initialized")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720403002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720403002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Alternative syntax for element.animate list of keyframes This patch implements and adds tests for the object-form for specifying a list of keyframes as an argument to element.animate() (http://w3c.github.io/web-animations/#processing-a-frames-argument). BUG=589701 ========== to ========== Alternative syntax for element.animate list of keyframes This patch implements and adds tests for the object-form for specifying a list of keyframes as an argument to element.animate() (http://w3c.github.io/web-animations/#processing-a-frames-argument). BUG=589701 Committed: https://crrev.com/c0846df098b6a1ba6487d7c351d1f3bb0d6f7690 Cr-Commit-Position: refs/heads/master@{#378170} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c0846df098b6a1ba6487d7c351d1f3bb0d6f7690 Cr-Commit-Position: refs/heads/master@{#378170} |
