[webkit-reviews] review granted: [Bug 178056] dragenter and dragleave shouldn't use the same data transfer object : [Attachment 323117] Skipped the test on Windows

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 178056: dragenter and dragleave shouldn't use the same data transfer object
https://bugs.webkit.org/show_bug.cgi?id=178056

Attachment 323117: Skipped the test on Windows

https://bugs.webkit.org/attachment.cgi?id=323117&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 323117
  --> https://bugs.webkit.org/attachment.cgi?id=323117
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.