Bug 112800

Summary: Allow port specific text track menu
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dino, esprehn+autocc, feature-media-reviews, jer.noble, ojan.autocc, rniwa, webkit-bug-importer, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
webkit-ews: commit-queue-
Update patch.
buildbot: commit-queue-
Updated patch.
buildbot: commit-queue-
Updated patch. none

Description Eric Carlson 2013-03-20 06:17:48 PDT 
Not every port will want the text track menu to look or behave the same.
Comment 1 Radar WebKit Bug Importer 2013-03-20 06:17:58 PDT 
<rdar://problem/13461749>
Comment 2 Eric Carlson 2013-03-20 06:27:29 PDT 
Created attachment 194036 [details]
Proposed patch
Comment 3 WebKit Review Bot 2013-03-20 06:29:54 PDT 
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.
Comment 4 Early Warning System Bot 2013-03-20 06:32:23 PDT 
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
Comment 5 Early Warning System Bot 2013-03-20 06:34:13 PDT 
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
Comment 6 Build Bot 2013-03-20 06:45:53 PDT 
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
Comment 7 Eric Carlson 2013-03-20 06:46:56 PDT 
Created attachment 194041 [details]
Update patch.
Comment 8 Build Bot 2013-03-20 06:59:24 PDT 
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
Comment 9 Build Bot 2013-03-20 07:16:39 PDT 
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
Comment 10 Eric Carlson 2013-03-20 07:49:03 PDT 
Created attachment 194054 [details]
Updated patch.
Comment 11 Build Bot 2013-03-20 08:19:49 PDT 
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
Comment 12 Build Bot 2013-03-20 08:40:16 PDT 
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
Comment 13 Eric Carlson 2013-03-20 08:42:49 PDT 
Created attachment 194067 [details]
Updated patch.
Comment 14 Dean Jackson 2013-03-20 11:50:37 PDT 
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
Comment 15 Eric Carlson 2013-03-20 13:19:34 PDT 
(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!
Comment 16 Eric Carlson 2013-03-20 13:19:59 PDT 
Committed in r146380 : https://trac.webkit.org/r146380