[webkit-reviews] review granted: [Bug 227539] PixelBufferConversion.h functions should be tested so they can be modified : [Attachment 432611] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted  review:
Bug 227539: PixelBufferConversion.h functions should be tested so they can be
modified
https://bugs.webkit.org/show_bug.cgi?id=227539

Attachment 432611: Patch

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




--- Comment #2 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 432611
  --> https://bugs.webkit.org/attachment.cgi?id=432611
Patch

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

Looks good overall; couple of tiny questions. Setting r+ to allow this to be
landed asynchronously.

One question: on which EWS bot did the new test run? Looked through a few, but
didn't find it.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5526
> +		FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa =
PBXBuildFile; };

This change looks wrong.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31778
> +				FD3160BF12B0272A00C1A359 /* (null) in Headers
*/,

Similarly here.

> Tools/TestWebKitAPI/Tests/WebCore/PixelBufferConversionTest.cpp:111
> +    v = v * 255 / alpha;

Here and in the premultiply routine: I never remember what intermediate types
C++ promotes these mathematical expressions to. Is there a need to upcast to
floating-point before dividing by alpha or 255, and then downcast back to
uint8_t?

> Tools/TestWebKitAPI/Tests/WebCore/PixelBufferConversionTest.cpp:228
> +    EXPECT_EQ(expectedPixels, resultPixels);

No tolerance necessary?