[webkit-reviews] review denied: [Bug 44127] [chromium] Thumbnails not generated for GPU Rendered Pages : [Attachment 66041] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 44127: [chromium] Thumbnails not generated for GPU Rendered Pages
https://bugs.webkit.org/show_bug.cgi?id=44127

Attachment 66041: Patch
https://bugs.webkit.org/attachment.cgi?id=66041&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:337
> +    OwnPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
nit: use OwnArrayPtr here

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
> +
nit: remove this extra blank line

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:94
> +    int rootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
nit: this should probably return IntSize:

  IntSize rootLayerTextureSize() const { ... }

There was a thread recently on webkit-dev about how we should be using IntSize,

IntPoint, etc. more to help clean up the codebase.

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:96
> +    void getFramebufferPixels(void *pixels, const int width, const int
height);
nit: can this function take an IntSize parameter?

  void getFramebufferPixels(void* pixels, const IntSize&);

> WebKit/chromium/src/WebViewImpl.cpp:982
> +	   if (canvas) {
nit: this function is now clearly way too long.  please break it up into
helper functions: one for SKIA and one for CG.

> WebKit/chromium/src/WebViewImpl.cpp:987
> +
nit: remove this blank line

> WebKit/chromium/src/WebViewImpl.cpp:1002
> +		       OwnPtr<skia::PlatformCanvas> canvasResize =
OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
why does this PlatformCanvas need to be heap allocated?

> WebKit/chromium/src/WebViewImpl.cpp:1010
> +			   canvas->drawBitmapRect(bitmapResize, &srcRect,
dstRect, 0);
in ImageSkia.cpp, i see similar code, but it tends to enable filtering
when doing linear re-sampling.

have you considered replacing all of this code with some code that
uses GraphicsContext and BitmapImage?  save for the GraphicsContext
construction, i bet you could write some portable code for this if
you used those classes.  WebFrameImpl::paint shows how to setup the
GraphicsContext.

One more comment:  is it really best to implement this using HW readback
instead of just engaging the software painting path?