Bug 113822 – [Mac] add "automatic" text track menu item

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

[Eric Carlson]

Add an "Automatic" item to the text track menu which will automatically enable a captions/subtitles track when the main audio track in a movie is not in the user's default language.

[Radar WebKit Bug Importer]

<rdar://problem/13559894>

[Eric Carlson]

Created attachment 196228 [details]
Proposed patch

[Build Bot]

Comment on attachment 196228 [details]
Proposed patch

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

[Eric Carlson]

Created attachment 196236 [details]
Updated patch.

[Jer Noble]

Comment on attachment 196236 [details]
Updated patch.

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

> Source/WebCore/html/HTMLMediaElement.cpp:626
> -    if (!m_loadTimer.isActive())
>          m_loadTimer.startOneShot(0);

Nit: whitespace.

> Source/WebCore/html/HTMLMediaElement.cpp:3151
> +        if (!trackList->contains(trackToSelect))
> +            return;

Should this have an "ASSERT(trackList->contains(trackToSelect))", or is it expected behavior to occasionally get a request to select a non-present track?

> Source/WebCore/html/HTMLMediaElement.h:118
> -        LoadTextTrackResource = 1 << 1,
> -        TextTrackChangesNotification = 1 << 2
> +        ConfigureTextTracks = 1 << 1,
> +        TextTrackChangesNotification = 1 << 2,
> +        ConfigureTextTrackDisplay = 1 << 3

Nit: I prefer to add a comma after the last item in an enum, so that if values are added in the future, the diff is cleaner.  But YMMV.

> Source/WebCore/html/shadow/MediaControlsApple.cpp:310
>  void MediaControlsApple::changedClosedCaptionsVisibility()
>  {
>      MediaControls::changedClosedCaptionsVisibility();
> -    if (m_closedCaptionsTrackList)
> -        m_closedCaptionsTrackList->resetTrackListMenu();
> +    if (m_closedCaptionsContainer && m_closedCaptionsContainer->isShowing())
> +        m_closedCaptionsContainer->hide();
> +

Shouldn't you be checking whether the closed captions are now visible or not before hiding the captions container?

> Source/WebCore/page/CaptionUserPreferences.h:85
> +    void setPrimaryAudioTrackLanguageOverride(const String& language) { m_primaryAudioTrackLanguageOverride = language;  }

This doesn't seem to be called from anywhere.  Is there a future patch coming which will set this value?  Also, these methods are not in the ChangeLog.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:140
> +    default:
> +        ASSERT_NOT_REACHED();
> +            break;

Nit: whitespace.

Technically, you can either return inside the default statement, or move the ASSERT out of the switch.  It seems just a little weird to break up the conditional this way.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:-603
> -static String languageIdentifier(const String& languageCode)
> -{
> -    if (languageCode.isEmpty())
> -        return languageCode;
> -
> -    String lowercaseLanguageCode = languageCode.lower();
> -
> -    // Need 2U here to disambiguate String::operator[] from operator(NSString*, int)[] in a production build.
> -    if (lowercaseLanguageCode.length() >= 3 && (lowercaseLanguageCode[2U] == '_' || lowercaseLanguageCode[2U] == '-'))
> -        lowercaseLanguageCode.truncate(2);
> -    
> -    return lowercaseLanguageCode;
> -}
> -

Nit: I realize this function was just moved up, but one alternative would have been to add a declaration for this function at the top of the file.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:659
> +        if (testingMode()) {
> +            audioTrackLanguage = primaryAudioTrackLanguageOverride();
> +        } else {

Nit: no braces needed here.

[Eric Carlson]

https://trac.webkit.org/r147675

[Benjamin Poulain]

This broke 2 media tests on the bots (WK1 and WK2): http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r147675%20(8684)/results.html

[Jer Noble]

(In reply to comment #7)
> This broke 2 media tests on the bots (WK1 and WK2): http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r147675%20(8684)/results.html

Looks like they just need rebaselines.

[Eric Carlson]

New results and and updated test landed in https://trac.webkit.org/r147679.