Bug 225900 – [Modern Media Controls] give more weight to tracks that are in non-default languages configured by the user

• Status: RESOLVED
• Resolution: MOVED
• Component: Media

[Devin Rousso]

e.g. if the user has both english (default) and spanish (alternate) languages configured, tracks that are in spanish should be weighted higher than french

[Radar WebKit Bug Importer]

<rdar://problem/78130864>

[Devin Rousso]

Created attachment 428903 [details]
Patch

[Peng Liu]

Comment on attachment 428903 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        that are in Spanish should be weighted higher than French.

I guess you mean "English" here. :-)

[Devin Rousso]

Comment on attachment 428903 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        that are in Spanish should be weighted higher than French.
> 
> I guess you mean "English" here. :-)

Actually no.  Well yes.  Kinda.  English would be ranked the highest.  Prior to this change, Spanish would be ranked lower than French since it's alphabetically after.  This change makes it so that Spanish would rank higher than French (but still lower than English) because it's a non-default configured language.

[Darin Adler]

Comment on attachment 428903 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:629
> +    RetainPtr<CFLocaleRef> currentLocale = adoptCF(CFLocaleCreate(kCFAllocatorDefault, defaultLanguage.createCFString().get()));

auto

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:630
>      RetainPtr<CFStringRef> localeIdentifier = adoptCF(CFLocaleCreateCanonicalLocaleIdentifierFromString(kCFAllocatorDefault, trackLanguageIdentifier.createCFString().get()));

auto

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:631
>      RetainPtr<CFStringRef> languageCF = adoptCF(CFLocaleCopyDisplayNameForPropertyValue(currentLocale.get(), kCFLocaleLanguageCode, localeIdentifier.get()));

auto

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:670
> +            const auto& textTrack = downcast<TextTrack>(track);

Just auto& is fine. It will be const& without explicitly writing that since track is a const&.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:770
> +        bool aIsPreferredLanguage = !codePointCompare(aLanguageDisplayName, preferredLanguageDisplayName);
> +        bool bIsPreferredLanguage = !codePointCompare(bLanguageDisplayName, preferredLanguageDisplayName);

There is no need to use !codePointCompare to check if two strings are exactly equal. The "==" operator does that and is more efficient than codePointCompare. The codePointCompare function is for operations like sorting, when we want to sort by code points and not in a language sensitive way.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:772
> +        if (aIsPreferredLanguage != bIsPreferredLanguage)
> +            return aIsPreferredLanguage;

This says that preferred languages should be sorted in the order of the preferredLanguages array. Is that what we want?

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:792
> +    if (auto trackDisplayComparison = codePointCompare(displayNameForTrack(&a), displayNameForTrack(&b)))

It’s not appropriate to use codePointCompare for sorting user-visible display names. The whole point of codePointCompare doesn’t do the language-rule-sensitive ordering we’d want for user interface. To compare things you’d see in a UI we’d want a compare designed for that purpose. In WTF we have Collator::collate for this sort of use, although I don’t see much use of it.

[Devin Rousso]

Created attachment 429031 [details]
Patch

[Darin Adler]

Comment on attachment 429031 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferences.cpp:241
> +    Collator collator;

Thanks for taking my pointer: Does this get the correct language for the collator? Does it run in the web process? If so, how does it inherit the appropriate language from the UI process?

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:770
> +    for (auto& preferredLanguage : userPreferredLanguages()) {
> +        String preferredLanguageDisplayName = displayNameForLanguageLocale(languageIdentifier(preferredLanguage));
> +        bool aIsPreferredLanguage = aLanguageDisplayName == preferredLanguageDisplayName;
> +        bool bIsPreferredLanguage = bLanguageDisplayName == preferredLanguageDisplayName;
> +        if (aIsPreferredLanguage != bIsPreferredLanguage)
> +            return aIsPreferredLanguage;
> +    }

I don’t understand the rule very well.

Is the idea that we sort the preferred languages in the order they appear in the userPreferredLanguages()? If so, then I think the code implements that rule.

Or maybe we sort all the preferred languages to the top of the menu, but we want them in an appropriate order.

[Devin Rousso]

Comment on attachment 429031 [details]
Patch

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

>> Source/WebCore/page/CaptionUserPreferences.cpp:241
>> +    Collator collator;
> 
> Thanks for taking my pointer: Does this get the correct language for the collator? Does it run in the web process? If so, how does it inherit the appropriate language from the UI process?

Thanks for the suggestion!  Switching my language to Spanish in the Language & Region view of System Preferences does cause `Collator` to use "es-US", so I think so?  FWIW the only other usage I see of it is also inside WebCore (`xsltUnicodeSortFunction`).

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:770
>> +    }
> 
> I don’t understand the rule very well.
> 
> Is the idea that we sort the preferred languages in the order they appear in the userPreferredLanguages()? If so, then I think the code implements that rule.
> 
> Or maybe we sort all the preferred languages to the top of the menu, but we want them in an appropriate order.

I think I actually misread my own logging when I was writing this change.  I thought `userPreferredLanguages` would return the list in the Language & Region view of System Preferences, but it actually just appears to return all the different variants for only the select item (e.g. I'd added Spanish as my new primary language and misread the output `["es-US", "es"]`).  As such, I'm going to close this bug and make new bugs for the changes I still think we should do.

FWIW my goal was to prefer things that are in the list over things that are not (as well as preferring things earlier/higher in the last than later/lower) as previously we only cared about the currently selected language.

[Devin Rousso]

I'll handle this in <https://webkit.org/b/226038>