Bug 178056 – dragenter and dragleave shouldn't use the same data transfer object

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ryosuke Niwa]

We're supposed to be creating a distinct DataTransfer object for each dragenter & dragleave event we fire.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 323117 [details]
Skipped the test on Windows

[Darin Adler]

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.

[Wenson Hsieh]

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"

[Darin Adler]

$ git grep borad

Found this typo in ChangeLog, but not in any pre-existing code.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 323132 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 323132 [details]
Patch for landing

Clearing flags on attachment: 323132

Committed r223031: <http://trac.webkit.org/changeset/223031>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34875120>

[Matt Lewis]

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.

[Ryosuke Niwa]

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

Thanks, Wenson!