WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
178618
[iOS] DocumentWriter::createDocument can spend ~100ms unnecessarily converting image UTIs to MIME types
https://bugs.webkit.org/show_bug.cgi?id=178618
Summary
[iOS] DocumentWriter::createDocument can spend ~100ms unnecessarily convertin...
Wenson Hsieh
Reported
2017-10-20 23:37:24 PDT
DocumentWriter::createDocument can spend ~100ms unnecessarily converting image UTIs to MIME types.
Attachments
Patch
(12.75 KB, patch)
2017-10-20 23:55 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(14.98 KB, patch)
2017-10-23 14:12 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2017-10-20 23:55:48 PDT
Created
attachment 324492
[details]
Patch
Wenson Hsieh
Comment 2
2017-10-21 00:02:16 PDT
<
rdar://problem/35108852
>
Said Abou-Hallawa
Comment 3
2017-10-23 12:16:23 PDT
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 4
2017-10-23 12:30:29 PDT
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
Comment 5
2017-10-23 13:57:17 PDT
(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
Comment 6
2017-10-23 14:12:07 PDT
Created
attachment 324585
[details]
Patch
WebKit Commit Bot
Comment 7
2017-10-23 15:37:36 PDT
Comment on
attachment 324585
[details]
Patch Clearing flags on attachment: 324585 Committed
r223860
: <
https://trac.webkit.org/changeset/223860
>
WebKit Commit Bot
Comment 8
2017-10-23 15:37:38 PDT
All reviewed patches have been landed. Closing bug.
Matt Lewis
Comment 9
2017-10-23 16:23:31 PDT
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
Comment 10
2017-10-23 16:59:52 PDT
(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
Comment 11
2017-10-23 18:42:59 PDT
Committed
r223872
: <
https://trac.webkit.org/changeset/223872
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug