Creating this map in the WebContent process will access the launch services daemon, which will be blocked.
<rdar://problem/58714891>
Created attachment 388163 [details] Patch
Created attachment 388164 [details] Patch
Created attachment 388165 [details] Patch
Created attachment 388166 [details] Patch
Created attachment 388167 [details] Patch
What happens if the mapping of mime types reported by launch services changes during the lifetime of the process?
Comment on attachment 388167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388167&action=review Not sure what the Windows failure on EWS is about. > Source/WebCore/platform/MIMETypeRegistry.cpp:285 > + // A table of common media MIME types and file extentions used when a platform's Spelling error here. It’s extensions, not "extentions". > Source/WebCore/platform/MIMETypeRegistry.cpp:287 > + static NeverDestroyed<std::initializer_list<TypeExtensionPair>> commonMediaTypes; Should be able to use constexpr for this whole thing rather than needing code to initialize it. > Source/WebCore/platform/MIMETypeRegistry.cpp:290 > + static std::once_flag onceFlag; > + std::call_once( Surprised we need this. Is this called from multiple threads? If not then we should just be able to use "= [] {" instead of all the call_once stuff as is done in many other places in WebKit such as canAtttachAuthorShadowRoot, hasEffectiveDisplayNoneForDisplayContents, etc. > Source/WebCore/platform/MIMETypeRegistry.cpp:381 > + static std::once_flag onceFlag; > + std::call_once( Ditto.
(In reply to Sam Weinig from comment #7) > What happens if the mapping of mime types reported by launch services > changes during the lifetime of the process? Good point! There should be no behavior change with this patch. The only difference is that we create the cached map in the UI process instead of in the WebContent process. Thanks for reviewing!
(In reply to Darin Adler from comment #8) > Comment on attachment 388167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388167&action=review > > Not sure what the Windows failure on EWS is about. > > > Source/WebCore/platform/MIMETypeRegistry.cpp:285 > > + // A table of common media MIME types and file extentions used when a platform's > > Spelling error here. It’s extensions, not "extentions". > > > Source/WebCore/platform/MIMETypeRegistry.cpp:287 > > + static NeverDestroyed<std::initializer_list<TypeExtensionPair>> commonMediaTypes; > > Should be able to use constexpr for this whole thing rather than needing > code to initialize it. > > > Source/WebCore/platform/MIMETypeRegistry.cpp:290 > > + static std::once_flag onceFlag; > > + std::call_once( > > Surprised we need this. Is this called from multiple threads? If not then we > should just be able to use "= [] {" instead of all the call_once stuff as is > done in many other places in WebKit such as canAtttachAuthorShadowRoot, > hasEffectiveDisplayNoneForDisplayContents, etc. > > > Source/WebCore/platform/MIMETypeRegistry.cpp:381 > > + static std::once_flag onceFlag; > > + std::call_once( > > Ditto. You're right. I don't believe we need this, since the function did not use call_once originally. I will update the patch, possibly asserting that this code is being run on the main thread. Thanks for reviewing!
(In reply to Per Arne Vollan from comment #9) > (In reply to Sam Weinig from comment #7) > > What happens if the mapping of mime types reported by launch services > > changes during the lifetime of the process? > > Good point! There should be no behavior change with this patch. The only > difference is that we create the cached map in the UI process instead of in > the WebContent process. > Sounds like there would be a subtle behavior change then, since the cache will now not be invalidated for the life of the UIProcess, whereas, on each WebProcess creation we used to get a fresh copy.
(In reply to Sam Weinig from comment #11) > (In reply to Per Arne Vollan from comment #9) > > (In reply to Sam Weinig from comment #7) > > > What happens if the mapping of mime types reported by launch services > > > changes during the lifetime of the process? > > > > Good point! There should be no behavior change with this patch. The only > > difference is that we create the cached map in the UI process instead of in > > the WebContent process. > > > > Sounds like there would be a subtle behavior change then, since the cache > will now not be invalidated for the life of the UIProcess, whereas, on each > WebProcess creation we used to get a fresh copy. Ah, yes, that is a good point! We could possibly recreate the map every time we create a new WebContent process, but that would be less performant than just using the previous one.
Created attachment 388595 [details] Patch
Created attachment 388601 [details] Patch
Comment on attachment 388601 [details] Patch Clearing flags on attachment: 388601 Committed r255050: <https://trac.webkit.org/changeset/255050>
Probably worth investigating whether one already exists / filing a radar for a callback from launch services that tells you if it changes.a
Reverted r255050 for reason: Introduced crashes on bots Committed r255081: <https://trac.webkit.org/changeset/255081>
Created attachment 388714 [details] Patch
(In reply to Sam Weinig from comment #16) > Probably worth investigating whether one already exists / filing a radar for > a callback from launch services that tells you if it changes.a Thanks! I am looking into this now.
(In reply to Per Arne Vollan from comment #19) > (In reply to Sam Weinig from comment #16) > > Probably worth investigating whether one already exists / filing a radar for > > a callback from launch services that tells you if it changes.a > > Thanks! I am looking into this now. There appears to be an existing mechanism for this.
Comment on attachment 388714 [details] Patch Clearing flags on attachment: 388714 Committed r255119: <https://trac.webkit.org/changeset/255119>