Bug 113822

<rdar://problem/13559894>

Created attachment 196228 [details] Proposed patch

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

Created attachment 196236 [details] Updated patch.

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.

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

(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.

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