Bug 143350

Summary: REGRESSION: Caption Menus show language codes instead of display names
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Critical CC: bfulgham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch jer.noble: review+

Description Brent Fulgham 2015-04-02 16:11:14 PDT
The menu of audio tracks presented by WebKit is showing language codes (e.g., "de-DE", or "fr-CA"), instead of display names (e.g, "German (Germany)" or "French (Canada)".

This seems to have been introduced in <http://trac.webkit.org/r174402>.
Comment 1 Brent Fulgham 2015-04-02 16:11:47 PDT
<rdar://problem/20094145>
Comment 2 Brent Fulgham 2015-04-02 16:21:48 PDT
Created attachment 250018 [details]
Patch
Comment 3 Jer Noble 2015-04-02 16:27:29 PDT
Comment on attachment 250018 [details]
Patch

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

r=me, with possible nit.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:611
> +    String label = track->label();
> +    String trackLanguageIdentifier = track->language();
> +
> +    RetainPtr<CFLocaleRef> currentLocale = adoptCF(CFLocaleCreate(kCFAllocatorDefault, defaultLanguage().createCFString().get()));
> +    RetainPtr<CFStringRef> localeIdentifier = adoptCF(CFLocaleCreateCanonicalLocaleIdentifierFromString(kCFAllocatorDefault, trackLanguageIdentifier.createCFString().get()));
> +    RetainPtr<CFStringRef> languageCF = adoptCF(CFLocaleCopyDisplayNameForPropertyValue(currentLocale.get(), kCFLocaleLanguageCode, localeIdentifier.get()));
> +    String language = languageCF.get();

This looks like duplicated code from trackDisplayName(AudioTrack*).  Should we pull this into buildStringForLocaleAndLanguage()? (And perhaps rename it to buildStringForDefaultLocaleAndLanguage()?)
Comment 4 Brent Fulgham 2015-04-02 16:30:34 PDT
Comment on attachment 250018 [details]
Patch

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

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:611
>> +    String language = languageCF.get();
> 
> This looks like duplicated code from trackDisplayName(AudioTrack*).  Should we pull this into buildStringForLocaleAndLanguage()? (And perhaps rename it to buildStringForDefaultLocaleAndLanguage()?)

Sure!
Comment 5 Brent Fulgham 2015-04-02 16:45:51 PDT
Created attachment 250020 [details]
Patch
Comment 6 Brent Fulgham 2015-04-02 17:27:54 PDT
Committed r182298: <http://trac.webkit.org/changeset/182298>
Comment 7 Darin Adler 2015-04-03 13:22:44 PDT
Comment on attachment 250020 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:582
> +static String trackDisplayName(AudioTrack* track)

Should take a reference.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:588
> +        displayName.append(audioTrackNoLabelText());

Should just return here. No real reason to append to an empty string builder and then convert back into a string.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:593
> +String CaptionUserPreferencesMediaAF::displayNameForTrack(AudioTrack* track) const

Should take a reference.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:598
> +static String trackDisplayName(TextTrack* track)

Should take a reference.