Bug 222557 - Apply transferred min/max sizes for intrinsic sizing
Summary: Apply transferred min/max sizes for intrinsic sizing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2021-03-01 09:14 PST by Rob Buis
Modified: 2021-03-11 10:11 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2021-03-01 09:15 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2021-03-08 08:58 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.43 KB, patch)
2021-03-08 12:11 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.09 KB, patch)
2021-03-09 01:43 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.60 KB, patch)
2021-03-10 13:57 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2021-03-10 23:50 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-03-01 09:14:30 PST
Apply transferred min/max sizes for intrinsic sizing.
Comment 1 Rob Buis 2021-03-01 09:15:59 PST
Created attachment 421833 [details]
Patch
Comment 2 Rob Buis 2021-03-08 08:58:03 PST
Created attachment 422573 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-03-08 09:15:23 PST
<rdar://problem/75172854>
Comment 4 Rob Buis 2021-03-08 12:11:27 PST
Created attachment 422601 [details]
Patch
Comment 5 Rob Buis 2021-03-08 13:39:25 PST
Comment on attachment 422601 [details]
Patch

Not sure about the percentage height code, let me know if there is a better way.
Comment 6 Simon Fraser (smfr) 2021-03-08 14:26:39 PST
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.
Comment 7 Rob Buis 2021-03-09 01:43:18 PST
Created attachment 422679 [details]
Patch
Comment 8 Rob Buis 2021-03-09 14:30:57 PST
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.
Comment 9 Simon Fraser (smfr) 2021-03-10 12:19:23 PST
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.
Comment 10 Rob Buis 2021-03-10 13:57:29 PST
Created attachment 422868 [details]
Patch
Comment 11 Rob Buis 2021-03-10 14:21:58 PST
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.
Comment 12 Rob Buis 2021-03-10 23:50:53 PST
Created attachment 422906 [details]
Patch
Comment 13 EWS 2021-03-11 10:11:04 PST
Committed r274287: <https://commits.webkit.org/r274287>

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