Summary: | Adopt UIContextMenu for WKFileUploadPanel | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, thorton, wenson_hsieh | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 213314 | ||||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2020-03-05 19:11:28 PST
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
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. |