Bug 118319 - [Mac] .webkitClosedCaptionsVisible doesn't work with "Automatic" caption mode
Summary: [Mac] .webkitClosedCaptionsVisible doesn't work with "Automatic" caption mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
: 118332 (view as bug list)
Depends on: 118333
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-02 12:40 PDT by Eric Carlson
Modified: 2023-10-14 09:18 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (6.16 KB, patch)
2013-07-02 12:52 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch that causes fewer regressions! (7.55 KB, patch)
2013-07-03 19:11 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch that causes fewer regressions, AND with a complete ChangeLog! (8.11 KB, patch)
2013-07-03 19:22 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-07-02 12:40:39 PDT
Setting video.webkitClosedCaptionsVisible = true has no effect when the caption menu is in "Automatic" mode.
Comment 1 Eric Carlson 2013-07-02 12:41:43 PDT
<rdar://problem/13834460>
Comment 2 Eric Carlson 2013-07-02 12:52:20 PDT
Created attachment 205938 [details]
Proposed patch
Comment 3 Eric Carlson 2013-07-02 13:30:47 PDT
Committed in r152318: https://trac.webkit.org/r152318
Comment 4 WebKit Commit Bot 2013-07-02 17:27:30 PDT
Re-opened since this is blocked by bug 118333
Comment 5 David Farler 2013-07-02 17:34:58 PDT
*** Bug 118332 has been marked as a duplicate of this bug. ***
Comment 6 David Farler 2013-07-02 17:36:22 PDT
<rdar://problem/14339340>
Comment 7 Eric Carlson 2013-07-03 19:11:37 PDT
Created attachment 206044 [details]
Patch that causes fewer regressions!
Comment 8 Eric Carlson 2013-07-03 19:22:46 PDT
Created attachment 206045 [details]
Patch that causes fewer regressions, AND with a complete ChangeLog!
Comment 9 Jer Noble 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;
Comment 10 Eric Carlson 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!
Comment 11 Eric Carlson 2013-07-05 13:38:03 PDT
Committed in r152422: https://trac.webkit.org/r152422
Comment 12 Ahmad Saleem 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