Bug 87610 – The eventSender.beginDragWithFiles had been implemented in windows, related tests listed in LayoutTests/platform/win/Skipped should be rebaselined

• Component: Tools / Tests
• Status: RESOLVED FIXED

[huangxueqing]

The eventSender.beginDragWithFiles had been implemented in windows by bug#86296, some of related tests listed in LayoutTests/platform/win/Skipped should be rebaselined.

[huangxueqing]

I found many cases skipped in Windows because eventSender.beginDragWithFiles was unimplemented actually did not. I investigated these cases failed reason as follow:
editing/pasteboard/dataTransfer-setData-getData.html    hit ASSERT(url == m_string); in KURL::KURL;
editing/pasteboard/data-transfer-items-drag-drop-entry.html    doMouseUp in eventSender did not fire Drop since m_draggingSourceOperationMask in dragData always DragOperationNone;
editing/pasteboard/data-transfer-items-drag-drop-file.html    doMouseUp in eventSender did not fire Drop since m_draggingSourceOperationMask in dragData always DragOperationNone;
editing/pasteboard/drag-files-to-editable-element.html    There were many functions such as fragmentFromFilenames in ClipboardUtilitiesWin did not implement yet;
editing/pasteboard/file-drag-to-editable.html    https://bugs.wekit.org/show_bug.cgi?id=38826;
editing/pasteboard/file-input-files-access.html    can not retrieve file size with relative file path in Windows;
fast/events/drag-dataTransferItemList-file-handling.html    event.dataTransfer.items.length event.dataTransfer.items.add undefined in Windows;
fast/events/drag-to-navigate.html    The test did not call testRunner.notifyDone;
fast/events/drop-handler-should-not-stop-navigate.html    prevent default event seems incorrect in windows, which lead to that drop event nerver be fired;
fast/events/drop-with-file-paths.html    prevent default event seems incorrect in windows, which lead to that drop event nerver be fired;
fast/events/dropzone-004.html    ondrop event nerver be fired;
fast/forms/file/input-file-directory-upload.html    DIRECTORY_UPLOAD disabled in Windows;
fast/forms/file/recover-file-input-in-unposted-form.html    FILE_SYSTEM disabled in Windows.


I rebaselined a test and removed PASS tests from skipped file in Windows.

[huangxueqing]

Created attachment 149189 [details]
patch

[Jessie Berlin]

Comment on attachment 149189 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149189&action=review

> LayoutTests/platform/win/Skipped:260
>  http/tests/security/clipboard/clipboard-file-access.html

It is wrong to leave all these other tests that still fail under the "# <rdar://problem/5230396> eventSender.beginDragWithFiles is unimplemented" - that makes it appear as if you haven't fixed this bug and hides the other issues you have identified.

You have already done the great work to identify the reason these tests continue to fail - please represent that in the Skipped file.

For instance, fast/forms/file/input-file-directory-upload.html is already in the Skipped list in another section "# Directory upload is not enabled.", so you can just remove it from this list. For those that don't already have sections in the Skipped list, you can add sections for them.

You should be able to totally remove "# <rdar://problem/5230396> eventSender.beginDragWithFiles is unimplemented" section as part of this patch.

[huangxueqing]

(In reply to comment #3)
> (From update of attachment 149189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149189&action=review
> 
> > LayoutTests/platform/win/Skipped:260
> >  http/tests/security/clipboard/clipboard-file-access.html
> 
> It is wrong to leave all these other tests that still fail under the "# <rdar://problem/5230396> eventSender.beginDragWithFiles is unimplemented" - that makes it appear as if you haven't fixed this bug and hides the other issues you have identified.
> 
> You have already done the great work to identify the reason these tests continue to fail - please represent that in the Skipped file.
> 
> For instance, fast/forms/file/input-file-directory-upload.html is already in the Skipped list in another section "# Directory upload is not enabled.", so you can just remove it from this list. For those that don't already have sections in the Skipped list, you can add sections for them.
> 
> You should be able to totally remove "# <rdar://problem/5230396> eventSender.beginDragWithFiles is unimplemented" section as part of this patch.

OK, thanks!

[huangxueqing]

Created attachment 150327 [details]
patch

[Jessie Berlin]

Comment on attachment 150327 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150327&action=review

r=me with the fixes I mentioned :)

> LayoutTests/platform/win/Skipped:249
> +# hit ASSERT(url == m_string) in KURL::KURL

Sorry if I wasn't clear about this last time. You should file a bug about this issue and include the bug number in the Skipped list so that somebody looking at the Skipped list can see more details and if any progress has been made on that bug.

> LayoutTests/platform/win/Skipped:252
> +# file size did not be retrieved via relative path

Ditto.

[huangxueqing]

(In reply to comment #6)
> (From update of attachment 150327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150327&action=review
> 
> r=me with the fixes I mentioned :)
> 
> > LayoutTests/platform/win/Skipped:249
> > +# hit ASSERT(url == m_string) in KURL::KURL
> 
> Sorry if I wasn't clear about this last time. You should file a bug about this issue and include the bug number in the Skipped list so that somebody looking at the Skipped list can see more details and if any progress has been made on that bug.

KURL alway append slash in the end of url which did not include neither fragment nor query, ClipboardWin::setData() construct KURL with a url which did not include a slash, please see bug#90425.

> 
> > LayoutTests/platform/win/Skipped:252
> > +# file size did not be retrieved via relative path
> 
> Ditto.
Please see bug#90426

[huangxueqing]

Created attachment 150519 [details]
patch

[Jessie Berlin]

Comment on attachment 150519 [details]
patch

Setting cq+

[WebKit Review Bot]

Comment on attachment 150519 [details]
patch

Rejecting attachment 150519 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Jessie Berlin]

Eric, is there a way for me to set my review+ on this patch such that it will override the webkit.review.bots's review-?

[Eric Seidel (no email)]

Sure.  You can just set r+ again and then you will have set it (instead of Haung) and then the bots will accept it.  The problem is simply that a non-reviewer set r+, so the bot rejected it.  if a reviewer sets r+ teh bots will happily land it. :)  Hope that helps!

[Jessie Berlin]

Comment on attachment 150519 [details]
patch

setting r+

[Jessie Berlin]

(In reply to comment #12)
> Sure.  You can just set r+ again and then you will have set it (instead of Haung) and then the bots will accept it.  The problem is simply that a non-reviewer set r+, so the bot rejected it.  if a reviewer sets r+ teh bots will happily land it. :)  Hope that helps!

Heh, when I previewed that change it didn't show me as setting the r+, but webkit.review.bot.

Seems to work when I go through with the change though :)

[WebKit Review Bot]

Comment on attachment 150519 [details]
patch

Clearing flags on attachment: 150519

Committed r121814: <http://trac.webkit.org/changeset/121814>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.