Bug 111934 – Caption menu does not include in-band captions

• Status: CLOSED
• Resolution: FIXED
• Component: Media

[Chris Petersen]

In Full screen video mode, balloon icon appears on toolbar but is non functional. I'm noticing this icon on toolbar with NB r145321. Perhaps this is work in progress but just noticed it.

1) Launch NB r145321
2) Go to http://www.apple.com/ipad-mini/overview/. Click on Watch This Video
3) After movie starts to play, click on full screen icon to enter full screen video mode
4) After entering full screen mode, notice the "balloon" icon appearing on toolbar ( to the left of full screen icon )
5) Clicking this balloon icon on the toolbar doesn't appears to do anything ( at least to me )

[Chris Petersen]

Created attachment 192369 [details]
screen shot of full screen video mode

[Eric Carlson]

Created attachment 200313 [details]
Proposed patch

[Eric Carlson]

<rdar://problem/13209948>

[Early Warning System Bot]

Comment on attachment 200313 [details]
Proposed patch

Attachment 200313 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/376313

[Early Warning System Bot]

Comment on attachment 200313 [details]
Proposed patch

Attachment 200313 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/376314

[Eric Carlson]

Created attachment 200318 [details]
Updated patch

[Jer Noble]

Comment on attachment 200318 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200313&action=review

Looks good generally, but I've got reservations about m_legacyWEbKitClosedCaptionsVisible.  See below.

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).
> +

Oops.

> Source/WebCore/html/HTMLMediaElement.cpp:846
> +    m_pendingActionFlags &= ~LoadMediaResource;

I know exactly what this does, but in the past I've gotten review comments where this construct was too confusing.  We should make a CLEAR_FLAG() and SET_FLAG() macro/inline that does this.

> Source/WebCore/html/HTMLMediaElement.cpp:4457
> +    m_legacyWebKitClosedCaptionsVisible = visible;
>      setClosedCaptionsVisible(visible);
> +    m_legacyWebKitClosedCaptionsVisible = m_haveVisibleTextTrack;

This is weird.  I assume that m_haveVisibleTextTrack being set is a side effect of setClosedCaptionsVisible?

> Source/WebCore/html/HTMLMediaElement.cpp:4462
> -    return closedCaptionsVisible();
> +    return m_legacyWebKitClosedCaptionsVisible;

So you can call setWebkitClosedCaptionsVisible(TRUE) and have webkitClosedCaptionsVisible() immediately return FALSE?  That's odd.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:622
> +    bool legacyOverride = mediaElement->webkitClosedCaptionsVisible();

If webkitClosedCaptionsVisible() is real being used as a "legacyOverride" can we rename it (or add a new function) "legacyClosedCaptionsShouldOverrideVisibility()" or something?

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66
> +    virtual bool isLegacyCCTrack() const = 0;

Since there's already a isClosedCaptions(), this should be "isLegacyClosedCaptions()" or "isLegacyClosedCaptionsTrack()".

> Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateLegacyAVFObjC.h:59
> +    virtual bool isLegacyCCTrack() const OVERRIDE { return true; }

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:179
> +    void processLegacyCCTracks();

Same thing, expand CC -> ClosedCaption.

[Eric Carlson]

Created attachment 200321 [details]
Updated patch

[Eric Carlson]

(In reply to comment #7)
> (From update of attachment 200318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200313&action=review
> 
> Looks good generally, but I've got reservations about m_legacyWEbKitClosedCaptionsVisible.  See below.
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests (OOPS!).
> > +
> 
> Oops.
> 
Fixed

> > Source/WebCore/html/HTMLMediaElement.cpp:846
> > +    m_pendingActionFlags &= ~LoadMediaResource;
> 
> I know exactly what this does, but in the past I've gotten review comments where this construct was too confusing.  We should make a CLEAR_FLAG() and SET_FLAG() macro/inline that does this.
> 
Fixed

> > Source/WebCore/html/HTMLMediaElement.cpp:4457
> > +    m_legacyWebKitClosedCaptionsVisible = visible;
> >      setClosedCaptionsVisible(visible);
> > +    m_legacyWebKitClosedCaptionsVisible = m_haveVisibleTextTrack;
> 
> This is weird.  I assume that m_haveVisibleTextTrack being set is a side effect of setClosedCaptionsVisible?
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4462
> > -    return closedCaptionsVisible();
> > +    return m_legacyWebKitClosedCaptionsVisible;
> 
> So you can call setWebkitClosedCaptionsVisible(TRUE) and have webkitClosedCaptionsVisible() immediately return FALSE?  That's odd.
> 
Yes, webkitClosedCaptionsVisible is meant to return true only if closed captions are visible, which can only be true if the file actually has a caption track. This has always been the case, media-captions.html tests this. 

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:622
> > +    bool legacyOverride = mediaElement->webkitClosedCaptionsVisible();
> 
> If webkitClosedCaptionsVisible() is real being used as a "legacyOverride" can we rename it (or add a new function) "legacyClosedCaptionsShouldOverrideVisibility()" or something?
> 
"webkitClosedCaptionsVisible" is web facing so we can't really change it.

> > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66
> > +    virtual bool isLegacyCCTrack() const = 0;
> 
> Since there's already a isClosedCaptions(), this should be "isLegacyClosedCaptions()" or "isLegacyClosedCaptionsTrack()".
> 
Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateLegacyAVFObjC.h:59
> > +    virtual bool isLegacyCCTrack() const OVERRIDE { return true; }
> 
> Ditto.
> 
Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:179
> > +    void processLegacyCCTracks();
> 
> Same thing, expand CC -> ClosedCaption.
>
Fixed.

[WebKit Commit Bot]

Comment on attachment 200321 [details]
Updated patch

Clearing flags on attachment: 200321

Committed r149503: <http://trac.webkit.org/changeset/149503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Lion build fix committed in r149510.

[Chris Petersen]

Excellent. Verified fixed in NB r149555 under Mac OS X 10.8.3.

[Eric Carlson]

(In reply to comment #13)
> Excellent. Verified fixed in NB r149555 under Mac OS X 10.8.3.

Thank you Chris!