Summary: | Replace Color::getHSL() with sRGBToHSL to ensure it at least gives somewhat sensible results for ExtendedColors and reduce code duplication | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, mifenton, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sam Weinig
2020-05-20 09:02:34 PDT
Created attachment 399845 [details]
Patch
Created attachment 399847 [details]
Patch
Comment on attachment 399847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399847&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:550 > + return lightness(color.value().toSRGBAComponentsLossy()); Ideally we'd be able to compute lightness for any colorspace without converting to sRGB. Which makes me think that lightness() should stay as a function on Color. This will be especially true when we have LAB color where lightness is just one of the components. > Source/WebCore/editing/cocoa/DataDetection.mm:628 > + auto underlineColor = Color(makeRGBAFromHSLA(hue, saturation, overrideLightness, overrideAlphaMultiplier * alpha)); Make sure the hue is correctly round-tripped here. > Source/WebCore/page/FrameView.cpp:368 > + if (lightness(backgroundColor.toSRGBAComponentsLossy()) <= .5f && backgroundColor.isVisible()) Yeah, this would be much better with backgroundColor.getHSL.lightness(). (In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 399847 [details] > Patch > > Source/WebCore/page/FrameView.cpp:368 > > + if (lightness(backgroundColor.toSRGBAComponentsLossy()) <= .5f && backgroundColor.isVisible()) > > Yeah, this would be much better with backgroundColor.getHSL.lightness(). Would you be ok with backgroundColor.lightness()? (In reply to Sam Weinig from comment #4) > (In reply to Simon Fraser (smfr) from comment #3) > > Comment on attachment 399847 [details] > > Patch > > > > Source/WebCore/page/FrameView.cpp:368 > > > + if (lightness(backgroundColor.toSRGBAComponentsLossy()) <= .5f && backgroundColor.isVisible()) > > > > Yeah, this would be much better with backgroundColor.getHSL.lightness(). > > Would you be ok with backgroundColor.lightness()? That's actually what I mean to write! Comment on attachment 399847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399847&action=review >> Source/WebCore/editing/cocoa/DataDetection.mm:628 >> + auto underlineColor = Color(makeRGBAFromHSLA(hue, saturation, overrideLightness, overrideAlphaMultiplier * alpha)); > > Make sure the hue is correctly round-tripped here. I think this is wrong now. Going to make an API test with round-tripping to see. Created attachment 399897 [details]
Patch
Committed r261967: <https://trac.webkit.org/changeset/261967> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399897 [details]. |