Summary: | [Mac] add "automatic" text track menu item | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, buildbot, dino, esprehn+autocc, feature-media-reviews, jer.noble, ojan.autocc, rniwa, webkit-bug-importer, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Eric Carlson
2013-04-02 13:44:02 PDT
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. |