Bug 190454

Created attachment 352007 [details] Patch

*** Bug 178821 has been marked as a duplicate of this bug. ***

<rdar://problem/32840877>

Comment on attachment 352007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352007&action=review This doesn't seem like the right approach. > Source/WebCore/page/Settings.yaml:587 > +imageFormatWhitelist: > + type: String > + defaultValue: defaultImageFormatWhitelist() It's not clear to me that the whitelist should be a setting. > Source/WebCore/platform/graphics/Image.h:203 > + String m_imageFormatWhitelist; Why does every image have to store the whitelist? The whitelist should live in some object that gets consulted (next to MimeTypeRegistery or something).

Comment on attachment 352007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352007&action=review > Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:81 > +@property (nonatomic, setter=_setImageFormatWhitelist:) NSString *_imageFormatWhitelist WK_API_AVAILABLE(macosx(10.14), ios(12.0)); Nit - TBA?

Created attachment 352252 [details] Patch

I have these open questions: 1. Is the SPI name AdditionalAllowedImageUTIs() appropriate? 2. Should the allowed formats be passed as UTIs or mime types? 3. It is okay to pass the image formats as one string? Or do we really have to have the allowed formats be passed as an array of strings? In WebPreferencesStore, the value of the preference is stored as this variant. using Value = Variant<String, bool, uint32_t, double>; If we need to pass it as an array on strings, I will need to add a new type to this Variant and add the necessary encoding and decoding. But this can be done in a separate patch before or after landing this patch.

(In reply to Said Abou-Hallawa from comment #7) > I have these open questions: > > 1. Is the SPI name AdditionalAllowedImageUTIs() appropriate? Doesn't seem crazy. Not sure about the "UTIs" part. > 2. Should the allowed formats be passed as UTIs or mime types? This is Cocoa-y API so probably UTIs. Also didn't you say that we can't represent all of the desired types as MIME types? > 3. It is okay to pass the image formats as one string? Or do we really have > to have the allowed formats be passed as an array of strings? Definitely think the SPI should take `NSArray <NSString *> *`. Weird delimited strings are bad API design. > In WebPreferencesStore, the value of the preference is stored as this > variant. > > using Value = Variant<String, bool, uint32_t, double>; > > If we need to pass it as an array on strings, I will need to add a new type > to this Variant and add the necessary encoding and decoding. But this can be > done in a separate patch before or after landing this patch.

Comment on attachment 352252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352252&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104 > +@property (nonatomic, setter=_setAdditionalAllowedImageUTIs:) NSString *_additionalAllowedImageUTIs; Missing availability macros. Also strings need to be `copy` because they can be secretly mutable. And actually you do copy, you just don't say it here. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:214 > +#include <WebCore/UTIRegistry.h> Why? > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203 > + 5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */, Why's the TGA a .png?

Comment on attachment 352252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352252&action=review > Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:73 > +static String& additionalAllowedImageUTIs() The name of this function reads like it returns a collection because you use the pluralized acronym form of Uniform Type Identifier, UTIs. But this function only returns a single String and the rest of the patch talks in pluralities though is singular in nature. I would prefer that the solution we come up with is to allow more than one UTI to added to the list of UTIs to recognize rather than making all name singular.

Created attachment 352281 [details] Patch

Comment on attachment 352252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352252&action=review >> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:73 >> +static String& additionalAllowedImageUTIs() > > The name of this function reads like it returns a collection because you use the pluralized acronym form of Uniform Type Identifier, UTIs. But this function only returns a single String and the rest of the patch talks in pluralities though is singular in nature. I would prefer that the solution we come up with is to allow more than one UTI to added to the list of UTIs to recognize rather than making all name singular. I changed the return type to be Vector<String> which contains the UTIs of the additional allowed image formats. >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104 >> +@property (nonatomic, setter=_setAdditionalAllowedImageUTIs:) NSString *_additionalAllowedImageUTIs; > > Missing availability macros. Also strings need to be `copy` because they can be secretly mutable. And actually you do copy, you just don't say it here. I added the (copy) attribute to the definition of the property. I added also availability macros: WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA); >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:214 >> +#include <WebCore/UTIRegistry.h> > > Why? Because I need to call WebCore::setAdditionalAllowedImageFormats. >> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203 >> + 5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */, > > Why's the TGA a .png? I did this trick to make WebKit open the image file as an ImageDocument. WebKit does not open files with TGA extension as ImageDocument. So I changed the file extension to "png" with keeping the data file as is. The alternative solution is to open the image through an html file which contains the markup "<img src='tga-image.tga'>"

Created attachment 352282 [details] Patch

Created attachment 352301 [details] Patch

Created attachment 352302 [details] Patch

Comment on attachment 352302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352302&action=review We talked about how to get rid of all the preference code. > Source/WebKit/Platform/mac/StringUtilities.mm:90 > + [nsStringArray enumerateObjectsUsingBlock:[&] (NSString *nsString, NSUInteger, BOOL *) { > + stringList.append(String(nsString)); > + }]; Why not just: for (NSString *string in nsStringArray) strings.append(string); And get rid of StringList. You probably also want uncheckedAppend/reserveInitialCapacity. You might even be able to find one of these somewhere already.

(In reply to Said Abou-Hallawa from comment #12) > Comment on attachment 352252 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352252&action=review > >> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203 > >> + 5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */, > > > > Why's the TGA a .png? > > I did this trick to make WebKit open the image file as an ImageDocument. > WebKit does not open files with TGA extension as ImageDocument. So I changed > the file extension to "png" with keeping the data file as is. The > alternative solution is to open the image through an html file which > contains the markup "<img src='tga-image.tga'>" This is weird. If I'm allowing TGA, why aren't I allowed to ImageDocument it?

Created attachment 352392 [details] Patch

Created attachment 352818 [details] Patch

Comment on attachment 352818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352818&action=review > Source/WebCore/platform/MIMETypeRegistry.cpp:96 > + for (auto& uti : additionalAllowedImageFormats()) { Are you just getting lucky that this isn't called before you initialize additionalAllowedImageFormats()? Otherwise they'll be missing here, right? Can you add some assertions that that's the case (maybe in the setter assert that we haven't already done initializeSupportedImageMIMETypes)? Or make it invalidate the lists. Or dynamically add things. > Source/WebKit/Platform/mac/StringUtilities.mm:85 > +Vector<String> webCoreStringListFromNsStringArray(NSArray<NSString *> *nsStringArray) NS should be capitalized. Also I swear we have a version of this somewhere already. Also Why "list" instead of "vector".

Created attachment 352835 [details] Patch

Comment on attachment 352818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352818&action=review >> Source/WebCore/platform/MIMETypeRegistry.cpp:96 >> + for (auto& uti : additionalAllowedImageFormats()) { > > Are you just getting lucky that this isn't called before you initialize additionalAllowedImageFormats()? Otherwise they'll be missing here, right? > > Can you add some assertions that that's the case (maybe in the setter assert that we haven't already done initializeSupportedImageMIMETypes)? > > Or make it invalidate the lists. Or dynamically add things. I dynamically add the mime types of the additionalAllowedImageFormats once they are set. But I assume they won't change even if multiple WebPages are created. >> Source/WebKit/Platform/mac/StringUtilities.mm:85 >> +Vector<String> webCoreStringListFromNsStringArray(NSArray<NSString *> *nsStringArray) > > NS should be capitalized. Also I swear we have a version of this somewhere already. > > Also Why "list" instead of "vector". Done. I could not find an implementation for this conversions. And I found this code in TestControllerCocoa.mm [globalWebViewConfiguration.websiteDataStore _getAllStorageAccessEntriesFor:parentView->platformView() completionHandler:^(NSArray<NSString *> *nsDomains) { Vector<String> domains; domains.reserveInitialCapacity(nsDomains.count); for (NSString *domain : nsDomains) domains.uncheckedAppend(domain); m_currentInvocation->didReceiveAllStorageAccessEntries(domains); }];

Created attachment 352840 [details] Patch

Created attachment 352845 [details] Patch

Comment on attachment 352845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352845&action=review Seems mostly OK but let's see one more version. > Source/WebCore/platform/MIMETypeRegistry.cpp:61 > +static HashSet<String, ASCIICaseInsensitiveHash>* supportedImageAdditionalMIMETypes; This doesn't read well. Maybe supportedAdditionalImageMIMETypes (though the "supported" is then confusing). > Source/WebCore/platform/MIMETypeRegistry.cpp:194 > + supportedImageAdditionalMIMETypes = new HashSet<String, ASCIICaseInsensitiveHash>; The modern way to do all of these would be with a static NeverDestroyed<> and a std::once in the caller. > Source/WebCore/platform/MIMETypeRegistry.cpp:683 > + // Reinitialize the supportedImageAdditionalMIMETypes if they have not been set or were set to an empty list. > + if (!supportedImageAdditionalMIMETypes || !supportedImageAdditionalMIMETypes->isEmpty()) > + return; > + initializeSupportedImageAdditionalMIMETypes(); Why doesn't this just null out supportedImageAdditionalMIMETypes so that the next call triggers a rebuild? The function can be called supportedImageAdditionalMIMETypesChanged(). > Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:88 > + // We are suppoed to set the additionalAllowedImageFormats only once even > + // if multiple WebPages are created. It's not clear if this comment is describing a bug (in which case it should be a FIXME) or explaining the code. > Source/WebCore/platform/graphics/cg/UTIRegistry.h:36 > +const HashSet<String>& defaultAllowedImageFormats(); > +const HashSet<String>& additionalAllowedImageFormats(); > +WEBCORE_EXPORT void setAdditionalAllowedImageFormats(const Vector<String>&); > +bool isAllowedImageFormat(const String&); The rename from "UTI" to "format" makes it less clear that these return UTIs. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104 > +@property (nonatomic, copy, setter=_setAdditionalAllowedImageFormats:) NSArray<NSString *> *_additionalAllowedImageFormats WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); We might want to think about this naming. Does the caller of the API understand the "additional" part? > Tools/TestWebKitAPI/Tests/WebKitCocoa/AdditionalAllowedImageFormats.mm:40 > + [configuration _setAdditionalAllowedImageFormats:@[@"com.truevision.tga-image"]]; Tests should also cover * adding a format that is already supported * adding a format that webkit can't render * passing an array with the same format listed twice

Created attachment 352908 [details] Patch

Created attachment 352924 [details] Patch

Comment on attachment 352924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352924&action=review > Source/WebCore/platform/MIMETypeRegistry.cpp:62 > + static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> supportedImageMIMETypes; > + static std::once_flag onceFlag; > + std::call_once(onceFlag, [] { This should be done in a separate patch.

Created attachment 353054 [details] Patch

Comment on attachment 353054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353054&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104 > +@property (nonatomic, copy, setter=_setSupportedAdditionalImageSourceTypes:) NSArray<NSString *> *_supportedAdditionalImageSourceTypes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); This name is cumbersome and doesn't make it clear that the arguments are UTIs. Maybe better as _additionalSupportedImageTypes: with a comment saying that the argument is an array of UTIs.

Created attachment 354278 [details] Patch

Attachment 354278 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3621: developmentRegion is not en. [xcodeproj/settings] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 353054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353054&action=review >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104 >> +@property (nonatomic, copy, setter=_setSupportedAdditionalImageSourceTypes:) NSArray<NSString *> *_supportedAdditionalImageSourceTypes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > This name is cumbersome and doesn't make it clear that the arguments are UTIs. Maybe better as _additionalSupportedImageTypes: with a comment saying that the argument is an array of UTIs. I changed all the names from ImageSourceTypes to ImageTypes all the way up till the mime registry, the UTI registry and the ImageDecoderCG. And I flipped SupportedAdditional to AdditionalSupported in the names as well.

Created attachment 354288 [details] Patch

Comment on attachment 354288 [details] Patch Clearing flags on attachment: 354288 Committed r238015: <https://trac.webkit.org/changeset/238015>

All reviewed patches have been landed. Closing bug.