Bug 208687 – Adopt UIContextMenu for WKFileUploadPanel

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Adopt UIContextMenu

[Megan Gardner]

Created attachment 392758 [details]
Patch

[Megan Gardner]

Created attachment 392764 [details]
Patch

[Megan Gardner]

Created attachment 392767 [details]
Patch

[Tim Horton]

Comment on attachment 392767 [details]
Patch

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

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:1100
> +typedef NS_ENUM(NSUInteger, _UIContextMenuLayout) {

You can limit this to only the values you need, as long as you specify their values explicitly

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:279
> +    [[UIViewController _viewControllerForFullScreenPresentationFromView:_view.get().get()] dismissViewControllerAnimated:NO completion:nil];

most (all?) of your .get().get()s should be .getAutoreleased()

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:421
> +        NSArray* actions;
> +        NSArray *mediaTypesBlock = UTIsForMIMETypes(_mimeTypes.get()).allObjects;

Stars on the right!

What is "mediaTypesBlock"? Also, it's not a block.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:423
> +        BOOL containsImageMediaTypeBlock = !mediaTypesBlock.count || arrayContainsUTIThatConformsTo(mediaTypesBlock, kUTTypeImage);

Contains a block? What is happening?

Also, why does it contain it if there aren't any (if count is 0)? Seems like a misleading name

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:427
> +        UIAction *browseAction = [UIAction actionWithTitle:[strongSelf _browseFilesButtonLabel] image:[UIImage systemImageNamed:@"ellipsis"] identifier:@"browse" handler:^(__kindof UIAction *action) {

If you add a null-check-and-early-return after creating strongSelf, you can just use 'self' in all these places; strongSelf is ensuring it's alive, so self will remain valid.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:448
> +    return [UIContextMenuConfiguration configurationWithIdentifier:@"file" previewProvider:nil actionProvider:actionMenuProvider];

Is this identifier important in any way? If it is, this. seems like a poor choice (should it have a WK prefix? or be the class name? or ... what do we do in other places?)

[Megan Gardner]

Created attachment 392781 [details]
Patch

[Megan Gardner]

Created attachment 392787 [details]
Patch

[Tim Horton]

Comment on attachment 392787 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:454
> +- (void)_initInteraction

Do not love "init". But also I think this will go away.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:457
> +    if (!_documentMenuInteraction) {
> +        _documentMenuInteraction = adoptNS([[UIContextMenuInteraction alloc] initWithDelegate:self]);

I think this will cause problems (having two UIContextMenuInteractions installed on the same view without more care) and so I think you should do the same workaround you did for DD, allowing you to just use the main UIContextMenuInteraction.

[Megan Gardner]

Created attachment 392889 [details]
Patch

[Tim Horton]

Comment on attachment 392889 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:457
> +        [self _cancel];

Why cancel?

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:465
> +- (void)_removeInteraction
> +{
> +    if (_documentMenuInteraction)
> +        [_view removeInteraction:_documentMenuInteraction.get()];
> +}

No need for all the underscores in method names in this file, this class is not SPI/API.

Also, do you want to nil it out here? Is it OK to remove it twice?

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:467
> +- (void)_initInteraction

As previously mentioned, this is not a great name. Maybe ensureContextMenuInteraction?

[Megan Gardner]

Created attachment 392896 [details]
Patch

[Tim Horton]

Comment on attachment 392896 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:427
> +        if (!strongSelf)
> +            return [UIMenu menuWithTitle:@"" children:@[]];

it is valid to return a nil menu, right? Wouldn't that be more appropriate?

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:477
> +- (void)_showFilePickerMenu

The underscores!

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:488
> +- (void)_showDocumentPickerMenu

Again!

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:493
> +    BOOL containsImageMediaType = !mediaTypes.count || arrayContainsUTIThatConformsTo(mediaTypes, kUTTypeImage);
> +    BOOL containsVideoMediaType = !mediaTypes.count || arrayContainsUTIThatConformsTo(mediaTypes, kUTTypeMovie);

Is this "contains", or maybe "allows"?

These are unused on Catalyst, maybe move them inside the #else?

[Megan Gardner]

Created attachment 392898 [details]
Patch

[Megan Gardner]

Created attachment 392926 [details]
Patch for landing

[Megan Gardner]

<rdar://problem/59358002>

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 392926 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 160571 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 392926 [details]
Patch for landing

Clearing flags on attachment: 392926

Committed r258092: <https://trac.webkit.org/changeset/258092>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.