[webkit-reviews] review granted: [Bug 225673] Factor copyImagePixel pixel conversion code into its own file : [Attachment 428315] 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 225673: Factor copyImagePixel pixel conversion code into its own file
https://bugs.webkit.org/show_bug.cgi?id=225673

Attachment 428315: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 428315
  --> https://bugs.webkit.org/attachment.cgi?id=428315
Patch

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

> Source/WebCore/platform/graphics/PixelBufferConversion.h:44
> +void convertImagePixels(const PixelBufferConversionView& source,
PixelBufferConversionView& destination, const IntSize&);

Not so clear that destination should be non-const here. I understand that the
pixels are overwritten, but is that really best expressed by passing a
non-const reference to the view? In every other way, the view is an in
parameter, not an out parameter. And seeing it passed as a reference doesn’t
really make that clear to me. It would be a huge programming error to expect
the function to set fields like destination.alphaFormat.

I’m also not entirely clear why IntSize is not part of the conversion view;
perhaps because it’s needed only for source and not destination? Or because it
must be the same for both?

Is there a better way to express this function? Any small adjustment that could
make semantics a little more obvious? Maybe it would be as simple as taking the
data pointer out of the view and perhaps it would have a different name then?
That certainly would make the source worse, but then you could see how the
source and destination const-ness differed.

Should be able to use const uint8_t* for the source pixels.