Introduce BorderImageLength and BorderImageLengthBox

Introduce BorderImageLength and BorderImageLengthBox

Getting rid of the Relative length type has been a goal for a while
and this is step one for getting rid of it for the remaining user:
border-image.

Specifically, the 'border-image-width' and 'border-image-outset'
longhand properties are the remaining users of the Relative length
type. Relative is used to indicate that the Length doesn't contain a
length at all, but a plain number (in this case a multiple of the
respective computed border width).

Wrap this usage within a BorderImageLength for now, to prepare for
BorderImageLength conditionally acting as a number itself and thus making
it possible to remove the Relative length type.

To limit scope, not all use of Length as a BorderImageLength is wrapped.
Notably parts of RenderStyle and the related animation code haven't
been converted. For this to work there is compatibility code that goes
from LengthBox to BorderImageLengthBox and back.

No functional changes expected. To a large part covered by existing
tests in LayoutTests/fast/borders/border-image-*.

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

[davve, 2013-11-01 12:43:42]

Be careful what you wish for :) (regarding comment in
http://code.google.com/p/chromium/issues/detail?id=304000).

This is patch 1/4 with the end goal of removing Relative. The rest, still
unpublished:

2/4: https://codereview.chromium.org/55813002
3/4: https://codereview.chromium.org/55833002
4/4: https://codereview.chromium.org/55843002

I'll publish them if you find the first part acceptable.

[davve, 2013-11-01 16:56:06]

On 2013/11/01 12:43:42, davve wrote:
> I'll publish them if you find the first part acceptable.

I changed my mind and published them all.

[Julien - ping for review, 2013-11-01 18:27:11]

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:182:
image.setOutset(LengthOrNumberBox({{ (["Length(0, Fixed)"]*4) | join(", ") }}));
We modify this call to setOutset() but leave the other ones to use LengthBox.
How about converting everyone?

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSComputedSt...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:477: top =
cssValuePool().createValue(box.top().length(), style);
Maybe be worth considering having a LengthOrNumber <-> CSSPrimitiveValue
conversion in CSSPrimitiveValueMapping.h (would be a follow-up change though)

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSToStyleMap...
File Source/core/css/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSToStyleMap...
Source/core/css/CSSToStyleMap.cpp:627:
box.setTop(LengthOrNumber(slices->top()->getIntValue()));
Nit: There is no explicit around LengthOrNumber(double) or
LengthOrNumber(Length) so you could use the implicit conversion to
LengthOrNumber.

https://codereview.chromium.org/55783002/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:1151: static LayoutUnit
computeBorderImageSide(LengthOrNumber borderSlice, LayoutUnit borderSide,
LayoutUnit imageSide, LayoutUnit boxExtent, RenderView* renderView)
const LengthOrNumber& to avoid a copy.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumber.h
File Source/platform/LengthOrNumber.h (right):

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumber.h:18: */
AFAICT Blink prefers a BSD license for new files unless you explicitly copied
from an existing one (http://www.chromium.org/blink/coding-style#TOC-License)

Also git seems to think this is a copy of LengthSize. You may want to use git cl
upload --no-find-copies to remove this smart copy detection.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumber.h:51: Length unifiedLength() const { return
m_length; }
Ideally this shouldn't exist but we have to integrate with the current code. I
would name it deprecatedUnifiedLength() to show that people should _not_ use
this function.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
File Source/platform/LengthOrNumberBox.h (right):

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:18: */
Same comment about the license.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:28: class PLATFORM_EXPORT LengthOrNumberBox
{
While the name describe what it is, I think it could be improved.

There are only 2 users of this class and both mean "multiple of the
border-width" by number so I think we should just narrow the scope of this class
and make this explicit. Not so great name but I couldn't come up with better:
LengthOrMultipleOfBorderWidthBox.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:50: explicit LengthOrNumberBox(LengthBox
lengthBox) :
Should we have a FIXME about removing this constructor. Also style violation,
the colon should be on the next line before m_left and the commas should be
aligned with the colon (like you did above)

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:58: LengthOrNumber left() const { return
m_left; }
const LengthOrNumber& to avoid a copy if we want?

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:63: void setLeft(const LengthOrNumber& o) {
m_left = o; }
|o| is never a good name, let's use |left|...

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:68: bool operator==(const LengthOrNumberBox&
o) const
OK, I lied, |o| is fine here (though |other| would be even more fine)

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:83: LengthBox lengthBox() const
Should we have a FIXME to remove this explicit conversion operator? Also we
should name it deprecatedLengthBox if we intent to remove it.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:100: 
Trailing empty lines.

[davve, 2013-11-04 09:55:36]

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
File Source/platform/LengthOrNumberBox.h (right):

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:28: class PLATFORM_EXPORT LengthOrNumberBox
{
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> While the name describe what it is, I think it could be improved.
> 
> There are only 2 users of this class and both mean "multiple of the
> border-width" by number so I think we should just narrow the scope of this
class
> and make this explicit. Not so great name but I couldn't come up with better:
> LengthOrMultipleOfBorderWidthBox.

Hm. If we rename the box, I think we should also rename what the box contains.
Given your suggestion, we would also rename LengthOrNumber to
LengthOrMultipleOfBorderWidth.

But as you rightly point out, it isn't a great name. So perhaps we shouldn't
name it after what it contains, but after what it is: a length in border-image
land. So my new suggestion would be BorderImageLength and BorderImageLengthBox.

Side note: One of the reasons I went with LengthOrNumber was that GridLength and
other similar instances in the spec could share datatype, but I'm not sure it is
worth it. If the pattern gets more common in the future, perhaps we could make
it a shared type then (or a shared base class).

[davve, 2013-11-04 12:42:53]

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:182:
image.setOutset(LengthOrNumberBox({{ (["Length(0, Fixed)"]*4) | join(", ") }}));
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> We modify this call to setOutset() but leave the other ones to use LengthBox.
> How about converting everyone?

I don't follow. Could you give an example?

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSComputedSt...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:477: top =
cssValuePool().createValue(box.top().length(), style);
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Maybe be worth considering having a LengthOrNumber <-> CSSPrimitiveValue
> conversion in CSSPrimitiveValueMapping.h (would be a follow-up change though)

Sure. I've put it on my todo for a later CL.

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSToStyleMap...
File Source/core/css/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/css/CSSToStyleMap...
Source/core/css/CSSToStyleMap.cpp:627:
box.setTop(LengthOrNumber(slices->top()->getIntValue()));
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Nit: There is no explicit around LengthOrNumber(double) or
> LengthOrNumber(Length) so you could use the implicit conversion to
> LengthOrNumber.

Thanks. I use an implicit constructor for both Length and double now.

https://codereview.chromium.org/55783002/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/55783002/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:1151: static LayoutUnit
computeBorderImageSide(LengthOrNumber borderSlice, LayoutUnit borderSide,
LayoutUnit imageSide, LayoutUnit boxExtent, RenderView* renderView)
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> const LengthOrNumber& to avoid a copy.

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumber.h
File Source/platform/LengthOrNumber.h (right):

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumber.h:18: */
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> AFAICT Blink prefers a BSD license for new files unless you explicitly copied
> from an existing one (http://www.chromium.org/blink/coding-style#TOC-License)
> 
> Also git seems to think this is a copy of LengthSize. You may want to use git
cl
> upload --no-find-copies to remove this smart copy detection.

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumber.h:51: Length unifiedLength() const { return
m_length; }
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Ideally this shouldn't exist but we have to integrate with the current code. I
> would name it deprecatedUnifiedLength() to show that people should _not_ use
> this function.

Good idea. Renamed to deprecatedUnifiedLength().

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
File Source/platform/LengthOrNumberBox.h (right):

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:18: */
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Same comment about the license.

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:50: explicit LengthOrNumberBox(LengthBox
lengthBox) :
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Should we have a FIXME about removing this constructor. Also style violation,
> the colon should be on the next line before m_left and the commas should be
> aligned with the colon (like you did above)

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:58: LengthOrNumber left() const { return
m_left; }
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> const LengthOrNumber& to avoid a copy if we want?

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:63: void setLeft(const LengthOrNumber& o) {
m_left = o; }
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> |o| is never a good name, let's use |left|...

OK.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:68: bool operator==(const LengthOrNumberBox&
o) const
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> OK, I lied, |o| is fine here (though |other| would be even more fine)

:) OK. (I like |other|)

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:83: LengthBox lengthBox() const
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Should we have a FIXME to remove this explicit conversion operator? Also we
> should name it deprecatedLengthBox if we intent to remove it.

OK on both points.

https://codereview.chromium.org/55783002/diff/1/Source/platform/LengthOrNumbe...
Source/platform/LengthOrNumberBox.h:100: 
On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> Trailing empty lines.

Thanks.

[Julien - ping for review, 2013-11-04 16:31:40]

lgtm

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right):

https://codereview.chromium.org/55783002/diff/1/Source/build/scripts/template...
Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:182:
image.setOutset(LengthOrNumberBox({{ (["Length(0, Fixed)"]*4) | join(", ") }}));
On 2013/11/04 12:42:54, David Vest wrote:
> On 2013/11/01 18:27:11, Julien Chaffraix - PST wrote:
> > We modify this call to setOutset() but leave the other ones to use
LengthBox.
> > How about converting everyone?
> 
> I don't follow. Could you give an example?

Mhh, it looks like I was the one confused and thought we had missed some cases
from the auto-generated CSS logic based the remaining getters/setters left on
RenderStyle (setBorderImageOutset, setBorderImageWidth...)

It's fine to push converting them to a follow-up change even if that means some
deprecated functions.

https://codereview.chromium.org/55783002/diff/120001/Source/core/css/CSSToSty...
File Source/core/css/CSSToStyleMap.h (left):

https://codereview.chromium.org/55783002/diff/120001/Source/core/css/CSSToSty...
Source/core/css/CSSToStyleMap.h:27: #include "platform/LengthBox.h"
Yay to reducing #include bloat!

https://codereview.chromium.org/55783002/diff/120001/Source/core/rendering/st...
File Source/core/rendering/style/NinePieceImage.h (right):

https://codereview.chromium.org/55783002/diff/120001/Source/core/rendering/st...
Source/core/rendering/style/NinePieceImage.h:63:
NinePieceImage(PassRefPtr<StyleImage>, LengthBox imageSlices, bool fill,
BorderImageLengthBox borderSlices, BorderImageLengthBox outset,
ENinePieceImageRule horizontalRule, ENinePieceImageRule verticalRule);
Nit: const BorderImageLengthBox& ?

https://codereview.chromium.org/55783002/diff/120001/Source/platform/BorderIm...
File Source/platform/BorderImageLength.h (right):

https://codereview.chromium.org/55783002/diff/120001/Source/platform/BorderIm...
Source/platform/BorderImageLength.h:38: class PLATFORM_EXPORT BorderImageLength
{
I like the new name.

It may be worth adding a comment to say what it represents (same with
BorderImageLengthBox). Ideally with a link to one of the properties' definition
for good measure.

https://codereview.chromium.org/55783002/diff/120001/Source/platform/BorderIm...
Source/platform/BorderImageLength.h:62: // FIXME: Remove when the conversion to
LengthOrMultipleOfBorderWidthBox is complete
Nit: Sentences end with a dot :)

(I haven't repeated this nitpick to the other comments in BorderImageLengthBox)

[commit-bot: I haz the power, 2013-11-04 21:11:37]

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

[commit-bot: I haz the power, 2013-11-04 22:36:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 09:16:23]

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

[commit-bot: I haz the power, 2013-11-05 10:28:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-07 12:40:54]

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

[commit-bot: I haz the power, 2013-11-07 13:39:36]

Change committed as 161527