Bug 251758 – Allow Length to be used with Markable<>

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Allow Length to be used with Markable<> to optimize padding.

[Radar WebKit Bug Importer]

<rdar://problem/105059616>

[Simon Fraser (smfr)]

Created attachment 464852 [details]
Patch

[Darin Adler]

Comment on attachment 464852 [details]
Patch

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

I’ll review again (or someone else can after tests are passing.

> Source/WebCore/platform/Length.h:329
> +    if (isEmptyValue() && other.isEmptyValue())
> +        return true;

I think you’d want to go further and make sure that if either is an empty value you don’t compare with the other. Like:

    if (isEmptyValue() || other.isEmptyValue())
        return isEmptyValue() && other.isEmptyValue();

And maybe do this check early before any other checks.

> Source/WebCore/platform/Length.h:330
> +

I don’t think adding the blank lines makes this easier to read.

[Simon Fraser (smfr)]

Created attachment 464877 [details]
Patch

[Darin Adler]

Did you see my comment about empty value comparison? I noticed the new patch doesn’t yet take my advice, but I also don’t see a comment from you saying why not.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #5)
> Did you see my comment about empty value comparison? I noticed the new patch
> doesn’t yet take my advice, but I also don’t see a comment from you saying
> why not.

I did not! I will revise the patch.

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/33261

[EWS]

Committed 283293@main (73d6aa835129): <https://commits.webkit.org/283293@main>

Reviewed commits have been landed. Closing PR #33261 and removing active labels.