Bug 189252 – Move SystemPreview code from WebKitAdditions to WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Move SystemPreview code from WebKitAdditions to WebKit

[Radar WebKit Bug Importer]

<rdar://problem/44080245>

[Dean Jackson]

Created attachment 348796 [details]
Patch

[Wenson Hsieh]

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.

[Dean Jackson]

You're right, but just the rel type one should go. The others are still useful.

[Sam Weinig]

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.

[Dean Jackson]

Committed r235626: <https://trac.webkit.org/changeset/235626>

[Darin Adler]

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?