[webkit-reviews] review denied: [Bug 35288] [Haiku] Implement ImageBuffer support : [Attachment 50678] Patch implementing all ImageBuffer methods.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Stephan Aßmus
<superstippi at gmx.de>'s request for review:
Bug 35288: [Haiku] Implement ImageBuffer support
https://bugs.webkit.org/show_bug.cgi?id=35288

Attachment 50678: Patch implementing all ImageBuffer methods.
https://bugs.webkit.org/attachment.cgi?id=50678&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> Index: WebCore/platform/graphics/haiku/ImageBufferData.h
> +#include "OwnPtr.h"

It doesn't look like OwnPtr.h is used. Also I believe it is typically included
as <wtf/OwnPtr.h>


> Index: WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp
> +static inline void convertFromInternalData(const uint8* sourceRows, unsigned
sourceBytesPerRow,
> +					      uint8* destRows, unsigned
destBytesPerRow,
> +					      unsigned rows, unsigned columns,
> +					      bool premultiplied)
> +{
> +    if (premultiplied) {
> +	   // Internal storage is not pre-multiplied, pre-multiply on the fly.
> +	   for (unsigned y = 0; y < rows; y++) {
> +	       const uint8* s = sourceRows;

Please avoid single letter variables (well "i" is ok as a loop vairable, etc.
and x, y are ok, but in this case "s" is an abbreviation of ?


> +    unsigned char* destRows = data;
> +    // Offset the destination pointer to point at the first pixel of the
> +    // intersection rect.
> +    destRows += (rect.x() - (int)sourceRect.left) * 4 + (rect.y() -
(int)sourceRect.top) * destBytesPerRow;

Use c++ style casting instead of c style casting.


> +static void putImageData(ImageData* source, const IntRect& sourceRect, const
IntPoint& destPoint, ImageBufferData& imageData, const IntSize& size, bool
premultiplied)
> +{
> +    // If the source image is outside the destination image, we can return
at
> +    // this point.
> +    // TODO: Check if this isn't already done in WebCore.

Use FIXME instead of TODO in WebKit code.



> +	   roster->GetOutputFormats(translators[i], &outputFormats,
&formatCount);
> +	   for (int32 j = 0; j < formatCount; j++) {
> +	       if (outputFormats[j].group == B_TRANSLATOR_BITMAP
> +		   && mimeTypeString == outputFormats[j].MIME) {

No need to wrap this line (unless you really want to).