Summary: | Fix longstanding FIXME in parseNumericColor about not doubly clamping color components | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | 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
Sam Weinig
2021-01-31 18:14:03 PST
Created attachment 418838 [details]
Patch
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. (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. Created attachment 418922 [details]
Patch
Ok, this one should be good to review. (In reply to Sam Weinig from comment #5) > Ok, this one should be good to review. Apparently not. Hm. Created attachment 419010 [details]
Patch
Comment on attachment 419010 [details]
Patch
Ok, now!
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. Committed r272226: <https://trac.webkit.org/changeset/272226> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419010 [details]. |