[webkit-reviews] review denied: [Bug 44127] [chromium] Thumbnails not generated for GPU Rendered Pages : [Attachment 64628] Patch to allow pixel readback from GPU for thumbnail generation.

[bugzilla-daemon at webkit.org]

David Levin <levin 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 64628:  Patch to allow pixel readback from GPU for thumbnail
generation. 
https://bugs.webkit.org/attachment.cgi?id=64628&action=review

------- Additional Comments from David Levin <levin at chromium.org>
r- see Vangelis' comment (and a minor style issue in the bug too) plus the
style issue below.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:509
 +  void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
Just canvas should suffice.

WebKit avoids abbreviates and in general adding type information to variable
names (so Ptr is out on both accounts).

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:526
 +	// Flip pixels vertically ...
"// Flip pixels vertically."
works too. 

This comes from a person who uses "..." to often in the past....


WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:529
 +	    unsigned char *ptr1 = static_cast<unsigned char *>(pixels) + row1 *
bitmap.rowBytes();
The * (when used as a pointer as opposed to multiplication) should be near the
type.

There are lots of instances of this problem in this patch.

WebKit/chromium/public/WebView.h:215
 +	virtual void readPixels(WebCanvas *canvas) {}
The parameter name |canvas| adds no information in the function definition, so
it should be omitted. (There are other instances of this.)

Also WebKit tends to put a space inside of braces like this, so s/{}/{ }/

(And the * is in the wrong place.)