[webkit-reviews] review granted: [Bug 222072] [iOS] Specify a _UIDataOwner when reading or writing from the system pasteboard : [Attachment 420989] Non-internal SDK build fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 19 11:12:42 PST 2021
Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 222072: [iOS] Specify a _UIDataOwner when reading or writing from the
system pasteboard
https://bugs.webkit.org/show_bug.cgi?id=222072
Attachment 420989: Non-internal SDK build fix
https://bugs.webkit.org/attachment.cgi?id=420989&action=review
--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 420989
--> https://bugs.webkit.org/attachment.cgi?id=420989
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?
More information about the webkit-reviews
mailing list