[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