[webkit-reviews] review denied: [Bug 136082] Replace use of WKCopyCFLocalizationPreferredName with NSLocale public API : [Attachment 236820] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 136082: Replace use of WKCopyCFLocalizationPreferredName with NSLocale
public API
https://bugs.webkit.org/show_bug.cgi?id=136082

Attachment 236820: Patch
https://bugs.webkit.org/attachment.cgi?id=236820&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=236820&action=review


This is a very welcome improvement. It's quite difficult to verify that
behavior is correct in all code paths, but we should definitely stop using
script manager, it's just crazy at this point.

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:214
> -    RetainPtr<CFStringRef> localization =
adoptCF(WKCopyCFLocalizationPreferredName(0));
> +    NSString *localization = [[NSLocale currentLocale] localeIdentifier];
>      if (localization && _CFBundleSetupXPCBootstrapPtr()) {

This null check is a mistake from adopting _CFBundleSetupXPCBootstrap, the code
for getting localization should be simply removed.

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:460
> -    CString appleLanguagesArgument = String("('" +
String(adoptCF(WKCopyCFLocalizationPreferredName(0)).get()) + "')").utf8();
> +    CString appleLanguagesArgument = String("('" + String([[NSLocale
currentLocale] localeIdentifier]) + "')").utf8();

I think that this is wrong. The goal here is to get main bundle localization,
not user locale. Also, locale is not language.

This code only runs for legacy process (not XPC), so it's a little tricky to
test. I would just add these lines in some regular code path and rebuild to
verify what they do.

The way to test this is to set system language to anything but English,
re-login, and then to run your local build of Safari. It is not localized, so
WKCopyCFLocalizationPreferredName will return English, and [NSLocale
currentLocale] will return the locale as set up in system preferences.

I'm not 100% sure about what we need instead, probably
_CFBundleCopyLanguageSearchListInBundle(CFBundleGetMainBundle()).

> Source/WebKit/mac/Plugins/WebBasePluginPackage.mm:194
> +	       if (![localizationName isEqualToString:[[NSLocale currentLocale]
localeIdentifier]])

I have never seen this code before, but looks like we need main process
localization language here as well, not current locale.

> Source/WebKit/mac/Plugins/Hosted/NetscapePluginHostManager.mm:119
> -					 (NSString *)localization.get(),
@"localization",
> +					 [[NSLocale currentLocale]
localeIdentifier], @"localization",

Ditto.