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.
Created attachment 88748 [details] Patch
Comment on attachment 88748 [details] Patch I forgot to update the deps for the chromium port.
Created attachment 88867 [details] Patch
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.
Created attachment 88875 [details] Patch
(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.
Comment on attachment 88875 [details] Patch r=me
Committed r83353: <http://trac.webkit.org/changeset/83353>
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
Reverted r83353 for reason: The new test doesn't pass on all Chromium platforms. Committed r83412: <http://trac.webkit.org/changeset/83412>
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.
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.
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.
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.
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.
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.
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.
Created attachment 89080 [details] Patch
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.
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
Committed r83494: <http://trac.webkit.org/changeset/83494>