[iOS] DocumentWriter::createDocument can spend ~100ms unnecessarily converting image UTIs to MIME types

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

DocumentWriter::createDocument can spend ~100ms unnecessarily converting image UTIs to MIME types.

[Wenson Hsieh]

Created attachment 324492 [details]
Patch

[Wenson Hsieh]

<rdar://problem/35108852>

[Said Abou-Hallawa]

Comment on attachment 324492 [details]
Patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:78
> +    static const char* const knownImageMIMETypes[] = {
> +        "image/tiff", "image/gif", "image/jpeg", "image/vnd.microsoft.icon", "image/jp2", "image/png", "image/bmp",
> +        // Favicons don't have a MIME type in the registry either.
> +        "image/x-icon",
> +        // We only get one MIME type per UTI, hence our need to add this manually.
> +        "image/pjpeg"
> +    };

Now we will have three lists to maintain: the allowedImageUTIs() in UTIRegistry.cpp and supportedImageMIMETypes and supportedImageResourceMIMETypes in this file. If we want to add support for another image format, for example, we will need to change two completely independent places: the s_allowedImageUTIs[] in allowedImageUTIs() and knownImageMIMETypes[] in this file.

I would suggest having only one list, either UTIs or MIME types, it does not matter,  but have a conversion function from one type to the other. The conversion  function will have a lookup table from UTI to MIME type or vice versa. It can also assert that the return value matches the return of MIMETypeForImageSourceType() if it converts from UTI to MIME type. So it will tell the developer who changes s_allowedImageUTIs[] that he needs to change lookup table in the conversion function as well.

> Source/WebCore/platform/MIMETypeRegistry.cpp:600
> -        initializeMIMETypeRegistry();
> +        initializePDFAndPostScriptMIMETypes();

I do not think this is clean implementation. Can't we have something like this:

bool MIMETypeRegistry::isPDFOrPostScriptMIMEType(const String& mimeType)
{
    return isPDFMIMEType(mimeType) || isPostScriptMIMEType(mimeType);
}

bool MIMETypeRegistry::isPostScriptMIMEType(const String& mimeType)
{
    return mimeType == "application/postscript";
}

> Source/WebCore/platform/MIMETypeRegistry.cpp:675
>  HashSet<String, ASCIICaseInsensitiveHash>& MIMETypeRegistry::getPDFAndPostScriptMIMETypes()
>  {
>      if (!pdfAndPostScriptMIMETypes)
> -        initializeMIMETypeRegistry();
> +        initializePDFAndPostScriptMIMETypes();
>      return *pdfAndPostScriptMIMETypes;
>  }

Who is calling this function? It is not an exported function and I could not find a place in WebCore which calls it.

[Wenson Hsieh]

Comment on attachment 324492 [details]
Patch

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

>> Source/WebCore/platform/MIMETypeRegistry.cpp:78
>> +    };
> 
> Now we will have three lists to maintain: the allowedImageUTIs() in UTIRegistry.cpp and supportedImageMIMETypes and supportedImageResourceMIMETypes in this file. If we want to add support for another image format, for example, we will need to change two completely independent places: the s_allowedImageUTIs[] in allowedImageUTIs() and knownImageMIMETypes[] in this file.
> 
> I would suggest having only one list, either UTIs or MIME types, it does not matter,  but have a conversion function from one type to the other. The conversion  function will have a lookup table from UTI to MIME type or vice versa. It can also assert that the return value matches the return of MIMETypeForImageSourceType() if it converts from UTI to MIME type. So it will tell the developer who changes s_allowedImageUTIs[] that he needs to change lookup table in the conversion function as well.

The conversion functions MIMETypeFromUTI and UTIFromMIMEType already exist; are you proposing that we tweak them to check a hard-coded lookup table of UTI <=> MIME type before hitting UTTypeCopyPreferredTagWithClass?

>> Source/WebCore/platform/MIMETypeRegistry.cpp:600
>> +        initializePDFAndPostScriptMIMETypes();
> 
> I do not think this is clean implementation. Can't we have something like this:
> 
> bool MIMETypeRegistry::isPDFOrPostScriptMIMEType(const String& mimeType)
> {
>     return isPDFMIMEType(mimeType) || isPostScriptMIMEType(mimeType);
> }
> 
> bool MIMETypeRegistry::isPostScriptMIMEType(const String& mimeType)
> {
>     return mimeType == "application/postscript";
> }

Good point -- this also allows us to just get rid of pdfAndPostScriptMIMETypes altogether!

>> Source/WebCore/platform/MIMETypeRegistry.cpp:675
>>  }
> 
> Who is calling this function? It is not an exported function and I could not find a place in WebCore which calls it.

Yeah...it looks like nobody calls this. We should probably just remove it.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 324492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324492&action=review
> 
> >> Source/WebCore/platform/MIMETypeRegistry.cpp:78
> >> +    };
> > 
> > Now we will have three lists to maintain: the allowedImageUTIs() in UTIRegistry.cpp and supportedImageMIMETypes and supportedImageResourceMIMETypes in this file. If we want to add support for another image format, for example, we will need to change two completely independent places: the s_allowedImageUTIs[] in allowedImageUTIs() and knownImageMIMETypes[] in this file.
> > 
> > I would suggest having only one list, either UTIs or MIME types, it does not matter,  but have a conversion function from one type to the other. The conversion  function will have a lookup table from UTI to MIME type or vice versa. It can also assert that the return value matches the return of MIMETypeForImageSourceType() if it converts from UTI to MIME type. So it will tell the developer who changes s_allowedImageUTIs[] that he needs to change lookup table in the conversion function as well.
> 
> The conversion functions MIMETypeFromUTI and UTIFromMIMEType already exist;
> are you proposing that we tweak them to check a hard-coded lookup table of
> UTI <=> MIME type before hitting UTTypeCopyPreferredTagWithClass?

Talked to Said in person -- we agree that there should be a mechanism to enforce keeping supported MIME types in sync with allowedImageUTIs. Patch forthcoming.

> 
> >> Source/WebCore/platform/MIMETypeRegistry.cpp:600
> >> +        initializePDFAndPostScriptMIMETypes();
> > 
> > I do not think this is clean implementation. Can't we have something like this:
> > 
> > bool MIMETypeRegistry::isPDFOrPostScriptMIMEType(const String& mimeType)
> > {
> >     return isPDFMIMEType(mimeType) || isPostScriptMIMEType(mimeType);
> > }
> > 
> > bool MIMETypeRegistry::isPostScriptMIMEType(const String& mimeType)
> > {
> >     return mimeType == "application/postscript";
> > }
> 
> Good point -- this also allows us to just get rid of
> pdfAndPostScriptMIMETypes altogether!
> 
> >> Source/WebCore/platform/MIMETypeRegistry.cpp:675
> >>  }
> > 
> > Who is calling this function? It is not an exported function and I could not find a place in WebCore which calls it.
> 
> Yeah...it looks like nobody calls this. We should probably just remove it.

[Wenson Hsieh]

Created attachment 324585 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324585 [details]
Patch

Clearing flags on attachment: 324585

Committed r223860: <https://trac.webkit.org/changeset/223860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

This revision broke the windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/5458/steps/compile-webkit/logs/stdio

error:

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\MIMETypeRegistry.cpp(39): fatal error C1083: Cannot open include file: 'UTIUtilities.h': No such file or directory [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Wenson Hsieh]

(In reply to Matt Lewis from comment #9)
> This revision broke the windows build:
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 5458/steps/compile-webkit/logs/stdio
> 
> error:
> 
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\platform\MIMETypeRegistry.cpp(39): fatal error
> C1083: Cannot open include file: 'UTIUtilities.h': No such file or directory
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Build fix in https://trac.webkit.org/changeset/223862/webkit

[Michael Catanzaro]

Committed r223872: <https://trac.webkit.org/changeset/223872>