Bug 222072 – [iOS] Specify a _UIDataOwner when reading or writing from the system pasteboard

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/74208576>

[Wenson Hsieh]

Created attachment 420882 [details]
Patch

[Wenson Hsieh]

Comment on attachment 420882 [details]
Patch

I'm going to see if I can make this patch simpler...

[Wenson Hsieh]

Created attachment 420988 [details]
v2

[Wenson Hsieh]

Created attachment 420989 [details]
Non-internal SDK build fix

[Devin Rousso]

Comment on attachment 420989 [details]
Non-internal SDK build fix

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

r=me, nice work!

> Source/WebCore/platform/DataOwnerType.h:34
> +    Shared

Style: missing trailing comma

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:108
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;

Is this actually needed?  I try to avoid using `default` because it can make finding all the uses of an `enum` inside a `switch` harder because there is no build error.

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:148
> +    MESSAGE_CHECK_COMPLETION(dataOwner.hasValue(), completionHandler({ }));

NIT: Is the `.hasValue()` actually needed?

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:657
> +Optional<DataOwnerType> WebPasteboardProxy::dataOwner(IPC::Connection& connection, const String& pasteboardName, Optional<PageIdentifier> pageID, PasteboardAccessIntent intent) const

NIT: This function name makes me think that it's a simple getter, but it's not.  Perhaps a more specific name like `determineDataOwner` (or even `dataOwnerForPasteboard`)?

> Source/WebKit/UIProcess/PasteboardAccessIntent.h:32
> +    Write

Style: missing trailing comma

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7475
> +    auto coreType = [] (_UIDataOwner platformType) {

NIT: Can you just make this a `static` function outside?

[Wenson Hsieh]

Comment on attachment 420989 [details]
Non-internal SDK build fix

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

>> Source/WebCore/platform/DataOwnerType.h:34
>> +    Shared
> 
> Style: missing trailing comma

Added!

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:108
>> +        break;
> 
> Is this actually needed?  I try to avoid using `default` because it can make finding all the uses of an `enum` inside a `switch` harder because there is no build error.

Good point — removed this `default` case.

>> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:148
>> +    MESSAGE_CHECK_COMPLETION(dataOwner.hasValue(), completionHandler({ }));
> 
> NIT: Is the `.hasValue()` actually needed?

I added it here for a bit more clarity, though it is indeed not needed.

I'll make this just `MESSAGE_CHECK_COMPLETION(dataOwner, ~);`

>> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:657
>> +Optional<DataOwnerType> WebPasteboardProxy::dataOwner(IPC::Connection& connection, const String& pasteboardName, Optional<PageIdentifier> pageID, PasteboardAccessIntent intent) const
> 
> NIT: This function name makes me think that it's a simple getter, but it's not.  Perhaps a more specific name like `determineDataOwner` (or even `dataOwnerForPasteboard`)?

Fair point! I think `dataOwnerForPasteboard` is redundant because we're already in `WebPasteboardProxy`, so I'll rename this to `determineDataOwner`.

>> Source/WebKit/UIProcess/PasteboardAccessIntent.h:32
>> +    Write
> 
> Style: missing trailing comma

Added!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7475
>> +    auto coreType = [] (_UIDataOwner platformType) {
> 
> NIT: Can you just make this a `static` function outside?

Done.

[Wenson Hsieh]

Created attachment 421005 [details]
For EWS

[Wenson Hsieh]

Comment on attachment 420989 [details]
Non-internal SDK build fix

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

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:339
> +            return completionHandler(newChangeCount);

Oops. Since the `return` here is now inside the lambda, we'll end up calling the completion handle twice in this method, which is what's causing these API tests to fail:

• DragAndDropTests.DragImageLocationForLinkInSubframe
• DragAndDropTests.DataTransferTypesOnDragStartForLink
• DragAndDropTests.PreventingMouseDownShouldPreventDragStart

I changed this to MESSAGE_CHECK on a null `webProcessProxy` instead.

[Wenson Hsieh]

Created attachment 421028 [details]
For EWS

[Wenson Hsieh]

Committed r273184: <https://commits.webkit.org/r273184>