Bug 118319

Summary: [Mac] .webkitClosedCaptionsVisible doesn't work with "Automatic" caption mode
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, commit-queue, dfarler, dino, esprehn+autocc, glenn, jer.noble
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 118333    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Patch that causes fewer regressions!
none
Patch that causes fewer regressions, AND with a complete ChangeLog! none

Eric Carlson
Reported 2013-07-02 12:40:39 PDT
Setting video.webkitClosedCaptionsVisible = true has no effect when the caption menu is in "Automatic" mode.
Attachments
Proposed patch (6.16 KB, patch)
2013-07-02 12:52 PDT, Eric Carlson
no flags
Patch that causes fewer regressions! (7.55 KB, patch)
2013-07-03 19:11 PDT, Eric Carlson
no flags
Patch that causes fewer regressions, AND with a complete ChangeLog! (8.11 KB, patch)
2013-07-03 19:22 PDT, Eric Carlson
no flags
Eric Carlson
Comment 1 2013-07-02 12:41:43 PDT
Eric Carlson
Comment 2 2013-07-02 12:52:20 PDT
Created attachment 205938 [details] Proposed patch
Eric Carlson
Comment 3 2013-07-02 13:30:47 PDT
WebKit Commit Bot
Comment 4 2013-07-02 17:27:30 PDT
Re-opened since this is blocked by bug 118333
David Farler
Comment 5 2013-07-02 17:34:58 PDT
*** Bug 118332 has been marked as a duplicate of this bug. ***
David Farler
Comment 6 2013-07-02 17:36:22 PDT
Eric Carlson
Comment 7 2013-07-03 19:11:37 PDT
Created attachment 206044 [details] Patch that causes fewer regressions!
Eric Carlson
Comment 8 2013-07-03 19:22:46 PDT
Created attachment 206045 [details] Patch that causes fewer regressions, AND with a complete ChangeLog!
Jer Noble
Comment 9 2013-07-05 11:20:45 PDT
Comment on attachment 206045 [details] Patch that causes fewer regressions, AND with a complete ChangeLog! View in context: https://bugs.webkit.org/attachment.cgi?id=206045&action=review > Source/WebCore/html/HTMLMediaElement.cpp:301 > , m_closedCaptionsVisible(false) > + , m_webkitClosedCaptionsVisible(false) It seems awkward to have these two ivars named so similarly. How about m_webkitLegacyClosedCaptionOverride? > Source/WebCore/html/HTMLMediaElement.cpp:4530 > + m_webkitClosedCaptionsVisible = visible; This would be: m_webkitLegacyClosedCaptionOverride = visible; > Source/WebCore/html/HTMLMediaElement.cpp:4532 > + m_webkitClosedCaptionsVisible = m_closedCaptionsVisible; You can remove this line if... (ctd) > Source/WebCore/html/HTMLMediaElement.cpp:4537 > - return m_closedCaptionsVisible; > + return m_webkitClosedCaptionsVisible; You rename this: m_closedCaptionsVisible && m_webkitLegacyClosedCaptionOverride;
Eric Carlson
Comment 10 2013-07-05 13:37:55 PDT
(In reply to comment #9) > (From update of attachment 206045 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206045&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:301 > > , m_closedCaptionsVisible(false) > > + , m_webkitClosedCaptionsVisible(false) > > It seems awkward to have these two ivars named so similarly. How about m_webkitLegacyClosedCaptionOverride? > > > Source/WebCore/html/HTMLMediaElement.cpp:4530 > > + m_webkitClosedCaptionsVisible = visible; > > This would be: m_webkitLegacyClosedCaptionOverride = visible; > > > Source/WebCore/html/HTMLMediaElement.cpp:4532 > > + m_webkitClosedCaptionsVisible = m_closedCaptionsVisible; > > You can remove this line if... (ctd) > > > Source/WebCore/html/HTMLMediaElement.cpp:4537 > > - return m_closedCaptionsVisible; > > + return m_webkitClosedCaptionsVisible; > > You rename this: m_closedCaptionsVisible && m_webkitLegacyClosedCaptionOverride; All good suggestions, I will make them before landing. Thanks!
Eric Carlson
Comment 11 2013-07-05 13:38:03 PDT
Ahmad Saleem
Comment 12 2023-10-14 09:18:25 PDT
Landed finally with this but didn't get marked as 'RESOLVED FIXED': https://github.com/WebKit/WebKit/commit/97a3125e7fc58bc3583d6ca841d59eb507fe1dcf
Note You need to log in before you can comment on or make changes to this bug.