[webkit-reviews] review denied: [Bug 136082] Replace use of WKCopyCFLocalizationPreferredName with NSLocale public API : [Attachment 236861] With actually proper includes this time.

[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 236861: With actually proper includes this time.
https://bugs.webkit.org/attachment.cgi?id=236861&action=review

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


r- because of the broken test, and because of canonicalization in
preferredBundleLocalizationName.

> Source/WebCore/ChangeLog:12
> +	   * platform/mac/WebCoreNSStringExtras.mm: Replacements for the
aspects of
> +	   WKCopyCFLocalizationPreferredName.

I would prefer to have a meaningful name for the header, and probably have the
functions work with WTF Strings. This functionality has nothing to do with
NSString.

This technique used to be more common in WebKit early years, but I don't see a
lot of upsides to it. The downsides are:

1. Internal code leaking into clients such as Safari, which start to use it and
thus complicate WebKit refactoring.
2. More difficulty figuring out where functions are declared and implemented.
3. Potential for name conflicts with Foundation and other frameworks.

> Source/WebCore/ChangeLog:17
> +	   * WebCore.order: Remove mention of
WKCopyCFLocalizationPreferredName.

We generally don't update WebCore.order even when removing functions, there is
no need to.

> Source/WebCore/platform/mac/Language.mm:35
> +#import <wtf/text/CString.h>

This added include seems unnecessary.

> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:126
> +NSString *preferredBundleLocalizationName()

This is the localization name, so the function name is good. I would personally
have named it "effectiveBundleLocalizationName". I'd also add a comment
explaining that we only look at the first localization here. This may cause
complications when NSBundle uses two localizations at once, e.g. to get some
strings from en.lproj, and others form en_GB.lproj.

It's not a serious issue in practice, so it's not even a FIXME. I just find
that explaining such things saves a lot of time and confusion later.

But then again, perhaps we can simply do this at each call site, and not expose
a function?

> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:128
> +    return canonicalLocalizationName([[[NSBundle mainBundle]
preferredLocalizations] objectAtIndex:0]);

Nice find using -[NSBundle preferredLocalizations] instead of the longer CF
equivalent.

But we shouldn't be canonicalizing these. The result is provided by
NS/CFBundle, and is used by it, so it should be good as is.

> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:131
> +NSString *canonicalLocalizationName(NSString *language)

This function name is incorrect. It returns a locale identifier, which (if we
look the other way for a moment) can be used as language identifier, but it has
nothing to do with CFBundle and localizations.

I'd name the function canonicalWebLanguageName() for now, and at some point
later, we may find out that different web technologies use different
canonicalizations.

> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:134
> +    LangCode languageCode;
> +    RegionCode regionCode;

This needs a FIXME, explaining that Script Manager codes cannot represent all
languages that are supported by the OS, so we should move away from them.

> Source/WebCore/platform/mac/WebCoreNSStringExtras.mm:138
> +	   return @"";

Are callers prepared to deal with an empty string? I'd use @"en" here.