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 tree / graph
 
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 		hi: 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
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

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>