| Summary: | Allow Length to be used with Markable<> | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
| Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
| Status: | NEW --- | ||||||||
| Severity: | Normal | CC: | bfulgham, changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Simon Fraser (smfr)
2023-02-05 11:01:00 PST
Created attachment 464852 [details]
Patch
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. Created attachment 464877 [details]
Patch
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. (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. |