Bug 176264 – [iOS DnD] Refactor drag and drop logic in WKContentView in preparation for supporting multiple drag items in a drag session

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Primarily, add a staging mechanism for writing items to the WebItemProviderPasteboard and coalescing drag item information in the UI process. Additionally, split some logic out of WKContentView and into a separate helper class to make hacking on this easier in later patches.

[Wenson Hsieh]

Created attachment 319834 [details]
First pass

[Wenson Hsieh]

Work towards <rdar://problem/31144674>

[Darin Adler]

Comment on attachment 319834 [details]
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*.

[Wenson Hsieh]

Comment on attachment 319834 [details]
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?

Done!

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

Done!

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

Good call -- moved all inline implementations that aren't just simple getters to DragDropInteractionState.mm.

>> 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?

Good point -- this should be a UIImage * instead of a RetainPtr. Fixed.

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

True (though, looking at this again, I'm unsure why this needs to be !! in the first place -- it seems just checking m_stagedDragSource should be sufficient and a lot cleaner.)

>> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:103
>> +        return nil;
> 
> This can be written as a one-liner:
> 
>     return WTFMove(m_dragStartCompletionBlock);

Done. Much cleaner!

>> Source/WebKit/UIProcess/ios/DragDropInteractionState.h:112
>> +        return nil;
> 
> Ditto.

Done!

>> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:85
>> +static RetainPtr<UIImage> uiImageForImage(RefPtr<Image> image)
> 
> Argument type should just be Image*.

Done!

[Wenson Hsieh]

Comment on attachment 319834 [details]
First pass

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:182
> +    if ([pasteboard respondsToSelector:@selector(stageRegistrationList:)])

On second thought, only staging the registration list here and not setting the item provider array will break bincompat with older (iOS 11) UIKit -- we'll need to continue doing both for now. (For internal reference: see <rdar://problem/34244415>).

[Wenson Hsieh]

Created attachment 319862 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 319862 [details]
Patch for landing

Clearing flags on attachment: 319862

Committed r221595: <http://trac.webkit.org/changeset/221595>

[Jonathan Bedard]

Committed r222265: <http://trac.webkit.org/changeset/222265>

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #8)
> Committed r222265: <http://trac.webkit.org/changeset/222265>

A small build fix for iOS 11.

[Radar WebKit Bug Importer]

<rdar://problem/34693785>