[webkit-reviews] review granted: [Bug 172891] Refactor -[WebItemProviderPasteboard valuesForPasteboardType:inItemSet:] to check readable types : [Attachment 311907] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 172891: Refactor -[WebItemProviderPasteboard
valuesForPasteboardType:inItemSet:] to check readable types
https://bugs.webkit.org/show_bug.cgi?id=172891

Attachment 311907: Patch

https://bugs.webkit.org/attachment.cgi?id=311907&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 311907
  --> https://bugs.webkit.org/attachment.cgi?id=311907
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311907&action=review

I’m concerned that we don’t have coverage for the existing cases; the new tests
only cover the new feature. The old code handled 13 different UTIs. Do we have
tests that cover all 13 of those to make sure the new code path relying on
UIKit methods still works with all 13 of them? And what about each UTI with
each possible different type of data (string vs. attributed string, etc.)? What
about UTIs we should definitely not handle; are there any of those we should be
testing?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:399
> +    if (type == String(kUTTypeText) || type == String(kUTTypeHTML) || type
== String(kUTTypePlainText)) {
> +	   ASSERT([value isKindOfClass:[NSString class]] || [value
isKindOfClass:[NSAttributedString class]]);

Can all three of these really have NSAttributedString? Or is it only one or two
of them?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:321
> +static Class classForTypeIdentifier(NSString *typeIdentifier, NSString
**outTypeIdentifierToLoad)

The out argument could be a reference instead of a pointer. There is only one
call site and it doesn’t need the flexibility to pass a null pointer in. Or we
could still use a pointer, but no need for the null checks.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:323
> +    if (outTypeIdentifierToLoad)

This if statement isn’t needed.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:336
> +	   if (outTypeIdentifierToLoad)

This if statement isn’t needed.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:353
> +	   NSString *typeIdentifierToLoad = nil;

No need to initialize this to nil. The function never returns without setting
this. But some people might be more comfortable if you left that in.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:355
> +	   if (!readableClass || !typeIdentifierToLoad)

The null check for typeIdentifierToLoad here is unnecessary. The function
doesn’t return a non-nil class and nil for a type identifier, and so we should
not have a dead code path checking for that.