Convert animation and renderer code to know about BorderImageLength

Convert animation and renderer code to know about BorderImageLength

This propagates BorderImageLengthBox further out in the code, but
actually converts it to an actual number or length as soon as possible
in the animation code, and animates it as the individual type.

Now we can also remove the compatibility conversion methods between
BorderImageLengthBox and LengthBox.

Also, update a couple of "Weird number of spaces at line-start"
indentations in CSSPropertyAnimation.cpp to not make the new code
inconsistent with the old wrt. indentation.

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

[Julien - ping for review, 2013-11-02 00:00:03]

Gave a quick look at the change. I think the transition should be covered by
tests (assuming that it's not already).

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
I don't think this is totally correct. In this case, we can still interpolate
from a Length to a number or vice-versa: Lengths being treated as real numbers
for the purpose of interpolation
(http://dev.w3.org/csswg/css-transitions/#animatable-types).

It's probably gross but Length::blend handles the interpolation correctly so you
would just have to consider a number as Fixed Length and that should give you
the right result (but it will need to be converted back to a number or else we
lose some information).

https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyle.h:477: LengthOrNumberBox
borderImageOutset() const { return surround->border.image().outset(); }
const LengthOrNumberBox& to avoid copies (if aware).

[alancutter (OOO until 2018), 2013-11-02 00:36:01]

> One thing to note is that AnimatableLengthBox will animate boxes of
> numbers too, making the box slightly misnamed. In practice, it's
> indeed oblivious to the type of the four animation values it contains.
> It's not a given that it is worth the trouble to rename it to
> AnimatableBox since we have new animation code coming around the
> corner.

I'm not sure I understand the last sentence. I would encourage renaming the
class to AnimatableBox if it is going to end up containing things other than
Lengths.

[davve, 2013-11-02 13:32:25]

On 2013/11/02 00:36:01, alancutter wrote:
> > One thing to note is that AnimatableLengthBox will animate boxes of
> > numbers too, making the box slightly misnamed. In practice, it's
> > indeed oblivious to the type of the four animation values it contains.
> > It's not a given that it is worth the trouble to rename it to
> > AnimatableBox since we have new animation code coming around the
> > corner.
> 
> I'm not sure I understand the last sentence. I would encourage renaming the
> class to AnimatableBox if it is going to end up containing things other than
> Lengths.

It already does, but disguised as "relative" lengths. I'm more than willing to
do the rename but would it be acceptable to do it in a separate CL?

[davve, 2013-11-03 13:38:42]

On 2013/11/02 13:32:25, David Vest wrote:
> It already does, but disguised as "relative" lengths. I'm more than willing to
> do the rename but would it be acceptable to do it in a separate CL?

Filed 314470 for this.

[shans, 2013-11-03 23:59:42]

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> I don't think this is totally correct. In this case, we can still interpolate
> from a Length to a number or vice-versa: Lengths being treated as real numbers
> for the purpose of interpolation
> (http://dev.w3.org/csswg/css-transitions/#animatable-types).
> 
> It's probably gross but Length::blend handles the interpolation correctly so
you
> would just have to consider a number as Fixed Length and that should give you
> the right result (but it will need to be converted back to a number or else we
> lose some information).

If we're interpolating from a length to a number, then we're doing something
like:
line-height: 50px;
to
line-height: 4;
(which means 4x font-size)

We could convert the 4 to a pixel value (say 40px if font-size is 10px), then
interpolate, and this would work; but how do we represent the result in the
computed style? Furthermore, how do we inherit the result correctly?

In particular, if we're at 50% then this should be a value which is half-way
between 50px and four times whatever the current font-size is (i.e. a value that
is responsive to changes in font-size). We could use a calc expression in the
computed style for this (calc(25px + 200%)), but this is an unreachable value
(there are no specified styles that one could use to generate this computed
style), and will have a different effect if used as a specified style.

I think the current behaviour is probably the best we can do right now.

[shans, 2013-11-04 00:09:55]

On 2013/11/03 23:59:42, shans wrote:
>
https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
> File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):
> 
>
https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
> Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
> On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> > I don't think this is totally correct. In this case, we can still
interpolate
> > from a Length to a number or vice-versa: Lengths being treated as real
numbers
> > for the purpose of interpolation
> > (http://dev.w3.org/csswg/css-transitions/#animatable-types).
> > 
> > It's probably gross but Length::blend handles the interpolation correctly so
> you
> > would just have to consider a number as Fixed Length and that should give
you
> > the right result (but it will need to be converted back to a number or else
we
> > lose some information).
> 
> If we're interpolating from a length to a number, then we're doing something
> like:
> line-height: 50px;
> to
> line-height: 4;
> (which means 4x font-size)
> 
> We could convert the 4 to a pixel value (say 40px if font-size is 10px), then
> interpolate, and this would work; but how do we represent the result in the
> computed style? Furthermore, how do we inherit the result correctly?
> 
> In particular, if we're at 50% then this should be a value which is half-way
> between 50px and four times whatever the current font-size is (i.e. a value
that
> is responsive to changes in font-size). We could use a calc expression in the
> computed style for this (calc(25px + 200%)), but this is an unreachable value
> (there are no specified styles that one could use to generate this computed
> style), and will have a different effect if used as a specified style.
> 
> I think the current behaviour is probably the best we can do right now.

Tim Loh just pointed out that line-height doesn't hit this code path. Let's try
again...

If we're interpolating from a length to a number, then we're doing something
like:
border-image-width: 50px;
to
border-image-width: 4;
(which means 4x border-width)

We could convert the 4 to a pixel value (say 40px if font-size is 10px), then
interpolate, and this would work; but how do we represent the result in the
computed style? Furthermore, how do we inherit the result correctly?

In particular, if we're at 50% then this should be a value which is half-way
between 50px and four times whatever the current border-width is (i.e. a value
that
is responsive to changes in border-width). We can't use a calc expression in the
computed style for this (calc(25px + 200%)) because this means 25px + twice the
width
of the box.

[davve, 2013-11-04 15:02:27]

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> I don't think this is totally correct. In this case, we can still interpolate
> from a Length to a number or vice-versa: Lengths being treated as real numbers
> for the purpose of interpolation
> (http://dev.w3.org/csswg/css-transitions/#animatable-types).

But no worse than the current code though? (see LengthBox variant of blendFunc)
for the current type check. If not worse than current, can we handle this in a
separate bug/issue?

https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyle.h:477: LengthOrNumberBox
borderImageOutset() const { return surround->border.image().outset(); }
On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> const LengthOrNumberBox& to avoid copies (if aware).
OK.

[davve, 2013-11-04 17:18:42]

On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> Gave a quick look at the change. I think the transition should be covered by
> tests (assuming that it's not already).

The animation and interpolation is covered by
LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}*
tests. At least the length case is covered. The number case seems to be
currently non-testable using that test framework because of bug
http://crbug.com/304000.

[Julien - ping for review, 2013-11-04 18:15:53]

lgtm

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/1/Source/core/frame/animation/C...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
On 2013/11/04 15:02:28, David Vest wrote:
> On 2013/11/02 00:00:03, Julien Chaffraix - PST wrote:
> > I don't think this is totally correct. In this case, we can still
interpolate
> > from a Length to a number or vice-versa: Lengths being treated as real
numbers
> > for the purpose of interpolation
> > (http://dev.w3.org/csswg/css-transitions/#animatable-types).
> 
> But no worse than the current code though? (see LengthBox variant of
blendFunc)
> for the current type check. If not worse than current, can we handle this in a
> separate bug/issue?

Fine point. Agreed to punt this to a separate issue, especially considering that
the testing is blocked on a bug!

@shans, I think your proposal would work... that is assuming you don't also
animate 'border-width'.

https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
This is missing a FIXME per our discussion.

https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm...
File Source/platform/BorderImageLengthBox.h (right):

https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm...
Source/platform/BorderImageLengthBox.h:44: BorderImageLengthBox(Length t, Length
r, Length b, Length l)
Do we still want to keep this constructor?

https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm...
Source/platform/BorderImageLengthBox.h:60: BorderImageLengthBox(const
BorderImageLength& t, const BorderImageLength& r, const BorderImageLength& b,
const BorderImageLength& l)
Remember my stick about |o|, well it applies also to |t|, |r|, |b| and |l| (and
most one letter variable names)

[Steve Block, 2013-11-05 03:16:12]

> The animation and interpolation is covered by
> LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}*
> tests. At least the length case is covered. The number case seems to be
> currently non-testable using that test framework because of bug
> http://crbug.com/304000.
What exactly is the problem? Can you file a bug? I have plans to beef up those
tests so will look into it.

[davve, 2013-11-07 13:12:59]

On 2013/11/05 03:16:12, Steve Block wrote:
> > The animation and interpolation is covered by
> > LayoutTests/animations/interpolation/{border-image,webkit-mask-box-image}*
> > tests. At least the length case is covered. The number case seems to be
> > currently non-testable using that test framework because of bug
> > http://crbug.com/304000.
> What exactly is the problem? Can you file a bug? I have plans to beef up those
> tests so will look into it.

Not sure there is a bug to file. It looked like the framework tried setting the
expected value in some manner, but because of http://crbug.com/304000 no
non-integer values can be set (since the css parser doesn't allow it yet). When
fixing that bug, there should be no issue.

[davve, 2013-11-07 13:18:45]

https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat...
File Source/core/frame/animation/CSSPropertyAnimation.cpp (right):

https://codereview.chromium.org/55813002/diff/100001/Source/core/frame/animat...
Source/core/frame/animation/CSSPropertyAnimation.cpp:82: return to;
On 2013/11/04 18:15:54, Julien Chaffraix - PST wrote:
> This is missing a FIXME per our discussion.
OK.

https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm...
File Source/platform/BorderImageLengthBox.h (right):

https://codereview.chromium.org/55813002/diff/100001/Source/platform/BorderIm...
Source/platform/BorderImageLengthBox.h:60: BorderImageLengthBox(const
BorderImageLength& t, const BorderImageLength& r, const BorderImageLength& b,
const BorderImageLength& l)
On 2013/11/04 18:15:54, Julien Chaffraix - PST wrote:
> Remember my stick about |o|, well it applies also to |t|, |r|, |b| and |l|
(and
> most one letter variable names)

OK.

[Steve Block, 2013-11-07 22:48:39]

OK, got it. For the short term, I'll add some testing that uses integer numbers.

[commit-bot: I haz the power, 2013-11-11 05:20:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/55813002/290001

[commit-bot: I haz the power, 2013-11-11 10:33:11]

Change committed as 161715

[Steve Block, 2013-11-14 03:21:47]

I added some additional changes in https://codereview.chromium.org/54123007
It looks like http://crbug.com/304000 can be closed now?

[Steve Block, 2013-11-14 03:22:20]

> I added some additional changes in https://codereview.chromium.org/54123007
s/changes/tests