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).
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.
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.
Created attachment 39386 [details] speculative fix (not tested yet)
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.
Created attachment 39388 [details] speculative fix, with changelog entry OK, here is the same fix with a changelog entry.
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).
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.
http://webkit.org/b/48229 is gibberish, I meant http://trac.webkit.org/changeset/48229.
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.
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. :(
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.
Any objections to me rolling this out for now and re-opening the original bug?
Please roll it out. We can't leave the build bot failing tests for days at a time like this!
I agree. Regressions should be rolled out. I'm happy to do so if you'd like.
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.
I'll take a look at bugzilla-tool. bugzilla-tool rollout is still rather experimental.
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.
Heck, we could even just r+/cq+ the rollout patch I attached and it should do the right thing. ;)
Ah, rollout threw and exception for me running under SVN. Will fix.
This looks related to my work to fix bug 24731.
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