Bug 212143 – Replace Color::getHSL() with sRGBToHSL to ensure it at least gives somewhat sensible results for ExtendedColors and reduce code duplication

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Replace Color::getHSL() with sRGBToHSL to ensure it at least gives somewhat sensible results for ExtendedColors and reduce code duplication

[Sam Weinig]

Created attachment 399845 [details]
Patch

[Sam Weinig]

Created attachment 399847 [details]
Patch

[Simon Fraser (smfr)]

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().

[Sam Weinig]

(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()?

[Simon Fraser (smfr)]

(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!

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 399897 [details]
Patch

[EWS]

Committed r261967: <https://trac.webkit.org/changeset/261967>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399897 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63467460>