[webkit-reviews] review granted: [Bug 212945] Extended Color: Streamline SimpleColor premulitply/unpremultiply code : [Attachment 401461] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 212945: Extended Color: Streamline SimpleColor premulitply/unpremultiply
code
https://bugs.webkit.org/show_bug.cgi?id=212945

Attachment 401461: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 401461
  --> https://bugs.webkit.org/attachment.cgi?id=401461
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?