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

Fri Apr 8 14:26:46 PDT 2011

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

--- Comment #6 from Daniel Cheng <dcheng at chromium.org>  2011-04-08 14:26:46 PST ---
(In reply to comment #4)
> (From update of attachment 88867 [details])
> 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.
> 

Done.

> > 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.
> 

Done.

> > 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.
> 

Done. This was an artifact of some test debugging that got left in.

> > 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.
> 

Done.

> > 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.

I've removed the comment in the ChangeLog and updated the implementation with more info.

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