RESOLVED FIXED 79861
DragData::asFilenames push same file to result in windows platform 
https://bugs.webkit.org/show_bug.cgi?id=79861
Summary DragData::asFilenames push same file to result in windows platform
huangxueqing
Reported 2012-02-28 20:04:28 PST
It seems like a careless error in DragDataWin.cpp, DragData::asFilenames call windows api for (unsigned i = 0; i < numFiles; i++) { if (!DragQueryFileW(hdrop, 0, filename, WTF_ARRAY_LENGTH(filename))) continue; result.append((UChar*)filename); } which always push 0-index file to result. If you drag some files to WinLauncher, thie filelist return same file name. IMO, use DragQueryFileW(hdrop, i, filename, WTF_ARRAY_LENGTH(filename).
Attachments
patch (1.43 KB, patch)
2012-02-28 20:22 PST, huangxueqing 		eric: review-
DetailsFormatted DiffDiff
patch with a test (4.42 KB, patch)
2012-06-03 01:05 PDT, huangxueqing 		no flags
DetailsFormatted DiffDiff
patch with a test (4.69 KB, patch)
2012-06-06 07:46 PDT, huangxueqing 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
huangxueqing
Comment 1 2012-02-28 20:22:43 PST
Created attachment 129379 [details] patch
Jon Lee
Comment 2 2012-02-29 10:53:49 PST
I find this bug odd, because it has been lingering around for a really long time. A scrub through the file shows that this code still existed back in 2007. But the fix makes sense. Unfortunately, I'm not a reviewer.
Ryosuke Niwa
Comment 3 2012-02-29 10:55:23 PST
Comment on attachment 129379 [details] patch Can we add a test for this?
huangxueqing
Comment 4 2012-02-29 19:11:56 PST
(In reply to comment #2) > I find this bug odd, because it has been lingering around for a really long time. A scrub through the file shows that this code still existed back in 2007. > > But the fix makes sense. Unfortunately, I'm not a reviewer. Actually, this function only be called while drag more than one files to <input type="file" multiply=""> element without webkit2 since safari use webkit2 retrive files name from m_dragDataMap. Which maybe the reason why this bug did not be fixed long times. We use webkit without webkit2 in windows port, I had fixed this in our browser, therefore, i think we should fixed it even though only windows port without webkit2 will use it.
huangxueqing
Comment 5 2012-02-29 19:20:15 PST
(In reply to comment #3) > (From update of attachment 129379 [details]) > Can we add a test for this? As you know, asFilenames was only called by WebView::drop, which actually was windows OLE dragdrop, I could not provide a proper LayoutTest case to simulate drag more than one files to <input type="file" multiply="">, which was tested manually with WinLauncher and it's fine. I can provide a test page for manually if you think it's necessary.
Alexey Proskuryakov
Comment 6 2012-03-02 13:49:20 PST
*** Bug 54214 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 7 2012-03-02 13:50:12 PST
<rdar://problem/8988693>
Eric Seidel (no email)
Comment 8 2012-04-19 15:47:46 PDT
Comment on attachment 129379 [details] patch How do we test this?
huangxueqing
Comment 9 2012-04-23 07:37:21 PDT
(In reply to comment #8) > (From update of attachment 129379 [details]) > How do we test this? The test case as: <body> <script> function handleFiles() { var inputelement = document.getElementById("fileinput"); var files = inputelement.files; for (var i=0; i<inputelement.files.length; ++i) { var newElement = document.createElement("div"); newElement.innerHTML = inputelement.files[i].name; document.body.appendChild(newElement); } } </script> <input id="fileinput" type=file multiple="true" onchange="handleFiles()"> <p> You should see files you dragged into input element as below: </p> </body> When you drag and drop more than one files such as file1, file2 and file3 to input element, the result in page was: file1 file1 file1. I have no idear how to add this case to LayoutTests since it needs user manually test, do you think should i add it to ManualTests?
Alexey Proskuryakov
Comment 10 2012-04-23 10:22:12 PDT
Sounds like this should be testable in DumpRenderTree with eventSender.beginDragWithFiles().
huangxueqing
Comment 11 2012-04-23 20:04:18 PDT
(In reply to comment #10) > Sounds like this should be testable in DumpRenderTree with eventSender.beginDragWithFiles(). It seems like that eventSender.beginDragWithFiles() only be implemented in Chromium port, while this bug was in windows, and the drag&drop have different implementation in Chromium and windows port, maybe this case cannot be tested by this method?
Alexey Proskuryakov
Comment 12 2012-04-23 22:44:59 PDT
Well, it's implemented for multiple ports, but not Windows indeed. Perhaps adding an implementation should be a requirement for this fix.
huangxueqing
Comment 13 2012-04-23 23:42:39 PDT
(In reply to comment #12) > Well, it's implemented for multiple ports, but not Windows indeed. Perhaps adding an implementation should be a requirement for this fix. All right, I will file another bug to do this.
huangxueqing
Comment 14 2012-06-03 00:23:56 PDT
(In reply to comment #12) > Well, it's implemented for multiple ports, but not Windows indeed. Perhaps adding an implementation should be a requirement for this fix. eventSender.beginDragWithFiles had been implemented in bug#86296
huangxueqing
Comment 15 2012-06-03 01:05:18 PDT
Created attachment 145477 [details] patch with a test
Jon Lee
Comment 16 2012-06-05 12:28:55 PDT
Comment on attachment 145477 [details] patch with a test View in context: https://bugs.webkit.org/attachment.cgi?id=145477&action=review > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:1 > +<body> Instead of using custom output for the expected file, please use the js-test-{pre,post}.js pattern (see any of the tests in fast/forms/file/ as an example). Because this test does not test the drag event, this should really be in fast/forms/file. And is there any reason why this has to be windows-specific, instead of all platforms? > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:6 > + layoutTestController.dumpAsText() Nit: missing semicolon > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:16 > + var targetY = 10; Why are targetX and targetY being defined twice?
huangxueqing
Comment 17 2012-06-06 06:54:19 PDT
(In reply to comment #16) > (From update of attachment 145477 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145477&action=review > > > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:1 > > +<body> > > Instead of using custom output for the expected file, please use the js-test-{pre,post}.js pattern (see any of the tests in fast/forms/file/ as an example). > Done > Because this test does not test the drag event, this should really be in fast/forms/file. And is there any reason why this has to be windows-specific, instead of all platforms? OK, i will move it to fast/forms/file. This bug was only reproduce in webkit1 Windows port, I don't know whether this test does make sense to other ports. > > > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:6 > > + layoutTestController.dumpAsText() > > Nit: missing semicolon As you said, i will use js-test-{pre,post}.js rather than custom expected file. BTW, this js was a wonderful tool to write test case. > > > LayoutTests/platform/win/fast/events/drag-and-drop-files.html:16 > > + var targetY = 10; > > Why are targetX and targetY being defined twice? Typo
huangxueqing
Comment 18 2012-06-06 07:46:43 PDT
Created attachment 146028 [details] patch with a test
huangxueqing
Comment 19 2012-06-26 06:59:29 PDT
Oop, anyone review it please? Thanks
WebKit Review Bot
Comment 20 2012-06-26 21:23:03 PDT
Comment on attachment 146028 [details] patch with a test Clearing flags on attachment: 146028 Committed r121319: <http://trac.webkit.org/changeset/121319>
WebKit Review Bot
Comment 21 2012-06-26 21:23:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.