Bug 208687

Created attachment 392758 [details] Patch

Created attachment 392764 [details] Patch

Created attachment 392767 [details] Patch

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?)

Created attachment 392781 [details] Patch

Created attachment 392787 [details] Patch

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.

Created attachment 392889 [details] Patch

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?

Created attachment 392896 [details] Patch

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?

Created attachment 392898 [details] Patch

Created attachment 392926 [details] Patch for landing

<rdar://problem/59358002>

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 on attachment 392926 [details] Patch for landing Clearing flags on attachment: 392926 Committed r258092: <https://trac.webkit.org/changeset/258092>

All reviewed patches have been landed. Closing bug.