Bug 172891 – Refactor -[WebItemProviderPasteboard valuesForPasteboardType:inItemSet:] to check readable types

• Component: WebCore Misc.
• Status: NEW

[Wenson Hsieh]

<rdar://problem/32204540>

[Wenson Hsieh]

Created attachment 311907 [details]
Patch

[Darin Adler]

Comment on attachment 311907 [details]
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.

[Wenson Hsieh]

Comment on attachment 311907 [details]
Patch

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

Since WebItemProviderPasteboard is only used for drag and drop on iOS, I don't think there's significant risk to existing pasteboard-related functionality on iOS (e.g. copy and paste). The existing DataInteractionTests cover most of these codepaths by simulating external item providers (i.e. drops coming in from a different app). Part of the problem before was that this code previously attempted to map specific UTIs to object types, but UIKit already handles this responsibility, so there were many codepaths that didn't make sense before -- for instance, attempting to initialize an attributed string using kUTTypeHTML. Our testing for this codepath (drop handling) is centered around simulating scenarios that we encounter from other apps around the system, both from apps that use platform UIKit NSItemProviderWriting-conformant objects to generate item providers, and apps that directly write NSData to item providers for specific UTIs.

So far, these are some of the scenarios are tested for drops into web content:
- kUTTypeJSON as a file upload
- kUTTypeHTML both as file upload and insertion into rich editable
- NSAttributedString (kUTTypeRTFD) into rich editable
- NSString (kUTTypeUTF8PlainText) into rich/plain editable areas
- Web archive data into rich and plain editable areas
- kUTTypeURL into rich/plain editable areas
- kUTTypeGIF as file upload
- kUTTypePNG into rich editable areas/file inputs
- kUTTypeJPEG into file inputs

As you pointed out, we need more test coverage here to check for false positives (UTIs that WebKit believes can be handled, but end up doing nothing on drop). I'll be adding more tests for these cases in subsequent patches. It looks like there isn't test coverage of UIColor either, and this is something I'll add as well (it should change the typing style to the given color). I'll also add tests for kUTTypeUTF16PlainText into plaintext inputs and rich contenteditable areas.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:399
>> +        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?

Good point -- we should never produce an attributed string for kUTTypePlainText. I'll split that out into a different case.

>> 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.

Sounds good! Changed to use a reference.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:323
>> +    if (outTypeIdentifierToLoad)
> 
> This if statement isn’t needed.

Removed.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:336
>> +        if (outTypeIdentifierToLoad)
> 
> This if statement isn’t needed.

Removed.

>> 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.

This makes sense -- I've removed this.

>> 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.

Removed the null check.

[Wenson Hsieh]

Created attachment 312049 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312049 [details]
Patch for landing

Clearing flags on attachment: 312049

Committed r217818: <http://trac.webkit.org/changeset/217818>