[Alignment] Single class for holding the alignment data.

[Alignment] Single class for holding the alignment data.

The new CSS3 Box Alignment specification changed the value of alignment
properties, like align-self, align-items and justify-content to be
complex values rather than single keywords.

We have implemented the new spec by defining different class fields for
each kind of keyword, eg. <item-position>, <content-position>, <overflow>
and <content-distribution>. This approach has been proved to be fairly
inconsitent and buggy, as several Flexbox regressions have shown so far.

This patch defines a single class to hold all the alignment data stored
before in the different StyleRareNonInherited class fields. This way we
can detect style changes more easily, provide more consitent behavior
and ensuring backward compatibility with Flexbox implementation.

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

[jfernandez, 2015-04-09 15:08:48]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jchaffraix@chromium.org, timloh@chromium.org

[Julien - ping for review, 2015-04-09 15:23:20]

It's weird that the style application logic wasn't updated to use the new class.
Is this something you want to move to a follow-up change or ...?

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
Source/core/style/StyleAlignmentData.h:15: StyleAlignmentData(ContentPosition,
ContentDistributionType, OverflowAlignment);
I would love a class comment to explain these constructors.

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
Source/core/style/StyleAlignmentData.h:20: return m_itemPosition ==
o.m_itemPosition && m_contentPosition == o.m_contentPosition && m_distribution
== o.m_distribution && m_positionType == o.m_positionType && m_overflow ==
o.m_overflow && m_positionType == o.m_positionType;
Nit: This doesn't follow the order below (m_positionType and m_overflow are
swapped).

[jfernandez, 2015-04-09 15:55:55]

About the issue of not updating the style application, I thought that keeping
the different ComputedStyle methods for accessing the alignment data was
interesting. My patch just holds the info in the new StyleAlignmentData class
but we still have to ask for specific alignment keywords, <item-position>,
<content-position>, <overflow>, ... 

So in that sense, nothing changed outside the core/style classes, hence layout
still uses the same API for accessing the data.

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
Source/core/style/StyleAlignmentData.h:15: StyleAlignmentData(ContentPosition,
ContentDistributionType, OverflowAlignment);
On 2015/04/09 15:23:20, Julien - PST CC moar reviewers wrote:
> I would love a class comment to explain these constructors.

Done.

https://codereview.chromium.org/1070143002/diff/1/Source/core/style/StyleAlig...
Source/core/style/StyleAlignmentData.h:20: return m_itemPosition ==
o.m_itemPosition && m_contentPosition == o.m_contentPosition && m_distribution
== o.m_distribution && m_positionType == o.m_positionType && m_overflow ==
o.m_overflow && m_positionType == o.m_positionType;
On 2015/04/09 15:23:20, Julien - PST CC moar reviewers wrote:
> Nit: This doesn't follow the order below (m_positionType and m_overflow are
> swapped).

Done.

[Timothy Loh, 2015-04-10 05:42:44]

Same as Julien has said, we should update the StyleBuilder logic now. It looks
like we can just use the StyleBuilderConverter for applying values and the
default code-paths for initial/inherit?

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.cpp (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:28:
StyleAlignmentData::StyleAlignmentData(const StyleAlignmentData& o)
Can we omit this and use the default copy ctor?

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:33: ItemPosition m_itemPosition;
Pretty sure you still need to specify these as bitfields otherwise they'll get
packed really badly.

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleRareNonInheritedData.h:154: StyleAlignmentData
m_alignContent;
Best to move all of these (and the ones below) outside of the bitfield section
for better packing.

[jfernandez, 2015-04-10 23:34:33]

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.cpp (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/04/10 05:42:44, Timothy Loh wrote:
> 2015

Done.

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:28:
StyleAlignmentData::StyleAlignmentData(const StyleAlignmentData& o)
On 2015/04/10 05:42:44, Timothy Loh wrote:
> Can we omit this and use the default copy ctor?

Done.

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/04/10 05:42:44, Timothy Loh wrote:
> 2015

Done.

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:33: ItemPosition m_itemPosition;
On 2015/04/10 05:42:44, Timothy Loh wrote:
> Pretty sure you still need to specify these as bitfields otherwise they'll get
> packed really badly.

Done.

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1070143002/diff/20001/Source/core/style/Style...
Source/core/style/StyleRareNonInheritedData.h:154: StyleAlignmentData
m_alignContent;
On 2015/04/10 05:42:44, Timothy Loh wrote:
> Best to move all of these (and the ones below) outside of the bitfield section
> for better packing.

Done.

[Timothy Loh, 2015-04-13 00:56:43]

This is looking pretty good now :-)

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:26: bool operator==(ItemPosition
position) const
These overloads seem a bit weird to me, why do we need them?

[Julien - ping for review, 2015-04-13 14:46:20]

lgtm with the comments fixed.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:339: ItemPositionType
positionType = NonLegacyPosition;
This is subtly duplicating the logic from the initial function on ComputedStyle.
I would be more comfortable using one of them here instead.

If that means having 6 functions that call back into these 2 ones so that we use
the right initial values, that's an improvement in my book.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:361: OverflowAlignment
overflow = OverflowAlignmentDefault;
Same comment.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.h:61: static StyleAlignmentData
convertSelfAlignmentData(StyleResolverState&, CSSValue*);
This name doesn't really reflect the callers as it's called for both *-self and
*-items properties.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.cpp (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:26: }
Any reason why this is not inlined?

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:26: bool operator==(ItemPosition
position) const
On 2015/04/13 at 00:56:43, Timothy Loh wrote:
> These overloads seem a bit weird to me, why do we need them?

They are used above when we do e.g. justifyItems() == LegacyPosition. I do agree
that this is weird and it's an easy foot-gun.

[jfernandez, 2015-04-14 00:18:43]

In order to use the ComputedStyle initial value functions of each property we
need to use different converters. The code to handle most of the alignment
properties is pretty similar (almost identical), just why I used the Template
approach before. However, I think the last patch minimizes code duplication as
much as possible.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:339: ItemPositionType
positionType = NonLegacyPosition;
On 2015/04/13 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> This is subtly duplicating the logic from the initial function on
ComputedStyle.
> I would be more comfortable using one of them here instead.
> 
> If that means having 6 functions that call back into these 2 ones so that we
use
> the right initial values, that's an improvement in my book.

Done.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.cpp:361: OverflowAlignment
overflow = OverflowAlignmentDefault;
On 2015/04/13 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> Same comment.

Done.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleBuilderConverter.h:61: static StyleAlignmentData
convertSelfAlignmentData(StyleResolverState&, CSSValue*);
On 2015/04/13 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> This name doesn't really reflect the callers as it's called for both *-self
and
> *-items properties.

Ok, that's true. f we want to use the ComputedStyle initial value functions I'll
have to define a converter function for each property, which is probably a
better idea.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.cpp (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.cpp:26: }
On 2015/04/13 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> Any reason why this is not inlined?

No reason. It can be inlined, indeed.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:26: bool operator==(ItemPosition
position) const
On 2015/04/13 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> On 2015/04/13 at 00:56:43, Timothy Loh wrote:
> > These overloads seem a bit weird to me, why do we need them?
> 
> They are used above when we do e.g. justifyItems() == LegacyPosition. I do
agree
> that this is weird and it's an easy foot-gun.

Done.

[esprehn, 2015-04-14 00:48:55]

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

[esprehn, 2015-04-14 00:48:55]

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:26: bool operator==(ItemPosition
position) const
On 2015/04/13 at 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> On 2015/04/13 at 00:56:43, Timothy Loh wrote:
> > These overloads seem a bit weird to me, why do we need them?
> 
> They are used above when we do e.g. justifyItems() == LegacyPosition. I do
agree that this is weird and it's an easy foot-gun.

I'd like to avoid an operator== overload to a totally different type.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:41: bool operator==(ItemPositionType
positionType) const
Can we remove these and instead be explicit.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:51: unsigned m_itemPosition : 4; //
ItemPosition
Remove the m_ prefix if these are going to be public (or add getters).

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleRareNonInheritedData.h:155: StyleAlignmentData
m_justifySelf;
This is making this object a bunch bigger since the fields don't pack anymore.
Can you measure the difference?

[jfernandez, 2015-04-14 22:56:21]

I still have to verify how this patch affects to the StyleRareNonInherited class
size, as @esprehn asked in his review, but the last patch contains the rest of
the suggested changes.

@timloh could you please give your opinion on the StyleConverter changes ? I
tried to use the ComputedStyle initial value functions, as suggested, but that
lead to define a converter function for each property. They are almost
identical, but I tried to reduce the code duplication as much as possible.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleAlignmentData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:26: bool operator==(ItemPosition
position) const
On 2015/04/14 00:48:55, esprehn wrote:
> On 2015/04/13 at 14:46:20, Julien - OOO 18 Apr - 14 May wrote:
> > On 2015/04/13 at 00:56:43, Timothy Loh wrote:
> > > These overloads seem a bit weird to me, why do we need them?
> > 
> > They are used above when we do e.g. justifyItems() == LegacyPosition. I do
> agree that this is weird and it's an easy foot-gun.
> 
> I'd like to avoid an operator== overload to a totally different type.

Acknowledged.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:41: bool operator==(ItemPositionType
positionType) const
On 2015/04/14 00:48:55, esprehn wrote:
> Can we remove these and instead be explicit.

Yes, it'd be a good idea. However, I finally removed them and I just called
specific getter methods to perform the different value comparisons across the
code base.  I was experimenting with functions like "isEqualTo(ItemPosition)"
instead of the operator overloading, but I don't think an advantage from just
using the getter approach.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleAlignmentData.h:51: unsigned m_itemPosition : 4; //
ItemPosition
On 2015/04/14 00:48:55, esprehn wrote:
> Remove the m_ prefix if these are going to be public (or add getters).

I finally made private the class fields, so they still have the m_prefix.

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1070143002/diff/40001/Source/core/style/Style...
Source/core/style/StyleRareNonInheritedData.h:155: StyleAlignmentData
m_justifySelf;
On 2015/04/14 00:48:55, esprehn wrote:
> This is making this object a bunch bigger since the fields don't pack anymore.
> Can you measure the difference?

As we commented in the IRC, I'll try yo land a patch with a compile time
assertion to check out the size increment.

[Timothy Loh, 2015-04-15 09:47:10]

On 2015/04/14 22:56:21, jfernandez wrote:
> @timloh could you please give your opinion on the StyleConverter changes ? I
> tried to use the ComputedStyle initial value functions, as suggested, but that
> lead to define a converter function for each property. They are almost
> identical, but I tried to reduce the code duplication as much as possible.

I'd probably first unify the initialFoo functions so you only have two of them.
You can specify in CSSProperties.in which function to grab the initial value
from. I'd rather not have identical converter functions (although.. I don't
really know what names to give these...)

[jfernandez, 2015-04-15 23:03:04]

On 2015/04/15 09:47:10, Timothy Loh wrote:
> On 2015/04/14 22:56:21, jfernandez wrote:
> > @timloh could you please give your opinion on the StyleConverter changes ? I
> > tried to use the ComputedStyle initial value functions, as suggested, but
that
> > lead to define a converter function for each property. They are almost
> > identical, but I tried to reduce the code duplication as much as possible.
> 
> I'd probably first unify the initialFoo functions so you only have two of
them.
> You can specify in CSSProperties.in which function to grab the initial value
> from. I'd rather not have identical converter functions (although.. I don't
> really know what names to give these...)

Patch set 6 implements the approach you suggested. I think it looks fine, even
though I have some concerns on not having initial value function for each
property;
It's true that so far, they are all the same, but specs might change in the
future.

Regarding @esprehn concerns about the StyleRareNonInheritedData class size
increase, I submitted a patch in issue #1086123002 we control how much it
grows. I measured locally this change and it's 16 bytes (both using a single
class
or two, like in the last patch). We have been considering to use a DataRef or
RefPtr
to reduce the size increase, but we are not sure if it's worth; DataRef template
does
not fit perfectly and it'd be less clean code.

[Timothy Loh, 2015-04-24 10:29:53]

I don't think there'll be any problems changing things if the specs change.
lgtm.

https://codereview.chromium.org/1070143002/diff/120001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1070143002/diff/120001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1241: void setAlignContent(ContentPosition
position) { rareNonInheritedData.access()->m_alignContent.setPosition(position);
}
Can we keep the previous names? Also maybe better to leave out the setters which
aren't used.

[jfernandez, 2015-04-26 19:44:15]

On 2015/04/24 10:29:53, Timothy Loh wrote:
> I don't think there'll be any problems changing things if the specs change.
> lgtm.
> 

Well, this change is not only about specs changing, but a new class to manage
all the alignment data instead of using several unsigned fields. The rationale
of
this change is that is more consistent to deal with style changes and
invalidations.

As it's appreciated in the patch, we are increasing the StyleRareNonInherited
class
size in 16 bytes, respecting the old approach of using different unsigned
fields. 

I guess we are all on the same page so I'll go ahead and land this patch.

[jfernandez, 2015-04-26 19:44:54]

After applying the suggested changes, I'll try the CQ now.

https://codereview.chromium.org/1070143002/diff/120001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1070143002/diff/120001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1241: void setAlignContent(ContentPosition
position) { rareNonInheritedData.access()->m_alignContent.setPosition(position);
}
On 2015/04/24 10:29:52, Timothy Loh wrote:
> Can we keep the previous names? Also maybe better to leave out the setters
which
> aren't used.

Well. some of the setters are used, actually; not all of them though. I think
it's better to have them all defined for consistency, rather that just the ones
being used. 

I'll get back the previous names, as you have suggested.

[jfernandez, 2015-04-26 19:45:27]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2015-04-26 19:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1070143002/#ps140001
(title: "Rebased and got back previous setter names.")

[commit-bot: I haz the power, 2015-04-26 19:45:35]

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

[commit-bot: I haz the power, 2015-04-26 21:02:16]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194466