Bug 143350 - REGRESSION: Caption Menus show language codes instead of display names
Summary: REGRESSION: Caption Menus show language codes instead of display names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-02 16:11 PDT by Brent Fulgham
Modified: 2015-04-03 13:22 PDT (History)
2 users (show)

See Also:

Attachments
Patch (9.86 KB, patch)
2015-04-02 16:21 PDT, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2015-04-02 16:45 PDT, Brent Fulgham 		jer.noble: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.