Bug 183484 – Remove GridLayout runtime flag

• Component: New Bugs
• Status: RESOLVED FIXED

[Javier Fernandez]

Remove GridLayout runtime flag

[Javier Fernandez]

Created attachment 335351 [details]
Patch

[Javier Fernandez]

Created attachment 335352 [details]
Patch

[Javier Fernandez]

Created attachment 335364 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 335364 [details]
Patch

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

Thanks for doing this, I'd send a mail to the thread in webkit-dev pointing to this issue and wait a few days before landing it.
Anyway I agree we should do it, added some comments inline.

> Source/WebKit/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

Nit: I think the "Reviewed by NOBODY (OOPS!)." is misplaced.

> Source/WebKitLegacy/mac/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

Nit: Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2536
> +static Ref<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(const StyleContentAlignmentData& data)

Why we need this change?
We were calling this with CSSValueStretch for align-content and CSSValueFlexStart for justify-content,
why we don't need to do this anymore?

Probably it'd be good to document this change in the ChangeLog as it's not direct to get the relationship
with the flag removal.

> Source/WebCore/css/parser/CSSParserMode.h:138
>              & key.textAutosizingEnabled                     << 3

Shouldn't we update the number here to "2"?

> Tools/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html:26
>  function runTest(gridEnabled)

Mmmm, should we keep this test?

This test calls twice runTest() with false and true, grid is always enabled now so...

> LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html:-84
> -test(function() {
> -    checkSelfAlignmentValues(true);
> -}, "Even when grid layout is ENABLED, new Self-Alignment values should not violate assertions in FlexibleBox layout logic..");
> -
> -test(function() {
> -    checkDefaultAlignmentValues(true);
> -}, "Even when grid layout is ENABLED, new Default-Alignment values should not violate assertions in FlexibleBox layout logic..");

Shouldn't we keep these tests yet?

> LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html:-61
> -debug('<br>Verifying initial values are supported when grid is ENABLED.');
> -checkInitialValues(true);

Again I'm wondering if we should keep this part of the test.

[Javier Fernandez]

Comment on attachment 335364 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2536
>> +static Ref<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(const StyleContentAlignmentData& data)
> 
> Why we need this change?
> We were calling this with CSSValueStretch for align-content and CSSValueFlexStart for justify-content,
> why we don't need to do this anymore?
> 
> Probably it'd be good to document this change in the ChangeLog as it's not direct to get the relationship
> with the flag removal.

Well, since grid is enabled by default, I don't expect the normalBrehaviorValueID to be used anymore. So removing would be a logical step and simplifies the code.

We needed to keep backward compatibility with the alignment properties defined in the Flexbox spec. The new spec states that both, align-content and justify-content have 'normal' as initial/default value. Then, the new spec resolves the differences between those properties adding the term "behavior". For both properties, "normal" behaves as "stretch". Then, for justify-content, "normal" behaves as "flex-start".

This behavior logic is implemented at rendering/layout side, so it has nothing to do with CSS parsing and computed style. The new spec states on this regard that both properties have a computed style as specified, so it'll be "normal" in both cases. This could be a source of problems for backward compatibility, indeed, but only in terms of computed style. 

https://drafts.csswg.org/css-align/#align-justify-content
https://drafts.csswg.org/css-flexbox/#align-content-property
https://drafts.csswg.org/css-flexbox/#propdef-justify-content

I have to say that there is not an easy way to solve/avoid this backward compatibility issues, and if anybody consider them a serious problem I'll try to discuss it with the CSSWG. 

I agree that this change should be documented in the Changelog.

>> Source/WebCore/css/parser/CSSParserMode.h:138
>>              & key.textAutosizingEnabled                     << 3
> 
> Shouldn't we update the number here to "2"?

Sure.

>> LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html:26
>>  function runTest(gridEnabled)
> 
> Mmmm, should we keep this test?
> 
> This test calls twice runTest() with false and true, grid is always enabled now so...

I think the test is still valid, but you're right that we don't need to evaluate the case of grid being disabled.

>> LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html:-84
>> -}, "Even when grid layout is ENABLED, new Default-Alignment values should not violate assertions in FlexibleBox layout logic..");
> 
> Shouldn't we keep these tests yet?

The purpose of this test is to ensure we run the CSS parsing fastpath code when grid is disabled (which follows the Flexbox spec). Since this patch removes such codepath, I don't think this test makes much sense.

>> LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html:-61
>> -checkInitialValues(true);
> 
> Again I'm wondering if we should keep this part of the test.

We already have tests to ensure the initial values are valid. The only purpose of this test is to ensure that the initial/default values set when grid is disabled as also valid. Since grid is enabled by default and we have removed the parsing codepath run when it's disabled, I don't see what this test would verify.

[Javier Fernandez]

Created attachment 335420 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 335420 [details]
Patch

r=me too, thanks for doing this!

[Manuel Rego Casasnovas]

Comment on attachment 335364 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2536
>>> +static Ref<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(const StyleContentAlignmentData& data)
>> 
>> Why we need this change?
>> We were calling this with CSSValueStretch for align-content and CSSValueFlexStart for justify-content,
>> why we don't need to do this anymore?
>> 
>> Probably it'd be good to document this change in the ChangeLog as it's not direct to get the relationship
>> with the flag removal.
> 
> Well, since grid is enabled by default, I don't expect the normalBrehaviorValueID to be used anymore. So removing would be a logical step and simplifies the code.
> 
> We needed to keep backward compatibility with the alignment properties defined in the Flexbox spec. The new spec states that both, align-content and justify-content have 'normal' as initial/default value. Then, the new spec resolves the differences between those properties adding the term "behavior". For both properties, "normal" behaves as "stretch". Then, for justify-content, "normal" behaves as "flex-start".
> 
> This behavior logic is implemented at rendering/layout side, so it has nothing to do with CSS parsing and computed style. The new spec states on this regard that both properties have a computed style as specified, so it'll be "normal" in both cases. This could be a source of problems for backward compatibility, indeed, but only in terms of computed style. 
> 
> https://drafts.csswg.org/css-align/#align-justify-content
> https://drafts.csswg.org/css-flexbox/#align-content-property
> https://drafts.csswg.org/css-flexbox/#propdef-justify-content
> 
> I have to say that there is not an easy way to solve/avoid this backward compatibility issues, and if anybody consider them a serious problem I'll try to discuss it with the CSSWG. 
> 
> I agree that this change should be documented in the Changelog.

Ah yeah sorry, I didn't realize about the gridEnabled used inside the method.
I wasn't seen the relation of this change with the flag.

>>> LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html:-84
>>> -}, "Even when grid layout is ENABLED, new Default-Alignment values should not violate assertions in FlexibleBox layout logic..");
>> 
>> Shouldn't we keep these tests yet?
> 
> The purpose of this test is to ensure we run the CSS parsing fastpath code when grid is disabled (which follows the Flexbox spec). Since this patch removes such codepath, I don't think this test makes much sense.

Ok.

>>> LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html:-61
>>> -checkInitialValues(true);
>> 
>> Again I'm wondering if we should keep this part of the test.
> 
> We already have tests to ensure the initial values are valid. The only purpose of this test is to ensure that the initial/default values set when grid is disabled as also valid. Since grid is enabled by default and we have removed the parsing codepath run when it's disabled, I don't see what this test would verify.

Ok, thanks for the explanation.

[WebKit Commit Bot]

Comment on attachment 335420 [details]
Patch

Clearing flags on attachment: 335420

Committed r229531: <https://trac.webkit.org/changeset/229531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38397876>