Bug 58106

Summary:

[chromium] Implement image/png support in DataTransferItems

Product:

WebKit

Reporter:

Daniel Cheng <dcheng>

Component:

Platform

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dimich, hayato, jianli, levin, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	levin: review+

Daniel Cheng

Reported 2011-04-07 18:03:27 PDT

Pretty much what it says. For now, we use an implementation that performs more copies than strictly necessary. This is partially to provide proof-of-concept that a similar will implementation for drag-and-drop will work (which will suffer these inefficiencies no matter what). For Chrome 13, it would make sense to create a new registerClipboardBlobData() IPC to reduce the number of copies needed.

Attachments
Patch (11.94 KB, patch) 2011-04-07 18:16 PDT, Daniel Cheng	no flags	Details Formatted Diff Diff
Patch (12.36 KB, patch) 2011-04-08 13:32 PDT, Daniel Cheng	no flags	Details Formatted Diff Diff
Patch (12.46 KB, patch) 2011-04-08 14:26 PDT, Daniel Cheng	no flags	Details Formatted Diff Diff
Patch (12.40 KB, patch) 2011-04-11 13:56 PDT, Daniel Cheng	levin: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Daniel Cheng

Comment 1 2011-04-07 18:16:42 PDT

Created attachment 88748 [details] Patch

Daniel Cheng

Comment 2 2011-04-07 18:40:54 PDT

Comment on attachment 88748 [details] Patch I forgot to update the deps for the chromium port.

Daniel Cheng

Comment 3 2011-04-08 13:32:28 PDT

Created attachment 88867 [details] Patch

Dmitry Titov

Comment 4 2011-04-08 13:59:27 PDT

Comment on attachment 88867 [details] Patch 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.

Daniel Cheng

Comment 5 2011-04-08 14:26:27 PDT

Created attachment 88875 [details] Patch

Daniel Cheng

Comment 6 2011-04-08 14:26:46 PDT

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

Dmitry Titov

Comment 7 2011-04-08 15:00:59 PDT

Comment on attachment 88875 [details] Patch r=me

Daniel Cheng

Comment 8 2011-04-08 15:59:44 PDT

Committed r83353: <http://trac.webkit.org/changeset/83353>

Kent Tamura

Comment 9 2011-04-10 18:44:38 PDT

The test has never been passed on all of Chromium ports. I'll roll it out. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fpasteboard%2Fdata-transfer-items-image-png.html&showExpectations=true&showLargeExpectations=true&group=%40ToT%20-%20chromium.org

Kent Tamura

Comment 10 2011-04-10 18:50:11 PDT

Reverted r83353 for reason: The new test doesn't pass on all Chromium platforms. Committed r83412: <http://trac.webkit.org/changeset/83412>

Daniel Cheng

Comment 11 2011-04-10 21:18:17 PDT

Hm. The test passed when I built it in in a Chromium checkout, but it fails if I only check out WebKit and build using build-webkit --chromium. Is there something obvious I'm missing? The DEPS looks correct to me, and I'm not sure what else is causing this difference in behavior.

Daniel Cheng

Comment 12 2011-04-11 01:28:26 PDT

I've done more investigation, and I don't understand why the layout test only works sometimes: Works: run-webkit-tests.sh editing/pasteboard/data-transfer-items-image-png.html Doesn't work: run-webkit-tests.sh editing/pasteboard Works: Add a <div id="console"></div> element, and at the end of the paste event handler, add the following snippet: document.getElementById('console').appendChild(document.createTextNode('a')); Doesn't work: Adding the console div element, but doing this instead at the end of the paste event handler: document.getElementById('console').appendChild(document.createElement('br')); In all cases, when the layout test fails, it behaves as if no changes to the DOM had occurred. The pasted image does not appear at all.

Hayato Ito

Comment 13 2011-04-11 03:59:05 PDT

I can reproduce the issue locally: Works: - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/data-transfer-items-image-png.html - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/copy-display-none.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html Doesn't work (data-transfer-items-image-png.html failed): - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/crash-copy.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html So I am wondering that something is wrong with the combination of (crash-copy.html and data-transfer-items-image-png.html). I guess the order matters. (In reply to comment #12) > I've done more investigation, and I don't understand why the layout test only works sometimes: > > Works: > run-webkit-tests.sh editing/pasteboard/data-transfer-items-image-png.html > > Doesn't work: > run-webkit-tests.sh editing/pasteboard > > Works: > Add a <div id="console"></div> element, and at the end of the paste event handler, add the following snippet: > document.getElementById('console').appendChild(document.createTextNode('a')); > > Doesn't work: > Adding the console div element, but doing this instead at the end of the paste event handler: > document.getElementById('console').appendChild(document.createElement('br')); > > In all cases, when the layout test fails, it behaves as if no changes to the DOM had occurred. The pasted image does not appear at all.

Hayato Ito

Comment 14 2011-04-11 04:16:32 PDT

I've found that it works if we add '--run-singly' option: Works: > new-run-webkit-tests --run-singly --debug --verbose LayoutTests/editing/pasteboard/copy-crash.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html Doesn't work: > new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/copy-crash.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html (In reply to comment #13) > I can reproduce the issue locally: > > Works: > - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/data-transfer-items-image-png.html > - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/copy-display-none.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html > > Doesn't work (data-transfer-items-image-png.html failed): > - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard > - new-run-webkit-tests --debug --verbose LayoutTests/editing/pasteboard/crash-copy.html LayoutTests/editing/pasteboard/data-transfer-items-image-png.html > > So I am wondering that something is wrong with the combination of (crash-copy.html and data-transfer-items-image-png.html). I guess the order matters. > > > (In reply to comment #12) > > I've done more investigation, and I don't understand why the layout test only works sometimes: > > > > Works: > > run-webkit-tests.sh editing/pasteboard/data-transfer-items-image-png.html > > > > Doesn't work: > > run-webkit-tests.sh editing/pasteboard > > > > Works: > > Add a <div id="console"></div> element, and at the end of the paste event handler, add the following snippet: > > document.getElementById('console').appendChild(document.createTextNode('a')); > > > > Doesn't work: > > Adding the console div element, but doing this instead at the end of the paste event handler: > > document.getElementById('console').appendChild(document.createElement('br')); > > > > In all cases, when the layout test fails, it behaves as if no changes to the DOM had occurred. The pasted image does not appear at all.

Hayato Ito

Comment 15 2011-04-11 06:08:04 PDT

I can reproduce this also. I confirmed that '-expected.html' strangely affects '-actual.png'. That is related with the fact that new-run-webkit-tests will re-run a test if the test failed. Let me explain: At first run: 1. The driver runs data-transfer-items-image-png.html. The result is an unexpected one because 'no changes to the DOM had occurred'. I have no ideas why this strange behavior happens. 2. The driver runs data-transfer-items-image-png-expected.html. In this run, changes to the DOM occurred. new-run-webkit-tests will try to re-run this test. At re-run: 3. The driver re-runs data-transfer-items-image-png.html. The result is an expected one because changes to the DOM already occurred at stage '2'. 4. The driver runs data-transfer-items-image-png-expected.html. So the final result of '-actual-png', which is the result of '3', might differ depending on whether the run of '-expected.html' changes DOM or not at stage '2'. The same driver (DumpRenderTree instance) is used at all stages. (In reply to comment #12) > I've done more investigation, and I don't understand why the layout test only works sometimes: > > Works: > run-webkit-tests.sh editing/pasteboard/data-transfer-items-image-png.html > > Doesn't work: > run-webkit-tests.sh editing/pasteboard > > Works: > Add a <div id="console"></div> element, and at the end of the paste event handler, add the following snippet: > document.getElementById('console').appendChild(document.createTextNode('a')); > > Doesn't work: > Adding the console div element, but doing this instead at the end of the paste event handler: > document.getElementById('console').appendChild(document.createElement('br')); > > In all cases, when the layout test fails, it behaves as if no changes to the DOM had occurred. The pasted image does not appear at all.

Hayato Ito

Comment 16 2011-04-11 06:28:45 PDT

Let me add the additional info: If the first run fails and the ru-run passes, the 'actual-png' in the result directory is the result of stage 1, not stage 3. (In reply to comment #15) > I can reproduce this also. I confirmed that '-expected.html' strangely affects '-actual.png'. > > That is related with the fact that new-run-webkit-tests will re-run a test if the test failed. > Let me explain: > > At first run: > 1. The driver runs data-transfer-items-image-png.html. The result is an unexpected one because 'no changes to the DOM had occurred'. I have no ideas why this strange behavior happens. > 2. The driver runs data-transfer-items-image-png-expected.html. In this run, changes to the DOM occurred. > > new-run-webkit-tests will try to re-run this test. > > At re-run: > 3. The driver re-runs data-transfer-items-image-png.html. The result is an expected one because changes to the DOM already occurred at stage '2'. > 4. The driver runs data-transfer-items-image-png-expected.html. > > > So the final result of '-actual-png', which is the result of '3', might differ depending on whether the run of '-expected.html' changes DOM or not at stage '2'. > > The same driver (DumpRenderTree instance) is used at all stages. > > > (In reply to comment #12) > > I've done more investigation, and I don't understand why the layout test only works sometimes: > > > > Works: > > run-webkit-tests.sh editing/pasteboard/data-transfer-items-image-png.html > > > > Doesn't work: > > run-webkit-tests.sh editing/pasteboard > > > > Works: > > Add a <div id="console"></div> element, and at the end of the paste event handler, add the following snippet: > > document.getElementById('console').appendChild(document.createTextNode('a')); > > > > Doesn't work: > > Adding the console div element, but doing this instead at the end of the paste event handler: > > document.getElementById('console').appendChild(document.createElement('br')); > > > > In all cases, when the layout test fails, it behaves as if no changes to the DOM had occurred. The pasted image does not appear at all.

Daniel Cheng

Comment 17 2011-04-11 13:33:51 PDT

The root cause seems to be the iframe is not loading properly when the test is run after copy-crash.html. (gdb) break WebCore::Editor::copy Breakpoint 1 at 0x120fe68: file /Users/dcheng/src/WebKit/Source/WebCore/WebCore.gyp/../editing/Editor.cpp, line 1187. (gdb) run --test-shell ~/src/WebKit/LayoutTests/editing/pasteboard/copy-crash.html ~/src/WebKit/LayoutTests/editing/pasteboard/data-transfer-items-image-png.html #URL:file:///Users/dcheng/src/WebKit/LayoutTests/editing/pasteboard/copy-crash.html Breakpoint 1, WebCore::Editor::copy (this=0x2a807ac8) at /Users/dcheng/src/WebKit/Source/WebCore/WebCore.gyp/../editing/Editor.cpp:1187 1187 if (tryDHTMLCopy()) (gdb) print m_frame->document()->m_url.utf8String().data() $1 = 0x9422e30 "file:///Users/dcheng/src/WebKit/LayoutTests/editing/pasteboard/copy-crash.html" (gdb) continue #URL:file:///Users/dcheng/src/WebKit/LayoutTests/editing/pasteboard/data-transfer-items-image-png.html Breakpoint 1, WebCore::Editor::copy (this=0x784a0c8) at /Users/dcheng/src/WebKit/Source/WebCore/WebCore.gyp/../editing/Editor.cpp:1187 1187 if (tryDHTMLCopy()) (gdb) print m_frame->document()->m_url.utf8String().data() $2 = 0x941aeb0 "about:blank" If data-transfer-items-image-png runs before copy-crash, then the iframe loads as expected. Very bizarre.

Daniel Cheng

Comment 18 2011-04-11 13:56:39 PDT

Created attachment 89080 [details] Patch

Daniel Cheng

Comment 19 2011-04-11 13:57:06 PDT

The race on load should be fixed now; we wait for onload to fire before running the layout test. This should fix the associated flakiness.

David Levin

Comment 20 2011-04-11 13:58:31 PDT

Comment on attachment 89080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89080&action=review > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:117 > + // TODO: This is pretty inefficient. We copy the data from the browser FIXME

Daniel Cheng

Comment 21 2011-04-11 14:04:30 PDT

Committed r83494: <http://trac.webkit.org/changeset/83494>

Note You need to log in before you can comment on or make changes to this bug.