Bug 208687 - Adopt UIContextMenu for WKFileUploadPanel
Summary: Adopt UIContextMenu for WKFileUploadPanel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks: 213314
  Show dependency treegraph
 
Reported: 2020-03-05 19:11 PST by Megan Gardner
Modified: 2020-06-17 13:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (16.23 KB, patch)
2020-03-06 12:53 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (16.42 KB, patch)
2020-03-06 13:23 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (16.72 KB, patch)
2020-03-06 13:42 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (17.28 KB, patch)
2020-03-06 15:09 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (17.26 KB, patch)
2020-03-06 15:23 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (17.94 KB, patch)
2020-03-07 13:45 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (18.10 KB, patch)
2020-03-07 14:00 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.40 KB, patch)
2020-03-07 14:26 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (19.39 KB, patch)
2020-03-07 18:37 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-03-05 19:11:28 PST
Adopt UIContextMenu
Comment 1 Megan Gardner 2020-03-06 12:53:15 PST
Created attachment 392758 [details]
Patch
Comment 2 Megan Gardner 2020-03-06 13:23:26 PST
Created attachment 392764 [details]
Patch
Comment 3 Megan Gardner 2020-03-06 13:42:47 PST
Created attachment 392767 [details]
Patch
Comment 4 Tim Horton 2020-03-06 14:01:24 PST
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?)
Comment 5 Megan Gardner 2020-03-06 15:09:36 PST
Created attachment 392781 [details]
Patch
Comment 6 Megan Gardner 2020-03-06 15:23:54 PST
Created attachment 392787 [details]
Patch
Comment 7 Tim Horton 2020-03-07 11:20:38 PST
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.
Comment 8 Megan Gardner 2020-03-07 13:45:16 PST
Created attachment 392889 [details]
Patch
Comment 9 Tim Horton 2020-03-07 13:49:27 PST
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?
Comment 10 Megan Gardner 2020-03-07 14:00:07 PST
Created attachment 392896 [details]
Patch
Comment 11 Tim Horton 2020-03-07 14:13:51 PST
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?
Comment 12 Megan Gardner 2020-03-07 14:26:14 PST
Created attachment 392898 [details]
Patch
Comment 13 Megan Gardner 2020-03-07 18:37:19 PST
Created attachment 392926 [details]
Patch for landing
Comment 14 Megan Gardner 2020-03-07 18:38:51 PST
<rdar://problem/59358002>
Comment 15 WebKit Commit Bot 2020-03-07 19:40:57 PST
The commit-queue encountered the following flaky tests while processing attachment 392926 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 160571 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2020-03-07 19:41:31 PST
Comment on attachment 392926 [details]
Patch for landing

Clearing flags on attachment: 392926

Committed r258092: <https://trac.webkit.org/changeset/258092>
Comment 17 WebKit Commit Bot 2020-03-07 19:41:32 PST
All reviewed patches have been landed.  Closing bug.