[webkit-reviews] review granted: [Bug 176264] [iOS DnD] Refactor drag and drop logic in WKContentView in preparation for supporting multiple drag items in a drag session : [Attachment 319834] First pass

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 176264: [iOS DnD] Refactor drag and drop logic in WKContentView in
preparation for supporting multiple drag items in a drag session
https://bugs.webkit.org/show_bug.cgi?id=176264

Attachment 319834: First pass

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 319834
  --> https://bugs.webkit.org/attachment.cgi?id=319834
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=319834&action=review

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:54
> +    bool possiblyNeedsDragPreviewUpdate;

Initialize this?

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:56
> +    NSInteger itemIdentifier;

Initialize this?

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:59
> +class DragDropInteractionState {

It’s a bit hard to get a sense of what this class does, because there are so
many inline function bodies interspersed. I strongly suggest moving some of
them out of the class so we can see what the class’s interface is more clearly.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:66
> +    void stageDragItem(const WebCore::DragItem&, RetainPtr<UIImage>);

RetainPtr<UIImage> is a strange argument type. Normally it would just be
UIImage *; why does the argument need to be a RetainPtr?

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:69
> +	   return !!m_stagedDragSource && stagedDragSource().action !=
WebCore::DragSourceActionNone;

Can write m_stagedDragSource.has_value() instead of !!m_stagedDragSource.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:103
> +	   if (auto block = m_dragStartCompletionBlock) {
> +	       m_dragStartCompletionBlock = nil;
> +	       return block;
> +	   }
> +	   return nil;

This can be written as a one-liner:

    return WTFMove(m_dragStartCompletionBlock);

> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:112
> +	   if (auto block = m_dragCancelSetDownBlock) {
> +	       m_dragCancelSetDownBlock = nil;
> +	       return block;
> +	   }
> +	   return nil;

Ditto.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:85
> +static RetainPtr<UIImage> uiImageForImage(RefPtr<Image> image)

Argument type should just be Image*.