Bug 178056

Created attachment 323111 [details] Fixes the bug and refactors code

Created attachment 323117 [details] Skipped the test on Windows

Comment on attachment 323117 [details] Skipped the test on Windows View in context: https://bugs.webkit.org/attachment.cgi?id=323117&action=review > Source/WebCore/page/DragController.cpp:697 > + if (!destinationOperation) > + operation = defaultOperationForDrag(sourceOperation); > + else if (!(sourceOperation & *destinationOperation)) // The element picked an operation which is not supported by the source > operation = DragOperationNone; > - } > + else > + operation = *destinationOperation; Stylistic question about * vs. using the value() function on optionals. I almost always use the named function, but I am not sure it’s better. > Source/WebCore/page/EventHandler.cpp:2277 > +bool EventHandler::dispatchDragEnterOrDragOverEvent(const AtomicString& eventType, Element& target, const PlatformMouseEvent& event, > + std::unique_ptr<Pasteboard>&& pasteborad, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) Out arguments are kind of ugly. Sure would be nice if it could all be in the return value instead of a return value plus an out argument. > Source/WebCore/page/EventHandler.cpp:2289 > +bool EventHandler::updateDragAndDrop(const PlatformMouseEvent& event, std::function<std::unique_ptr<Pasteboard>()> makePasteboard, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) Ditto. Also, a std::function can be expensive to copy, so it would be nice if it was a const&, assuming it’s always called synchronously and not saved for later.

Comment on attachment 323117 [details] Skipped the test on Windows View in context: https://bugs.webkit.org/attachment.cgi?id=323117&action=review >> Source/WebCore/page/EventHandler.cpp:2277 >> + std::unique_ptr<Pasteboard>&& pasteborad, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) > > Out arguments are kind of ugly. Sure would be nice if it could all be in the return value instead of a return value plus an out argument. Nit - "pasteborad" => "pasteboard"

$ git grep borad Found this typo in ChangeLog, but not in any pre-existing code.

(In reply to Darin Adler from comment #3) > Comment on attachment 323117 [details] > Skipped the test on Windows > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323117&action=review > > > Source/WebCore/page/DragController.cpp:697 > > + if (!destinationOperation) > > + operation = defaultOperationForDrag(sourceOperation); > > + else if (!(sourceOperation & *destinationOperation)) // The element picked an operation which is not supported by the source > > operation = DragOperationNone; > > - } > > + else > > + operation = *destinationOperation; > > Stylistic question about * vs. using the value() function on optionals. I > almost always use the named function, but I am not sure it’s better. Fixed to use .value(). > > Source/WebCore/page/EventHandler.cpp:2277 > > +bool EventHandler::dispatchDragEnterOrDragOverEvent(const AtomicString& eventType, Element& target, const PlatformMouseEvent& event, > > + std::unique_ptr<Pasteboard>&& pasteborad, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) > > Out arguments are kind of ugly. Sure would be nice if it could all be in the > return value instead of a return value plus an out argument. Okay. I'm gonna add the following struct and make this function return that. struct DragTargetResponse { bool accept; std::optional<DragOperation> operation; }; > > Source/WebCore/page/EventHandler.cpp:2289 > > +bool EventHandler::updateDragAndDrop(const PlatformMouseEvent& event, std::function<std::unique_ptr<Pasteboard>()> makePasteboard, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) > > Ditto. > > Also, a std::function can be expensive to copy, so it would be nice if it > was a const&, assuming it’s always called synchronously and not saved for > later. Oops, that's a good catch. Fixed.

(In reply to Wenson Hsieh from comment #4) > Comment on attachment 323117 [details] > Skipped the test on Windows > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323117&action=review > > >> Source/WebCore/page/EventHandler.cpp:2277 > >> + std::unique_ptr<Pasteboard>&& pasteborad, DragOperation sourceOperation, bool draggingFiles, std::optional<DragOperation>& destinationOperation) > > > > Out arguments are kind of ugly. Sure would be nice if it could all be in the return value instead of a return value plus an out argument. > > Nit - "pasteborad" => "pasteboard" Fixed.

Created attachment 323132 [details] Patch for landing

Comment on attachment 323132 [details] Patch for landing Clearing flags on attachment: 323132 Committed r223031: <http://trac.webkit.org/changeset/223031>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34875120>

This patch caused a build break on windows: https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/5053 https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/5053/steps/compile-webkit/logs/stdio Error: C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\page\EventHandler.cpp(2285): error C2440: 'return': cannot convert from 'initializer list' to 'WebCore::EventHandler::DragTargetResponse' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\page\EventHandler.cpp(2285): note: No constructor could take the source type, or constructor overload resolution was ambiguous If this can't be addressed quickly, we will need to revert the patch.

Wenson fixed it in https://trac.webkit.org/changeset/223050 https://trac.webkit.org/changeset/223054 Thanks, Wenson!