Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
811993002:
Animation: Implement DoubleStyleInterpolation in StringKeyframe (Closed)
|
Created:
6 years ago by jadeg Modified:
5 years, 10 months ago Reviewers:
shans, alancutter (OOO until 2018), dstockwell, evemj (not active), samli, Eric Willigers CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, Eric Willigers, Mike Lawther (Google), rjwright, shans, Steve Block, Timothy Loh Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdding the case handling of CSSValues in string keyframe which use DoubleStyleInterpolation.
Have also implemented more clamping cases in DoubleStyleInterpolation to handle these cases.
BUG=443039
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190004
Patch Set 1 #Patch Set 2 : Implemented LengthBox, Double and fixed Length #
Total comments: 27
Patch Set 3 : Implementation of Double #Patch Set 4 : Add length Prtoperty #
Total comments: 9
Patch Set 5 : Opacity runs on compositor #
Total comments: 6
Patch Set 6 : Implement value cache function #
Total comments: 5
Patch Set 7 : Created more layout tests #
Total comments: 52
Patch Set 8 : Apply all of Alan and Doug's changes #
Total comments: 10
Patch Set 9 : Alter clamping #Patch Set 10 : rebase #
Total comments: 1
Patch Set 11 : Remove shape image threshold test #Messages
Total messages: 45 (8 generated)
jadeg@chromium.org changed reviewers: + dstockwell@chromium.org, ericwilligers@chromium.org, evem@chromium.org, shans@chromium.org
https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:155: case CSSPropertyBorderImageSlice: Let's move the two LengthBoxStyleInterpolation cases to a separate CL
samli@chromium.org changed reviewers: + samli@chromium.org
Also add a better title & description for this CL describing exactly what is being changed here. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:166: Remove new line https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:169: ditto
https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (left): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:114: case CSSPropertyWordSpacing: Removing case CSSPropertyLetterSpacing: case CSSPropertyOutlineOffset: case CSSPropertyWordSpacing: is incorrect: letter-spacing and outline-offset and word-spacing may be any length (including negative). Tested in http://jsfiddle.net/ericwilligers/jkppdmoL/ https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:121: return DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, NoClamp); clamp to non-negative values https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:123: return LengthStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, range); ValueRangeNonNegative https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:141: case CSSPropertyFlexBasis: Move CSSPropertyFlexBasis up to use ValueRangeNonNegative (like CSSPropertyHeight), since Negative values are invalid. Spec: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis Tested in http://jsfiddle.net/ericwilligers/bL2rzfv7/ (CSSPropertyTextIndent case below is ok - text indent may be negative)
https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (left): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:82: case CSSPropertyBorderLeftWidth: case CSSPropertyBorderBottomWidth: case CSSPropertyBorderLeftWidth: case CSSPropertyBorderRightWidth: case CSSPropertyBorderTopWidth: case CSSPropertyFontSize: should all use non-negative length (like CSSPropertyHeight) https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:92: case CSSPropertyLetterSpacing: letter spacer and word spacing and outline offset here are incorrect - they are (optionally negative) lengths, like CSSPropertyLeft. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:93: case CSSPropertyOrphans: The following are always integers: orphans webkit-column-count widows z-index https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:97: case CSSPropertyShapeImageThreshold: shape-image-threshold is clamped 0..1, just like opacity https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:106: case CSSPropertyWebkitColumnWidth: OutlineWidth and Perspective and WebkitBorderHorizontalSpacing and WebkitBorderVerticalSpacing and WebkitColumnGap and WebkitColumnWidth are non-negative lengths, like CSSPropertyWidth They are not double. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:111: if (DoubleStyleInterpolation::canCreateFrom(*fromCSSValue) && DoubleStyleInterpolation::canCreateFrom(*toCSSValue)) Note that Chrome does not permit negative values for the majority of these properties. The exceptions are FillOpacity FloodOpacity StopOpacity StrokeOpacity (and even these are later clamped to 0..1) ZIndex.
Is this change still in progress?
https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (left): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:82: case CSSPropertyBorderLeftWidth: On 2014/12/22 07:35:38, Eric Willigers wrote: > case CSSPropertyBorderBottomWidth: > case CSSPropertyBorderLeftWidth: > case CSSPropertyBorderRightWidth: > case CSSPropertyBorderTopWidth: > case CSSPropertyFontSize: > should all use non-negative length (like CSSPropertyHeight) Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:114: case CSSPropertyWordSpacing: On 2014/12/22 05:02:41, Eric Willigers wrote: > Removing > case CSSPropertyLetterSpacing: > case CSSPropertyOutlineOffset: > case CSSPropertyWordSpacing: > is incorrect: letter-spacing and outline-offset and word-spacing may be any > length (including negative). > > Tested in http://jsfiddle.net/ericwilligers/jkppdmoL/ Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:92: case CSSPropertyLetterSpacing: On 2014/12/22 07:35:38, Eric Willigers wrote: > letter spacer and word spacing and outline offset here are incorrect - they are > (optionally negative) lengths, like CSSPropertyLeft. Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:93: case CSSPropertyOrphans: On 2014/12/22 07:35:38, Eric Willigers wrote: > The following are always integers: orphans webkit-column-count widows z-index Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:97: case CSSPropertyShapeImageThreshold: On 2014/12/22 07:35:38, Eric Willigers wrote: > shape-image-threshold is clamped 0..1, just like opacity Have changed this, but just to note that this clamping was not picked up in the layout tests. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:106: case CSSPropertyWebkitColumnWidth: On 2014/12/22 07:35:38, Eric Willigers wrote: > OutlineWidth and Perspective and WebkitBorderHorizontalSpacing and > WebkitBorderVerticalSpacing and WebkitColumnGap and WebkitColumnWidth are > non-negative lengths, like CSSPropertyWidth > They are not double. Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:111: if (DoubleStyleInterpolation::canCreateFrom(*fromCSSValue) && DoubleStyleInterpolation::canCreateFrom(*toCSSValue)) On 2014/12/22 07:35:38, Eric Willigers wrote: > Note that Chrome does not permit negative values for the majority of these > properties. The exceptions are > FillOpacity > FloodOpacity > StopOpacity > StrokeOpacity > (and even these are later clamped to 0..1) > ZIndex. Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:111: if (DoubleStyleInterpolation::canCreateFrom(*fromCSSValue) && DoubleStyleInterpolation::canCreateFrom(*toCSSValue)) On 2014/12/22 07:35:38, Eric Willigers wrote: > Note that Chrome does not permit negative values for the majority of these > properties. The exceptions are > FillOpacity > FloodOpacity > StopOpacity > StrokeOpacity > (and even these are later clamped to 0..1) > ZIndex. Just to check, I have found no clamping rules for case CSSPropertyFlexGrow: case CSSPropertyFlexShrink: case CSSPropertyWebkitColumnRuleWidth: Not sure if this is correct or just a hole in the layout tests https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:121: return DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, NoClamp); On 2014/12/22 05:02:41, Eric Willigers wrote: > clamp to non-negative values Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:123: return LengthStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, range); On 2014/12/22 05:02:41, Eric Willigers wrote: > ValueRangeNonNegative Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:141: case CSSPropertyFlexBasis: On 2014/12/22 05:02:41, Eric Willigers wrote: > Move CSSPropertyFlexBasis up to use ValueRangeNonNegative (like > CSSPropertyHeight), since Negative values are invalid. > > Spec: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis > Tested in http://jsfiddle.net/ericwilligers/bL2rzfv7/ > > > > (CSSPropertyTextIndent case below is ok - text indent may be negative) Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:155: case CSSPropertyBorderImageSlice: On 2014/12/19 00:16:13, Eric Willigers wrote: > Let's move the two LengthBoxStyleInterpolation cases to a separate CL Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:166: On 2014/12/22 03:41:15, samli wrote: > Remove new line Done. https://codereview.chromium.org/811993002/diff/20001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:169: On 2014/12/22 03:41:15, samli wrote: > ditto Done.
Looks good. LengthStyleInterpolation should no longer be in the description. https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/Do... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:25: if (clamp == ClampOpacity) switch (clamp) https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:89: return DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, ClampZero); Do we need a break? https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:137: clamp = ClampOrphans; // Fall through https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:156: clamp = ClampZoom; // Fall through
https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/Do... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:25: if (clamp == ClampOpacity) On 2015/01/14 01:43:36, Eric Willigers wrote: > switch (clamp) Done. https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:89: return DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, ClampZero); On 2015/01/14 01:43:36, Eric Willigers wrote: > Do we need a break? No if it's not a double then it will be a length https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:137: clamp = ClampOrphans; On 2015/01/14 01:43:36, Eric Willigers wrote: > // Fall through Done. https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:156: clamp = ClampZoom; On 2015/01/14 01:43:36, Eric Willigers wrote: > // Fall through Done.
https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:89: return DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, ClampZero); On 2015/01/18 22:59:41, jadeg wrote: > On 2015/01/14 01:43:36, Eric Willigers wrote: > > Do we need a break? > > No if it's not a double then it will be a length OK, then we need // Fall through
On 2015/01/18 23:16:14, Eric Willigers wrote: > https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... > File Source/core/animation/StringKeyframe.cpp (right): > > https://codereview.chromium.org/811993002/diff/60001/Source/core/animation/St... > Source/core/animation/StringKeyframe.cpp:89: return > DoubleStyleInterpolation::create(*fromCSSValue, *toCSSValue, property, > ClampZero); > On 2015/01/18 22:59:41, jadeg wrote: > > On 2015/01/14 01:43:36, Eric Willigers wrote: > > > Do we need a break? > > > > No if it's not a double then it will be a length > > OK, then we need > // Fall through Done
lgtm
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); Why did this change? https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = clampTo<float>(doubleValue, 0.000000000000000000000000000000000000000000001); nextafterf(0, 1)? https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:172: toStringPropertySpecificKeyframe(end)->m_animatableValueCache = to; This code is identical to the code below. This should be refactored out as a function like ensureAnimatableValueCaches(fromKeyframe, fromCSSValue, toKeyframe, toCSSValue, element, property) with a FIXME to remove it.
https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); On 2015/01/19 02:52:30, alancutter wrote: > Why did this change? Done. https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = clampTo<float>(doubleValue, 0.000000000000000000000000000000000000000000001); On 2015/01/19 02:52:30, alancutter wrote: > nextafterf(0, 1)? This clamps to zero, not the correct value https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/St... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/St... Source/core/animation/StringKeyframe.cpp:172: toStringPropertySpecificKeyframe(end)->m_animatableValueCache = to; On 2015/01/19 02:52:30, alancutter wrote: > This code is identical to the code below. This should be refactored out as a > function like ensureAnimatableValueCaches(fromKeyframe, fromCSSValue, > toKeyframe, toCSSValue, element, property) with a FIXME to remove it. Done.
lgtm
https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/K... File Source/core/animation/Keyframe.h (right): https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/K... Source/core/animation/Keyframe.h:74: No need for this extra line https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (left): https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:165: break; Unindent this line and the following line. https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:317: only need one blank line here
On 2015/01/22 at 22:14:23, jadeg wrote: > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > File Source/core/animation/DoubleStyleInterpolation.cpp (right): > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); > On 2015/01/19 02:52:30, alancutter wrote: > > Why did this change? > > Done. Please reply to questions, 'Done.' means your reviewer needs to go back and figure out what actually happened. > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = clampTo<float>(doubleValue, 0.000000000000000000000000000000000000000000001); > On 2015/01/19 02:52:30, alancutter wrote: > > nextafterf(0, 1)? > > This clamps to zero, not the correct value I don't understand the comment, what range is RangeDoubleFractions meant to represent?
On 2015/01/23 00:44:23, dstockwell wrote: > On 2015/01/22 at 22:14:23, jadeg wrote: > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > File Source/core/animation/DoubleStyleInterpolation.cpp (right): > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = > clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); > > On 2015/01/19 02:52:30, alancutter wrote: > > > Why did this change? > > > > Done. > > Please reply to questions, 'Done.' means your reviewer needs to go back and > figure out what actually happened. > Sorry, I meant that I changed it back to what it was previously. > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = > clampTo<float>(doubleValue, 0.000000000000000000000000000000000000000000001); > > On 2015/01/19 02:52:30, alancutter wrote: > > > nextafterf(0, 1)? > > > > This clamps to zero, not the correct value > > I don't understand the comment, what range is RangeDoubleFractions meant to > represent? It might be poor naming here, but I meant that it has to be a proper fraction between zero and one. The double is in reference to the difference between this and the opacity (now float range) in accuracy (opacity is 0.9999). I thought this was due to rounding somewhere.
On 2015/01/23 at 02:18:49, jadeg wrote: > On 2015/01/23 00:44:23, dstockwell wrote: > > On 2015/01/22 at 22:14:23, jadeg wrote: > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > File Source/core/animation/DoubleStyleInterpolation.cpp (right): > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = > > clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > Why did this change? > > > > > > Done. > > > > Please reply to questions, 'Done.' means your reviewer needs to go back and > > figure out what actually happened. > > > Sorry, I meant that I changed it back to what it was previously. > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = > > clampTo<float>(doubleValue, 0.000000000000000000000000000000000000000000001); > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > nextafterf(0, 1)? > > > > > > This clamps to zero, not the correct value > > > > I don't understand the comment, what range is RangeDoubleFractions meant to > > represent? > It might be poor naming here, but I meant that it has to be a proper fraction between zero and one. The double is in reference to the difference between this and the opacity (now float range) in accuracy (opacity is 0.9999). I thought this was due to rounding somewhere. I think you'll need a third parameter then, looks like it only clamps to be greater than 0 currently. nextafterf(0, 1) should you the first value after 0 in the direction of 1 But I'm still not sure whether RangeDoubleFractions means > 0 && < 1 or >= 0, I think it needs a better name.
flexGrow and flexShrink must be non-negative. Please add layout test cases, and implement clamping. shapeImageThreshold has a change in behavior: shapeImageThreshold from [2] to [4] is now [0.5] at -0.75, was previously 1. This isn't covered by any existing layout test case, so we should add a case. zIndex has a change in rounding behavior: zIndex from [2] to [4] is now [2] at 0.3, was previously [3]. zIndex from [-2] to [-4] is now [-3] at 0.1, was previously [-2]. https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:46: doubleValue = clampTo<double>(doubleValue, 0.000000000000000000000000000000000000000000001); Consider using clampTo<double>(doubleValue, std::numeric_limits<double>::denorm_min()); like AnimatedStyleBuilder.cpp's case CSSPropertyZoom See http://en.cppreference.com/w/cpp/types/numeric_limits/denorm_min https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/100001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:81: no need for blank line
opacity has a change in behavior: opacity from [-2] to [-4] is now [0.5] at -1.25, was previously 0. This isn't covered by any existing layout test case, so we should add a case. widows has a change in rounding behavior: widows from [2] to [4] is now [2] at 0.3, was previously [3]
A summary of my comments: CSSPropertyZIndex CSSPropertyWidows We should be rounding instead of introducing truncation. CSSPropertyOpacity CSSPropertyShapeImageThreshold The change in behavior is fine. CSSPropertyFlexGrow CSSPropertyFlexShrink Any negative values must be clamped to 0 For each of the 6 properties, we need a layout test case to document the behavior.
On 2015/01/23 04:55:03, dstockwell wrote: > On 2015/01/23 at 02:18:49, jadeg wrote: > > On 2015/01/23 00:44:23, dstockwell wrote: > > > On 2015/01/22 at 22:14:23, jadeg wrote: > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > File Source/core/animation/DoubleStyleInterpolation.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = > > > clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); > > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > > Why did this change? > > > > > > > > Done. > > > > > > Please reply to questions, 'Done.' means your reviewer needs to go back and > > > figure out what actually happened. > > > > > Sorry, I meant that I changed it back to what it was previously. > > > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = > > > clampTo<float>(doubleValue, > 0.000000000000000000000000000000000000000000001); > > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > > nextafterf(0, 1)? > > > > > > > > This clamps to zero, not the correct value > > > > > > I don't understand the comment, what range is RangeDoubleFractions meant to > > > represent? > > It might be poor naming here, but I meant that it has to be a proper fraction > between zero and one. The double is in reference to the difference between this > and the opacity (now float range) in accuracy (opacity is 0.9999). I thought > this was due to rounding somewhere. > > I think you'll need a third parameter then, looks like it only clamps to be > greater than 0 currently. > > nextafterf(0, 1) should you the first value after 0 in the direction of 1 > > But I'm still not sure whether RangeDoubleFractions means > 0 && < 1 or >= 0, I > think it needs a better name. Have changed the name to RangeGreaterThanZero, and changed clamping to clampTo<double>(doubleValue, std::numeric_limits<double>::denorm_min()); as per eric's suggestion
On 2015/01/30 07:32:14, jadeg wrote: > On 2015/01/23 04:55:03, dstockwell wrote: > > On 2015/01/23 at 02:18:49, jadeg wrote: > > > On 2015/01/23 00:44:23, dstockwell wrote: > > > > On 2015/01/22 at 22:14:23, jadeg wrote: > > > > > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > > File Source/core/animation/DoubleStyleInterpolation.cpp (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > > Source/core/animation/DoubleStyleInterpolation.cpp:30: doubleValue = > > > > clampTo<float>(doubleValue, 0, nextafter(0.9999, 0)); > > > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > > > Why did this change? > > > > > > > > > > Done. > > > > > > > > Please reply to questions, 'Done.' means your reviewer needs to go back > and > > > > figure out what actually happened. > > > > > > > Sorry, I meant that I changed it back to what it was previously. > > > > > > > > > > > > > > > https://codereview.chromium.org/811993002/diff/80001/Source/core/animation/Do... > > > > > Source/core/animation/DoubleStyleInterpolation.cpp:36: doubleValue = > > > > clampTo<float>(doubleValue, > > 0.000000000000000000000000000000000000000000001); > > > > > On 2015/01/19 02:52:30, alancutter wrote: > > > > > > nextafterf(0, 1)? > > > > > > > > > > This clamps to zero, not the correct value > > > > > > > > I don't understand the comment, what range is RangeDoubleFractions meant > to > > > > represent? > > > It might be poor naming here, but I meant that it has to be a proper > fraction > > between zero and one. The double is in reference to the difference between > this > > and the opacity (now float range) in accuracy (opacity is 0.9999). I thought > > this was due to rounding somewhere. > > > > I think you'll need a third parameter then, looks like it only clamps to be > > greater than 0 currently. > > > > nextafterf(0, 1) should you the first value after 0 in the direction of 1 > > > > But I'm still not sure whether RangeDoubleFractions means > 0 && < 1 or >= 0, > I > > think it needs a better name. > > Have changed the name to RangeGreaterThanZero, and changed clamping to > clampTo<double>(doubleValue, std::numeric_limits<double>::denorm_min()); as per > eric's suggestion Sorry retract this, eric's function returns 1 not the correct value. Implemented nextafterf and it worked fine.
On 2015/01/29 02:09:14, Eric Willigers wrote: > A summary of my comments: > > CSSPropertyZIndex > CSSPropertyWidows > We should be rounding instead of introducing truncation. > > CSSPropertyOpacity > CSSPropertyShapeImageThreshold > The change in behavior is fine. > > CSSPropertyFlexGrow > CSSPropertyFlexShrink > Any negative values must be clamped to 0 > > For each of the 6 properties, we need a layout test case to document the > behavior. Done but I think the flex properties actually use default style interpolation, as I discovered once I implemented the tests.
Can an owner please review. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:197: case CSSPropertyFlexGrow: Can we use RangeNonNegative for flex-grow and flex-shrink?
https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:39: case RangeFractions: Why did this one get renamed from RangeOpacityFIXME? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:49: doubleValue = clampTo<int>((doubleValue + 0.5), 1); Just use round? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:51: case RangeGreaterThanOne: Isn't this RangeGreaterThanOrEqualToOne? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:58: if (doubleValue < 0) Doesn't this use round() in AnimatedStyleBuidler.cpp? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:61: doubleValue = clampTo<int>(doubleValue + 0.5); Does this still work without the clampTo<int>? (just rounding) https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.h:50: Can drop all changes to this file now. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:165: if (range == RangeAll) This fall through logic is a bit complicated, perhaps this should be extracted into a separate function? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:166: range = RangeFloor; AnimatedStyleBuilder rounds and then clamps greater than 1. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:179: range = RangeFractions; This seems different to AnimatedStyleBuidler. The clamping here seems to exclude 1?
You should list the properties being switched over to the new model in the description. https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... File LayoutTests/animations/interpolation/flex-interpolation.html (right): https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... LayoutTests/animations/interpolation/flex-interpolation.html:75: {at: 1.5, is: '1'} This is incorrect, these are animatable as non-negative numbers: http://dev.w3.org/csswg/css-flexbox-1/#flex-grow-property https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... File LayoutTests/animations/interpolation/shape-image-threshold.html (right): https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... LayoutTests/animations/interpolation/shape-image-threshold.html:29: {at: 1.5, is: '4'} Why does this test pass? Your code now clamps between [0, 1). https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (left): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:46: ASSERT_NOT_REACHED(); Leave this assert in. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:39: case RangeFractions: RangeZeroToLessThanOne for clarity. BTW, you should add RangeZeroToOne for shape-image-threshold since it doesn't require opacity's hacky hacks. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:45: case RangeGreaterThanZero: RangePositive? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:48: case RangeRound: RangeRoundPositive for clarity. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:53: break; RangeAtLeastOne for correctness. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:57: case RangeAbsoluteValueFloor: RangeRound. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:61: doubleValue = clampTo<int>(doubleValue + 0.5); Just use round() instead of (doubleValue + 0.5) here and above. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.h:50: Unnecessary newline. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/K... File Source/core/animation/Keyframe.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/K... Source/core/animation/Keyframe.h:74: Unnecessary newline. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/L... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/L... Source/core/animation/LengthStyleInterpolation.cpp:29: ASSERT(canCreateFrom(value)); Leave this assert in. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:125: case CSSPropertyWebkitColumnWidth: Adding more LengthStyleInterpolation handling should be done in a separate patch. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:172: case CSSPropertyShapeImageThreshold: This should clamp to [0, 1] instead of [0, 1). https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:193: } No need for braces around single line if body. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:326: void StringKeyframe::PropertySpecificKeyframe::ensureAnimatableValueCaches(CSSPropertyID property, Keyframe::PropertySpecificKeyframe& end, Element* element, CSSValue* fromCSSValue, CSSValue* toCSSValue) const element, from and to should be references. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.h:54: void ensureAnimatableValueCaches(CSSPropertyID, Keyframe::PropertySpecificKeyframe&, Element*, CSSValue*, CSSValue*) const; Include parameter names here: end, from, to.
https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... File LayoutTests/animations/interpolation/flex-interpolation.html (right): https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... LayoutTests/animations/interpolation/flex-interpolation.html:75: {at: 1.5, is: '1'} On 2015/02/04 20:24:37, alancutter wrote: > This is incorrect, these are animatable as non-negative numbers: > http://dev.w3.org/csswg/css-flexbox-1/#flex-grow-property The CSSValues dont seem to conform to this though, so I added fail cases to the tests. https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... File LayoutTests/animations/interpolation/shape-image-threshold.html (right): https://codereview.chromium.org/811993002/diff/120001/LayoutTests/animations/... LayoutTests/animations/interpolation/shape-image-threshold.html:29: {at: 1.5, is: '4'} On 2015/02/04 20:24:37, alancutter wrote: > Why does this test pass? Your code now clamps between [0, 1). Im really not sure, maybe you could come and help investigate this? But ive moved it around, and it works either here or if I dont clamp it at all. It's being weird. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (left): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:46: ASSERT_NOT_REACHED(); On 2015/02/04 20:24:37, alancutter wrote: > Leave this assert in. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:39: case RangeFractions: I renamed it because Eve named it this in her Interpolation change anticipating that I would come and rename it according to its properties, as the Ranges are supposed to be non property specific. Renamed to RangeZeroToLessThanOne. Also done with ShapeImageThreshold https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:45: case RangeGreaterThanZero: On 2015/02/04 20:24:37, alancutter wrote: > RangePositive? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:48: case RangeRound: On 2015/02/04 20:24:37, alancutter wrote: > RangeRoundPositive for clarity. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:49: doubleValue = clampTo<int>((doubleValue + 0.5), 1); On 2015/02/04 11:09:27, dstockwell wrote: > Just use round? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:51: case RangeGreaterThanOne: On 2015/02/04 11:09:27, dstockwell wrote: > Isn't this RangeGreaterThanOrEqualToOne? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:53: break; On 2015/02/04 20:24:37, alancutter wrote: > RangeAtLeastOne for correctness. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:57: case RangeAbsoluteValueFloor: On 2015/02/04 20:24:37, alancutter wrote: > RangeRound. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:58: if (doubleValue < 0) On 2015/02/04 11:09:28, dstockwell wrote: > Doesn't this use round() in AnimatedStyleBuidler.cpp? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:61: doubleValue = clampTo<int>(doubleValue + 0.5); On 2015/02/04 11:09:28, dstockwell wrote: > Does this still work without the clampTo<int>? (just rounding) Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.cpp:61: doubleValue = clampTo<int>(doubleValue + 0.5); On 2015/02/04 20:24:37, alancutter wrote: > Just use round() instead of (doubleValue + 0.5) here and above. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... File Source/core/animation/DoubleStyleInterpolation.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/D... Source/core/animation/DoubleStyleInterpolation.h:50: On 2015/02/04 11:09:28, dstockwell wrote: > Can drop all changes to this file now. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/K... File Source/core/animation/Keyframe.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/K... Source/core/animation/Keyframe.h:74: On 2015/02/04 20:24:37, alancutter wrote: > Unnecessary newline. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/L... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/L... Source/core/animation/LengthStyleInterpolation.cpp:29: ASSERT(canCreateFrom(value)); On 2015/02/04 20:24:37, alancutter wrote: > Leave this assert in. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:125: case CSSPropertyWebkitColumnWidth: On 2015/02/04 20:24:37, alancutter wrote: > Adding more LengthStyleInterpolation handling should be done in a separate > patch. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:165: if (range == RangeAll) On 2015/02/04 11:09:28, dstockwell wrote: > This fall through logic is a bit complicated, perhaps this should be extracted > into a separate function? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:166: range = RangeFloor; On 2015/02/04 11:09:28, dstockwell wrote: > AnimatedStyleBuilder rounds and then clamps greater than 1. Im confused if you mean that I should change the name of this or change the clamping? https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:172: case CSSPropertyShapeImageThreshold: On 2015/02/04 20:24:37, alancutter wrote: > This should clamp to [0, 1] instead of [0, 1). Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:179: range = RangeFractions; On 2015/02/04 11:09:28, dstockwell wrote: > This seems different to AnimatedStyleBuidler. The clamping here seems to exclude > 1? Yes now has been changed to RangeZeroToLessThanOne https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:193: } On 2015/02/04 20:24:37, alancutter wrote: > No need for braces around single line if body. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:197: case CSSPropertyFlexGrow: On 2015/02/04 06:16:27, Eric Willigers wrote: > Can we use RangeNonNegative for flex-grow and flex-shrink? Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:326: void StringKeyframe::PropertySpecificKeyframe::ensureAnimatableValueCaches(CSSPropertyID property, Keyframe::PropertySpecificKeyframe& end, Element* element, CSSValue* fromCSSValue, CSSValue* toCSSValue) const On 2015/02/04 20:24:37, alancutter wrote: > element, from and to should be references. Done. https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... File Source/core/animation/StringKeyframe.h (right): https://codereview.chromium.org/811993002/diff/120001/Source/core/animation/S... Source/core/animation/StringKeyframe.h:54: void ensureAnimatableValueCaches(CSSPropertyID, Keyframe::PropertySpecificKeyframe&, Element*, CSSValue*, CSSValue*) const; On 2015/02/04 20:24:37, alancutter wrote: > Include parameter names here: end, from, to. Done.
lgtm https://codereview.chromium.org/811993002/diff/140001/LayoutTests/animations/... File LayoutTests/animations/interpolation/flex-interpolation-expected.txt (right): https://codereview.chromium.org/811993002/diff/140001/LayoutTests/animations/... LayoutTests/animations/interpolation/flex-interpolation-expected.txt:26: FAIL: flex-grow from [0] to [1] was [1] at 1.5, expected [1.5] Looks like we have issues with flex to deal with. Let's leave it out of this patch and fix up AnimatableValue separately to keep things simple. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:79: void setRange(InterpolationRange* range, InterpolationRange setTo) This should be a reference.
https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:79: void setRange(InterpolationRange* range, InterpolationRange setTo) This is not what I meant. Instead I was suggesting that we should create a function of the form: InterpolationRange range(CSSPropertyID) That way we can avoid the hard to read & error-prone fall-through logic. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:173: setRange(&range, RangeFloor); RangeFloor seems wrong, in AnimatedStyleBuilder.cpp this is rounded and clamped >= 1 https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:178: case CSSPropertyFillOpacity: Flood/Stop/Stroke should be ZeroToOne (as in AnimatedStyleBuilder.cpp) CSSPropertyOpacity should remain ZeroToLessThanOne, but this should still be called RangeOpacityFIXME. It's called *FIXME because it should really be RangeZeroToOne but can't be due to other limitations. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:191: case CSSPropertyWebkitColumnRuleWidth: CSSPropertyColumnRuleWidth should be rounded (as in AnimatedStyleBuilder.cpp)
New patchsets have been uploaded after l-g-t-m from alancutter@chromium.org
https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:79: void setRange(InterpolationRange* range, InterpolationRange setTo) On 2015/02/06 10:44:47, dstockwell wrote: > This is not what I meant. Instead I was suggesting that we should create a > function of the form: > > InterpolationRange range(CSSPropertyID) > > That way we can avoid the hard to read & error-prone fall-through logic. Done. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:173: setRange(&range, RangeFloor); On 2015/02/06 10:44:48, dstockwell wrote: > RangeFloor seems wrong, in AnimatedStyleBuilder.cpp this is rounded and clamped > >= 1 Done. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:178: case CSSPropertyFillOpacity: On 2015/02/06 10:44:48, dstockwell wrote: > Flood/Stop/Stroke should be ZeroToOne (as in AnimatedStyleBuilder.cpp) > > CSSPropertyOpacity should remain ZeroToLessThanOne, but this should still be > called RangeOpacityFIXME. It's called *FIXME because it should really be > RangeZeroToOne but can't be due to other limitations. Done. https://codereview.chromium.org/811993002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:191: case CSSPropertyWebkitColumnRuleWidth: On 2015/02/06 10:44:47, dstockwell wrote: > CSSPropertyColumnRuleWidth should be rounded (as in AnimatedStyleBuilder.cpp) Done.
lgtm! https://codereview.chromium.org/811993002/diff/180001/LayoutTests/animations/... File LayoutTests/animations/interpolation/shape-image-threshold-expected.txt (right): https://codereview.chromium.org/811993002/diff/180001/LayoutTests/animations/... LayoutTests/animations/interpolation/shape-image-threshold-expected.txt:26: PASS: shapeImageThreshold from [2] to [4] was [2] at 0 The expectations above 1 should just be 1, they are passing due to normalization in the test runner.
New patchsets have been uploaded after l-g-t-m from dstockwell@chromium.org
The CQ bit was checked by jadeg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/811993002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12...) win_gpu_triggered_tests on tryserver.chromium.gpu (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by jadeg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/811993002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190004 |
