[Webkit-unassigned] [Bug 58106] [chromium] Implement image/png support in DataTransferItems

Fri Apr 8 13:59:27 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=58106

Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88867|review?                     |review-
               Flag|                            |

--- Comment #4 from Dmitry Titov <dimich at chromium.org>  2011-04-08 13:59:27 PST ---
(From update of attachment 88867)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.