Bug 222072 - [iOS] Specify a _UIDataOwner when reading or writing from the system pasteboard
Summary: [iOS] Specify a _UIDataOwner when reading or writing from the system pasteboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 222071
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-17 14:24 PST by Wenson Hsieh
Modified: 2021-02-19 16:51 PST (History)
14 users (show)

See Also:


Attachments
Patch (117.19 KB, patch)
2021-02-18 15:58 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
v2 (83.37 KB, patch)
2021-02-19 10:45 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Non-internal SDK build fix (83.46 KB, patch)
2021-02-19 10:52 PST, Wenson Hsieh
drousso: review+
Details | Formatted Diff | Diff
For EWS (81.02 KB, patch)
2021-02-19 12:02 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (81.27 KB, patch)
2021-02-19 13:35 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-02-17 14:24:50 PST
<rdar://problem/74208576>
Comment 1 Wenson Hsieh 2021-02-18 15:58:13 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-02-19 09:51:48 PST
Comment on attachment 420882 [details]
Patch

I'm going to see if I can make this patch simpler...
Comment 3 Wenson Hsieh 2021-02-19 10:45:38 PST Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-02-19 10:52:53 PST
Created attachment 420989 [details]
Non-internal SDK build fix
Comment 5 Devin Rousso 2021-02-19 11:12:42 PST
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?
Comment 6 Wenson Hsieh 2021-02-19 11:21:16 PST
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.
Comment 7 Wenson Hsieh 2021-02-19 12:02:57 PST
Created attachment 421005 [details]
For EWS
Comment 8 Wenson Hsieh 2021-02-19 13:32:06 PST
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.
Comment 9 Wenson Hsieh 2021-02-19 13:35:14 PST
Created attachment 421028 [details]
For EWS
Comment 10 Wenson Hsieh 2021-02-19 16:51:48 PST
Committed r273184: <https://commits.webkit.org/r273184>