Bug 226038 – [Modern Media Controls] REGRESSION(r254389) media controls needs the full list of language preferences for ordering tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Devin Rousso]

media controls need access to the full list of language preferences in order to properly sort/order the list of tracks in the controls UI (e.g. if a `<video>` has subtitles for English, Spanish, and French, and the user has English (default) and French (alternate) configured in the Language & Region view of System Preferences, WebKit should order the subtitles list English, French, and then Spanish)

[Devin Rousso]

Created attachment 429210 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 429210 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.h:253
> +    typedef String (*DefaultLanguageFunctionPtr)(ShouldMinimizeLanguages);

I think this is philosophically the wrong direction. I can't think of a case where JavaScript would ever even have the possibility of passing No here. Can we consider reverting this typedef to what it was originally, and using a wrapper function instead?

> Source/WTF/wtf/Language.cpp:197
> +static Vector<String>& platformPreferredLanguagesOverride()
> +{
> +    static LazyNeverDestroyed<Vector<String>> override;
> +    static std::once_flag onceKey;
> +    std::call_once(onceKey, [&] {
> +        override.construct();
> +    });
> +    return override;
> +}
> +
> +Vector<String> platformUserPreferredLanguagesOverride()
> +{
> +    return platformPreferredLanguagesOverride();
> +}
> +
> +void overridePlatformUserPreferredLanguages(const Vector<String>& override)
> +{
> +    platformPreferredLanguagesOverride() = override;
> +    languageDidChange();
> +}

The "platform" prefix seems wrong here, since these functions are not platform-specific. (platformUserPreferredLanguages() is in LanguageCF.cpp and is thus platform-specific.)

It seems unfortunate to have two levels of overriding for what is (conceptually) a single feature. Can we do this instead by using Obj-C to swizzle out the runtime of +[NSLocale minimizedLanguagesFromLanguages:] in the web process (using injected bundle)?

> Source/WTF/wtf/cf/LanguageCF.cpp:66
> +    static LazyNeverDestroyed<Vector<String>> languages;
> +    static std::once_flag onceKey;
> +    std::call_once(onceKey, [&] {
> +        languages.construct();
> +    });

What's the point of using "Lazy"NeverDestroyed if you always immediately construct it? Why not just use regular NeverDestroyed? I see that preferredLanguages() does this too.... maybe this pattern is more thread safe or something?

> Source/WTF/wtf/cf/LanguageCF.cpp:147
> +                    auto baseLanguage = baseLanguageFromLanguage(language);

Why are we doing this? I don't see any explanation at all in the changeLog. What's the relation between where or not we should be minimizing languages and whether or not base languages appear as duplicates in the list? I don't think we should be doing this.

> Source/WTF/wtf/cf/LanguageCF.cpp:153
>      }

Can we delete the double return at the end of this function while we're here?

[Myles C. Maxfield]

Comment on attachment 429210 [details]
Patch

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

> Source/WTF/wtf/cf/LanguageCF.cpp:125
>      if (userPreferredLanguages.isEmpty()) {

I wonder, since we're making changes in this area, if it makes sense to move this cache up to the non-platform-specific layer. All platforms will want to cache results.

[Chris Dumez]

Comment on attachment 429210 [details]
Patch

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

>> Source/WTF/wtf/cf/LanguageCF.cpp:66
>> +    });
> 
> What's the point of using "Lazy"NeverDestroyed if you always immediately construct it? Why not just use regular NeverDestroyed? I see that preferredLanguages() does this too.... maybe this pattern is more thread safe or something?

We want to be using LazyNeverDestroyed<> with a call_once for global statics that can be accessed from multiple threads. This is because static variable initialization is not thread-safe in WebKit (because we disable that at compiler level for perf reasons).

That said, it looks to me here that ALL call sites grab the preferredLanguagesMutex lock before calling this. As a result, it does not seem we need any additional synchronization and we should be able to use NeverDestroyed and get rid of the call_once().

[Devin Rousso]

Comment on attachment 429210 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:253
>> +    typedef String (*DefaultLanguageFunctionPtr)(ShouldMinimizeLanguages);
> 
> I think this is philosophically the wrong direction. I can't think of a case where JavaScript would ever even have the possibility of passing No here. Can we consider reverting this typedef to what it was originally, and using a wrapper function instead?

Sure.  I'll try using a `[] () { return defaultLanguage(); }` inline lambda.

>> Source/WTF/wtf/Language.cpp:197
>> +}
> 
> The "platform" prefix seems wrong here, since these functions are not platform-specific. (platformUserPreferredLanguages() is in LanguageCF.cpp and is thus platform-specific.)
> 
> It seems unfortunate to have two levels of overriding for what is (conceptually) a single feature. Can we do this instead by using Obj-C to swizzle out the runtime of +[NSLocale minimizedLanguagesFromLanguages:] in the web process (using injected bundle)?

The real thing I'm after here is replacing `CFLocaleCopyPreferredLanguages` as we don't want to have to modify the bots to have more than one user configured language.  I agree that the naming is odd.  My thinking was that we're overriding how the platform gets the list of user languages.  I'll see about renaming.

>>> Source/WTF/wtf/cf/LanguageCF.cpp:66
>>> +    });
>> 
>> What's the point of using "Lazy"NeverDestroyed if you always immediately construct it? Why not just use regular NeverDestroyed? I see that preferredLanguages() does this too.... maybe this pattern is more thread safe or something?
> 
> We want to be using LazyNeverDestroyed<> with a call_once for global statics that can be accessed from multiple threads. This is because static variable initialization is not thread-safe in WebKit (because we disable that at compiler level for perf reasons).
> 
> That said, it looks to me here that ALL call sites grab the preferredLanguagesMutex lock before calling this. As a result, it does not seem we need any additional synchronization and we should be able to use NeverDestroyed and get rid of the call_once().

I'll change this to be a `CheckedLock` with `WTF_REQUIRES_LOCK` and `NeverDestroyed`.

>> Source/WTF/wtf/cf/LanguageCF.cpp:125
>>      if (userPreferredLanguages.isEmpty()) {
> 
> I wonder, since we're making changes in this area, if it makes sense to move this cache up to the non-platform-specific layer. All platforms will want to cache results.

While I do agree that caching could be done at a higher level, there appear to be other uses of `platformUserPreferredLanguages`.  I'll see if they still need to be there and if not I'll do this.

>> Source/WTF/wtf/cf/LanguageCF.cpp:147
>> +                    auto baseLanguage = baseLanguageFromLanguage(language);
> 
> Why are we doing this? I don't see any explanation at all in the changeLog. What's the relation between where or not we should be minimizing languages and whether or not base languages appear as duplicates in the list? I don't think we should be doing this.

This matches part of the implementation of `+[NSLocale minimizedLanguagesFromLanguages:]`.  The reason why we want this is that media tracks can specify `lang="en"` (in addition to `lang="en-US"`) and so as a result we want `"en"` to match if the user prefers `"en-US"` (and vice versa).  I'll add a comment in the ChangeLog.

[Devin Rousso]

Created attachment 429350 [details]
Patch

[Devin Rousso]

Created attachment 429383 [details]
Patch

[Devin Rousso]

Created attachment 429405 [details]
Patch

[Chris Dumez]

Comment on attachment 429405 [details]
Patch

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

> Source/WTF/wtf/Language.h:38
> +enum class ShouldMinimizeLanguages { Yes, No };

We prefer to put the No first in the enum, so that its value is 0, and Yes is 1.

> Source/WTF/wtf/cf/LanguageCF.cpp:93
> +            CFArrayAppendValue(array.get(), item.isolatedCopy().createCFString().get());

Why isolateCopy just to create a CFString?

> Source/WTF/wtf/cocoa/LanguageCocoa.mm:70
> +#if PLATFORM(IOS)

Could use #elif

Also, are we sure we want iOS only? Not Marzipan, WatchOS, TVOS?

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:480
> +            temp.reserveCapacity(captionLanguagesCount + preferredLanguages.size());

reserveInitialCapacity() since you've just created the vector.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:482
> +                temp.append(static_cast<CFStringRef>(CFArrayGetValueAtIndex(captionLanguages.get(), i)));

Can use uncheckedAppend() since you've reserved capacity.

[Devin Rousso]

Created attachment 429441 [details]
Patch

[Devin Rousso]

Created attachment 429458 [details]
Patch

[Devin Rousso]

Created attachment 429469 [details]
Patch

[Devin Rousso]

Created attachment 429472 [details]
Patch

[Devin Rousso]

Created attachment 429473 [details]
Patch

[Devin Rousso]

Created attachment 429476 [details]
Patch

[Devin Rousso]

Created attachment 429483 [details]
Patch

[Devin Rousso]

Created attachment 429530 [details]
Patch

[Chris Dumez]

Comment on attachment 429530 [details]
Patch

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

> Source/WTF/wtf/Language.cpp:40
> +static CheckedLock cachedPlatformPreferredLanguagesLock;

Note that we should use Lock in new code, not CheckedLock. I am about to remove the CheckedLock alias.

Sorry about all the reshuffling.

[Myles C. Maxfield]

Comment on attachment 429530 [details]
Patch

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

> Source/WTF/wtf/Language.cpp:219
> +Vector<String> initialPlatformUserPreferredLanguagesOverrideForTesting()
> +{
> +    Locker locker { initialPlatformPreferredLanguagesOverrideForTestingLock };
> +    return initialPlatformPreferredLanguagesOverrideForTesting();
>  }

Devin and I talked about a way to remove this.

> Source/WTF/wtf/cf/LanguageCF.cpp:115
> +            // Convert languages with country codes to equivalent languages without country codes (if possible).
> +            // e.g. "en-US" becomes "en", which is useful when trying to match HTML `lang="en"`.
> +            auto baseLanguage = baseLanguageFromLanguage(platformLanguage);
> +            if (baseLanguage && baseLanguage != platformLanguage && CFStringCompare(baseLanguage.get(), platformLanguage, 0) != kCFCompareEqualTo)
> +                languages.append(httpStyleLanguageCode(baseLanguage.get(), shouldMinimizeLanguages));

Devin and I talked about a way to avoid this.

[Devin Rousso]

Created attachment 429616 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 429616 [details]
Patch

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

I can't review the media and subtitle specific code, but the language code stuff looks good.

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:35
> +// Needed by `Source/WTF/wtf/cocoa/LanguageCocoa.mm`.
> +WK_INTERNATIONAL_SUPPORT_LDFLAGS = -framework InternationalSupport;
> +
> +OTHER_LDFLAGS_BASE = $(OTHER_LDFLAGS_HIDE_SYMBOLS) -iframework $(SDK_DIR)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks -force_load "$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/libWTF.a" $(WK_INTERNATIONAL_SUPPORT_LDFLAGS);

Why is this necessary? The only change to LanguageCocoa.mm is a comment.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:486
> +    if (!testingMode() && MediaAccessibilityLibrary()) {
> +        auto captionLanguages = adoptCF(MACaptionAppearanceCopySelectedLanguages(kMACaptionAppearanceDomainUser));
> +        CFIndex captionLanguagesCount = captionLanguages ? CFArrayGetCount(captionLanguages.get()) : 0;
> +        if (captionLanguagesCount) {
> +            Vector<String> temp;
> +            temp.reserveInitialCapacity(captionLanguagesCount + preferredLanguages.size());
> +            for (CFIndex i = 0; i < captionLanguagesCount; i++)
> +                temp.uncheckedAppend(static_cast<CFStringRef>(CFArrayGetValueAtIndex(captionLanguages.get(), i)));
> +            temp.appendVector(WTFMove(preferredLanguages));
> +            preferredLanguages = WTFMove(temp);
>          }
>      }

I think WebKit prefers early return style like the red code below this, rather than the triangle style here.

> LayoutTests/media/modern-media-controls/tracks-support/sorted-by-user-preferred-languages.html:36
> +    shouldBecomeDifferent("shadowRoot.querySelector('button.tracks')", "null", () => {

This is a big scary. Does this only work because we're inside a test?

> WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101500/InternationalSupport.framework/InternationalSupport.tbd:7
> +--- !tapi-tbd-v3
> +archs:           [ x86_64 ]
> +install-name:    '/System/Library/PrivateFrameworks/InternationalSupport.framework/InternationalSupport'
> +platform: zippered
> +exports:
> +  - archs:           [ x86_64 ]
> +...

Are you sure you need these .tbd files? I don't think we're calling anything additional from InternationalSupport in this patch.

[Myles C. Maxfield]

Comment on attachment 429616 [details]
Patch

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

>> LayoutTests/media/modern-media-controls/tracks-support/sorted-by-user-preferred-languages.html:36
>> +    shouldBecomeDifferent("shadowRoot.querySelector('button.tracks')", "null", () => {
> 
> This is a big scary. Does this only work because we're inside a test?

*bit scary

[Eric Carlson]

Comment on attachment 429616 [details]
Patch

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

The media specific parts look fine to me modulo the comment about the test.

> LayoutTests/media/modern-media-controls/tracks-support/sorted-by-user-preferred-languages.html:9
> +    <track src="../../lorem-ipsum.vtt" kind="captions"     srclang="en" label="en">

As we discussed, I think it would be less confusing to not use the same string for `label and `srclang`.

[Devin Rousso]

Comment on attachment 429616 [details]
Patch

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

>> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:35
>> +OTHER_LDFLAGS_BASE = $(OTHER_LDFLAGS_HIDE_SYMBOLS) -iframework $(SDK_DIR)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks -force_load "$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/libWTF.a" $(WK_INTERNATIONAL_SUPPORT_LDFLAGS);
> 
> Why is this necessary? The only change to LanguageCocoa.mm is a comment.

I was using this in previous patches to get non-internal EWS working.  It may not be necessary anymore.  Will try without it.

>>> LayoutTests/media/modern-media-controls/tracks-support/sorted-by-user-preferred-languages.html:36
>>> +    shouldBecomeDifferent("shadowRoot.querySelector('button.tracks')", "null", () => {
>> 
>> This is a big scary. Does this only work because we're inside a test?
> 
> *bit scary

Yes this is from `Internals::shadowRoot`.  We use this extensively in media controls tests.  It is not exposed to the web.

>> WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101500/InternationalSupport.framework/InternationalSupport.tbd:7
>> +...
> 
> Are you sure you need these .tbd files? I don't think we're calling anything additional from InternationalSupport in this patch.

ditto (JavaScriptCore.xcconfig:35)

[Devin Rousso]

Created attachment 429694 [details]
Patch

[EWS]

Committed r278064 (238145@main): <https://commits.webkit.org/238145@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78484571>