Bug 142596 – WebKit on iOS should support file upload from iCloud Drive

• Component: WebKit2
• Status: RESOLVED FIXED

[Jon Honeycutt]

WebKit on iOS should support file upload from iCloud Drive.

[Jon Honeycutt]

Created attachment 248457 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 248457 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        * UIProcess/ios/forms/WKFileUploadPanel.mm:

Maybe its just me, but I find this ChangeLog format completely unreadable. Some empty lines would help break things up. Many of the comments are not needed if the method is obvious. Not important to change, but I think another style would encourage reviewers to read your ChangeLog comments more.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:172
> +- (instancetype)initWithFileURL:(NSURL *)fileURL;

Style: Normally we put the -init above properties.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:274
> +    RetainPtr<UIDocumentMenuViewController> _sourceMenu;

Nit: I think this name is a bit poor. Maybe _documentMenuController would be clearer?
Style: The other lines have the comments at the end of the line. You could do that here as well.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:400
> +        // FIXME: We should support more MIME type -> UTI mappings.

We should! Lets make a bugzilla bug and put that in this comment.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:483
> +    _sourceMenu = adoptNS([[UIDocumentMenuViewController alloc] initWithDocumentTypes:[self _mediaTypesForDocumentPickerMenu] inMode:UIDocumentPickerModeImport]);

We should also have a FIXME and bug here about eventually supporting multiple selection with UIDocumentViewController.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:487
> +    NSString *photoLibraryString = WEB_UI_STRING_KEY("Photo Library", "Photo Library (file upload action sheet)", "File Upload alert sheet button string for choosing an existing media item from the Photo Library");

Nit: Since you made a helper for the camera string, you could make one for this too if only to reduce the WEB_UI_STRING bloat down to one place.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:581
> +- (void)documentPickerWasCancelled:(UIDocumentPickerViewController *)imagePicker

Nit: "imagePicker" => "documentPicker" or "controller"

[Joseph Pecoraro]

Looks good to me. I am not a WebKit2 Owner though, so it will need an Owner's approval.

[Jon Honeycutt]

Created attachment 248506 [details]
WIP Take 2

WIP Take 2 patch that tests for the -[UIDocumentMenuViewController _setIgnoreApplicationEntitlementForImport:] SPI rather than using WKSI to test whether the current app has the correct entitlement.

[WebKit Commit Bot]

Attachment 248506 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:76:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:670:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 248506 [details]
WIP Take 2

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

This looks pretty good to me, but I have enough concerns that I think another round is called for here.

> Source/WebKit2/Platform/spi/ios/UIKitSPI.h:78
> +#if defined(__has_include) && __has_include(<UIKit/UIDocumentMenuViewController_Private.h>)
> +#import <UIKit/UIDocumentMenuViewController_Private.h>
> +#else
> +@interface UIDocumentMenuViewController (Details)
> +@property (nonatomic, assign, setter = _setIgnoreApplicationEntitlementForImport:, getter = _ignoreApplicationEntitlementForImport) BOOL _ignoreApplicationEntitlementForImport;
> +@end
> +#endif

You should unconditionally import the header file inside the USE(APPLE_INTERNAL_SDK) block, because we actually want the build to fail if a header we expect in the SDK goes missing.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:122
> +    LOG_ERROR("_WKVideoFileUploadItem: Error creating thumbnail image for image: %@", file);

What's _WKVideoFileUploadItem?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:136
> +    RetainPtr<CGImageRef> imageRef = adoptCF([generator copyCGImageAtTime:kCMTimeZero actualTime:nil error:&error]);
> +    if (error) {

Nit: the documentation only promises that error will be non-nil if an error occurs, but it doesn't promise that it will be nil if one doesn't (although it probably always is). I think the right way to check for an error is to check if imageRef is NULL.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:137
> +        LOG_ERROR("_WKVideoFileUploadItem: Error creating image for video: %@", file);

What's _WKVideoFileUploadItem?

Also, it wouldn't hurt to include the error in the log message.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:159
> +    RetainPtr<CFStringRef> fileUTI = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (CFStringRef)fileExtension, 0));
> +
> +    if (UTTypeConformsTo(fileUTI.get(), kUTTypeImage))
> +        return iconForImageFile(file);
> +
> +    if (UTTypeConformsTo(fileUTI.get(), kUTTypeMovie))
> +        return iconForVideoFile(file);

Is it safe to call UTTypeConformsTo() when fileUTI is NULL?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:349
> +    if ([UIDocumentMenuViewController instancesRespondToSelector:@selector(_setIgnoreApplicationEntitlementForImport:)]) {
> +        [self _showDocumentPickerMenu];
> +        return;
> +    }

It seems strange to use a runtime check to decide whether we should show the old or the new picker. Can't we decide this at compile time based on __IPHONE_OS_VERSION_MIN_REQUIRED?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:389
> -- (NSArray *)_mediaTypesForPickerSourceType:(UIImagePickerControllerSourceType)sourceType
> +- (NSArray *)_UTIsForMIMETypes

This could be a free function: static NSArray *UTIsForMIMETypes(NSArray *mimeTypes)

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:417
> +- (NSArray *)_mediaTypesForDocumentPickerMenu

I don't feel strongly about this, but I don't love this name, since it reads like the method should take an argument. Could it just be called _mediaTypes?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:489
> +    _documentMenuController = adoptNS([UIDocumentMenuViewController alloc]);
> +    [_documentMenuController _setIgnoreApplicationEntitlementForImport:YES];
> +    [_documentMenuController initWithDocumentTypes:[self _mediaTypesForDocumentPickerMenu] inMode:UIDocumentPickerModeImport];
> +    [_documentMenuController setDelegate:self];

Sending a message to _documentMenuController before it's been initialized is really concerning. It's not safe to use an object before it's initialized.

[Jon Honeycutt]

(In reply to comment #6)
> Comment on attachment 248506 [details]
> WIP Take 2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248506&action=review
> 
> This looks pretty good to me, but I have enough concerns that I think
> another round is called for here.
> 
> > Source/WebKit2/Platform/spi/ios/UIKitSPI.h:78
> > +#if defined(__has_include) && __has_include(<UIKit/UIDocumentMenuViewController_Private.h>)
> > +#import <UIKit/UIDocumentMenuViewController_Private.h>
> > +#else
> > +@interface UIDocumentMenuViewController (Details)
> > +@property (nonatomic, assign, setter = _setIgnoreApplicationEntitlementForImport:, getter = _ignoreApplicationEntitlementForImport) BOOL _ignoreApplicationEntitlementForImport;
> > +@end
> > +#endif
> 
> You should unconditionally import the header file inside the
> USE(APPLE_INTERNAL_SDK) block, because we actually want the build to fail if
> a header we expect in the SDK goes missing.

This header doesn't exist yet. It will be part of the next UIKit submission. I'm hoping to land this patch before the header exists.


> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:122
> > +    LOG_ERROR("_WKVideoFileUploadItem: Error creating thumbnail image for image: %@", file);
> 
> What's _WKVideoFileUploadItem?

I moved this code from a _WKVideoFileUploadItem method. Will fix.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:136
> > +    RetainPtr<CGImageRef> imageRef = adoptCF([generator copyCGImageAtTime:kCMTimeZero actualTime:nil error:&error]);
> > +    if (error) {
> 
> Nit: the documentation only promises that error will be non-nil if an error
> occurs, but it doesn't promise that it will be nil if one doesn't (although
> it probably always is). I think the right way to check for an error is to
> check if imageRef is NULL.

I just moved this code, but I think it's fine to change this.


> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:137
> > +        LOG_ERROR("_WKVideoFileUploadItem: Error creating image for video: %@", file);
> 
> What's _WKVideoFileUploadItem?
> 
> Also, it wouldn't hurt to include the error in the log message.

More moved code. Will fix.


> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:159
> > +    RetainPtr<CFStringRef> fileUTI = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (CFStringRef)fileExtension, 0));
> > +
> > +    if (UTTypeConformsTo(fileUTI.get(), kUTTypeImage))
> > +        return iconForImageFile(file);
> > +
> > +    if (UTTypeConformsTo(fileUTI.get(), kUTTypeMovie))
> > +        return iconForVideoFile(file);
> 
> Is it safe to call UTTypeConformsTo() when fileUTI is NULL?

Yes.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:349
> > +    if ([UIDocumentMenuViewController instancesRespondToSelector:@selector(_setIgnoreApplicationEntitlementForImport:)]) {
> > +        [self _showDocumentPickerMenu];
> > +        return;
> > +    }
> 
> It seems strange to use a runtime check to decide whether we should show the
> old or the new picker. Can't we decide this at compile time based on
> __IPHONE_OS_VERSION_MIN_REQUIRED?

We need to ensure that this new SPI exists before trying to create a document picker menu. After the new UIKit is submitted, I'll be removing this runtime check.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:389
> > -- (NSArray *)_mediaTypesForPickerSourceType:(UIImagePickerControllerSourceType)sourceType
> > +- (NSArray *)_UTIsForMIMETypes
> 
> This could be a free function: static NSArray *UTIsForMIMETypes(NSArray
> *mimeTypes)

This operates on the _mimeTypes ivar. Do you think it's clearer to call a function than to call an instance method?

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:417
> > +- (NSArray *)_mediaTypesForDocumentPickerMenu
> 
> I don't feel strongly about this, but I don't love this name, since it reads
> like the method should take an argument. Could it just be called _mediaTypes?

Well, there's a -_mediaTypesForPickerSourceType:, so I want to make it clear that it's for the document picker. I think I'll rename this to -_documentPickerMenuMediaTypes.

> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:489
> > +    _documentMenuController = adoptNS([UIDocumentMenuViewController alloc]);
> > +    [_documentMenuController _setIgnoreApplicationEntitlementForImport:YES];
> > +    [_documentMenuController initWithDocumentTypes:[self _mediaTypesForDocumentPickerMenu] inMode:UIDocumentPickerModeImport];
> > +    [_documentMenuController setDelegate:self];
> 
> Sending a message to _documentMenuController before it's been initialized is
> really concerning. It's not safe to use an object before it's initialized.

Yes, but unfortunately, that's the way this SPI works; we need to set this flag to suppress an assertion that's in the initializer. <rdar://problem/20137692> covers adding a better SPI.

Thanks for the review! I'll post a new patch soon.

[Jon Honeycutt]

Created attachment 248571 [details]
Patch

[WebKit Commit Bot]

Attachment 248571 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:76:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:670:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

(In reply to comment #7)
> > Sending a message to _documentMenuController before it's been initialized is
> > really concerning. It's not safe to use an object before it's initialized.
> 
> Yes, but unfortunately, that's the way this SPI works; we need to set this
> flag to suppress an assertion that's in the initializer.
> <rdar://problem/20137692> covers adding a better SPI.

There's another case of this bizarre pattern in the swipe code, and I think it's best to always leave a comment because it is *really* surprising when you stumble upon it.

[Andy Estes]

Comment on attachment 248571 [details]
Patch

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

> Source/WebKit2/Platform/spi/ios/UIKitSPI.h:78
> +#if defined(__has_include) && __has_include(<UIKit/UIDocumentMenuViewController_Private.h>)
> +#import <UIKit/UIDocumentMenuViewController_Private.h>
> +#else
> +@interface UIDocumentMenuViewController (Details)
> +@property (nonatomic, assign, setter = _setIgnoreApplicationEntitlementForImport:, getter = _ignoreApplicationEntitlementForImport) BOOL _ignoreApplicationEntitlementForImport;
> +@end
> +#endif

Let's add a FIXME to remove the __has_include() once this header is in a build.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:349
> +    if ([UIDocumentMenuViewController instancesRespondToSelector:@selector(_setIgnoreApplicationEntitlementForImport:)]) {
> +        [self _showDocumentPickerMenu];
> +        return;
> +    }

Let's add a FIXME to remove this check once the right version of UIKit is in a build.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:489
> +    // FIXME: Support multiple file selection when implemented. <rdar://17177981>
> +    _documentMenuController = adoptNS([UIDocumentMenuViewController alloc]);
> +    [_documentMenuController _setIgnoreApplicationEntitlementForImport:YES];
> +    [_documentMenuController initWithDocumentTypes:[self _documentPickerMenuMediaTypes] inMode:UIDocumentPickerModeImport];
> +    [_documentMenuController setDelegate:self];

This is so bizarre that adding a comment mentioning <rdar://problem/20137692> would be helpful.

[Andy Estes]

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 248506 [details]
> > WIP Take 2
> >  
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:389
> > > -- (NSArray *)_mediaTypesForPickerSourceType:(UIImagePickerControllerSourceType)sourceType
> > > +- (NSArray *)_UTIsForMIMETypes
> > 
> > This could be a free function: static NSArray *UTIsForMIMETypes(NSArray
> > *mimeTypes)
> 
> This operates on the _mimeTypes ivar. Do you think it's clearer to call a
> function than to call an instance method?

I think it's just as clear to pass _mimeTypes as an argument to a helper, and doing so avoids a message send, but I don't feel strongly that you make this change.

[Jon Honeycutt]

Thanks for the comments and review!

Landed in <http://trac.webkit.org/changeset/181476>.