Bug 212485 – [macOS]: The File Picker of the <input> file element should show the selection filter

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

When clicking an <input> file element, we should show the selection filter. The selection filter should reflect what is in the "accept" attribute of the <input> element.

According to the specs: https://www.w3.org/TR/html51/sec-forms.html#file-upload-state-typefile, the "accept" attribute can only give the UA a hint of what file types will be accepted. So the selection filter should also allow changing the filter to "All files". Accepting files, which are not in the "accept" attribute, is what the drag/drop does. So the selecting files through the File Picker should be compatible with the dragging and dropping them.

[Radar WebKit Bug Importer]

<rdar://problem/63731665>

[Said Abou-Hallawa]

Created attachment 400991 [details]
Patch

[Said Abou-Hallawa]

Created attachment 400993 [details]
Patch

[Tim Horton]

Comment on attachment 400993 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:40
> +        NSBundle *bundle = [NSBundle bundleForClass:[NSURLFileTypeMappings class]];
> +        NSData *data = [NSData dataWithContentsOfFile:[bundle pathForResource:@"types" ofType:@"plist"]];

Is there not a way to do with with CoreServices API instead of loading some random plist out of some other project's bundle? This seems pretty crazy.

[Simon Fraser (smfr)]

Comment on attachment 400993 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:36
> +    static NSDictionary<NSString *, NSSet<NSString *> *> *mimeWildcardsToExtensionList;

This would be better as NeverDestroyed<RetainPtr<>>

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:40
>> +        NSData *data = [NSData dataWithContentsOfFile:[bundle pathForResource:@"types" ofType:@"plist"]];
> 
> Is there not a way to do with with CoreServices API instead of loading some random plist out of some other project's bundle? This seems pretty crazy.

Agreed.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:46
> +        NSDictionary<NSString *, NSArray<NSString *> *> *mimeTypeToExtensionList = [NSPropertyListSerialization propertyListWithData:data options:NSPropertyListImmutable format:nil error:nil];

Better to explicitly capture this in a RetainPtr<>

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:47
> +        NSMutableDictionary<NSString *, NSMutableSet<NSString *> *> *map = [NSMutableDictionary new];

This is leaked.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:121
> +    NSArray<NSString *> *acceptedMIMETypes = [self _acceptedMIMETypes];
> +    NSArray<NSString *> *acceptedFileExtensions = [self _acceptedFileExtensions];
> +
> +    NSMutableSet<NSString *> *allowedFileExtensions = [NSMutableSet set];
> +
> +    NSURLFileTypeMappings *fileTypeMappings = [NSURLFileTypeMappings sharedMappings];
> +    [acceptedMIMETypes enumerateObjectsUsingBlock:^(NSString *mimeType, NSUInteger index, BOOL* stop) {
> +        if ([mimeType hasSuffix:@"*"])
> +            [allowedFileExtensions unionSet:extensionsForMIMEWildcardType(mimeType)];
> +        else
> +            [allowedFileExtensions addObjectsFromArray:[fileTypeMappings extensionsForMIMEType:mimeType]];
> +    }];
> +
> +    NSMutableArray<NSString *> *whitelistedFileExtensions = [NSMutableArray array];
> +    [acceptedFileExtensions enumerateObjectsUsingBlock:^(NSString *extension, NSUInteger index, BOOL *stop) {
> +        ASSERT([extension characterAtIndex:0] == '.');
> +        [whitelistedFileExtensions addObject:[extension substringFromIndex:1]];
> +    }];
> +    
> +    [allowedFileExtensions addObjectsFromArray:whitelistedFileExtensions];
> +    return [allowedFileExtensions allObjects];

I would like to see more RetainPtr<> here.

[Said Abou-Hallawa]

Created attachment 401104 [details]
Patch

[Tim Horton]

Comment on attachment 401104 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/NSURLFileTypeMappingsSPI.h:38
> +    NSURLFileTypeMappingsInternal *_internal;

This is IPI, you really ought not do this. Also, I don't see it used?

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:40
> +        NSData *data = [NSData dataWithContentsOfFile:[bundle pathForResource:@"types" ofType:@"plist"]];

This is still here :(

[Tim Horton]

Comment on attachment 401104 [details]
Patch

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

> Source/WebCore/PAL/ChangeLog:9
> +        To get the bundle of NSURLFileTypeMappings, we need to expose the interface
> +        of NSURLFileTypeMappings.

This does not seem right. [NSBundle bundleForClass:NSClassFromString(@"NSURLFileTypeMappings")] would surely work without it.

That said, let's keep talking about ways to avoid this crazy.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:47
> +        RetainPtr<NSMutableDictionary<NSString *, NSMutableSet<NSString *> *>> map = [NSMutableDictionary new];

The original (mutable) copy of the map leaks too :)

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:88
> +        _allowedFileExtensions = [[NSMutableArray arrayWithArray:allowedFileExtensions] retain];

This still leaks

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:620
> +    NSTextField* label = [[NSTextField alloc] initWithFrame:CGRectZero];

This still leaks

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

This still leaks

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:631
> +    [button addItemWithTitle:@"All Files                          "];

The spaces :|

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:637
> +    NSStackView* horizontalView = [[NSStackView alloc] initWithFrame:CGRectZero];

This still leaks

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:643
> +    NSStackView* filterView = [[NSStackView alloc] initWithFrame:CGRectZero];

This still leaks

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:660
> +    FileExtensionsPopupTarget *popupTarget = [[FileExtensionsPopupTarget alloc] initWithOpenPanel: openPanel allowedFileExtensions: parameters._allowedFileExtensions];

This still leaks

[Said Abou-Hallawa]

Created attachment 401136 [details]
Patch

[Said Abou-Hallawa]

Created attachment 401198 [details]
Patch

[Tim Horton]

Comment on attachment 401198 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/UTIPrivSPI.h:30
> +#import <CoreServices/CoreServicesPriv.h>

We already have CoreServicesSPI.h, lets just use that

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:52
> +            NSString *mimeTypeWildcard = [[mimeType componentsSeparatedByString:@"/"][0] stringByAppendingString:@"/*"];
> +            if (!map[mimeTypeWildcard])
> +                map[mimeTypeWildcard] = [NSMutableSet set];
> +            [map[mimeTypeWildcard] addObject:extension];

You can inline this right into the only callsite. The only reason I had it in a block in the test app was to share code between the different experiments :D

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:57
> +            NSString *mimeType = CFBridgingRelease(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)(uti), kUTTagClassMIMEType));

Let's not use CFBridgingRelease in non-ARC code. adoptCF and RetainPtr, instead.

[Said Abou-Hallawa]

Comment on attachment 401104 [details]
Patch

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

>> Source/WebCore/PAL/ChangeLog:9
>> +        of NSURLFileTypeMappings.
> 
> This does not seem right. [NSBundle bundleForClass:NSClassFromString(@"NSURLFileTypeMappings")] would surely work without it.
> 
> That said, let's keep talking about ways to avoid this crazy.

This was fixed with the help of Tim Horton. The SPI _UTCopyDeclaredTypeIdentifiers() can be used to enumerate all the type identifiers, aka UITs. The mime type of the UTI can be gotten form UTTypeCopyPreferredTagWithClass(..., kUTTagClassMIMEType). The extensions of the UTI can be gotten form UTTypeCopyAllTagsWithClass(..., kUTTagClassFilenameExtension). From the mime type and the extensions a hash map can be built tor mapping the wildcard mime type to file extensions.

>> Source/WebCore/PAL/pal/spi/cocoa/NSURLFileTypeMappingsSPI.h:38
>> +    NSURLFileTypeMappingsInternal *_internal;
> 
> This is IPI, you really ought not do this. Also, I don't see it used?

The changes in this header file were reverted.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:40
>> +        NSData *data = [NSData dataWithContentsOfFile:[bundle pathForResource:@"types" ofType:@"plist"]];
> 
> This is still here :(

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:47
>> +        RetainPtr<NSMutableDictionary<NSString *, NSMutableSet<NSString *> *>> map = [NSMutableDictionary new];
> 
> The original (mutable) copy of the map leaks too :)

Fixed. The original map is now created by calling [NSMutableDictionary dictionary].

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:88
>> +        _allowedFileExtensions = [[NSMutableArray arrayWithArray:allowedFileExtensions] retain];
> 
> This still leaks

This NSArray is now released in the dealloc() of this interface.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:620
>> +    NSTextField* label = [[NSTextField alloc] initWithFrame:CGRectZero];
> 
> This still leaks

This object is now 'autorelease'.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:628
>> +    NSPopUpButton *button = [[NSPopUpButton alloc] initWithFrame:CGRectZero pullsDown:NO];
> 
> This still leaks

Ditto.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:631
>> +    [button addItemWithTitle:@"All Files                          "];
> 
> The spaces :|

I wanted to increase the width of the NSPopupButton but I could not figure it out how I can do it with auto-layout. So adding spaces was just a workaround till I find the right way to do.

Also I could not constrain the width of the NSTextField above to fit its contents. Currently NSTextField and NSPopupButton have the same width which is not what we want.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:637
>> +    NSStackView* horizontalView = [[NSStackView alloc] initWithFrame:CGRectZero];
> 
> This still leaks

Ditto.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:660
>> +    FileExtensionsPopupTarget *popupTarget = [[FileExtensionsPopupTarget alloc] initWithOpenPanel: openPanel allowedFileExtensions: parameters._allowedFileExtensions];
> 
> This still leaks

This variable is made a member of WK2BrowserWindowController so it can live after runOpenPanelWithParameters() exists while the OpenPanel is still shown.

[Said Abou-Hallawa]

Created attachment 401206 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 401198 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/cocoa/UTIPrivSPI.h:30
>> +#import <CoreServices/CoreServicesPriv.h>
> 
> We already have CoreServicesSPI.h, lets just use that

The new file was removed. The new SPI was added to CoreServicesSPI.h.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:52
>> +            [map[mimeTypeWildcard] addObject:extension];
> 
> You can inline this right into the only callsite. The only reason I had it in a block in the test app was to share code between the different experiments :D

Done.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:57
>> +            NSString *mimeType = CFBridgingRelease(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)(uti), kUTTagClassMIMEType));
> 
> Let's not use CFBridgingRelease in non-ARC code. adoptCF and RetainPtr, instead.

Done. But I left the one in extensionsForMIMEType(). I am not sure what the right thing here though.

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #14)
> Comment on attachment 401198 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401198&action=review
> >> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:57
> >> +            NSString *mimeType = CFBridgingRelease(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)(uti), kUTTagClassMIMEType));
> > 
> > Let's not use CFBridgingRelease in non-ARC code. adoptCF and RetainPtr, instead.
> 
> Done. But I left the one in extensionsForMIMEType(). I am not sure what the
> right thing here though.

I meant for that review comment to apply to all instances.

[Said Abou-Hallawa]

Created attachment 401241 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 401198 [details]
Patch

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

>>>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:57
>>>> +            NSString *mimeType = CFBridgingRelease(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)(uti), kUTTagClassMIMEType));
>>> 
>>> Let's not use CFBridgingRelease in non-ARC code. adoptCF and RetainPtr, instead.
>> 
>> Done. But I left the one in extensionsForMIMEType(). I am not sure what the right thing here though.
> 
> I meant for that review comment to apply to all instances.

I made a single function which returns the extensions for any mime type including the wildcard mime types. So the extensionsForMIMEType() above was deleted and extensionsForWildcardMIMEType() was renamed to extensionsForMIMEType().

[Said Abou-Hallawa]

Created attachment 401407 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 401407 [details]
Patch

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

> 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.

[Darin Adler]

Comment on attachment 401407 [details]
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?

[Simon Fraser (smfr)]

Comment on attachment 401407 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:75
>> +        }];
> 
> 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");

Please do not land this patch with "whitelist". We are actively trying to eliminate blacklist/whitelist terminology.

[Said Abou-Hallawa]

Comment on attachment 401407 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:37
>> +    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.

Fixed. I used 

    static auto extensionsForMIMETypeMap = makeNeverDestroyed([] {
        ...
    };

    return extensionsForMIMETypeMap.get().get();

>> 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]);

Done.

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

Done.

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

Done.

>> 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();

I am not sure how this should work. adopt(...).get() will return the raw pointer. But once the RetainedPtr is released this raw pointer will be released. Should we use leakRef() instead? And how will mimeTypeToExtensions[mimeType] be released eventually? Or because it is stored in a NeverDestroyed, it is okay?

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

Done.

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

Done.

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

Done.

>>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:75
>>> +        }];
>> 
>> 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");
> 
> Please do not land this patch with "whitelist". We are actively trying to eliminate blacklist/whitelist terminology.

Fixed. I used

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

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

Done.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:122
>> +    NSMutableSet<NSString *> *allowedFileExtensions = [NSMutableSet set];
> 
> I suggest auto and adoptNS here to avoid autorelease.

Done.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:129
>> +    NSMutableArray<NSString *> *additionalAllowedFileExtensions = [NSMutableArray array];
> 
> I suggest auto and adoptNS here to avoid autorelease.

Done.

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

Done.

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

Done.

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

No it should be localized. But because there are expected changes in this function, I want to postpone the localization step till I finish these changes. For mini browser, which is the only caller for these functions it's okay because mini browser is not localized. Before these functions are called by Safari, I will make sure all the strings are localized.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:78
>> +    NSOpenPanel *_openPanel;
> 
> What guarantees this object will not be retained and outlive the open panel?

This object is released in the dealloc WK2BrowserWindowController. It is also released before it is assigned a new value in runOpenPanelWithParameters.

This object uses _openPanel only when responding to the selection change in the NSPopupButton. NSPopupButton will keep sending selection change as long as OpenPanel is still alive.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:79
>> +    NSArray<NSString *> *_allowedFileExtensions;
> 
> Why not use RetainPtr for this?

Because it is .m file.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:124
>> +    FileExtensionsPopupTarget *_fileExtensionsPopupTarget;
> 
> Why not use RetainPtr for this?

Ditto.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:630
>> +    NSTextField* label = [[[NSTextField alloc] initWithFrame:NSZeroRect] autorelease];
> 
> I suggest using adoptNS instead of autorelease.

Ditto.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:652
>> +    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.

This is .m file. I can change it to .mm file but I would prefer to do this in a separate patch.

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:659
>> +    buttonFrame.size = NSMakeSize(MAX(NSWidth(buttonFrame), minButtonWidth), NSHeight(buttonFrame));
> 
> Could use std::max instead of MAX.

This is .m file. I could not include the std header file <algorithm>.

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

I meant to use 'spacing'. Fixed.

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

Mini browser is not localized.

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

The ownership of the FilterView is moved to the NSOpenPanel, see https://developer.apple.com/documentation/appkit/nssavepanel/1525544-accessoryview.

[Said Abou-Hallawa]

Created attachment 401619 [details]
Patch

[EWS]

Committed r262895: <https://trac.webkit.org/changeset/262895>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401619 [details].

[Darin Adler]

Comment on attachment 401407 [details]
Patch

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

>>> 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();
> 
> I am not sure how this should work. adopt(...).get() will return the raw pointer. But once the RetainedPtr is released this raw pointer will be released. Should we use leakRef() instead? And how will mimeTypeToExtensions[mimeType] be released eventually? Or because it is stored in a NeverDestroyed, it is okay?

The dictionary retains the value stored in it (the set). The RetainPtr is destroyed only at the end of the full expression, after the set is retained by the dictionary. It does release the set, which is what we want.

[Darin Adler]

Comment on attachment 401407 [details]
Patch

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

>>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:78
>>> +    NSOpenPanel *_openPanel;
>> 
>> What guarantees this object will not be retained and outlive the open panel?
> 
> This object is released in the dealloc WK2BrowserWindowController. It is also released before it is assigned a new value in runOpenPanelWithParameters.
> 
> This object uses _openPanel only when responding to the selection change in the NSPopupButton. NSPopupButton will keep sending selection change as long as OpenPanel is still alive.

That first line isn’t an answer. There’s a common misunderstanding that "releasing an object" deallocates the object. As I know you are aware, it’s not guaranteed that an object released will be deallocated at just that moment. Maybe there’s some reason we know nothing else will ever retain this, but that seems a fragile assumption.

The second line is the reason this is safe, but it’s the kind of high level reasoning that doesn’t seem strong enough to prevent object lifetime bugs when later when we do refactoring or we have small misunderstandings.

I suppose this code is OK, but it seems a little risky.

>>> 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?
> 
> The ownership of the FilterView is moved to the NSOpenPanel, see https://developer.apple.com/documentation/appkit/nssavepanel/1525544-accessoryview.

OK, but that doesn’t answer my question. The setAccessoryView: method is going to retain the filterView, and yes, it takes ownership of it.

But my question was whether we need to release the filterView or not. I see now that we don’t because createFilterView returns an autoreleased object. So it should not be named "create" or "copy".