Bug 212945

Created attachment 401404 [details]
Patch

Now that the signatures only take SimpleColors, I am not sure if I should just make these members of SimpleColor. For now, this feels a lot more clear.

For simple value types with no hidden state like SimpleColor, I personally want member functions kept to the absolute minimum. Free functions are far more flexible.

But also, I wish this was a different programming language, where syntax and encapsulation weren’t so closely tied together.

Created attachment 401461 [details]
Patch

Comment on attachment 401461 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        - Removing overloads that didn't took the seperated components, keeping 

"didn't took the separated" is not good grammar, and spells "separated" wrong

> Source/WebCore/ChangeLog:14
> +        - Simplifying the names from makePremultipliedSimpleColor/makeUnpremultipliedSimpleColor
> +          to premultiplyFlooring/premultiplyCeiling/unpremultiply.

The use of "ceil" here as a verb with "ceiling" as its gerund is not awesome.

No call for premultiplying with rounding?

I’m not sure how I feel about these functions with verb names. I think we would call these "premultiplied" rather than "premultiply", but not sure that fully works with the rest of the naming.

> Source/WebCore/ChangeLog:15
> +        - Where component order is impotant, use valueAsARGB() explicitly to

"impotant"

> Source/WebCore/platform/graphics/Color.cpp:269
> +    // Since premultiplyCeiling() bails on zero alpha, special-case that.

Seems like something we might eventually want to address.

> Source/WebCore/platform/graphics/Color.cpp:270
> +    auto premultFrom = from.alpha() ? premultiplyCeiling(from.toSRGBASimpleColorLossy()) : Color::transparent;

I would have renamed all these "premult" to "premultiplied".

> Source/WebCore/platform/graphics/cairo/ImageBufferCairoImageSurfaceBackend.cpp:83
> +            auto pixelColor = unpremultiply(SimpleColor { *pixel });

I’m not sure I am on board with the strategy where we use valueAsARGB() to emphasize the meaning of the returned integer, but construct SimpleColor from a 32-bit integer with no disambiguation for the meaning there. Maybe there’s a future step coming to solve that?

(In reply to Darin Adler from comment #5)
> Comment on attachment 401461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401461&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        - Removing overloads that didn't took the seperated components, keeping 
> 
> "didn't took the separated" is not good grammar, and spells "separated" wrong
> 
> > Source/WebCore/ChangeLog:14
> > +        - Simplifying the names from makePremultipliedSimpleColor/makeUnpremultipliedSimpleColor
> > +          to premultiplyFlooring/premultiplyCeiling/unpremultiply.
> 
> The use of "ceil" here as a verb with "ceiling" as its gerund is not awesome.
> 
> No call for premultiplying with rounding?

Agreed, though I stole the name from your patch on 180620 :)

> 
> I’m not sure how I feel about these functions with verb names. I think we
> would call these "premultiplied" rather than "premultiply", but not sure
> that fully works with the rest of the naming.
> 
> > Source/WebCore/ChangeLog:15
> > +        - Where component order is impotant, use valueAsARGB() explicitly to
> 
> "impotant"

Will fix.

> 
> > Source/WebCore/platform/graphics/Color.cpp:269
> > +    // Since premultiplyCeiling() bails on zero alpha, special-case that.
> 
> Seems like something we might eventually want to address.

Yeah, since we check for 0 already there, we could totally just return transparent there. Will look into that in a follow up.

> 
> > Source/WebCore/platform/graphics/Color.cpp:270
> > +    auto premultFrom = from.alpha() ? premultiplyCeiling(from.toSRGBASimpleColorLossy()) : Color::transparent;
> 
> I would have renamed all these "premult" to "premultiplied".

Will fix.

> 
> > Source/WebCore/platform/graphics/cairo/ImageBufferCairoImageSurfaceBackend.cpp:83
> > +            auto pixelColor = unpremultiply(SimpleColor { *pixel });
> 
> I’m not sure I am on board with the strategy where we use valueAsARGB() to
> emphasize the meaning of the returned integer, but construct SimpleColor
> from a 32-bit integer with no disambiguation for the meaning there. Maybe
> there’s a future step coming to solve that?

Yeah, my thought was to introduce a makeSimpleColorFromARGB (or some name like that) at some point and remove the constructor. From Simon's comments in other bugs, the fact that we use ARGB as the format is a bit surprising to some, so might as well make it explicit.

Thanks for the review.

Committed r262810: <https://trac.webkit.org/changeset/262810>

<rdar://problem/64179100>

Maybe at some point we’ll change our minds and use a different class for the premultiplied data. Presumably you can’t use it in a Color and have it work correctly. It’s another aspect of how to interpret the channel values, analogous to color space. And SimpleColor is 4 bytes that are implicitly in the sRGB space and should probably also be implicitly not premultiplied for the same reason. As opposed to a more general "4 channels with no judgment about how they are used".