[webkit-reviews] review granted: [Bug 212485] [macOS]: The File Picker of the <input> file element should show the selection filter : [Attachment 401407] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 212485: [macOS]: The File Picker of the <input> file element should show
the selection filter
https://bugs.webkit.org/show_bug.cgi?id=212485

Attachment 401407: Patch

https://bugs.webkit.org/attachment.cgi?id=401407&action=review




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 401407
  --> https://bugs.webkit.org/attachment.cgi?id=401407
Patch

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

Looks OK. Where are the tests for this new code?

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:37
> +    static NeverDestroyed<RetainPtr<NSDictionary<NSString *, NSSet<NSString
*> *>>> s_mimeTypeToExtensions;
> +    static dispatch_once_t onceToken;

Do we need a thread-safe one-time mechanism here? If so, why choose
dispatch_once of std::call_once? If not, can we just write this with normal C++
initialization idioms?

    static NSDictionary<NSString *, NSSet<NSString *> *map =
createMIMETypeToExtensionsMap().leakRef();

Or use a block if you prefer.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:40
> +	   NSMutableDictionary<NSString *, NSMutableSet<NSString *> *>
*mimeTypeToExtensions = [NSMutableDictionary dictionary];

I suggest avoiding autorelease here:

    auto mimeTypeToExtensions = adoptNS([[NSMutableDictionary alloc] init]);

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:41
> +	   RetainPtr<NSArray> allUTIs =
adoptCF(_UTCopyDeclaredTypeIdentifiers());

I suggest using auto for code like this. Also I am surprised we can just assign
from a RetainPtr<CFArrayRef> into a RetainPtr<NSArray> without a cast. Does
this really compile?

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:43
> +	   void (^addExtensionForMIMEType)(NSString *mimeType, NSString
*extension) = ^(NSString *mimeType, NSString *extension) {

And here.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:45
> +		   mimeTypeToExtensions[mimeType] = [NSMutableSet set];

It’s a little bit unfortunate to autorelease the set here. Could write this so
autorelease is not involved:

    mimeTypeToExtensions[mimeType] = adoptNS([[NSMutableSet alloc]
init]).get();

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:49
> +	   void (^addExtensionsForMIMEType)(NSString *mimeType,
NSArray<NSString *> *extensions) = ^(NSString *mimeType, NSArray<NSString *>
*extensions) {

And here.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:61
> +	       RetainPtr<NSString> mimeType =
adoptCF(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)(uti),
kUTTagClassMIMEType));

And here.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:64
> +	       RetainPtr<NSArray> extensions =
adoptCF(UTTypeCopyAllTagsWithClass((__bridge CFStringRef)(uti),
kUTTagClassFilenameExtension));

And here.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:75
> +	   RetainPtr<NSDictionary<NSString *, NSArray<NSString *> *>>
whitelistedMimeTypes = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:
> +	       [NSArray arrayWithObjects: @"webp" , nil], @"image/webp",
> +	       nil
> +	   ]);
> +
> +	   [whitelistedMimeTypes.get()
enumerateKeysAndObjectsUsingBlock:^(NSString *mimeType, NSArray<NSString *>
*extensions, BOOL *) {
> +	       addExtensionsForMIMEType(mimeType, extensions);
> +	   }];

And here. Also I suggest not using the term "whitelist". Also using literals
makes this way nicer:

    auto allowedMIMETypes = @{ @"image/webp": @[ @"webp" ] };

When it’s written this way, it becomes pretty clear that "allowedMIMETypes" is
a strange name for this dictionary, and also that it’s strange to use a
dictionary. Why do we need to make a dictionary and enumerate it? Why not just
write this one line of code:

    addExtensionForMIMEType(@"image/webp", @"webp");

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:120
> +    NSArray<NSString *> *acceptedMIMETypes = [self _acceptedMIMETypes];
> +    NSArray<NSString *> *acceptedFileExtensions = [self
_acceptedFileExtensions];

I suggest auto here.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:122
> +    NSMutableSet<NSString *> *allowedFileExtensions = [NSMutableSet set];

I suggest auto and adoptNS here to avoid autorelease.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:129
> +    NSMutableArray<NSString *> *additionalAllowedFileExtensions =
[NSMutableArray array];

I suggest auto and adoptNS here to avoid autorelease.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:142
> +    NSArray<NSString *> *acceptedMIMETypes = [self _acceptedMIMETypes];
> +    NSArray<NSString *> *acceptedFileExtensions = [self
_acceptedFileExtensions];

auto

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:148
> +	   return @"Custom Files";

Maybe a named constant for this string. Is it OK that this is not localized?

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:160
> +	   return [NSString stringWithFormat:@"%@%@ Files", [[mimeTypePrefix
substringToIndex:1] uppercaseString], [mimeTypePrefix substringFromIndex:1]];

Is it OK that this is not localized?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:78
> +    NSOpenPanel *_openPanel;

What guarantees this object will not be retained and outlive the open panel?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:79
> +    NSArray<NSString *> *_allowedFileExtensions;

Why not use RetainPtr for this?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:124
> +    FileExtensionsPopupTarget *_fileExtensionsPopupTarget;

Why not use RetainPtr for this?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:630
> +    NSTextField* label = [[[NSTextField alloc] initWithFrame:NSZeroRect]
autorelease];

I suggest using adoptNS instead of autorelease.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:638
> +    NSPopUpButton *button = [[[NSPopUpButton alloc] initWithFrame:NSZeroRect
pullsDown:NO] autorelease];

Ditto.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:646
> +    NSView* filterView = [[[NSView alloc] initWithFrame:NSZeroRect]
autorelease];

Ditto.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:652
> +    const CGFloat margin = 20;
> +    const CGFloat spacing = 2;
> +    const CGFloat minButtonWidth = 230;

constexpr is better than const for this use.

Also would be helpful to have a comment saying where these values come from.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:659
> +    buttonFrame.size = NSMakeSize(MAX(NSWidth(buttonFrame), minButtonWidth),
NSHeight(buttonFrame));

Could use std::max instead of MAX.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:670
> +    buttonFrame.origin = NSMakePoint(NSMaxX(labelFrame) + 2,
(NSHeight(filterViewFrame) - NSHeight(buttonFrame)) / 2);

Where does the magic number in the "+ 2" come from? Why is it correct?

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:693
>> +	    titles = @[@"All Files"];
> 
> I removed the extra spaces, which I used as a hack to increase the width of
the NSPopupButton, and I fixed the layout of the controls.

Is it OK that the string "All Files" is not localized? If so, why?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:695
> +    NSView *filterView = [self createFilterView:titles
popupTarget:_fileExtensionsPopupTarget];

This method name has "create" in it. Does that mean we need to adopt or release
its return value? Or does it just happen to have a name that violates the Cocoa
copy/create ownership rule?