Summary: | REGRESSION: Caption Menus show language codes instead of display names | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||
Component: | Media | Assignee: | 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
Brent Fulgham
2015-04-02 16:11:14 PDT
Created attachment 250018 [details]
Patch
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 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! Created attachment 250020 [details]
Patch
Committed r182298: <http://trac.webkit.org/changeset/182298> 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. |