[webkit-reviews] review denied: [Bug 58106] [chromium] Implement image/png support in DataTransferItems : [Attachment 88867] Patch

Fri Apr 8 13:59:26 PDT 2011

Dmitry Titov <dimich at chromium.org> has denied Daniel Cheng
<dcheng at chromium.org>'s request for review:
Bug 58106: [chromium] Implement image/png support in DataTransferItems
https://bugs.webkit.org/show_bug.cgi?id=58106

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88867&action=review

Looks good, some comments below. r- because of 1 second delay in the test - if
it is actually needed for the test to pass, it looks suspicious.

> LayoutTests/editing/pasteboard/data-transfer-items-image-png-expected.html:3
> +<body onpaste="paste(event)">

probably does not need onpaste.

> LayoutTests/editing/pasteboard/data-transfer-items-image-png.html:3
> +<body onpaste="paste(event)">

To make it more explicit, I'd eiher add an explicit padding to body (so the
mouseMove(1,1) lands explicitly on body) or swapped iframe with the explanation
text.

> LayoutTests/editing/pasteboard/data-transfer-items-image-png.html:14
> +    console.log(items.length);

Either make it reflected in the result of the test (by assigning into some
element.innerText?) or lets remove it.

> LayoutTests/editing/pasteboard/data-transfer-items-image-png.html:25
> +    }, 1000);

Do you really need a full second here? It seems 0 would be enough.

> Source/WebCore/ChangeLog:11
> +	   The initial implementation is somewhat inefficient in terms of
copies,

This statement should either be removed or fleshed out with explicit data. What
is exactly the inefficiency, when and how will it be fixed.
Also, this patch seems to not be a proof-of-concept but rather a working code
with a test :-)

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:121
> +	   // Yikes.

You can remove "Yikes." It doesn't add more info here.
Please make sure there is a bug on improving this and reference it in this
bug's comments.