Move SystemPreview code from WebKitAdditions to WebKit
<rdar://problem/44080245>
Created attachment 348796 [details] Patch
Comment on attachment 348796 [details] Patch rs=me You might be intending to remove these in a separate patch, but in any case, it looks like there's some internal testing support that we don't need anymore: Internals::systemPreviewRelType Internals::isSystemPreviewLink Internals::isSystemPreviewImage …as well as its usages in a few LayoutTests.
You're right, but just the rel type one should go. The others are still useful.
Comment on attachment 348796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348796&action=review > Source/WebCore/platform/MIMETypeRegistry.cpp:735 > +const HashSet<String, ASCIICaseInsensitiveHash>& MIMETypeRegistry::getSystemPreviewMIMETypes() > +{ > + if (!systemPreviewMIMETypes) > + initializeSystemPreviewMIMETypes(); > + return *systemPreviewMIMETypes; > +} > + > +bool MIMETypeRegistry::isSystemPreviewMIMEType(const String& mimeType) > +{ > + if (mimeType.isEmpty()) > + return false; > + return getSystemPreviewMIMETypes().contains(mimeType); > +} The more common / "modern" idiom for something like this would be to have getSystemPreviewMIMETypes() contain something like: static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> systemPreviewMIMETypes = std::initializer_list<String> { "model/vnd.usdz+zip", ... }; return systemPreviewMIMETypes; But since that is not idiomatic in this file, that could be left to a follow up cleanup. > Source/WebCore/rendering/RenderThemeIOS.mm:1869 > + static NSBundle *arKitBundle; > + static dispatch_once_t onceToken; > + dispatch_once(&onceToken, ^{ > +#if PLATFORM(IOS_SIMULATOR) > + dlopen("/System/Library/PrivateFrameworks/AssetViewer.framework/AssetViewer", RTLD_NOW); > + arKitBundle = [NSBundle bundleForClass:NSClassFromString(@"ASVThumbnailView")]; > +#else > + arKitBundle = [NSBundle bundleWithURL:[NSURL fileURLWithPath:@"/System/Library/PrivateFrameworks/AssetViewer.framework"]]; > +#endif > + }); > + > + return arKitBundle; Is this actually called from different threads? If not, this doesn't need to be a dispatch once and can just be a normal static initialization. > Source/WebCore/rendering/RenderThemeIOS.mm:1887 > +}; Unnecessary trailing semicolon. > Source/WebCore/rendering/RenderThemeIOS.mm:1892 > + static CGPDFPageRef logoPage; > + static dispatch_once_t onceToken; Same question about threads here. > Source/WebCore/rendering/RenderThemeIOS.mm:1898 > +}; Unnecessary trailing semicolon. > Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:47 > +static String getUTIForMIMEType(const String& mimeType) This function feels a bit weird. It's only caller always passes something that is a SystemPreviewMIMEType and really wants an NSString. Could this, at least for now while there is only one UTI, just be a function to get the "SystemPreview" UTI as an NSString? > Source/WebKit/UIProcess/ios/WKSystemPreviewView.mm:53 > +static String getUTIForMIMEType(const String& mimeType) This should return an NSString. > Source/WebKit/UIProcess/ios/WKSystemPreviewView.mm:107 > NSString *contentType; > -#if USE(APPLE_INTERNAL_SDK) > - contentType = WebKit::getUTIForMIMEType(_mimeType.get()); > -#else > - NSString *extension = [[_suggestedFilename.get() pathExtension] lowercaseString]; > - RetainPtr<CFStringRef> contentTypeCF = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (__bridge CFStringRef)extension, nil)); > - contentType = (NSString *)contentTypeCF.get(); > -#endif > + contentType = getUTIForMIMEType(_mimeType.get()); This can be on one line now.
Committed r235626: <https://trac.webkit.org/changeset/235626>
Comment on attachment 348796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348796&action=review >> Source/WebCore/platform/MIMETypeRegistry.cpp:735 >> +} > > The more common / "modern" idiom for something like this would be to have getSystemPreviewMIMETypes() contain something like: > > static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> systemPreviewMIMETypes = std::initializer_list<String> { "model/vnd.usdz+zip", ... }; > return systemPreviewMIMETypes; > > But since that is not idiomatic in this file, that could be left to a follow up cleanup. Slightly better to add the ""_s suffixes so we get the ASCIILiteral optimization when creating those WTF::String objects. >> Source/WebKit/UIProcess/ios/WKSystemPreviewView.mm:53 >> +static String getUTIForMIMEType(const String& mimeType) > > This should return an NSString. WebKit coding style specifies that we don’t use the word "get" in the name of functions like this one. Maybe call this preferredUTIForMIMEType?