Bug 221194

Summary: Fix longstanding FIXME in parseNumericColor about not doubly clamping color components
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Sam Weinig 2021-01-31 18:14:03 PST
Fix long standing FIXME in parseNumericColor about not double clamping color components
Comment 1 Sam Weinig 2021-01-31 18:17:39 PST
Created attachment 418838 [details]
Patch
Comment 2 Darin Adler 2021-02-01 12:16:53 PST
Comment on attachment 418838 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Fix long standing FIXME in parseNumericColor about not double clamping color components

I realize now that "doubly clamping" is what you mean by "double clamping"; at first I thought it was referring to "double", the C++ type.
Comment 3 Sam Weinig 2021-02-01 13:01:35 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 418838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418838&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Fix long standing FIXME in parseNumericColor about not double clamping color components
> 
> I realize now that "doubly clamping" is what you mean by "double clamping";
> at first I thought it was referring to "double", the C++ type.

Heh, yeah, doubly is definitely more clear.
Comment 4 Sam Weinig 2021-02-01 14:15:27 PST Comment hidden (obsolete)
Comment 5 Sam Weinig 2021-02-01 14:18:33 PST Comment hidden (obsolete)
Comment 6 Sam Weinig 2021-02-01 15:40:21 PST Comment hidden (obsolete)
Comment 7 Sam Weinig 2021-02-02 09:31:30 PST Comment hidden (obsolete)
Comment 8 Sam Weinig 2021-02-02 11:09:45 PST
Comment on attachment 419010 [details]
Patch

Ok, now!
Comment 9 Darin Adler 2021-02-02 11:36:34 PST
Comment on attachment 419010 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:317
> +    ASSERT(localValue <= 255);

I see that these functions mostly use 255, not 0xFF, nor some symbolic constant or long horrible thing like static_cast<int>(std::numeric_limits<uint8_t>::max()).

For the record, I slightly prefer 0xFF. But there’s no 0xFF.0 for floating point, so maybe that’s a non-starter.

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:371
> +        uint8_t result = negative ? 0 : tenthAlphaValues[string[length - 2] - '0'];

Could stick the character into a local variable instead of the result, if we want to avoid having to state the type of the result.
Comment 10 EWS 2021-02-02 12:14:00 PST
Committed r272226: <https://trac.webkit.org/changeset/272226>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419010 [details].
Comment 11 Radar WebKit Bug Importer 2021-02-02 12:15:22 PST
<rdar://problem/73892310>