Bug 112800 – Allow port specific text track menu

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

[Eric Carlson]

Not every port will want the text track menu to look or behave the same.

[Radar WebKit Bug Importer]

<rdar://problem/13461749>

[Eric Carlson]

Created attachment 194036 [details]
Proposed patch

[WebKit Review Bot]

Attachment 194036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/track/track-user-preferences-expected.txt', u'LayoutTests/media/track/track-user-preferences.html', u'LayoutTests/media/video-controls-captions-trackmenu-localized.html', u'LayoutTests/media/video-controls-captions-trackmenu-sorted.html', u'LayoutTests/media/video-controls-captions-trackmenu.html', u'LayoutTests/platform/mac/media/video-controls-captions-trackmenu-expected.txt', u'LayoutTests/platform/mac/media/video-controls-captions-trackmenu-localized-expected.txt', u'LayoutTests/platform/mac/media/video-controls-captions-trackmenu-sorted-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/English.lproj/Localizable.strings', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/shadow/MediaControlElements.cpp', u'Source/WebCore/html/shadow/MediaControlElements.h', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/TextTrack.cpp', u'Source/WebCore/html/track/TextTrack.h', u'Source/WebCore/html/track/TextTrackList.cpp', u'Source/WebCore/html/track/TextTrackList.h', u'Source/WebCore/page/CaptionUserPreferences.cpp', u'Source/WebCore/page/CaptionUserPreferences.h', u'Source/WebCore/page/CaptionUserPreferencesMac.h', u'Source/WebCore/page/CaptionUserPreferencesMac.mm', u'Source/WebCore/platform/Language.cpp', u'Source/WebCore/platform/LocalizedStrings.cpp', u'Source/WebCore/platform/LocalizedStrings.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivate.h', u'Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.h', u'Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.mm', u'Source/WebCore/testing/InternalSettings.cpp']" exit_code: 1
Source/WebCore/page/CaptionUserPreferences.h:80:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 194036 [details]
Proposed patch

Attachment 194036 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17200462

[Early Warning System Bot]

Comment on attachment 194036 [details]
Proposed patch

Attachment 194036 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17195372

[Build Bot]

Comment on attachment 194036 [details]
Proposed patch

Attachment 194036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17214495

[Eric Carlson]

Created attachment 194041 [details]
Update patch.

[Build Bot]

Comment on attachment 194041 [details]
Update patch.

Attachment 194041 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17246053

[Build Bot]

Comment on attachment 194041 [details]
Update patch.

Attachment 194041 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17137556

[Eric Carlson]

Created attachment 194054 [details]
Updated patch.

[Build Bot]

Comment on attachment 194054 [details]
Updated patch.

Attachment 194054 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17136708

New failing tests:
media/video-controls-captions-trackmenu-localized.html

[Build Bot]

Comment on attachment 194054 [details]
Updated patch.

Attachment 194054 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17190697

[Eric Carlson]

Created attachment 194067 [details]
Updated patch.

[Dean Jackson]

Comment on attachment 194067 [details]
Updated patch.

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

> Source/WebCore/html/HTMLMediaElement.cpp:3199
> +        else {
>              track->setMode(TextTrack::showingKeyword());
> -            if (captionPreferences && track->language().length())
> -                captionPreferences->setPreferredLanguage(track->language());
>          }

Nit: does this end up with a single line else {} ?

> Source/WebCore/html/HTMLMediaElement.cpp:3206
> +    CaptionUserPreferences* captionPreferences = document()->page() ? document()->page()->group().captionPreferences() : 0;
> +    if (captionPreferences) {
> +        captionPreferences->setShouldShowCaptions(trackToSelect);
> +        if (trackToSelect && trackToSelect->language().length())
> +            captionPreferences->setPreferredLanguage(trackToSelect->language());

Should this be wrapped in a PLATFORM(MAC) in case not every platform wants to set the preferred language based on the selected track?

> Source/WebCore/html/shadow/MediaControlElements.cpp:887
> +        menuItem->setAttribute(trackIndexAttributeName(), String::number(i), ASSERT_NO_EXCEPTION);

Do we still need this now we have the menu to track map?

> Source/WebCore/page/CaptionUserPreferencesMac.mm:123
> +        return CaptionUserPreferences::shouldShowCaptions();
> +    
>      return MACaptionAppearanceGetShowCaptions(kMACaptionAppearanceDomainUser);

Nit: I think you're adding whitespace to the line here.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:133
> +        CaptionUserPreferences::setShouldShowCaptions(preference);
>          return;
>      }
> -
> +    
>      MACaptionAppearanceSetShowCaptions(kMACaptionAppearanceDomainUser, preference);

Nit: and maybe here.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:528
> +#endif  // HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
> +    
> +static String trackDisplayName(TextTrack* track)

Nit: and maybe there too.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:607
> +    bool aIsPreferredLanguage = !codePointCompare(aLanguageDisplayName, preferredLanguageDisplayName);

Can you use equalIgnoringCase here?

> Source/WebCore/page/CaptionUserPreferencesMac.mm:610
> +        return aIsPreferredLanguage ? true : false;

return aIsPreferredLanguage;

> Source/WebCore/page/CaptionUserPreferencesMac.mm:620
> +        return aIsMainContent ? true : false;

return aIsMainContent;

> Source/WebCore/page/CaptionUserPreferencesMac.mm:627
> +            return aIsMainContent ? true : false;

ditto

> Source/WebCore/page/CaptionUserPreferencesMac.mm:629
> +            return bIsMainContent ? true : false;

ditto

> Source/WebCore/platform/Language.cpp:160
> -    if (!localeName.isNull() && !localeName.isEmpty())
> -        return CFLocaleCopyDisplayNameForPropertyValue(CFLocaleCopyCurrent(), kCFLocaleIdentifier, localeName.createCFString().get());
> +    if (!localeName.isNull() && !localeName.isEmpty()) {
> +        RetainPtr<CFLocaleRef> currentLocale(AdoptCF, CFLocaleCopyCurrent());
> +        return CFLocaleCopyDisplayNameForPropertyValue(currentLocale.get(), kCFLocaleIdentifier, localeName.createCFString().get());
> +    }

My bad :(

> Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:59
>      virtual Kind kind() const { return Subtitles; }
> +
>      virtual bool isClosedCaptions() const { return false; }

Super nit: you didn't separate these in InbandTextTrackPrivateAVFObjC.h

[Eric Carlson]

(In reply to comment #14)
> (From update of attachment 194067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194067&action=review
> 
> Nit: does this end up with a single line else {} ?
> 
Fixed.

> Should this be wrapped in a PLATFORM(MAC) in case not every platform wants to set the preferred language based on the selected track?
> 
I ended up leaving this as-is because no other port implements the track menu yet and I figured anyone that doesn't want this behavior can change it when they do.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:887
> > +        menuItem->setAttribute(trackIndexAttributeName(), String::number(i), ASSERT_NO_EXCEPTION);
> 
> Do we still need this now we have the menu to track map?
> 
Yes, for the "Off" menu item. I thought about creating a static TextTrack for it but decided not to. I might come back to this.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:123
> > +        return CaptionUserPreferences::shouldShowCaptions();
> > +    
> >      return MACaptionAppearanceGetShowCaptions(kMACaptionAppearanceDomainUser);
> 
> Nit: I think you're adding whitespace to the line here.
> 
Fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:133
> > +        CaptionUserPreferences::setShouldShowCaptions(preference);
> >          return;
> >      }
> > -
> > +    
> >      MACaptionAppearanceSetShowCaptions(kMACaptionAppearanceDomainUser, preference);
> 
> Nit: and maybe here.
> 
Fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:528
> > +#endif  // HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
> > +    
> > +static String trackDisplayName(TextTrack* track)
> 
> Nit: and maybe there too.
> 
Fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:610
> > +        return aIsPreferredLanguage ? true : false;
> 
> return aIsPreferredLanguage;
> 
Duh - fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:620
> > +        return aIsMainContent ? true : false;
> 
> return aIsMainContent;
> 
Fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:627
> > +            return aIsMainContent ? true : false;
> 
> ditto
> 
Fixed.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:629
> > +            return bIsMainContent ? true : false;
> 
> ditto
> 
Fixed.

> > Source/WebCore/platform/graphics/InbandTextTrackPrivate.h:59
> >      virtual Kind kind() const { return Subtitles; }
> > +
> >      virtual bool isClosedCaptions() const { return false; }
> 
> Super nit: you didn't separate these in InbandTextTrackPrivateAVFObjC.h
>
Fixed.

Thanks!

[Eric Carlson]

Committed in r146380 : https://trac.webkit.org/r146380