Bug 144442 – [CSS Box Alignment] Upgrade justify-content parsing to CSS3 Box Alignment spec.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Javier Fernandez]

Upgrade the justify-content property to the last CSS3 Box Alignment specification. It defines a different enumeration for Positional and Distribution alignment, which requires changes in the FlexibleBox implementation.

[Javier Fernandez]

Created attachment 252094 [details]
Patch

[Build Bot]

Comment on attachment 252094 [details]
Patch

Attachment 252094 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4876104951136256

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/getComputedStyle/computed-style.html
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 252103 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 252094 [details]
Patch

Attachment 252094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4783139310272512

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/getComputedStyle/computed-style.html
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 252106 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Javier Fernandez]

Created attachment 252111 [details]
Patch

[Daniel Bates]

Comment on attachment 252111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252111&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22343
> +				9DAC7C521AF2CAA100437C44 /* CSSContentDistributionValue.cpp */,
> +				9DAC7C531AF2CAA100437C44 /* CSSContentDistributionValue.h */,

Please put this file in the list of files in alphabetical order according to the UNIX sort command.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:23950
> +				9DAC7C571AF2CB6400437C44 /* StyleContentAlignmentData.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26102
> +				9DAC7C551AF2CAA200437C44 /* CSSContentDistributionValue.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28521
> +				9DAC7C541AF2CAA100437C44 /* CSSContentDistributionValue.cpp in Sources */,

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1739
> +    bool isFlex = element->style().isDisplayFlexibleBox();

Please inline the value of this variable into the line below.

This variable is referenced exactly once in this function and I do not feel that its name adds any additional insight that is not already conveyed by its value, especially given the presence of the function name isDisplayFlexibleBox in he right hand side of this expression.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1835
> +static PassRefPtr<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(ContentPosition position, OverflowAlignment overflowAlignment, ContentDistributionType distribution)

The order of the parameters to this function disagrees with the order of the same parameters as implied by the name of this function. Specifically, the name of this function implies that the parameter overflowAlignment will be last. Either we should move parameter overflowAlignment to the end of the list of parameter to match the order implied by the function name or we should update the name of this function to reflect the order of the parameter list.

> Source/WebCore/css/CSSContentDistributionValue.cpp:49
> +

Please remove this empty line as I don't feel that it improves the readability of this code.

> Source/WebCore/css/CSSContentDistributionValue.cpp:55
> +        list->append(Ref<CSSValue>(*distribution()));
> +    if (m_position != CSSValueInvalid)
> +        list->append(Ref<CSSValue>(*position()));
> +    if (m_overflow != CSSValueInvalid)
> +        list->append(Ref<CSSValue>(*overflow()));

See my remark in CSSContentDistributionValue.h with regards to making CSSContentDistributionValue::{distribution, position, overflow}() return a Ref instead of a PassRefPtr. Then we do not need to explicitly construct a Ref when calling CSSValueList::append().

> Source/WebCore/css/CSSContentDistributionValue.cpp:56
> +

Ditto.

> Source/WebCore/css/CSSContentDistributionValue.h:37
> +    static PassRefPtr<CSSContentDistributionValue> create(CSSValueID distribution, CSSValueID position, CSSValueID overflow)

Notice that PassRefPtr defines a move constructor that takes a Ref. We should have this return Ref<CSSContentDistributionValue> instead of PassRefPtr<CSSContentDistributionValue> since we want to remove PassRefPtr in the future per <https://lists.webkit.org/pipermail/webkit-dev/2014-December/027100.html>

> Source/WebCore/css/CSSContentDistributionValue.h:45
> +    PassRefPtr<CSSPrimitiveValue> distribution() const { return cssValuePool().createIdentifierValue(m_distribution); }
> +    PassRefPtr<CSSPrimitiveValue> position() const { return cssValuePool().createIdentifierValue(m_position); }
> +    PassRefPtr<CSSPrimitiveValue> overflow() const { return cssValuePool().createIdentifierValue(m_overflow); }

Similarly, these functions should return a Ref instead of a PassRefPtr.

> Source/WebCore/css/CSSParser.cpp:3235
> +    return id == CSSValueSpaceBetween || id == CSSValueSpaceAround
> +        || id == CSSValueSpaceEvenly || id == CSSValueStretch;

This is OK as-is. I would have written this return statement using one line instead of spanning it across two lines because there are not many disjuncts:

return id == CSSValueSpaceBetween || id == CSSValueSpaceAround || id == CSSValueSpaceEvenly || id == CSSValueStretch;

> Source/WebCore/css/CSSParser.cpp:3296
> +    if (value->id == CSSValueAuto || isBaselinePositionKeyword(value->id)) {

Can we assume value is always non-null? From briefly looking at other CSSParser::parse* functions it seems that m_valueList->current() can be null (e.g. CSSParser::parseLegacyPosition()).

> Source/WebCore/css/CSSParser.cpp:3372
> +    } else {
> +        return false;
>      }

I know that you are preserving the style of this code. We omit braces for else clauses that have a one-line body.

> Source/WebCore/rendering/style/RenderStyleConstants.h:255
> +enum ContentPosition {ContentPositionAuto, ContentPositionBaseline, ContentPositionLastBaseline, ContentPositionCenter, ContentPositionStart, ContentPositionEnd, ContentPositionFlexStart, ContentPositionFlexEnd, ContentPositionLeft, ContentPositionRight};
> +enum ContentDistributionType {ContentDistributionDefault, ContentDistributionSpaceBetween, ContentDistributionSpaceAround, ContentDistributionSpaceEvenly, ContentDistributionStretch};

Are we not able to make this enum classes?

> Source/WebCore/rendering/style/StyleContentAlignmentData.h:30
> +

Please remove this empty line. One empty line (above) is sufficient to demarcate the end of the header includes from the start of the C++ namespace.

[Javier Fernandez]

Comment on attachment 252111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252111&action=review

Thanks so much for the detailed review.

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22343
>> +				9DAC7C531AF2CAA100437C44 /* CSSContentDistributionValue.h */,
> 
> Please put this file in the list of files in alphabetical order according to the UNIX sort command.

Done.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1739
>> +    bool isFlex = element->style().isDisplayFlexibleBox();
> 
> Please inline the value of this variable into the line below.
> 
> This variable is referenced exactly once in this function and I do not feel that its name adds any additional insight that is not already conveyed by its value, especially given the presence of the function name isDisplayFlexibleBox in he right hand side of this expression.

Done.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1835
>> +static PassRefPtr<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(ContentPosition position, OverflowAlignment overflowAlignment, ContentDistributionType distribution)
> 
> The order of the parameters to this function disagrees with the order of the same parameters as implied by the name of this function. Specifically, the name of this function implies that the parameter overflowAlignment will be last. Either we should move parameter overflowAlignment to the end of the list of parameter to match the order implied by the function name or we should update the name of this function to reflect the order of the parameter list.

Done.

>> Source/WebCore/css/CSSContentDistributionValue.cpp:49
>> +
> 
> Please remove this empty line as I don't feel that it improves the readability of this code.

Done.

>> Source/WebCore/css/CSSContentDistributionValue.cpp:55
>> +        list->append(Ref<CSSValue>(*overflow()));
> 
> See my remark in CSSContentDistributionValue.h with regards to making CSSContentDistributionValue::{distribution, position, overflow}() return a Ref instead of a PassRefPtr. Then we do not need to explicitly construct a Ref when calling CSSValueList::append().

Done.

>> Source/WebCore/css/CSSContentDistributionValue.cpp:56
>> +
> 
> Ditto.

Done.

>> Source/WebCore/css/CSSContentDistributionValue.h:37
>> +    static PassRefPtr<CSSContentDistributionValue> create(CSSValueID distribution, CSSValueID position, CSSValueID overflow)
> 
> Notice that PassRefPtr defines a move constructor that takes a Ref. We should have this return Ref<CSSContentDistributionValue> instead of PassRefPtr<CSSContentDistributionValue> since we want to remove PassRefPtr in the future per <https://lists.webkit.org/pipermail/webkit-dev/2014-December/027100.html>

Done.

>> Source/WebCore/css/CSSContentDistributionValue.h:45
>> +    PassRefPtr<CSSPrimitiveValue> overflow() const { return cssValuePool().createIdentifierValue(m_overflow); }
> 
> Similarly, these functions should return a Ref instead of a PassRefPtr.

Done.

>> Source/WebCore/css/CSSParser.cpp:3235
>> +        || id == CSSValueSpaceEvenly || id == CSSValueStretch;
> 
> This is OK as-is. I would have written this return statement using one line instead of spanning it across two lines because there are not many disjuncts:
> 
> return id == CSSValueSpaceBetween || id == CSSValueSpaceAround || id == CSSValueSpaceEvenly || id == CSSValueStretch;

Well., I can agree with that, but next functions with a similar logic span between several lines and this way all of them result in a similar length.

>> Source/WebCore/css/CSSParser.cpp:3296
>> +    if (value->id == CSSValueAuto || isBaselinePositionKeyword(value->id)) {
> 
> Can we assume value is always non-null? From briefly looking at other CSSParser::parse* functions it seems that m_valueList->current() can be null (e.g. CSSParser::parseLegacyPosition()).

Well, I think at some point it could be null, but now there is a check early returning if it is at the beginning of the CSSParser::parseValue method. I guess some parsing function still overprotect against null pointer access. I might be a good idea to do the same in this case, but I remember some previous reviews claiming it was not necessary to double check it.

>> Source/WebCore/css/CSSParser.cpp:3372
>>      }
> 
> I know that you are preserving the style of this code. We omit braces for else clauses that have a one-line body.

Done.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:255
>> +enum ContentDistributionType {ContentDistributionDefault, ContentDistributionSpaceBetween, ContentDistributionSpaceAround, ContentDistributionSpaceEvenly, ContentDistributionStretch};
> 
> Are we not able to make this enum classes?

All of them are just enums. Besides, I'm not sure I totally understand what we could gain making them classes.

>> Source/WebCore/rendering/style/StyleContentAlignmentData.h:30
>> +
> 
> Please remove this empty line. One empty line (above) is sufficient to demarcate the end of the header includes from the start of the C++ namespace.

Done.

[Javier Fernandez]

Created attachment 252181 [details]
Patch

[Javier Fernandez]

Created attachment 252185 [details]
Patch

[Javier Fernandez]

Created attachment 252202 [details]
Patch

[Dave Hyatt]

Comment on attachment 252202 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 252202 [details]
Patch

Clearing flags on attachment: 252202

Committed r183748: <http://trac.webkit.org/changeset/183748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.