Bug 251758 - Allow Length to be used with Markable<>
Summary: Allow Length to be used with Markable<>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ahmad Saleem
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-02-05 11:01 PST by Simon Fraser (smfr)
Modified: 2024-09-07 01:51 PDT (History)
11 users (show)

See Also:

Attachments
Patch (6.09 KB, patch)
2023-02-05 11:03 PST, Simon Fraser (smfr) 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2023-02-06 14:59 PST, Simon Fraser (smfr) 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2023-02-05 11:01:00 PST 
Allow Length to be used with Markable<> to optimize padding.
Comment 1 Radar WebKit Bug Importer 2023-02-05 11:01:28 PST 
<rdar://problem/105059616>
Comment 2 Simon Fraser (smfr) 2023-02-05 11:03:08 PST 
Created attachment 464852 [details]
Patch
Comment 3 Darin Adler 2023-02-05 22:19:50 PST 
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.
Comment 4 Simon Fraser (smfr) 2023-02-06 14:59:37 PST 
Created attachment 464877 [details]
Patch
Comment 5 Darin Adler 2023-02-06 16:04:31 PST 
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.
Comment 6 Simon Fraser (smfr) 2023-02-06 17:51:30 PST 
(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.
Comment 7 Ahmad Saleem 2024-09-06 14:28:45 PDT 
Pull request: https://github.com/WebKit/WebKit/pull/33261
Comment 8 EWS 2024-09-07 01:51:18 PDT 
Committed 283293@main (73d6aa835129): <https://commits.webkit.org/283293@main>

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