Bug 29129 – REGRESSION(r48229): Failures in drag and drop tests on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brian Weinstein]

The fix for https://bugs.webkit.org/show_bug.cgi?id=26700 (http://trac.webkit.org/changeset/48229) causes layout tests regressions on Windows.

Build before this revision: http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/4108 (2 test cases (<1%) had incorrect layout 14 test cases (<1%) crashed)
Build after this revision: http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/4109 (22 test cases (<1%) had incorrect layout 15 test cases (<1%) crashed)

I noticed there was a mac specific fix to this bug, in WebKit/mac/WebView/WebFrame.mm, but no corresponding fix on the Windows side of things. I'm not sure what the course of action should be, should this change be rolled out and tested more heavily on Windows? Should the corresponding fix be made on Windows? Should the test results be updated for this new behavior (doesn't look like the mac needed any updating).

[Eric Seidel (no email)]

A direct link to the results:
http://build.webkit.org/results/Windows%20Release%20(Tests)/r48257%20(4134)/results.html
Unless Jens or a windows person has immediate insight, we can just roll this out for now.  No sense in leaving the bots red.

[Jens Alfke]

Right -- one of the last pieces I added was to propagate the drag operation from the source to the receiver if it's an intra-page drag. Not doing this makes those drags always turn into copies, which breaks dragging text, of course.
I did this to the Mac code (WebFrame.mm) but forgot to make an equivalent change to Windows. 

It should be pretty trivial -- add the following second parameter to the dragSourceMovedTo() call at WebDropSource.cpp:116:
    page->dragController()->destinationDragOperation()
I'll upload a patch to this effect, but it's going to take me a while to get my Windows build up and running to try it out, so if someone else has convenient access to Windows and wants to try it, please do.

[Jens Alfke]

Created attachment 39386 [details]
speculative fix (not tested yet)

[Eric Seidel (no email)]

If you add a ChangeLog to your patch, we can commit a speculative fix. :)  You certainly wouldn't be the first person to use the windows buildbot your windows build.

[Jens Alfke]

Created attachment 39388 [details]
speculative fix, with changelog entry

OK, here is the same fix with a changelog entry.

[Brian Weinstein]

I'm currently updating and building and will be able to test this fix, but if someone else can beat me to it, feel free (I should be able to test it in about 10 minutes).

[Brian Weinstein]

I applied this patch (it need to include WebCore/DragController.h btw), but it didn't fix any of the regressions caused by http://webkit.org/b/48229.

[Brian Weinstein]

http://webkit.org/b/48229 is gibberish, I meant http://trac.webkit.org/changeset/48229.

[Jens Alfke]

Well, I'm confused then. The issue is that EventHandler::dragSourceMovedTo( ) now takes a second parameter that describes the drag operation (move/copy/link) being used by the receiver. This is needed so that the source can determine whether to delete the original, if it's a move.

The change to WebFrame.mm to pass this parameter value was the only platform-specific change in my original patch. The change here duplicates that one. If things still aren't working, it may be related to idiosyncrasies of the Windows platform drag-n-drop code. I have zero experience with Windows APIs, though.

[Eric Seidel (no email)]

Describing drag and drop's test coverage as "poor" is probably even too generous.  :)  Your test is probably just revealing existing bugs in the windows drag and drop code.  That said, we can't leave test failures on the bots, so either we'll need to the bugs you've revealed or roll out. :(

[David Levin]

Comment on attachment 39388 [details]
speculative fix, with changelog entry

r- simply based on Brian Weinstein's comment that this didn't fix the regression.

[Brian Weinstein]

Any objections to me rolling this out for now and re-opening the original bug?

[Mark Rowe (bdash)]

Please roll it out.  We can't leave the build bot failing tests for days at a time like this!

[Eric Seidel (no email)]

I agree. Regressions should be rolled out.  I'm happy to do so if you'd like.

[Brian Weinstein]

I tried using bugzilla-tool rollout, but I must have been doing it wrong and it didn't work for me, go ahead and roll it out.

[Eric Seidel (no email)]

I'll take a look at bugzilla-tool.  bugzilla-tool rollout is still rather experimental.

[Eric Seidel (no email)]

Created attachment 39480 [details]
rollout patch

WebKitTools/Scripts/bugzilla-tool rollout 48229
worked as expected for me...

I did not try building the result.

Were you using SVN or git?  I'd be interested in seeing your failure output.

[Eric Seidel (no email)]

Heck, we could even just r+/cq+ the rollout patch I attached and it should do the right thing. ;)

[Eric Seidel (no email)]

Ah, rollout threw and exception for me running under SVN.  Will fix.

[Daniel Bates]

This looks related to my work to fix bug 24731.

[Eric Seidel (no email)]

Comment on attachment 39480 [details]
rollout patch

Rolled out using:
bugzilla-tool rollout 48229 --complete-rollout

See:
https://bugs.webkit.org/show_bug.cgi?id=26700#c22