Bug 79861

Summary:

DragData::asFilenames push same file to result in windows platform

Product:

WebKit

Reporter:

huangxueqing <huangxueqing>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

anilnamde, ap, enrica, jonlee, webkit.review.bot

Priority:

Keywords:

InRadar, PlatformOnly

Version:

528+ (Nightly build)

Hardware:

OS:

Windows 7

Attachments:

Description	Flags
patch	eric: review-
patch with a test	none
patch with a test	none

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-	Details Formatted Diff Diff
patch with a test (4.42 KB, patch) 2012-06-03 01:05 PDT, huangxueqing	no flags	Details Formatted Diff Diff
patch with a test (4.69 KB, patch) 2012-06-06 07:46 PDT, huangxueqing	no flags	Details Formatted Diff Diff
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.