Animation: Add CSSPropertyClip and CSSPropertyBorderImageSlice to StringKeyframe

Animation: Add CSSPropertyClip and CSSPropertyBorderImageSlice to StringKeyframe

Implemented CSSBoarderImageSlice and CSSPropertyClip in
StringKeyframe.

CSSPropertyBorderImageSlice was incorrectly falling to LengthBox
interpolation rather than BorderImageSlice as it is supposed to.
CSSPropertyClip had to handle case of a rect side being an automatic
value.

BUG=452383
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190118

[jadeg, 2015-01-27 05:49:41]

jadeg@chromium.org changed reviewers:
+ ericwilligers@chromium.org, evem@chromium.org, samli@chromium.org

[Eric Willigers, 2015-01-27 23:21:14]

lgtm

In the description, change CSSBoarderImageSlice to CSSBorderImageSlice.

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/Length...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/Length...
Source/core/animation/LengthBoxStyleInterpolation.cpp:76: void
LengthBoxStyleInterpolation::apply(StyleResolverState& state) const
If you move apply and trace to before borderImageSlicetoInterpolableValue, the
diff will be smaller.

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
File Source/core/animation/StringKeyframe.cpp (left):

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
Source/core/animation/StringKeyframe.cpp:228: case
CSSPropertyWebkitMaskBoxImageSlice:
Move opening brace to the end of this line:

case CSSPropertyWebkitMaskBoxImageSlice: {

[evemj (not active), 2015-01-27 23:35:37]

lgtm

[dstockwell, 2015-01-30 04:34:07]

dstockwell@chromium.org changed reviewers:
+ dstockwell@chromium.org

[dstockwell, 2015-01-30 04:34:08]

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
Source/core/animation/StringKeyframe.cpp:230: fallBackToLegacy = true;
Why aren't we handling auto here?

[jadeg, 2015-02-04 23:16:26]

On 2015/01/30 04:34:08, dstockwell wrote:
>
https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
> File Source/core/animation/StringKeyframe.cpp (right):
> 
>
https://codereview.chromium.org/878863002/diff/1/Source/core/animation/String...
> Source/core/animation/StringKeyframe.cpp:230: fallBackToLegacy = true;
> Why aren't we handling auto here?

It was crashing before on auto, but after rebase this is no longer happening so
I have removed it.

[dstockwell, 2015-02-04 23:34:54]

lgtm

[jadeg, 2015-02-05 07:01:10]

New patchsets have been uploaded after l-g-t-m from dstockwell@chromium.org

[jadeg, 2015-02-11 03:15:33]

On 2015/02/04 23:34:54, dstockwell wrote:
> lgtm

Sorry my earlier patch was wrong, the auto values were not being automatically
implemented. I have implemented them now though, after discussing with shane
that they will have to be implemented like this until alan's applicator is
working.

[jadeg, 2015-02-11 03:15:52]

jadeg@chromium.org changed reviewers:
+ alancutter@chromium.org

[Eric Willigers, 2015-02-11 03:56:32]

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:24: } else if
(LengthStyleInterpolation::canCreateFrom(*side[i])) {
The control flow can be simplified using

else {
    ASSERT(LengthStyleInterpolation::canCreateFrom(*side[i]));
    ...
}

with no need for ASSERT_NOT_REACHED

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:95:
StyleBuilder::applyProperty(m_id, state,
toInterpolableBool(m_cachedValue.get())->value() ? m_endCSSValue.get() :
m_startCSSValue.get());
DefaultStyleInterpolation achieves this. We have the option of not using
LengthBoxStyleInterpolation for this case.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:102: {
m_startCSSValue and m_endCSSValue need to be traced.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.h (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:16: static bool
onlyLengthToAuto(Rect* startRect, Rect* endRect)
This method isn't trivial, so move implementation to the .cpp

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:24: static
PassRefPtrWillBeRawPtr<LengthBoxStyleInterpolation> maybeCreateFrom(CSSValue*
start, CSSValue* end, CSSPropertyID id)
This method isn't trivial, so move implementation to the .cpp

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolationTest.cpp (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolationTest.cpp:1: /*// Copyright 2014
The Chromium Authors. All rights reserved.
We shouldn't leave the tests commented out.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:268: {
Move brace to end of previous line.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:273: }
This brace and the preceding lines can be unindented.

[jadeg, 2015-02-12 05:06:00]

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:24: } else if
(LengthStyleInterpolation::canCreateFrom(*side[i])) {
On 2015/02/11 03:56:32, Eric Willigers wrote:
> The control flow can be simplified using
> 
> else {
>     ASSERT(LengthStyleInterpolation::canCreateFrom(*side[i]));
>     ...
> }
> 
> with no need for ASSERT_NOT_REACHED

Done.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:95:
StyleBuilder::applyProperty(m_id, state,
toInterpolableBool(m_cachedValue.get())->value() ? m_endCSSValue.get() :
m_startCSSValue.get());
On 2015/02/11 03:56:32, Eric Willigers wrote:
> DefaultStyleInterpolation achieves this. We have the option of not using
> LengthBoxStyleInterpolation for this case.

I agree, but I am just thinking now might be a good idea to keep this logic
contained? Not sure if this is important/valuable

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:102: {
On 2015/02/11 03:56:32, Eric Willigers wrote:
> m_startCSSValue and m_endCSSValue need to be traced.

Done.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.h (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:16: static bool
onlyLengthToAuto(Rect* startRect, Rect* endRect)
On 2015/02/11 03:56:32, Eric Willigers wrote:
> This method isn't trivial, so move implementation to the .cpp

Done.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:24: static
PassRefPtrWillBeRawPtr<LengthBoxStyleInterpolation> maybeCreateFrom(CSSValue*
start, CSSValue* end, CSSPropertyID id)
On 2015/02/11 03:56:32, Eric Willigers wrote:
> This method isn't trivial, so move implementation to the .cpp

Done.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:268: {
On 2015/02/11 03:56:32, Eric Willigers wrote:
> Move brace to end of previous line.

Done.

https://codereview.chromium.org/878863002/diff/60001/Source/core/animation/St...
Source/core/animation/StringKeyframe.cpp:273: }
On 2015/02/11 03:56:32, Eric Willigers wrote:
> This brace and the preceding lines can be unindented.

Done.

[dstockwell, 2015-02-13 02:30:36]

I think this looks OK. Some minor naming/const things to fix.

We may want to move BorderImageSlice to a separate interpolation, but perhaps
that can happen later.

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:14: bool
onlyLengthToAuto(Rect* startRect, Rect* endRect)
Not sure what this name means, is there a more descriptive one?

startRect/endRect should be references (since they are never null)

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:39: return
adoptRefWillBeNoop(new
LengthBoxStyleInterpolation(InterpolableBool::create(false),
InterpolableBool::create(true), id, false, start, end));
Is this different from a default interpolation (returning null)?

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:42: if
(start->isBorderImageSliceValue() && toCSSBorderImageSliceValue(start)->slices()
BorderImageSlice doesn't work the same way with 'auto'? It seems like we're
conflating to different types of interpolation here, maybe BorderImageSlice
should use an entirely different Interpolation class.

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:71:
PassRefPtr<CSSPrimitiveValue> findValue(InterpolableList* lengthBox, size_t i,
CSSPrimitiveValue* start[], CSSPrimitiveValue* end[])
findValue is a bit vague, is indexedValueToLength better?

lengthBox/start/end should be references (and probably const)

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.h (right):

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:18: static bool
usesDefault(const CSSValue&, const CSSValue&);
This doesn't seem to be used.

[evemj (not active), 2015-02-13 04:04:00]

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:14: bool
onlyLengthToAuto(Rect* startRect, Rect* endRect)
On 2015/02/13 02:30:35, dstockwell wrote:
> Not sure what this name means, is there a more descriptive one?
> 
> startRect/endRect should be references (since they are never null)

Done.

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:39: return
adoptRefWillBeNoop(new
LengthBoxStyleInterpolation(InterpolableBool::create(false),
InterpolableBool::create(true), id, false, start, end));
On 2015/02/13 02:30:35, dstockwell wrote:
> Is this different from a default interpolation (returning null)?

Moved to using DefaultStyleInterpolation in StringKeyframe

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:71:
PassRefPtr<CSSPrimitiveValue> findValue(InterpolableList* lengthBox, size_t i,
CSSPrimitiveValue* start[], CSSPrimitiveValue* end[])
On 2015/02/13 02:30:35, dstockwell wrote:
> findValue is a bit vague, is indexedValueToLength better?
> 
> lengthBox/start/end should be references (and probably const)

Done.

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.h (right):

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.h:18: static bool
usesDefault(const CSSValue&, const CSSValue&);
On 2015/02/13 02:30:35, dstockwell wrote:
> This doesn't seem to be used.

Done.

[jadeg, 2015-02-13 05:07:59]

The CQ bit was checked by jadeg@chromium.org

[commit-bot: I haz the power, 2015-02-13 05:08:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878863002/80001

[commit-bot: I haz the power, 2015-02-13 07:13:24]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190118

[sof, 2015-02-13 08:40:37]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-02-13 08:40:38]

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/878863002/diff/80001/Source/core/animation/Le...
Source/core/animation/LengthBoxStyleInterpolation.cpp:71:
PassRefPtr<CSSPrimitiveValue> findValue(InterpolableList* lengthBox, size_t i,
CSSPrimitiveValue* start[], CSSPrimitiveValue* end[])
On 2015/02/13 04:04:00, evemj wrote:
> On 2015/02/13 02:30:35, dstockwell wrote:
> > findValue is a bit vague, is indexedValueToLength better?
> > 
> > lengthBox/start/end should be references (and probably const)
> 
> Done.

Intentionally kept as findValue()? (This helper didn't have an Oilpan-compatible
return type; addressed by https://codereview.chromium.org/919363002/ )