[webkit-reviews] review denied: [Bug 23147] Introduce Skia to WebKit : [Attachment 26494] Add platform/graphics/skia/Image* files

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 23147: Introduce Skia to WebKit
https://bugs.webkit.org/show_bug.cgi?id=23147

Attachment 26494: Add platform/graphics/skia/Image* files
https://bugs.webkit.org/attachment.cgi?id=26494&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
0 is the WebKit way:
+    , m_platformContext(NULL)	// Canvas is set in ImageBuffer constructor.

Not needed in Webkit Style:
+    : m_canvas()

Wow, strange to have a success parameter on a constructor.

Best line ever:
+    unsigned char* data = result->data()->data().data();

I think WebKit style would have these be true camelCase instead of lowercase:
+    int originx = rect.x();
+    int destx = 0;
originX, destX

Style:
+    if (srcWidth == destIWidth && srcHeight == destIHeight) {
+	 // We don't need to resample if the source and destination are the
same.
+	 return RESAMPLE_NONE;
+    }

Style:
+			  float src_width,
+			  float src_height,
+			  float* dest_width,
+			  float* dest_height) {

I thought gfx:: was off-limits from WebCore code?
+	 gfx::Rect destBitmapSubset(destBitmapSubsetSkI.fLeft,

Style:
+    else {
+      resampling = computeResamplingMode(*bitmap,
+					  srcRect.width(), srcRect.height(),
+					  dest_bitmap_width,
dest_bitmap_height);
+    }

We should probably write out "bitmap" or "skImage" here:
+    const NativeImageSkia* bm = nativeImageForCurrentFrame();
+    if (!bm)
+	 return;  // It's too early and we don't have an image yet.

Spacing:
+ImageSource::ImageSource()
+  : m_decoder(0)
+{}

Nope:
+#define private protected
+#include "ImageSource.h"
That needs to be fixed before landing.