Bug 118319 – [Mac] .webkitClosedCaptionsVisible doesn't work with "Automatic" caption mode

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Setting video.webkitClosedCaptionsVisible = true has no effect when the caption menu is in "Automatic" mode.

[Eric Carlson]

<rdar://problem/13834460>

[Eric Carlson]

Created attachment 205938 [details]
Proposed patch

[Eric Carlson]

Committed in r152318: https://trac.webkit.org/r152318

[WebKit Commit Bot]

Re-opened since this is blocked by bug 118333

[David Farler]

*** Bug 118332 has been marked as a duplicate of this bug. ***

[David Farler]

<rdar://problem/14339340>

[Eric Carlson]

Created attachment 206044 [details]
Patch that causes fewer regressions!

[Eric Carlson]

Created attachment 206045 [details]
Patch that causes fewer regressions, AND with a complete ChangeLog!

[Jer Noble]

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]

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

Committed in r152422: https://trac.webkit.org/r152422

[Ahmad Saleem]

Landed finally with this but didn't get marked as 'RESOLVED FIXED': https://github.com/WebKit/WebKit/commit/97a3125e7fc58bc3583d6ca841d59eb507fe1dcf