Bug 222557 – Apply transferred min/max sizes for intrinsic sizing

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

[Rob Buis]

Apply transferred min/max sizes for intrinsic sizing.

[Rob Buis]

Created attachment 421833 [details]
Patch

[Rob Buis]

Created attachment 422573 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75172854>

[Rob Buis]

Created attachment 422601 [details]
Patch

[Rob Buis]

Comment on attachment 422601 [details]
Patch

Not sure about the percentage height code, let me know if there is a better way.

[Simon Fraser (smfr)]

Comment on attachment 422601 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:263
> +    bool m_hasAspectRatioPercentageHeightDescendant;

This probably introduced several bytes of padding. Is there a spare bit you can fit this into?

These "descendant has" bits are notoriously hard to keep up-to-date accurately. Is there a way to fix this without storing state like this?

> Source/WebCore/rendering/RenderBlock.cpp:2437
> +        const_cast<RenderBlock*>(this)->setHasAspectRatioPercentageHeightDescendant();

Yuck.

[Rob Buis]

Created attachment 422679 [details]
Patch

[Rob Buis]

Comment on attachment 422601 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:2437
>> +        const_cast<RenderBlock*>(this)->setHasAspectRatioPercentageHeightDescendant();
> 
> Yuck.

Agreed, but sometimes it can't be avoided :) Anyway I hope I have a better solution now.

[Simon Fraser (smfr)]

Comment on attachment 422679 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:842
> +            if (renderer->hasAspectRatio() || renderer->style().hasAspectRatio())

This is confusing; I feel like renderer->hasAspectRatio() could be renamed (maybe hasIntrinsicAspectRatio()).

We have other properties with the same name on both style and renderer; they generally mean the same thing, but in situations where a property does not apply to some kinds of renderers. This one is different though.

[Rob Buis]

Created attachment 422868 [details]
Patch

[Rob Buis]

Comment on attachment 422679 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:842
>> +            if (renderer->hasAspectRatio() || renderer->style().hasAspectRatio())
> 
> This is confusing; I feel like renderer->hasAspectRatio() could be renamed (maybe hasIntrinsicAspectRatio()).
> 
> We have other properties with the same name on both style and renderer; they generally mean the same thing, but in situations where a property does not apply to some kinds of renderers. This one is different though.

Agreed, I mixed up child.style().hasAspectRatio() and child.hasAspectRatio() quite some times in development (and vice versa). Fixed now.

[Rob Buis]

Created attachment 422906 [details]
Patch

[EWS]

Committed r274287: <https://commits.webkit.org/r274287>

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