REGRESSION(r254389): Cordova throws an exception because it expects a hyphen inside navigator.locale

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

REGRESSION(r254389): Cordova throws an exception because it expects a hyphen inside navigator.locale

[Myles C. Maxfield]

Created attachment 393329 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/59845517>

[Keith Rollin]

Comment on attachment 393329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393329&action=review

> Source/WTF/wtf/cocoa/LanguageCocoa.mm:40
> +#elif PLATFORM(IOS_FAMILY)
> +    static bool applicationVersionCheck = dyld_get_program_sdk_version() >= DYLD_IOS_VERSION_13_4;

I want to make sure this platform and version checking is correct. The PLATFORM(IOS_FAMILY) check will be True for tvOS and watchOS, as well as iOS. But that DYLD_IOS_VERSION_13_4 seems to be iOS-only. So should the pre-processor conditional actually be just PLATFORM(IOS)? And what should happen with macCatalyst? PLATFORM(IOS) will return False for macCatalyst, so if we want to support that platform, we'd also need a PLATFORM(MACCATALYST) check (I assume the iOS version number would be appropriate for that platform?).

[Chris Dumez]

Comment on attachment 393329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393329&action=review

> Source/WTF/wtf/cocoa/LanguageCocoa.mm:38
> +    static bool applicationVersionCheck = dyld_get_program_sdk_version() >= DYLD_MACOSX_VERSION_10_15_4;

I don't think calling dyld_get_program_sdk_version() in the WebKit2 WebContent process will do what you want. AFAIK, dyld_get_program_sdk_version() will only do what you want in the wK2 UIProcess or WK1.

[Myles C. Maxfield]

Comment on attachment 393329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393329&action=review

>> Source/WTF/wtf/cocoa/LanguageCocoa.mm:38
>> +    static bool applicationVersionCheck = dyld_get_program_sdk_version() >= DYLD_MACOSX_VERSION_10_15_4;
> 
> I don't think calling dyld_get_program_sdk_version() in the WebKit2 WebContent process will do what you want. AFAIK, dyld_get_program_sdk_version() will only do what you want in the wK2 UIProcess or WK1.

Good catch! I'm pursuing this in https://bugs.webkit.org/show_bug.cgi?id=209030.

[Myles C. Maxfield]

Comment on attachment 393329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393329&action=review

>> Source/WTF/wtf/cocoa/LanguageCocoa.mm:40
>> +    static bool applicationVersionCheck = dyld_get_program_sdk_version() >= DYLD_IOS_VERSION_13_4;
> 
> I want to make sure this platform and version checking is correct. The PLATFORM(IOS_FAMILY) check will be True for tvOS and watchOS, as well as iOS. But that DYLD_IOS_VERSION_13_4 seems to be iOS-only. So should the pre-processor conditional actually be just PLATFORM(IOS)? And what should happen with macCatalyst? PLATFORM(IOS) will return False for macCatalyst, so if we want to support that platform, we'd also need a PLATFORM(MACCATALYST) check (I assume the iOS version number would be appropriate for that platform?).

From my testing:

tvOS: 0xD0400
watchOS: 0xD0200
macOS: 0xA0F04
iOS: 0xD0400

[Myles C. Maxfield]

Created attachment 393557 [details]
WIP

[Myles C. Maxfield]

Created attachment 393565 [details]
Patch

[Darin Adler]

Comment on attachment 393565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393565&action=review

> Source/WTF/wtf/cocoa/LanguageCocoa.mm:44
> +#if PLATFORM(MAC)
> +    static bool applicationVersionCheck = applicationSDKVersion() >= DYLD_MACOSX_VERSION_10_15_4;
> +#elif PLATFORM(IOS)
> +    static bool applicationVersionCheck = applicationSDKVersion() >= DYLD_IOS_VERSION_13_4;
> +#elif
> +    static bool applicationVersionCheck = true;
> +#endif
> +    static bool result = applicationVersionCheck && [NSLocale respondsToSelector:@selector(minimizedLanguagesFromLanguages:)];

I suggest static const bool or static constexpr for all of these.

But also, not elegant to have two globals since the first is only used to compute the second. So write it like this:

    static const bool result = [] {
#if PLATFORM(MAC)
        if (applicationSDKVersion() < DYLD_MACOSX_VERSION_10_15_4)
            return false;
#endif
#if PLATFORM(IOS)
        if (applicationSDKVersion() < DYLD_IOS_VERSION_13_4)
            return false;
#elif
        return [NSLocale respondsToSelector:@selector(minimizedLanguagesFromLanguages:)];
    }();

[Myles C. Maxfield]

Created attachment 393810 [details]
Patch for committing

[EWS]

Committed r258658: <https://trac.webkit.org/changeset/258658>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393810 [details].