[webkit-reviews] review granted: [Bug 121818] Upstream changes to Pasteboard implementation for iOS : [Attachment 212843] Second part with fixes

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 121818: Upstream changes to Pasteboard implementation for iOS
https://bugs.webkit.org/show_bug.cgi?id=121818

Attachment 212843: Second part with fixes
https://bugs.webkit.org/attachment.cgi?id=212843&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212843&action=review


> Source/WebCore/editing/ios/EditorIOS.mm:387
> +    : frame(frame)
> +    , context(context)
> +    , allowPlainText(allowPlainText)
> +    , madeFragmentFromPlainText(false)

Indent.

> Source/WebCore/editing/ios/EditorIOS.mm:477
> +    NSURL *URL = [NSURL URLWithString:[NSString
stringWithFormat:@"%@://%@/%@", @"webkit-fake-url", UUIDString.get(),
relativePart]];
> +
> +    return URL;

This could be return [NSURL ...]

> Source/WebCore/editing/ios/EditorIOS.mm:494
> +    if (url.string().isEmpty())
> +	   return false;

This should be url.isEmpty(). URL::string() is evil :)

> Source/WebCore/editing/ios/EditorIOS.mm:607
> +
> +

Two blank lines here.

> Source/WebCore/platform/PasteboardStrategy.h:53
> +

No need for the blank line.

> Source/WebCore/platform/PasteboardStrategy.h:54
>  #endif

I would add // PLATFORM(IOS) because this block is a little long.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:179
> +    if (pasteboardType == String(kUTTypeURL))

Unrelated, but we should totally add WTF::String::operator==(NSString*)!

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:180
> +	   [representations setValue:[[[NSURL alloc] initWithString:text]
autorelease] forKey:pasteboardType];

Please use a adoptNS().get() here instead of autorelease.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:193
> +    NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index];

RetainPtr<> of [NSIndexSet alloc] initWith...]?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:204
> +    NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index];

RetainPtr<> of [NSIndexSet alloc] initWith...]?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:215
> +    if (![value isKindOfClass:[NSString class]]) {
> +	   return String();
> +    }

Brackets.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:221
> +    NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index];

RetainPtr?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:225
> +    if (![pasteboardItem.get() count])

No need for the get().

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:228
> +    id value = [pasteboardItem.get() objectAtIndex:0];

No need for the get().

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:230
> +	   ASSERT([value isKindOfClass:[NSURL class]]);

This Assert will always hit.