[CSS Grid Layout] Adapting align-self, align-items and justify-self to the last CSS 3 spec.

[CSS Grid Layout] Adapting align-self, align-items and justify-self to 
the last CSS 3 spec.

Added support and test cases for the new values, baseline last-baseline,
flex-start, flex-end. Properly set the initial value to 'auto' in all properties. 

Resolving the 'auto' Computed Value in the StyleAdjuster class, so it's
valid for both, CSSComputedStyleDeclaration and the corresponding
RenderObjects.

BUG=249451, 376823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179307

[jfernandez, 2014-07-03 13:22:57]

This is the initial implementation of what I think is the right way to resolve
the computed value for the "auto" case in the alignment css properties,
following the latest CSS 3 Alignment specification draft.

The changes in the StyleAdjuster made the resolution valid not only for the
CSSComputedDeclaration code, but for the specific RenderObjects involved in the
further layout phase; this statement applies to both, Gird and Flexible boxes.

[Timothy Loh, 2014-07-04 01:26:41]

I think the logic here should be in the StyleBuilder instead of
adjustRenderStyle (unless we need to compare against an adjusted display). You
might need to move the position property in CSSPropertyNames.in to the high
priority section.

cc esprehn who knows more about adjustRenderStyle; does this seem right to you?

[Julien - ping for review, 2014-07-08 16:05:39]

On 2014/07/04 at 01:26:41, timloh wrote:
> I think the logic here should be in the StyleBuilder instead of
adjustRenderStyle (unless we need to compare against an adjusted display). You
might need to move the position property in CSSPropertyNames.in to the high
priority section.
> 
> cc esprehn who knows more about adjustRenderStyle; does this seem right to
you?

I think it would work as we should rerun the cascade  (thus recomputing  when
changing 'position'. I was the one to suggest StyleAdjuster as I didn't know
about the high-priority properties to solve such dependencies.

https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGridTest.cpp (right):

https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGridTest.cpp:20: // Base class for RenderGrid
testing, it exposes some useful getter.
I don't think this is a RenderGrid testing, we only test RenderStyle, which
happens to be on a RenderGrid.

That's why I think this would be better in the css/ directly. Thus we could test
non-grid renderers.

[jfernandez, 2014-07-09 13:09:09]

Hi, thanks for the review.

On 2014/07/04 01:26:41, Timothy Loh wrote:
> I think the logic here should be in the StyleBuilder instead of
> adjustRenderStyle (unless we need to compare against an adjusted display). You
> might need to move the position property in CSSPropertyNames.in to the high
> priority section.
> 
> cc esprehn who knows more about adjustRenderStyle; does this seem right to
you?

I've moved the "auto" computed value logic to the builder, but the problem now
is that
builder functions are only called when specific CSS rules are set; hence, when
no rule is
set the justify-xxx and align-xxx are set to the initial/default values, which
is "auto",
according to the specs.

As far as I understand the specs, the computed value for "auto", either as
default or 
explicitly set, should be "start", "stretch" or the computed value in the
parent's xxx-items, 
depending on the "position" and "display" property values. 

Moving both, "display" and "position" to the high priority section works fine on
these
cases, but it does not work as expected for the default value. 

There is another angle on this issue, I think I've already commented about it,
that is
avoid duplicate logic in the CSSComputedStyleDeclaration and in the specific
RenderObjects
implementing the alignment and justification properties. That was the previous
approach for
align-items and align-self, but I thought it was better to share the logic.
That's why I thought
the StyleAdjuster was the proper place to implement it.

[jfernandez, 2014-07-09 13:14:52]

Hi Julien, 

On 2014/07/08 16:05:39, Julien Chaffraix - PST wrote:
>
>
https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderGridTest.cpp (right):
> 
>
https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGridTest.cpp:20: // Base class for RenderGrid
> testing, it exposes some useful getter.
> I don't think this is a RenderGrid testing, we only test RenderStyle, which
> happens to be on a RenderGrid.

Well, I've just defined the test cases related to this issue, but I had in mind
to use this
class for future RenderGrid related unit tests. Actually, I did it because you
had suggested
so in a  previous review: 

https://codereview.chromium.org/333423005/#msg8

> 
> That's why I think this would be better in the css/ directly. Thus we could
test
> non-grid renderers.

I think current layout tests already cover these cases for grid, flex and
regular blocks.
I had understood that perhaps verifying the resolved value was correct at
rendering
was also useful/required.

[esprehn, 2014-07-11 22:38:10]

Why isn't this going to break all the existing flexbox content?

https://codereview.chromium.org/363133003/diff/20001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-align-items-expected.txt (right):

https://codereview.chromium.org/363133003/diff/20001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-align-items-expected.txt:29: PASS
getComputedStyle(element, '').getPropertyValue('align-items') is 'start'
Changing the initial value of align-items from stretch to start seems like it's
going to break a lot of content...

[jfernandez, 2014-07-11 22:52:40]

On 2014/07/11 22:38:10, esprehn wrote:
> Why isn't this going to break all the existing flexbox content?
> 

As it's stated in the specs, it computes to "start" for everything except "flex"
or grid".

>
https://codereview.chromium.org/363133003/diff/20001/LayoutTests/fast/alignme...
> File LayoutTests/fast/alignment/parse-align-items-expected.txt (right):
> 
>
https://codereview.chromium.org/363133003/diff/20001/LayoutTests/fast/alignme...
> LayoutTests/fast/alignment/parse-align-items-expected.txt:29: PASS
> getComputedStyle(element, '').getPropertyValue('align-items') is 'start'
> Changing the initial value of align-items from stretch to start seems like
it's
> going to break a lot of content...

As I said, it changes the default value to "auto", which it will be resolved to
the actual computed value depending on the type of element.

[esprehn, 2014-07-12 00:39:19]

Why not make the initial value ItemPositionStart and then only change it to
Stretch for the other display types? It's going to almost always be auto which
means you're adding a bunch of if statement overhead here, if you could early
return for all things that are not display flex/grid that would reduce it.

https://codereview.chromium.org/363133003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/363133003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleAdjuster.cpp:261: void
StyleAdjuster::adjustStyleForAlignment(RenderStyle* style, RenderStyle*
parentStyle)
These should both be references.

[jfernandez, 2014-07-12 09:35:31]

On 2014/07/12 00:39:19, esprehn wrote:
> Why not make the initial value ItemPositionStart and then only change it to
> Stretch for the other display types? It's going to almost always be auto which
> means you're adding a bunch of if statement overhead here, if you could early
> return for all things that are not display flex/grid that would reduce it.
> 

Yes, that can definitively be done that way. The key question, though, was
whether
this logic should be implemented in StyleAdjuster or StyleBuilder. 

It seems the "natural" place for this would be the StyleBuilder; however, I
noticed that
the StyleBuilderFunctions are not called when there are no style rules to match.
That
caused that many properties were computed to "auto", which is what we are trying
to
avoid. 

Your suggestion about using "start" for the general case and only switch to
stretch
for the specific displays would solve this issue, so it would be possible to
implement
the computed value resolution logic in the builder. Do you think is better to
use the
style builder for this, as other have already stated in this thread ? 

>
https://codereview.chromium.org/363133003/diff/20001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/363133003/diff/20001/Source/core/css/resolver...
> Source/core/css/resolver/StyleAdjuster.cpp:261: void
> StyleAdjuster::adjustStyleForAlignment(RenderStyle* style, RenderStyle*
> parentStyle)
> These should both be references.

Yeah, Ill do this if we finally go for the StyleAdjust approach.

[jfernandez, 2014-07-13 15:09:20]

Hi,  

On 2014/07/12 00:39:19, esprehn wrote:
> Why not make the initial value ItemPositionStart and then only change it to
> Stretch for the other display types? It's going to almost always be auto which
> means you're adding a bunch of if statement overhead here, if you could early
> return for all things that are not display flex/grid that would reduce it.
> 

The problem with this approach is that RenderStyle::initialXXX are static
methods.
It could be implemented as an instance one, so we can figure out the used
display,
but that is hard to use from the StyleRareNonInheritedData class, which is used
to
setup the initial values of the align-{self, items} and justify-{self, items}
properties.

I don't think is possible to do this and detect later whether the "start" was
the
intial/default value or the one set by the user.

Also, the justfy-items property admit a special value "legacy {left, center,
right}
which makes the "auto" value (either the default/initial or the one set by the
user)
of the children to be the inherited (without the 'legacy' keyword).

 http://dev.w3.org/csswg/css-align-3/#valuedef-legacy

[Julien - ping for review, 2014-07-14 15:57:38]

> Why not make the initial value ItemPositionStart and then only change it to
Stretch for the other display types?

This proposal goes against what we are doing for other properties and how the
specification is written: the initial value is 'auto', not 'stretch'. It's the
used value that differs based on the renderer type. The upside of the proposal
seems pretty weak compared to the double inconsistency.

[jfernandez, 2014-07-14 16:42:56]

On 2014/07/14 15:57:38, Julien Chaffraix - PST wrote:
> > Why not make the initial value ItemPositionStart and then only change it to
> Stretch for the other display types?
> 
> This proposal goes against what we are doing for other properties and how the
> specification is written: the initial value is 'auto', not 'stretch'. It's the
> used value that differs based on the renderer type. The upside of the proposal
> seems pretty weak compared to the double inconsistency.

I totally agree. 

So, I think we have an agreement on the approach to follow ? I think the best
is to use the StyleAdjuster to resolve the "auto" value, since the StyleBuilder
does not apply when there is no style rules defined.

If that's the case, the only change required to the last patch would be to use 
references, instead of raw pointers, right ? Is that enough to get th lgtm ? 

Thanks for the review.

[Julien - ping for review, 2014-07-14 17:16:54]

Please use the UI to reply to comment, not just reply to the email as it breaks
conversation threads and force me to reply to each of them individually :(

> >
https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderGridTest.cpp (right):
> > 
> >
https://codereview.chromium.org/363133003/diff/20001/Source/core/rendering/Re...
> > Source/core/rendering/RenderGridTest.cpp:20: // Base class for RenderGrid
> > testing, it exposes some useful getter.
> > I don't think this is a RenderGrid testing, we only test RenderStyle, which
> > happens to be on a RenderGrid.
> 
> Well, I've just defined the test cases related to this issue, but I had in
mind to use this
> class for future RenderGrid related unit tests. Actually, I did it because you
had suggested
> so in a  previous review: 
> 
> https://codereview.chromium.org/333423005/#msg8

There is nothing specific to RenderGrid in your testing scaffolding, it's
actually what https://codereview.chromium.org/351973004 does. It would be better
to just put this code into a common file using RenderObject, with maybe some
specialization for grid if you want a RenderGrid. That would enable better code
reuse.

[Julien - ping for review, 2014-07-14 17:18:56]

> So, I think we have an agreement on the approach to follow ? I think the best
> is to use the StyleAdjuster to resolve the "auto" value, since the
StyleBuilder
> does not apply when there is no style rules defined.
> 
> If that's the case, the only change required to the last patch would be to use

> references, instead of raw pointers, right ? Is that enough to get th lgtm ? 

I think we are in agreement. lgtm with the previous code sharing potential
fixed.

[jfernandez, 2014-07-14 21:15:32]

On 2014/07/14 17:18:56, Julien Chaffraix - PST wrote:
> > So, I think we have an agreement on the approach to follow ? I think the
best
> > is to use the StyleAdjuster to resolve the "auto" value, since the
> StyleBuilder
> > does not apply when there is no style rules defined.
> > 
> > If that's the case, the only change required to the last patch would be to
use
> 
> > references, instead of raw pointers, right ? Is that enough to get th lgtm ?

> 
> I think we are in agreement. lgtm with the previous code sharing potential
> fixed.

I'll remove the RenderGirdTest code for this patch, so I can add my testing
functions to the RenderObjectTests defined in the patch provided in
https://codereview.chromium.org/351973004.

[Timothy Loh, 2014-07-16 01:10:48]

On 2014/07/14 16:42:56, jfernandez wrote:
> So, I think we have an agreement on the approach to follow ? I think the best
> is to use the StyleAdjuster to resolve the "auto" value, since the
StyleBuilder
> does not apply when there is no style rules defined.

Seems reasonable to me, lgtm.

[jfernandez, 2014-07-16 23:52:51]

When running the flexbox layout tests I've detected some failures due to the
lack of style recalculation whenever the alingItems or justifyItems is touched. 

This was not necessary before because current implementation computes all the
properties on each call to the getComputedStyle call, so it does again in the
RenderGird and RenderFlow codebase when using the properties. 

This patch was intended to avoid that code duplication while providing an
consistent computedStyle for the properties when using the 'auto' or the
initial/default values. 

However, I'm not totally sure this style recalculation could imply an impact on
performance worth studying. If that's the case, perhaps the previous approach is
still the best one, even assuming the code duplication and inconsistency. 

I still think the "legacy" keyword has to be managed in the StyleAdjuster
anyway, though.
 
Sorry for bothering again with this, even getting the lgtm, but this patch will
be the base of several more regarding alignment and grid, not to mention it
affects the flexbox implementation as well.

[jfernandez, 2014-07-21 22:58:48]

Could anybody provide additional feedback on this ? Even though it got lgtm
already, I did a change that i think is worth reviewing again.

[Julien - ping for review, 2014-07-22 01:57:00]

Still looks fine with these comments fixed.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/css-properties.html (right):

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/css-properties.html:81: flexbox.style.display =
'-webkit-flex';
Let's use the unprefixed version as we support it.

I would also argue to reset it above when change style.display.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/css-properties.html:86:
shouldBeEqualToString('window.getComputedStyle(document.documentElement,
null).webkitAlignSelf', 'auto');
While we touched these lines, I would unprefix them.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-self.html (right):

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-self.html:182: container =
document.createElement("div");
The need for |container| seems artificial, what's wrong with <body>?

[jfernandez, 2014-07-23 21:42:27]

Applied suggested changes and prepared the patch to land.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/css-properties.html (right):

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/css-properties.html:81: flexbox.style.display =
'-webkit-flex';
On 2014/07/22 01:56:59, Julien Chaffraix - PST wrote:
> Let's use the unprefixed version as we support it.
> 
> I would also argue to reset it above when change style.display.

Acknowledged.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/css-properties.html:86:
shouldBeEqualToString('window.getComputedStyle(document.documentElement,
null).webkitAlignSelf', 'auto');
On 2014/07/22 01:56:59, Julien Chaffraix - PST wrote:
> While we touched these lines, I would unprefix them.

Done.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-self.html (right):

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-self.html:182: container =
document.createElement("div");
On 2014/07/22 01:56:59, Julien Chaffraix - PST wrote:
> The need for |container| seems artificial, what's wrong with <body>?

Acknowledged.

[jfernandez, 2014-07-23 21:42:52]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-23 21:43:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/363133003/80001

[jfernandez, 2014-07-23 23:22:53]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-23 23:24:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/363133003/100001

[commit-bot: I haz the power, 2014-07-24 05:24:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16826)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18614)

[commit-bot: I haz the power, 2014-07-24 05:35:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 05:35:45]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[jfernandez, 2014-07-24 08:30:34]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-24 08:31:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/363133003/100001

[commit-bot: I haz the power, 2014-07-24 09:17:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16877)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18685)

[commit-bot: I haz the power, 2014-07-24 09:28:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 09:28:54]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[jfernandez, 2014-07-24 09:46:13]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-24 09:47:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/363133003/100001

[commit-bot: I haz the power, 2014-07-24 10:12:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16886)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18694)

[commit-bot: I haz the power, 2014-07-24 10:20:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 10:20:38]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[jfernandez, 2014-07-25 00:38:54]

It seems that the RenderFullScreen class derives from RenderFlexibleBox, not
sure the reason behind that, though, but it looks like an implementation detail.

The issue is that due to the change I'm proposing in this patch, the
default/initial value of alignSelf/justifySelf properties is either "start" or
"stretch" depending on the nature of the elements (flex/grid or regular
elements). 

The WebFrameTest.FullscreenLayerNonScrollable unit tests calls to the
RenderFullScreen::wrapRenderer method to insert a RenderFullScreen object in the
RenderTree. This should trigger some style recalculations, but due the lack of a
node in the DOM Tree associated to the inserted element, the
alignSefl/justifySelf of its children is not recalculated so it keeps the
"start" value associated to non-flex elements.

It seems also that the RenderflexibleBox does not handle yet the
ItemPositionStat, among other values, which is triggering the ASSERT in the
debug builds when running the mentioned unit tests.

The last patch introduces a change to make the RenderFullScreen class to derive
from RenderBlockFlow, which apparently fixes the unit tests crashes without
breaking anything else.

[esprehn, 2014-07-25 00:43:38]

On 2014/07/25 at 00:38:54, jfernandez wrote:
> ...
> 
> The last patch introduces a change to make the RenderFullScreen class to
derive from RenderBlockFlow, which apparently fixes the unit tests crashes
without breaking anything else.

That doesn't seem right, I though it was a flexbox so it would stretch the full
screen element properly?

[jfernandez, 2014-07-27 00:07:32]

A new approach implemented in the last patch; it's similar to the last one
approved already, but because of some issues  I've decided to adjust only the
align/justify-items properties. these properties are the ones used to resolve
"auto" values in align/justify-self, which is the one eventually used to
implement the alignment.

The issues I had to face are related to the fact that certain renders derive
from RenderFlexibleBox class while not using the "flex· display. Also because of
the anonymous flexboxes introduced in the render tree, which don't trigger a
style recalc. 

The new approach seems to work fine in all the corner cases found so far, but
I'd like to have some additional feedback.

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
File LayoutTests/fast/alignment/parse-justify-self.html (right):

https://codereview.chromium.org/363133003/diff/60001/LayoutTests/fast/alignme...
LayoutTests/fast/alignment/parse-justify-self.html:182: container =
document.createElement("div");
On 2014/07/22 01:56:59, Julien Chaffraix - PST wrote:
> The need for |container| seems artificial, what's wrong with <body>?

I thought body could be used, but I realized later that changing the body
display forces a relayout of the whole page and when setting it to flex, an
ASSERT is triggered because of some ItemPosition values not supported yet by the
RenderFlexibleBox class.

I think using container is simpler and clearer, since display property changes
only affects to such subtree.

The new patch provided follows this approach.

[svillar, 2014-07-29 10:40:06]

Not my field of expertise, but I'll informally review.

https://codereview.chromium.org/363133003/diff/180001/Source/core/css/CSSComp...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/363133003/diff/180001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1561: ItemPosition
resolvedPosition = itemPosition == ItemPositionAuto ? ItemPositionStart :
itemPosition;
Maybe we need some ASSERT here? I guess you're assuming that the display: is not
set so we're not talking about a flexbox or a grid.

https://codereview.chromium.org/363133003/diff/180001/Source/core/rendering/s...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/363133003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1708: //  - 'start' for everything
else.
I don't think you need these comments. Code is self-explanatory

https://codereview.chromium.org/363133003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1724: // any legacy keywords) on all
other boxes.
Ditto.

https://codereview.chromium.org/363133003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1740: // 'start' for everything
else.
Ditto.

https://codereview.chromium.org/363133003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1752: // any 'legacy' keywords) on
all other boxes.
Ditto.

[jfernandez, 2014-07-30 14:16:47]

I had to do several changes in my patch, manly because of how the
RenderFlexibleBox is being used to implement some html elements, like ranges,
buttons. There were also some unit tests failing because of the fullscreen
implementation.

Basically, the new approach is to resolve during the cascade the flex an grid
cases, those setting properly the display value. The absolute-positioned
elements are also resolved during the style cascade.

There are other cases that use the RenderFlexibleBox without the proper display,
or new RenderObjects are added to the tree. The "auto" resolution still have to
be done at the Render side for those cases.

[esprehn, 2014-07-31 00:50:28]

This lgtm, but the tests are way too big. Can you please break the tests into
lots of smaller tests instead? It's impossible to correctly review the tests
now. (You can do that in the next patch).

https://codereview.chromium.org/363133003/diff/200001/LayoutTests/fast/alignm...
File LayoutTests/fast/alignment/parse-align-items.html (right):

https://codereview.chromium.org/363133003/diff/200001/LayoutTests/fast/alignm...
LayoutTests/fast/alignment/parse-align-items.html:8: 
This test is way way too big, please break it into lots of smaller tests.

[jfernandez, 2014-07-31 10:29:53]

Trying CQ now, but ill provide better and smaller tests in the next patches.

https://codereview.chromium.org/363133003/diff/200001/LayoutTests/fast/alignm...
File LayoutTests/fast/alignment/parse-align-items.html (right):

https://codereview.chromium.org/363133003/diff/200001/LayoutTests/fast/alignm...
LayoutTests/fast/alignment/parse-align-items.html:8: 
On 2014/07/31 00:50:28, esprehn wrote:
> This test is way way too big, please break it into lots of smaller tests.

Acknowledged.

[jfernandez, 2014-07-31 10:30:01]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-31 10:30:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/363133003/200001

[commit-bot: I haz the power, 2014-07-31 11:38:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-31 12:41:22]

Change committed as 179307