[webkit-reviews] review granted: [Bug 113822] [Mac] add "automatic" text track menu item : [Attachment 196236] Updated patch.

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 113822: [Mac] add "automatic" text track menu item
https://bugs.webkit.org/show_bug.cgi?id=113822

Attachment 196236: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=196236&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
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.