Stop calling userPreferredLanguages() in the UIProcess since this can be slow and may keep the main thread busy. Nowadays, the WebProcess is able to call it by itself anyway. The sandbox is allowing it.
<rdar://problem/64317593>
Created attachment 401938 [details] Patch
Comment on attachment 401938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401938&action=review > Source/WTF/ChangeLog:3 > + Stop calling userPreferredLanguages() in the UIProcess Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option. It's not visible in this patch whether that goes in overrideLanguages already.
Comment on attachment 401938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401938&action=review >> Source/WTF/ChangeLog:3 >> + Stop calling userPreferredLanguages() in the UIProcess > > Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option. > > It's not visible in this patch whether that goes in overrideLanguages already. I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess. I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that.
Comment on attachment 401938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401938&action=review >>> Source/WTF/ChangeLog:3 >>> + Stop calling userPreferredLanguages() in the UIProcess >> >> Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option. >> >> It's not visible in this patch whether that goes in overrideLanguages already. > > I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess. > > I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that. We perform work to make CFLocale languages in WebProcess match UI process, but it's very much not obvious if they are going to return the same result always. I would object to landing this at the current level of understanding.
(In reply to Alexey Proskuryakov from comment #5) > Comment on attachment 401938 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401938&action=review > > >>> Source/WTF/ChangeLog:3 > >>> + Stop calling userPreferredLanguages() in the UIProcess > >> > >> Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option. > >> > >> It's not visible in this patch whether that goes in overrideLanguages already. > > > > I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess. > > > > I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that. > > We perform work to make CFLocale languages in WebProcess match UI process, > but it's very much not obvious if they are going to return the same result > always. I would object to landing this at the current level of understanding. I do not understand what you mean by "We perform work to make CFLocale languages in WebProcess match UI process". All this patch does is call CFLocaleCopyPreferredLanguages() from the WebProcess instead of the UIProcess (like it used to a while back). I change language in system Preferences on both macOS and iOS and navigator.languages returns the right languages.
I explained it in the first comment why testing just the global defaults is insufficient. We need to make this work in all these situations: 1. AppleLanguages user default is different between WebContent and UI process. 2. Available localizations are different between WebContent and UI process. CFLocaleCopyPreferredLanguages takes both defaults and localizations into consideration. There is no "just" about moving it between processes.
> like it used to a while back I'm not sure what exactly you mean by "a while back". Was that a recent change? Do you know which one?
(In reply to Alexey Proskuryakov from comment #7) > I explained it in the first comment why testing just the global defaults is > insufficient. We need to make this work in all these situations: > > 1. AppleLanguages user default is different between WebContent and UI > process. > > 2. Available localizations are different between WebContent and UI process. > > CFLocaleCopyPreferredLanguages takes both defaults and localizations into > consideration. There is no "just" about moving it between processes. Ok, I think I am able to reproduce the issue you were concerned about. I did: dvdo defaults write com.apple.Safari AppleLanguages '("en-US", "fr-FR")' In the UIProcess, I see both but in the WebContent process I only see en-US. I did not know this was a supported mechanism to change the locale of Safari.
(In reply to Chris Dumez from comment #9) > (In reply to Alexey Proskuryakov from comment #7) > > I explained it in the first comment why testing just the global defaults is > > insufficient. We need to make this work in all these situations: > > > > 1. AppleLanguages user default is different between WebContent and UI > > process. > > > > 2. Available localizations are different between WebContent and UI process. > > > > CFLocaleCopyPreferredLanguages takes both defaults and localizations into > > consideration. There is no "just" about moving it between processes. > > Ok, I think I am able to reproduce the issue you were concerned about. I did: > dvdo defaults write com.apple.Safari AppleLanguages '("en-US", "fr-FR")' > > In the UIProcess, I see both but in the WebContent process I only see en-US. > I did not know this was a supported mechanism to change the locale of Safari. There is code in XPCServiceMain.mm that tries to deal with AppleLanguages. Did not seem to work for me. Will try and figure out why.
Created attachment 401958 [details] Patch
(In reply to Chris Dumez from comment #11) > Created attachment 401958 [details] > Patch This iteration forwards AppleLanguages UserDefaults from the UIProcess to the WebContent process to keep it working.
Created attachment 401959 [details] Patch
I won't have time to think about this deeply soon, but given that you made these scenarios work, the patch must be OK.
Comment on attachment 401959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401959&action=review R=me. After this patch, can the language related code in XPCServiceMain.mm be removed? > Source/WebKit/UIProcess/WebProcessPool.cpp:1085 > + userPreferredLanguages(); Is this call needed?
Comment on attachment 401959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401959&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:1085 >> + userPreferredLanguages(); > > Is this call needed? Hahah lol, I had added it for debugging purposes :) Will drop. Thanks for catching.
Created attachment 402006 [details] Patch
Committed r263094: <https://trac.webkit.org/changeset/263094>
(In reply to Per Arne Vollan from comment #15) > Comment on attachment 401959 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401959&action=review > > R=me. After this patch, can the language related code in XPCServiceMain.mm > be removed? No, my patch actually relies on the code in XPCServiceMain to forward the AppleLanguages UserDefaults from the UIProcess to the WebProcess.
Comment on attachment 401959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401959&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:261 > + Vector<String> overrideLanguages; > + overrideLanguages.reserveInitialCapacity([languages count]); > + for (NSString *language in languages) > + overrideLanguages.uncheckedAppend(language); > + return overrideLanguages; The makeNSArray function in VectorCocoa.h does this for you; you don’t have to write it out.
(In reply to Darin Adler from comment #20) > Comment on attachment 401959 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401959&action=review > > > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:261 > > + Vector<String> overrideLanguages; > > + overrideLanguages.reserveInitialCapacity([languages count]); > > + for (NSString *language in languages) > > + overrideLanguages.uncheckedAppend(language); > > + return overrideLanguages; > > The makeNSArray function in VectorCocoa.h does this for you; you don’t have > to write it out. I assume you mean makeVector<String>(). It looks great, I did not know it existed. I will fix shortly, thanks.
Reopening to attach new patch.
Created attachment 402017 [details] Patch
Comment on attachment 402017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402017&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253 > NeverDestroyed<Vector<String>> overrideLanguages = []() { Just noticed that "static" is missing here, so we are probably leaking vectors. > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:255 > NSArray *languages = [[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"]; > - if (!languages) > - return Vector<WTF::String> { }; > - > - Vector<String> overrideLanguages; > - overrideLanguages.reserveInitialCapacity([languages count]); > - for (NSString *language in languages) > - overrideLanguages.uncheckedAppend(language); > - return overrideLanguages; > + return makeVector<String>(languages); Could go one step further and make this one line.
Comment on attachment 402017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402017&action=review >> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253 >> NeverDestroyed<Vector<String>> overrideLanguages = []() { > > Just noticed that "static" is missing here, so we are probably leaking vectors. Should probably be static const NeverDestroyed.
Created attachment 402018 [details] Patch
Created attachment 402019 [details] Patch
Comment on attachment 402019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402019&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:255 > + static const NeverDestroyed<Vector<String>> overrideLanguages = []() { > + return makeVector<String>([[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"]); > }(); Oh, don’t even need a lambda now!
Created attachment 402020 [details] Patch
Comment on attachment 402019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402019&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:256 > return overrideLanguages.get(); Not sure we need to write the get() here; I think it works without it.
Comment on attachment 402020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402020&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253 > + static const NeverDestroyed<Vector<String>> overrideLanguages = makeVector<String>([[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"]); Well, that is much more concise :)
Created attachment 402021 [details] Patch
(In reply to Darin Adler from comment #30) > Comment on attachment 402019 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402019&action=review > > > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:256 > > return overrideLanguages.get(); > > Not sure we need to write the get() here; I think it works without it. It is indeed not needed, I dropped it.
Committed r263100: <https://trac.webkit.org/changeset/263100> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402021 [details].