Bug 238181 – Parsing of contain-intrinsic-size and adding a runtime flag for it

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

[cathiechen]

https://github.com/w3c/csswg-drafts/blob/main/css-sizing-4/Overview.bs

[cathiechen]

Created attachment 455318 [details]
Patch

[cathiechen]

Created attachment 455367 [details]
Patch

[Rob Buis]

Comment on attachment 455367 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        value for Length and AutoAndLenght type.

AutoAndLength.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4101
> +    list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueAuto));

Better here to use autoValue I think.

[cathiechen]

Created attachment 455758 [details]
Patch

[Rob Buis]

Patch LGTM to me :) I will leave it for an Apple person to do the final review though.

[Simon Fraser (smfr)]

Comment on attachment 455758 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserContext.cpp:219
> +    uint64_t bits2 = context.containIntrinsicSizeEnabled    << 0
> +        | (unsigned long long)context.mode                  << 1; // This is multiple bits, so keep it last.

Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63.

[cathiechen]

Comment on attachment 455758 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParserContext.cpp:219
>> +        | (unsigned long long)context.mode                  << 1; // This is multiple bits, so keep it last.
> 
> Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63.

Yes, sizeof(uint64_t) is 8. But I got this compiling error, not sure why...

```
./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        | context.containIntrinsicSizeEnabled               << 32
                                                            ^  ~~
1 error generated
```

[cathiechen]

Comment on attachment 455758 [details]
Patch

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

>>> Source/WebCore/css/parser/CSSParserContext.cpp:219
>>> +        | (unsigned long long)context.mode                  << 1; // This is multiple bits, so keep it last.
>> 
>> Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63.
> 
> Yes, sizeof(uint64_t) is 8. But I got this compiling error, not sure why...
> 
> ```
> ./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
>         | context.containIntrinsicSizeEnabled               << 32
>                                                             ^  ~~
> 1 error generated
> ```

Done! Thanks, Rob, for pointing out context.containIntrinsicSizeEnabled is only 32 bit, I just need to add a cast here.

[cathiechen]

Created attachment 455911 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90919423>

[cathiechen]

Comment on attachment 455911 [details]
Patch

Hi,
I think the patch is ready for review, please take a look. Thanks!

[Rob Buis]

Created attachment 456820 [details]
Patch

[Rob Buis]

Created attachment 456837 [details]
Patch

[Tim Nguyen (:ntim)]

Can you fix the assert:

https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/456837-28300-rerun/accessibility/mac/media-role-descriptions-stderr.txt

? Seems like this needs animation support.

[Rob Buis]

Created attachment 456899 [details]
Patch

[Rob Buis]

(In reply to Tim Nguyen (:ntim) from comment #14)
> Can you fix the assert:
> 
> https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-
> Debug-WK2-Tests-EWS/456837-28300-rerun/accessibility/mac/media-role-
> descriptions-stderr.txt
> 
> ? Seems like this needs animation support.

Thanks for the heads up, the latest patch should fix it.

[Rob Buis]

Created attachment 457027 [details]
Patch

[Rob Buis]

Created attachment 457541 [details]
Patch

[Rob Buis]

This is now ready for review :)

[Tim Nguyen (:ntim)]

Comment on attachment 457541 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserContext.cpp:217
> +        | (unsigned long long)context.containIntrinsicSizeEnabled << 32

Why does this need to be (unsigned long long)? I assume this is bad copy paste?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4097
> +            break;

Why not `return nullptr;`?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240
> +    unsigned containIntrinsicWidthType : 3;
> +    unsigned containIntrinsicHeightType : 3;

Does this not fit in 2? The enum only has 3 values and 2**2 == 4

[Rob Buis]

Created attachment 457708 [details]
Patch

[Rob Buis]

Comment on attachment 457541 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParserContext.cpp:217
>> +        | (unsigned long long)context.containIntrinsicSizeEnabled << 32
> 
> Why does this need to be (unsigned long long)? I assume this is bad copy paste?

Yes that was a bit lazy, fixed.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:4097
>> +            break;
> 
> Why not `return nullptr;`?

Why not indeed. Fixed.

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240
>> +    unsigned containIntrinsicHeightType : 3;
> 
> Does this not fit in 2? The enum only has 3 values and 2**2 == 4

It should fit indeed. Fixed.

[Simon Fraser (smfr)]

Comment on attachment 457708 [details]
Patch

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

Don't forget to fix RenderStyle::diff() in this or a future patch.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240
> +    unsigned containIntrinsicWidthType : 2;
> +    unsigned containIntrinsicHeightType : 2;

Add a `// ContainIntrinsicSizeType` comment on both lines

> Source/WebCore/style/StyleBuilderCustom.h:2167
> +    CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(CSSPropertyContainIntrinsicBlockSize, style.direction(), style.writingMode());

Maybe `auto resolvedID = `

[Tim Nguyen (:ntim)]

Comment on attachment 457708 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserContext.cpp:217
> +        | (uint64_t)context.containIntrinsicSizeEnabled     << 32

Does: 
| context.containIntrinsicSizeEnabled << 32

just work here? (just for consistency for the other *Enabled flags)

[cathiechen]

Comment on attachment 457708 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParserContext.cpp:217
>> +        | (uint64_t)context.containIntrinsicSizeEnabled     << 32
> 
> Does: 
> | context.containIntrinsicSizeEnabled << 32
> 
> just work here? (just for consistency for the other *Enabled flags)

Looks like not, we will get the following error:
```
./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        | context.containIntrinsicSizeEnabled               << 32
                                                            ^  ~~
1 error generated
```

For context.containIntrinsicSizeEnabled is 32 bit, we need to extend it to 64 bit, then it could be moved 32.

[cathiechen]

Comment on attachment 457708 [details]
Patch

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

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240
>> +    unsigned containIntrinsicHeightType : 2;
> 
> Add a `// ContainIntrinsicSizeType` comment on both lines

Done

>> Source/WebCore/style/StyleBuilderCustom.h:2167
>> +    CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(CSSPropertyContainIntrinsicBlockSize, style.direction(), style.writingMode());
> 
> Maybe `auto resolvedID = `

Done

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #23)
> Comment on attachment 457708 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457708&action=review
> 
> Don't forget to fix RenderStyle::diff() in this or a future patch.
> 

Thanks! We will handle this in the implementing patch:)

[cathiechen]

Created attachment 457970 [details]
Patch

[cathiechen]

Created attachment 457971 [details]
Patch

[EWS]

Committed r293090 (249801@main): <https://commits.webkit.org/249801@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457971 [details].

[Oriol Brufau]

Comment on attachment 457971 [details]
Patch

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

> Source/WebCore/css/CSSProperties.json:5327
> +                "logical-property-group": {

Logical properties are resolved into physical before being applied. So you can use skip-builder:true and then remove the logical BuilderCustom methods. That's what other logical properties do.

[Brent Fulgham]

*** Bug 204316 has been marked as a duplicate of this bug. ***