Summary: | [iOS] Specify a _UIDataOwner when reading or writing from the system pasteboard | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | annulen, bdakin, benjamin, cdumez, cmarcelo, ews-watchlist, gyuyoung.kim, hi, megan_gardner, ryuan.choi, sergio, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 222071 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2021-02-17 14:24:50 PST
Created attachment 420882 [details]
Patch
Comment on attachment 420882 [details]
Patch
I'm going to see if I can make this patch simpler...
Created attachment 420988 [details]
v2
Created attachment 420989 [details]
Non-internal SDK build fix
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 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. Created attachment 421005 [details]
For EWS
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. Created attachment 421028 [details]
For EWS
Committed r273184: <https://commits.webkit.org/r273184> |