Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
851633002:
Animation: Add template for ListStyleInterpolation (Closed)
|
Created:
5 years, 11 months ago by evemj (not active) Modified:
5 years, 10 months ago Reviewers:
ericwilligers, shans, alancutter (OOO until 2018), dstockwell, jadeg, samli, Eric Willigers CC:
dstockwell, 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. |
DescriptionAnimation: Add template for ListStyleInterpolation
Remove LengthPoint3DStyle interpolation as CSSPropertyPerspectiveOrigin and CSSPropertyTransformOrigin can be handled using ListStyleInterpolation<LengthStyleInterpolation>.
Currently only used for lists of lengths and shadows, but could potentially be used for any CSSValueList interpolation (such as transform or basic shape).
BUG=450448
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189490
Patch Set 1 : Add template for ListStyleInterpolation #
Total comments: 47
Patch Set 2 : Create new methods in ShadowStyleInterpolation #
Total comments: 9
Patch Set 3 : Add separate variables for value lists #
Total comments: 6
Patch Set 4 : #
Total comments: 13
Patch Set 5 : Correct rounding in Interpolation LayoutTests and remove WillBeHeapVector #
Total comments: 31
Patch Set 6 : Remove InterpolableBool from ShadowStyleInterpolation #
Total comments: 6
Patch Set 7 : Add default parameter to listToInterpolableValue and change inheritance of Interpolation.h #Patch Set 8 : Add trace to ListStyleInterpolationImpl #Patch Set 9 : Remove trace from liststyleinterpolation #Patch Set 10 : Add else to shadowToInterpolableValue #Messages
Total messages: 50 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
evem@chromium.org changed reviewers: + jadeg@chromium.org
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:13: Do you need this platform/Length.h? https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:24: static PassRefPtrWillBeRawPtr<ListStyleInterpolationImpl<T, NI>> create(const CSSValue& start, const CSSValue& end, CSSPropertyID id, InterpolationRange range = RangeAll) This seems like a strange way to assign a value? https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: if (!T::canCreateFrom(*toCSSValueList(start).item(i), *toCSSValueList(end).item(i))) { Can you put a , in an if statement? https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:118: friend class ListStyleInterpolationTest; Is there a way to make one of these classes inherit from the other so that you dont have to do this twice? https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:138: #endif // ListStyleInterpolation_h Need newline at end (pedantic i know) https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolationTest.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:1: #include "config.h" Need license shtuffs https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:95: Remove this new line .... https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:96: please https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:97: } // namespace blink New line https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:86: } // namespace blink Newline!!!
lgtm
evem@chromium.org changed reviewers: + alancutter@chromium.org, ericwilligers@google.com, samli@chromium.org, shans@chromium.org
https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/LengthStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/LengthStyleInterpolation.h:42: Don't think there is any value to having lengthToIntepolableValue() or interpolableValueToLength(). Seems like these are just aliases but don't really add any value since they are static and are hence called like LengthStyleInterpolation::toInterpolableValue() which is clear enough. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:16: template<typename T, typename NI> template <typename.. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:24: static PassRefPtrWillBeRawPtr<ListStyleInterpolationImpl<T, NI>> create(const CSSValue& start, const CSSValue& end, CSSPropertyID id, InterpolationRange range = RangeAll) On 2015/01/21 05:36:34, jadeg wrote: > This seems like a strange way to assign a value? Seems fine. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:24: static PassRefPtrWillBeRawPtr<ListStyleInterpolationImpl<T, NI>> create(const CSSValue& start, const CSSValue& end, CSSPropertyID id, InterpolationRange range = RangeAll) <T, NI> > use git cl format https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:27: Vector<typename T::NonInterpolableType> nonInterpolableDataToo; Rename it to startNonInterpolableData, endNonInterpolableData https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: if (!T::canCreateFrom(*toCSSValueList(start).item(i), *toCSSValueList(end).item(i))) { On 2015/01/21 05:36:34, jadeg wrote: > Can you put a , in an if statement? It's not in the if statement. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:99: } For consistency with below, remove braces. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:128: if (toCSSValueList(start).length() == toCSSValueList(end).length()) { Combine into one if statement https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:18: if (start.isShadowValue() || end.isShadowValue()) { Should be && https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:21: return false; Just simplify to "return start.isShadowValue() && end.isShadowValue() && ...(style == style)...;" https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:50: result->set(4, InterpolableBool::create(false)); This is messy and hard to read. A possible solution is to create a static method in this file which takes a length-type CSSPrimitiveValue and returns InterpolableValue. Then for x, y, blur, spread we just have: result->set(0, PrimitiveValueToInterpolableValue(shadowValue->x)); (maybe choose a better name). Color will have to be separate, but that's fine. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:67: RangeAll) : nullptr; I think the same would work for these 4 values https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:77: if (start.isValueList() && end.isValueList()) { Do we need to check they are the same length as well? (I don't know the answer) https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:79: if (canCreateFrom(*toCSSValueList(start).item(i), *toCSSValueList(end).item(i))) Shouldn't this be a "!canCreateFrom()"? https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:218: // FIXME: Handle CSSValueLists. Is this something we can fix now? Maybe in another patch. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:226: return interpolation.release(); Fix indentation please.
https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:24: static PassRefPtrWillBeRawPtr<ListStyleInterpolationImpl<T, NI>> create(const CSSValue& start, const CSSValue& end, CSSPropertyID id, InterpolationRange range = RangeAll) On 2015/01/23 03:23:36, samli wrote: > <T, NI> > > use git cl format Using >> is fine nowadays
Patchset #2 (id:160001) has been deleted
https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/LengthStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/LengthStyleInterpolation.h:42: On 2015/01/23 03:23:36, samli wrote: > Don't think there is any value to having lengthToIntepolableValue() or > interpolableValueToLength(). Seems like these are just aliases but don't really > add any value since they are static and are hence called like > LengthStyleInterpolation::toInterpolableValue() which is clear enough. Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/23 03:23:36, samli wrote: > 2015 Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:13: On 2015/01/21 05:36:34, jadeg wrote: > Do you need this platform/Length.h? Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:27: Vector<typename T::NonInterpolableType> nonInterpolableDataToo; On 2015/01/23 03:23:36, samli wrote: > Rename it to startNonInterpolableData, endNonInterpolableData Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:99: } On 2015/01/23 03:23:36, samli wrote: > For consistency with below, remove braces. Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:128: if (toCSSValueList(start).length() == toCSSValueList(end).length()) { On 2015/01/23 03:23:36, samli wrote: > Combine into one if statement Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:138: #endif // ListStyleInterpolation_h On 2015/01/21 05:36:34, jadeg wrote: > Need newline at end (pedantic i know) I think the codereview page doesn't show the new lines but they're there https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolationTest.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:1: #include "config.h" On 2015/01/21 05:36:34, jadeg wrote: > Need license shtuffs Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:95: On 2015/01/21 05:36:34, jadeg wrote: > Remove this new line .... Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:96: On 2015/01/21 05:36:34, jadeg wrote: > please Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/L... Source/core/animation/ListStyleInterpolationTest.cpp:97: } // namespace blink On 2015/01/21 05:36:34, jadeg wrote: > New line Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:18: if (start.isShadowValue() || end.isShadowValue()) { On 2015/01/23 03:23:36, samli wrote: > Should be && Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:21: return false; On 2015/01/23 03:23:36, samli wrote: > Just simplify to "return start.isShadowValue() && end.isShadowValue() && > ...(style == style)...;" Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:50: result->set(4, InterpolableBool::create(false)); On 2015/01/23 03:23:36, samli wrote: > This is messy and hard to read. > > A possible solution is to create a static method in this file which takes a > length-type CSSPrimitiveValue and returns InterpolableValue. > > Then for x, y, blur, spread we just have: > result->set(0, PrimitiveValueToInterpolableValue(shadowValue->x)); > (maybe choose a better name). > > Color will have to be separate, but that's fine. Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:67: RangeAll) : nullptr; On 2015/01/23 03:23:36, samli wrote: > I think the same would work for these 4 values Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:77: if (start.isValueList() && end.isValueList()) { On 2015/01/23 03:23:36, samli wrote: > Do we need to check they are the same length as well? (I don't know the answer) Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:79: if (canCreateFrom(*toCSSValueList(start).item(i), *toCSSValueList(end).item(i))) On 2015/01/23 03:23:36, samli wrote: > Shouldn't this be a "!canCreateFrom()"? Done. https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:226: return interpolation.release(); On 2015/01/23 03:23:36, samli wrote: > Fix indentation please. Done.
Patchset #2 (id:180001) has been deleted
https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/140001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:218: // FIXME: Handle CSSValueLists. On 2015/01/23 03:23:36, samli wrote: > Is this something we can fix now? Maybe in another patch. Done.
ericwilligers@chromium.org changed reviewers: + ericwilligers@chromium.org
https://codereview.chromium.org/851633002/diff/200001/LayoutTests/animations/... File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851633002/diff/200001/LayoutTests/animations/... LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:25: FAIL: boxShadow from [15px 10px 5px 6px black] to [-15px -10px 25px -4px orange] was [rgb(153, 99, 0) -3px -2px 17px 4.44089209850063e-16px] at 0.6, expected [-3px -2px 17px 0px rgb(153, 99, 0)] (parsed as [rgb(153, 99, 0) -3px -2px 17px 0px]) Optional: The ugly 4.44089209850063e-16px can be avoided if we add an offset to the start and end keyframes, e.g. use from: '15px 10px 5px 7px black', to: '-15px -10px 25px -3px orange' https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:23: for (size_t i = 0; i < toCSSValueList(start).length(); i++) { Consider defining const CSSValueList& startList = toCSSValueList(start); const CSSValueList& endList = toCSSValueList(end); https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:58: static PassRefPtrWillBeRawPtr<CSSValue> interpolableValueToList(InterpolableValue* value, Vector<typename T::NonInterpolableType> nonInterpolableData, InterpolationRange range = RangeAll) Perhaps const Vector<typename T::NonInterpolableType>& https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:221: { Move brace to end of previous line. Following 5 lines can be un-indented. https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:231: { Move brace to end of previous line. Following lines can be un-indented.
Patchset #3 (id:220001) has been deleted
https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:23: for (size_t i = 0; i < toCSSValueList(start).length(); i++) { On 2015/01/28 02:21:23, Eric Willigers wrote: > Consider defining > const CSSValueList& startList = toCSSValueList(start); > const CSSValueList& endList = toCSSValueList(end); Done. https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:58: static PassRefPtrWillBeRawPtr<CSSValue> interpolableValueToList(InterpolableValue* value, Vector<typename T::NonInterpolableType> nonInterpolableData, InterpolationRange range = RangeAll) On 2015/01/28 02:21:23, Eric Willigers wrote: > Perhaps > const Vector<typename T::NonInterpolableType>& Done. https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:221: { On 2015/01/28 02:21:23, Eric Willigers wrote: > Move brace to end of previous line. Following 5 lines can be un-indented. Done. https://codereview.chromium.org/851633002/diff/200001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:231: { On 2015/01/28 02:21:23, Eric Willigers wrote: > Move brace to end of previous line. Following lines can be un-indented. Done.
https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:25: FAIL: boxShadow from [15px 10px 5px 6px black] to [-15px -10px 25px -4px orange] was [rgb(153, 99, 0) -3px -2px 17px 4.44089209850063e-16px] at 0.6, expected [-3px -2px 17px 0px rgb(153, 99, 0)] (parsed as [rgb(153, 99, 0) -3px -2px 17px 0px]) please fix the test instead. https://codereview.chromium.org/851633002/diff/240001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/240001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:15: template<typename T, typename NI> the 'NI' parameter doesn't appear to be used. It looks like we can remove the Impl indirection and just use T:NonInterpolableType directly in ListStyleInterpolation?
Patchset #4 (id:260001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #5 (id:320001) has been deleted
dstockwell@chromium.org changed reviewers: + dstockwell@chromium.org
https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/LengthPairStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/LengthPairStyleInterpolation.cpp:33: RefPtrWillBeRawPtr<CSSPrimitiveValue> first = (LengthStyleInterpolation::fromInterpolableValue(*lengthPair->get(0), range)); remove outer parens https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:15: template<typename T, typename NI> expand T and NI into CamelWords I have no idea what NI is yet :P https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:52: OwnPtrWillBeRawPtr<WillBeHeapVector<typename T::NonInterpolableType>> m_nonInterpolableData; Why heap vector? https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:59: typename T::NonInterpolableType elementData = false; Can't NonInterpolableType be something other than bool? https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) Don't we need to support lists of different lengths?
https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:59: typename T::NonInterpolableType elementData = false; On 2015/01/30 04:24:01, dstockwell wrote: > Can't NonInterpolableType be something other than bool? Yes. @eve: this shouldn't be here anymore :)
Patchset #4 (id:300001) has been deleted
https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:25: FAIL: boxShadow from [15px 10px 5px 6px black] to [-15px -10px 25px -4px orange] was [rgb(153, 99, 0) -3px -2px 17px 4.44089209850063e-16px] at 0.6, expected [-3px -2px 17px 0px rgb(153, 99, 0)] (parsed as [rgb(153, 99, 0) -3px -2px 17px 0px]) On 2015/01/29 03:13:11, shans wrote: > please fix the test instead. Done. https://codereview.chromium.org/851633002/diff/240001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/240001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:15: template<typename T, typename NI> On 2015/01/29 03:13:11, shans wrote: > the 'NI' parameter doesn't appear to be used. It looks like we can remove the > Impl indirection and just use T:NonInterpolableType directly in > ListStyleInterpolation? Need the NI parameter so that template can be specialized on void https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/LengthPairStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/LengthPairStyleInterpolation.cpp:33: RefPtrWillBeRawPtr<CSSPrimitiveValue> first = (LengthStyleInterpolation::fromInterpolableValue(*lengthPair->get(0), range)); On 2015/01/30 04:24:01, dstockwell wrote: > remove outer parens Done. https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:15: template<typename T, typename NI> On 2015/01/30 04:24:01, dstockwell wrote: > expand T and NI into CamelWords > > I have no idea what NI is yet :P Done. https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:52: OwnPtrWillBeRawPtr<WillBeHeapVector<typename T::NonInterpolableType>> m_nonInterpolableData; On 2015/01/30 04:24:01, dstockwell wrote: > Why heap vector? Done. I didn't fully understand how the WillBeHeapVector worked, but Eric explained it to me so I've changed it to be just be a Vector now https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:59: typename T::NonInterpolableType elementData = false; On 2015/01/30 04:24:01, dstockwell wrote: > Can't NonInterpolableType be something other than bool? Yep sorry that was a mistake! Done. https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) On 2015/01/30 04:24:01, dstockwell wrote: > Don't we need to support lists of different lengths? I think this class is just for simple lists (not repeatable lists) which only support lists of the same length.
https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:25: FAIL: boxShadow from [15px 10px 5px 6px black] to [-15px -10px 25px -4px orange] was [rgb(153, 99, 0) -3px -2px 17px 4.44089209850063e-16px] at 0.6, expected [-3px -2px 17px 0px rgb(153, 99, 0)] (parsed as [rgb(153, 99, 0) -3px -2px 17px 0px]) On 2015/01/29 03:13:11, shans wrote: > please fix the test instead. The interpolation tests should be rounding, but they don't understand this notation: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) On 2015/02/02 03:56:02, evemj wrote: > On 2015/01/30 04:24:01, dstockwell wrote: > > Don't we need to support lists of different lengths? > > I think this class is just for simple lists (not repeatable lists) which only > support lists of the same length. Will we create a separate template for repeatable lists? Seems like there will be a lot of overlap. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:20: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> startNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); Move these allocations closer to line 32, since we can early exit on line 28. I think this should be OwnPtr not OwnPtrWillBeRawPtr https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:28: return nullptr; Should this method be called maybeCreate? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:46: : StyleInterpolation(start, end, id), m_range(range), m_nonInterpolableData(nonInterpolableData) put each initialization on a separate line https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:46: : StyleInterpolation(start, end, id), m_range(range), m_nonInterpolableData(nonInterpolableData) put each initialization on a separate line https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:70: RefPtrWillBeRawPtr<CSSValueList> result = CSSValueList::createCommaSeparated(); Can we reserve capacity on a CSSValueList? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:72: ASSERT(nonInterpolableData.size() != 0); Need to assert that it's the same length as listValue. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) We should either move this check inside ListStyleInterpolationImpl or ASSERT it there. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:24: if (value && LengthStyleInterpolation::canCreateFrom(*value)) When can LengthStyleInterpolation::canCreateFrom return false here? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:26: return InterpolableBool::create(false); Using an InterpolableBool seems a bit of a hack. Maybe we need a new InterpolableValue to represent these skipped entries. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:43: result->set(4, InterpolableBool::create(false)); Is there any reason to set this to a bool over switching on the length of the list? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:66: RefPtrWillBeRawPtr<CSSPrimitiveValue> color = (!shadow->get(4)->isBool()) ? ColorStyleInterpolation::interpolableValueToColor(*shadow->get(4)) : nullptr; outer parens not needed https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:80: } these braces aren't indented correctly https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.h:44: static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> interpolableValueToLength(const InterpolableValue*, InterpolationRange = RangeAll); I think the interpolable value can be a reference here. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:235: if (ShadowStyleInterpolation::usesDefaultStyleInterpolation(*fromCSSValue, *toCSSValue)) This seems like the inverse of the legacy check? Can we remove this function when line 238 is fixed? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:235: if (ShadowStyleInterpolation::usesDefaultStyleInterpolation(*fromCSSValue, *toCSSValue)) This seems like the inverse of the legacy check? Can we remove this function when line 238 is fixed? https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:236: return DefaultStyleInterpolation::create(fromCSSValue, toCSSValue, property); Doesn't `break;` fall through to default style interpolation?
https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... File LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt (right): https://codereview.chromium.org/851633002/diff/240001/LayoutTests/animations/... LayoutTests/animations/interpolation/box-shadow-interpolation-expected.txt:25: FAIL: boxShadow from [15px 10px 5px 6px black] to [-15px -10px 25px -4px orange] was [rgb(153, 99, 0) -3px -2px 17px 4.44089209850063e-16px] at 0.6, expected [-3px -2px 17px 0px rgb(153, 99, 0)] (parsed as [rgb(153, 99, 0) -3px -2px 17px 0px]) On 2015/02/02 06:10:10, dstockwell wrote: > On 2015/01/29 03:13:11, shans wrote: > > please fix the test instead. > > The interpolation tests should be rounding, but they don't understand this > notation: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (oops, ignore this stale comment)
https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:26: return InterpolableBool::create(false); On 2015/02/02 06:10:12, dstockwell wrote: > Using an InterpolableBool seems a bit of a hack. Maybe we need a new > InterpolableValue to represent these skipped entries. We should use an InterpolableValue that represents a length 0 in this case, so that we can animate from a missing value to a provided value. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:236: return DefaultStyleInterpolation::create(fromCSSValue, toCSSValue, property); On 2015/02/02 06:10:12, dstockwell wrote: > Doesn't `break;` fall through to default style interpolation? AnimatableShadow incorrectly animates between inset and non-inset values, so it doesn't ever indicate it needs default interpolation. We could fix this in AnimatableShadow but it's probably better just to leave this here with a big FIXME for now?
Patchset #6 (id:380001) has been deleted
https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/340001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) On 2015/02/02 06:10:11, dstockwell wrote: > On 2015/02/02 03:56:02, evemj wrote: > > On 2015/01/30 04:24:01, dstockwell wrote: > > > Don't we need to support lists of different lengths? > > > > I think this class is just for simple lists (not repeatable lists) which only > > support lists of the same length. > > Will we create a separate template for repeatable lists? Seems like there will > be a lot of overlap. I got the impression that we would extend this template to deal with repeatable lists/other properties that use simple list style interpolation as needed https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:20: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> startNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); On 2015/02/02 06:10:11, dstockwell wrote: > Move these allocations closer to line 32, since we can early exit on line 28. > > I think this should be OwnPtr not OwnPtrWillBeRawPtr Done. I got compile errors when I tried to change to OwnPtr :( https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:28: return nullptr; On 2015/02/02 06:10:11, dstockwell wrote: > Should this method be called maybeCreate? Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:46: : StyleInterpolation(start, end, id), m_range(range), m_nonInterpolableData(nonInterpolableData) On 2015/02/02 06:10:11, dstockwell wrote: > put each initialization on a separate line Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:70: RefPtrWillBeRawPtr<CSSValueList> result = CSSValueList::createCommaSeparated(); On 2015/02/02 06:10:11, dstockwell wrote: > Can we reserve capacity on a CSSValueList? I don't think so (or at least I couldn't find a method for doing that) https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:72: ASSERT(nonInterpolableData.size() != 0); On 2015/02/02 06:10:11, dstockwell wrote: > Need to assert that it's the same length as listValue. Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:131: if (start.isValueList() && end.isValueList() && toCSSValueList(start).length() == toCSSValueList(end).length()) On 2015/02/02 06:10:11, dstockwell wrote: > We should either move this check inside ListStyleInterpolationImpl or ASSERT it > there. Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:24: if (value && LengthStyleInterpolation::canCreateFrom(*value)) On 2015/02/02 06:10:11, dstockwell wrote: > When can LengthStyleInterpolation::canCreateFrom return false here? Done. (Never oops) https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:26: return InterpolableBool::create(false); On 2015/02/02 23:49:11, shans wrote: > On 2015/02/02 06:10:12, dstockwell wrote: > > Using an InterpolableBool seems a bit of a hack. Maybe we need a new > > InterpolableValue to represent these skipped entries. > > We should use an InterpolableValue that represents a length 0 in this case, so > that we can animate from a missing value to a provided value. Done. Additionally we are now not handling the case where Color is not specified as part of the shadow value https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:66: RefPtrWillBeRawPtr<CSSPrimitiveValue> color = (!shadow->get(4)->isBool()) ? ColorStyleInterpolation::interpolableValueToColor(*shadow->get(4)) : nullptr; On 2015/02/02 06:10:12, dstockwell wrote: > outer parens not needed Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.cpp:80: } On 2015/02/02 06:10:11, dstockwell wrote: > these braces aren't indented correctly Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/ShadowStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/ShadowStyleInterpolation.h:44: static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> interpolableValueToLength(const InterpolableValue*, InterpolationRange = RangeAll); On 2015/02/02 06:10:12, dstockwell wrote: > I think the interpolable value can be a reference here. Done. https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:236: return DefaultStyleInterpolation::create(fromCSSValue, toCSSValue, property); On 2015/02/02 23:49:11, shans wrote: > On 2015/02/02 06:10:12, dstockwell wrote: > > Doesn't `break;` fall through to default style interpolation? > > AnimatableShadow incorrectly animates between inset and non-inset values, so it > doesn't ever indicate it needs default interpolation. > > We could fix this in AnimatableShadow but it's probably better just to leave > this here with a big FIXME for now? Done.
Patchset #7 (id:420001) has been deleted
https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... File Source/core/animation/StringKeyframe.cpp (right): https://codereview.chromium.org/851633002/diff/360001/Source/core/animation/S... Source/core/animation/StringKeyframe.cpp:236: return DefaultStyleInterpolation::create(fromCSSValue, toCSSValue, property); On 2015/02/02 at 23:49:11, shans wrote: > On 2015/02/02 06:10:12, dstockwell wrote: > > Doesn't `break;` fall through to default style interpolation? > > AnimatableShadow incorrectly animates between inset and non-inset values, so it doesn't ever indicate it needs default interpolation. > > We could fix this in AnimatableShadow but it's probably better just to leave this here with a big FIXME for now? Hmm, interesting, I see the spec has changed here, maybe the original wording was just bad. I guess either way is fine. Should be easy enough to fix in AnimatableShadow too. https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> endNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); I think end/startNonInterpolableData can be just OwnPtr<Vector<...>>, startValue/endValue below must be OwnPtrWillBeRawPtr<InterpolableValue> -- basically if the T in OwnPtr<T> is *WillBeGarbageCollected then you must use *WillBeRawPtr. https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:34: OwnPtrWillBeRawPtr<InterpolableValue> endValue = listToInterpolableValue(end, *endNonInterpolableData.get()); It seems like we just throw away the endNonInterpolableData values. Why don't we need them? Can we avoid this extra work?
https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> endNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); On 2015/02/03 02:22:18, dstockwell wrote: > I think end/startNonInterpolableData can be just OwnPtr<Vector<...>>, > startValue/endValue below must be OwnPtrWillBeRawPtr<InterpolableValue> -- > basically if the T in OwnPtr<T> is *WillBeGarbageCollected then you must use > *WillBeRawPtr. Done. Changed to own ptr and was getting compile error "Class 'ListStyleInterpolationImpl' requires finalization" specifically "Field 'm_nonInterpolableData' requires finalization". Eric helped me change Interpolation.h so it inherits from RefCountedWillBeGarbageCollectedFinalized instead of RefCountedWillBeGarbageCollected to get rid of this. https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:34: OwnPtrWillBeRawPtr<InterpolableValue> endValue = listToInterpolableValue(end, *endNonInterpolableData.get()); On 2015/02/03 02:22:18, dstockwell wrote: > It seems like we just throw away the endNonInterpolableData values. Why don't we > need them? Can we avoid this extra work? Done. Sam and I added a default parameter to listToInterpolableValue
Just one last issue to resolve. https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> endNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); On 2015/02/03 03:45:50, evemj wrote: > On 2015/02/03 02:22:18, dstockwell wrote: > > I think end/startNonInterpolableData can be just OwnPtr<Vector<...>>, > > startValue/endValue below must be OwnPtrWillBeRawPtr<InterpolableValue> -- > > basically if the T in OwnPtr<T> is *WillBeGarbageCollected then you must use > > *WillBeRawPtr. > > Done. > > Changed to own ptr and was getting compile error "Class > 'ListStyleInterpolationImpl' requires finalization" specifically "Field > 'm_nonInterpolableData' requires finalization". Eric helped me change > Interpolation.h so it inherits from RefCountedWillBeGarbageCollectedFinalized > instead of RefCountedWillBeGarbageCollected to get rid of this. Ahh, I see! I don't think that's a change we should make lightly. Can we revert that and have m_nonInterpolableData be a regular member (just Vector, not OwnPtr<Vector>)? Then you can make startNonInterpolableData a regular vector too and copy or use .swap to move the data from one to the other.
https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... File Source/core/animation/ListStyleInterpolation.h (right): https://codereview.chromium.org/851633002/diff/400001/Source/core/animation/L... Source/core/animation/ListStyleInterpolation.h:30: OwnPtrWillBeRawPtr<Vector<typename InterpolationType::NonInterpolableType>> endNonInterpolableData = adoptPtrWillBeNoop(new Vector<typename InterpolationType::NonInterpolableType>()); On 2015/02/03 05:20:40, dstockwell wrote: > On 2015/02/03 03:45:50, evemj wrote: > > On 2015/02/03 02:22:18, dstockwell wrote: > > > I think end/startNonInterpolableData can be just OwnPtr<Vector<...>>, > > > startValue/endValue below must be OwnPtrWillBeRawPtr<InterpolableValue> -- > > > basically if the T in OwnPtr<T> is *WillBeGarbageCollected then you must use > > > *WillBeRawPtr. > > > > Done. > > > > Changed to own ptr and was getting compile error "Class > > 'ListStyleInterpolationImpl' requires finalization" specifically "Field > > 'm_nonInterpolableData' requires finalization". Eric helped me change > > Interpolation.h so it inherits from RefCountedWillBeGarbageCollectedFinalized > > instead of RefCountedWillBeGarbageCollected to get rid of this. > > Ahh, I see! I don't think that's a change we should make lightly. Can we revert > that and have m_nonInterpolableData be a regular member (just Vector, not > OwnPtr<Vector>)? Then you can make startNonInterpolableData a regular vector too > and copy or use .swap to move the data from one to the other. Hopefully done!
lgtm
The CQ bit was checked by evem@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/851633002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41185)
The CQ bit was checked by evem@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/851633002/500001
Message was sent while issue was closed.
Committed patchset #10 (id:500001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189490 |
