Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26247003:
Web Animations CSS: Support animation of background-position and background-image (Closed)
|
Created:
7 years, 2 months ago by alancutter (OOO until 2018) Modified:
7 years, 2 months ago CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), dglazkov+blink, dstockwell, Timothy Loh, apavlov+blink_chromium.org, darktears, dino_apple.com, Eric Willigers Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionWeb Animations CSS: Support animation of background-position and background-image
Existing layout tests for background-image depend on background-size
animation being implemented before they can be enabled in TestExpectations.
The current implementation does not animate mis-matched background-position
value counts correctly and is expected to fail the newly added test.
The test passes under the Web Animations implementation.
Relevant spec:
http://dev.w3.org/csswg/css-backgrounds/#the-background-position
http://dev.w3.org/csswg/css-backgrounds/#the-background-image
BUG=257591
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159396
Patch Set 1 #
Total comments: 37
Patch Set 2 : Review changes #
Total comments: 18
Patch Set 3 : Review changes #
Total comments: 6
Patch Set 4 : Assertion rearrangement #Patch Set 5 : Fixed test expectations #Patch Set 6 : Refixed test expectations #Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation-expected.txt (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation-expected.txt:2: FAIL: background-position from [0px 0px, 80px 0px] to [40px 40px, 80px 80px, 0px 80px] was [-10px -10px, 80px -20px, 0px -20px, 0% 0%] at -0.25, expected [-10px -10px, 80px -20px, 0px -20px, 90px -10px] change description should explain why these are failing https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:54: if (!from->isSameType(to)) Needs a comment
https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.h (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.h:54: ASSERT(!values.isEmpty()); why? https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... Source/core/css/resolver/AnimatedStyleBuilder.cpp:99: ASSERT(!values.isEmpty()); I didn't see this assertion in the factory.
https://codereview.chromium.org/26247003/diff/1/Source/core/animation/css/CSS... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/css/CSS... Source/core/animation/css/CSSAnimatableValueFactory.cpp:133: inline static PassRefPtr<AnimatableValue> createFromFillLayers(const FillLayer* fillLayer, T getter, bool (FillLayer::*isSet)() const, const RenderStyle* style) This seems a bit yuck. Couldn't this just be a single method templated on CSSPropertyID? No need for function pointers... https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... Source/core/css/resolver/AnimatedStyleBuilder.cpp:116: void setImageOnFillLayers(FillLayer* fillLayer, const AnimatableValue* value) and here, if we template on CSSPropertyID we should only need one method
https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation-expected.txt (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation-expected.txt:8: FAIL: background-position from [0px 0px, 80px 0px] to [40px 40px, 80px 80px, 0px 80px] was [50px 50px, 80px 100px, 0px 100px, 0% 0%] at 1.25, expected [50px 50px, 80px 100px, 0px 100px, 30px 50px] Can you mention in the description what's so broken with the legacy implementation - it's hard to figure out from this. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:10: background-image: radial-gradient(circle 20px at 20px 20px, red 0px, red 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, yellow 0px, yellow 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, lime 0px, lime 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, blue 0px, blue 18px, transparent 20px); Does each of the radial-gradient()s need to be so complex? It adds unnecessary complexity to the test. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:22: to: '40px 40px, 80px 80px, 0px 80px' It looks like you have 4 backgrounds and test animating from 2 positions to 2 positions. It would be good to test equal numbers of positions, as well as numbers of positions that are equal to and greater than the number of images - http://dev.w3.org/csswg/css-backgrounds/#layering Also, we should test the 4 value syntax - http://dev.w3.org/csswg/css-backgrounds/#background-position https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:30: {at: 1.25, is: '50px 50px, 80px 100px, 0px 100px, 30px 50px'}, It might not be 'correct' style, but this would be much easier to read if you vertically aligned the values - https://codereview.chromium.org/26292004/diff/28001/LayoutTests/animations/in... https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:36: static size_t gcd(size_t a, size_t b) I think Blink style is to use size_t only for sizes. Values which happen to be always positive should use int and assertions. Also, spell out names - greatestCommonDivisor(). https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:41: static size_t lcm(size_t a, size_t b) Hmm, I've just implemented lowestCommonMultiple() in https://codereview.chromium.org/26292004/diff/28001/Source/core/animation/Ani.... We should share the two. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:54: if (!from->isSameType(to)) When could this happen? Should it be an assertion? https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:84: return true; If we added operator==() to AnimatableValue(), we could just use Vector<>::operator==() here. WDYT? https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.h (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.h:39: class AnimatableRepeatable : public AnimatableValue { 'Repeatable' seems like the wrong name - it should be a noun. 'RepeatedValue'? 'Vector'? 'List'?
https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:36: static size_t gcd(size_t a, size_t b) I think we prefer anonymous namespaces to static https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:50: Vector<RefPtr<AnimatableValue> > interpolatedValues(lcm(m_values.size(), otherValues.size())); If one or other of the lists has zero entries, lcm() will hit the assertion. Can this happen? What's the correct behavior?
I've changed the implementation to use a switch on a templated CSS property ID instead of function pointers. I don't think it's very good but it's worth putting up for review for comments. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation-expected.txt (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation-expected.txt:2: FAIL: background-position from [0px 0px, 80px 0px] to [40px 40px, 80px 80px, 0px 80px] was [-10px -10px, 80px -20px, 0px -20px, 0% 0%] at -0.25, expected [-10px -10px, 80px -20px, 0px -20px, 90px -10px] On 2013/10/08 22:52:05, dstockwell wrote: > change description should explain why these are failing Done. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation-expected.txt:8: FAIL: background-position from [0px 0px, 80px 0px] to [40px 40px, 80px 80px, 0px 80px] was [50px 50px, 80px 100px, 0px 100px, 0% 0%] at 1.25, expected [50px 50px, 80px 100px, 0px 100px, 30px 50px] On 2013/10/08 23:21:49, Steve Block wrote: > Can you mention in the description what's so broken with the legacy > implementation - it's hard to figure out from this. Done. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:10: background-image: radial-gradient(circle 20px at 20px 20px, red 0px, red 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, yellow 0px, yellow 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, lime 0px, lime 18px, transparent 20px), radial-gradient(circle 20px at 20px 20px, blue 0px, blue 18px, transparent 20px); On 2013/10/08 23:21:49, Steve Block wrote: > Does each of the radial-gradient()s need to be so complex? It adds unnecessary > complexity to the test. Simplified the syntax a little and put them on individual lines. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:22: to: '40px 40px, 80px 80px, 0px 80px' On 2013/10/08 23:21:49, Steve Block wrote: > It looks like you have 4 backgrounds and test animating from 2 positions to 2 positions. It's actually testing from 2 positions to 3 positions which should invoke a 6 length interpolation, more than the number of background-images. > It would be good to test equal numbers of positions, as well as > numbers of positions that are equal to and greater than the number of images - > http://dev.w3.org/csswg/css-backgrounds/#layering I think testing fewer than the number of images is also a good idea in retrospect to ensure the values are repeated over the image layers correctly. Added test interpolating one background-position value. > Also, we should test the 4 value syntax - > http://dev.w3.org/csswg/css-backgrounds/#background-position The 4 value syntax is reduced to a canonical 2 value form in the CSS parsing stage, this won't affect the interpolation coverage but it couldn't hurt to do so. https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:30: {at: 1.25, is: '50px 50px, 80px 100px, 0px 100px, 30px 50px'}, On 2013/10/08 23:21:49, Steve Block wrote: > It might not be 'correct' style, but this would be much easier to read if you > vertically aligned the values - > https://codereview.chromium.org/26292004/diff/28001/LayoutTests/animations/in... Done. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:36: static size_t gcd(size_t a, size_t b) On 2013/10/08 23:27:23, Steve Block wrote: > I think we prefer anonymous namespaces to static Done. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:41: static size_t lcm(size_t a, size_t b) On 2013/10/08 23:21:49, Steve Block wrote: > Hmm, I've just implemented lowestCommonMultiple() in > https://codereview.chromium.org/26292004/diff/28001/Source/core/animation/Ani.... > We should share the two. Given that you intend to rebase ontop of this patch it might be best to land it as it is here and then pull it out as a utility function in the rebase. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:50: Vector<RefPtr<AnimatableValue> > interpolatedValues(lcm(m_values.size(), otherValues.size())); On 2013/10/08 23:27:23, Steve Block wrote: > If one or other of the lists has zero entries, lcm() will hit the assertion. Can > this happen? What's the correct behavior? Added support for 0 length AnimatableRepeatables. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:54: if (!from->isSameType(to)) On 2013/10/08 23:21:49, Steve Block wrote: > When could this happen? Should it be an assertion? This is most likely to happen when a keyword is used as one of the values. See background-size for a possible scenario: http://dev.w3.org/csswg/css-backgrounds/#the-background-size Personally I think the spec made an unintuitive decision here but there it is. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:54: if (!from->isSameType(to)) On 2013/10/08 22:52:05, dstockwell wrote: > Needs a comment Added comment for method. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:84: return true; On 2013/10/08 23:21:49, Steve Block wrote: > If we added operator==() to AnimatableValue(), we could just use > Vector<>::operator==() here. WDYT? As discussed because it's a vector of RefPtrs we don't gain this shortcut. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.h (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.h:39: class AnimatableRepeatable : public AnimatableValue { On 2013/10/08 23:21:49, Steve Block wrote: > 'Repeatable' seems like the wrong name - it should be a noun. 'RepeatedValue'? > 'Vector'? 'List'? Repeatable refers to the way this value interpolates and matches the language used in the transitions spec: http://www.w3.org/TR/css3-transitions/#animtype-repeatable-list Added comment to class explaining this. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.h:54: ASSERT(!values.isEmpty()); On 2013/10/08 22:57:31, dstockwell wrote: > why? Removed non-zero length constraint. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/css/CSS... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/css/CSS... Source/core/animation/css/CSSAnimatableValueFactory.cpp:133: inline static PassRefPtr<AnimatableValue> createFromFillLayers(const FillLayer* fillLayer, T getter, bool (FillLayer::*isSet)() const, const RenderStyle* style) On 2013/10/08 23:02:46, dstockwell wrote: > This seems a bit yuck. Couldn't this just be a single method templated on > CSSPropertyID? No need for function pointers... I agree that function pointers and code duplication aren't very pretty but IMO the templated switch solution is even uglier. I'd added it to the patch for you to see. Perhaps I'm doing it in an ugly way but it's not entirely clear to me how to make it succinct without function pointers. https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... Source/core/css/resolver/AnimatedStyleBuilder.cpp:99: ASSERT(!values.isEmpty()); On 2013/10/08 22:57:31, dstockwell wrote: > I didn't see this assertion in the factory. Added assertion to factory. https://codereview.chromium.org/26247003/diff/1/Source/core/css/resolver/Anim... Source/core/css/resolver/AnimatedStyleBuilder.cpp:116: void setImageOnFillLayers(FillLayer* fillLayer, const AnimatableValue* value) On 2013/10/08 23:02:46, dstockwell wrote: > and here, if we template on CSSPropertyID we should only need one method Done.
https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... LayoutTests/animations/interpolation/background-position-interpolation.html:38: to: 'left 80px top 80px', test a matching case too? can we get a PASS from the existing implementation? https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:36: size_t greatestCommonDenominator(size_t a, size_t b) denominator -> divisor? https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:41: size_t lowestCommonMultiplier(size_t a, size_t b) multiplier -> multiple? https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:60: if (!from->isSameType(to)) comment here would still help https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/css... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/css... Source/core/animation/css/CSSAnimatableValueFactory.cpp:121: while (fillLayer) { Hmm, yeah I guess there's a bit too much going on here. Can we end up in a situation like this? [x-set, x-not-set, x-set] If not, seems OK to not terminate the loop early. Otherwise I think there are still a few ways to merge the switch statements and still exit, but its still not going to be so great. I don't hate the function pointer version... https://codereview.chromium.org/26247003/diff/16001/Source/core/css/resolver/... File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/css/resolver/... Source/core/css/resolver/AnimatedStyleBuilder.cpp:111: switch (property) { do we need this switch? if it's not one of these properties we should hit the ASSERT_NOT_REACHED on line 133
https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/1/LayoutTests/animations/interp... LayoutTests/animations/interpolation/background-position-interpolation.html:22: to: '40px 40px, 80px 80px, 0px 80px' Sorry, that was a typo. I meant 2 to 3. https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:36: static size_t gcd(size_t a, size_t b) What about using int rather than size_t? https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:41: static size_t lcm(size_t a, size_t b) Agreed https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... LayoutTests/animations/interpolation/background-position-interpolation.html:24: from: '0px 0px, 80px 0px', Superfluous space (or make these line up as you do for the expected values). https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:44: return 0; I think that handling of the case where one or both repeatable has zero length will be specific to the property. This method returns zero if either a or b is zero, but I don't think that producing a result with zero entries will be the common case (and may not be required at all). For example, for stroke-dasharray, we insert 2 values into the zero length list before interpolating. Given that you don't zero handling for this patch, I suggest we make this interpolateTo() method assert that both lengths are non-zero. Handling for the zero case can be done in the derived/wrapper class, or added here later if required. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:48: } // namespace
https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/1/Source/core/animation/Animata... Source/core/animation/AnimatableRepeatable.cpp:36: static size_t gcd(size_t a, size_t b) On 2013/10/09 23:19:05, Steve Block wrote: > What about using int rather than size_t? Because these functions are used for vector sizes based on vector sizes I think size_t is semantically appropriate. https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... File LayoutTests/animations/interpolation/background-position-interpolation.html (right): https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... LayoutTests/animations/interpolation/background-position-interpolation.html:24: from: '0px 0px, 80px 0px', On 2013/10/09 23:19:05, Steve Block wrote: > Superfluous space (or make these line up as you do for the expected values). Removed space. https://codereview.chromium.org/26247003/diff/16001/LayoutTests/animations/in... LayoutTests/animations/interpolation/background-position-interpolation.html:38: to: 'left 80px top 80px', On 2013/10/09 23:15:07, dstockwell wrote: > test a matching case too? can we get a PASS from the existing implementation? Added test for matching number of position values and added comments to each test. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:36: size_t greatestCommonDenominator(size_t a, size_t b) On 2013/10/09 23:15:07, dstockwell wrote: > denominator -> divisor? Done. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:41: size_t lowestCommonMultiplier(size_t a, size_t b) On 2013/10/09 23:15:07, dstockwell wrote: > multiplier -> multiple? Done. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:44: return 0; On 2013/10/09 23:19:05, Steve Block wrote: > I think that handling of the case where one or both repeatable has zero length > will be specific to the property. This method returns zero if either a or b is > zero, but I don't think that producing a result with zero entries will be the > common case (and may not be required at all). For example, for stroke-dasharray, > we insert 2 values into the zero length list before interpolating. > > Given that you don't zero handling for this patch, I suggest we make this > interpolateTo() method assert that both lengths are non-zero. Handling for the > zero case can be done in the derived/wrapper class, or added here later if > required. Done. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:48: } On 2013/10/09 23:19:05, Steve Block wrote: > // namespace Done. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:60: if (!from->isSameType(to)) On 2013/10/09 23:15:07, dstockwell wrote: > comment here would still help Done. https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/css... File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/animation/css... Source/core/animation/css/CSSAnimatableValueFactory.cpp:121: while (fillLayer) { On 2013/10/09 23:15:07, dstockwell wrote: > Hmm, yeah I guess there's a bit too much going on here. > > Can we end up in a situation like this? [x-set, x-not-set, x-set] > > If not, seems OK to not terminate the loop early. Otherwise I think there are > still a few ways to merge the switch statements and still exit, but its still > not going to be so great. I don't hate the function pointer version... Merged the switch statements using an if chain. https://codereview.chromium.org/26247003/diff/16001/Source/core/css/resolver/... File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right): https://codereview.chromium.org/26247003/diff/16001/Source/core/css/resolver/... Source/core/css/resolver/AnimatedStyleBuilder.cpp:111: switch (property) { On 2013/10/09 23:15:07, dstockwell wrote: > do we need this switch? if it's not one of these properties we should hit the > ASSERT_NOT_REACHED on line 133 Removed switch.
https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:43: if (!a && !b) This could be an OR Or switch back to the assert, since this code path should never be used. https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:56: ASSERT(lowestCommonMultiple(m_values.size(), otherValues.size())); I think we usually use a local variable, rather than repeating an expression for an assertion. The compiler should optimize it away in release builds anyway. https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:59: const AnimatableValue* from = m_values[i % m_values.size()].get(); We should also assert that neither list has length zero, else the behavior of i % size() here is undefined. Might be good to add a comment here or on the ctor that we only handle non-empty lists.
https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... File Source/core/animation/AnimatableRepeatable.cpp (right): https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:43: if (!a && !b) On 2013/10/10 03:34:52, Steve Block wrote: > This could be an OR > > Or switch back to the assert, since this code path should never be used. Done. https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:56: ASSERT(lowestCommonMultiple(m_values.size(), otherValues.size())); On 2013/10/10 03:34:52, Steve Block wrote: > I think we usually use a local variable, rather than repeating an expression for > an assertion. The compiler should optimize it away in release builds anyway. Asserts are now at the top of the function and inside lcm. https://codereview.chromium.org/26247003/diff/32001/Source/core/animation/Ani... Source/core/animation/AnimatableRepeatable.cpp:59: const AnimatableValue* from = m_values[i % m_values.size()].get(); On 2013/10/10 03:34:52, Steve Block wrote: > We should also assert that neither list has length zero, else the behavior of i > % size() here is undefined. > > Might be good to add a comment here or on the ctor that we only handle non-empty > lists. Added assert to constructor and top of function.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alancutter@chromium.org/26247003/42001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alancutter@chromium.org/26247003/58001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
Looks like you need to update the expected result for the legacy impl too.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alancutter@chromium.org/26247003/75001
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alancutter@chromium.org/26247003/75001
Message was sent while issue was closed.
Change committed as 159396 |
