[Track] Closed Caption button shouldn't be visible if all the track resources have failed loading

[Silvia Pfeiffer]

Closed Caption button shouldn't be visible if all the track resources have failed loading, e.g. a single track, but failed cross-origin
request, see https://bugs.webkit.org/show_bug.cgi?id=103110

[Victor Carbune]

On a second thought, I think showing a button in a disabled state would be 
better. This would indicate that <track> elements are present and are supposed
to be shown, but something went wrong.

Clicking on it in that disabled state should try reloading the tracks.
Would this make sense?

[Silvia Pfeiffer]

Sounds good to me.

[Eric Carlson]

(In reply to comment #1)
> On a second thought, I think showing a button in a disabled state would be 
> better. This would indicate that <track> elements are present and are supposed
> to be shown, but something went wrong.
> 
> Clicking on it in that disabled state should try reloading the tracks.
> Would this make sense?

In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action?

[Victor Carbune]

(In reply to comment #3)
> (In reply to comment #1)
> > On a second thought, I think showing a button in a disabled state would be 
> > better. This would indicate that <track> elements are present and are supposed
> > to be shown, but something went wrong.
> > 
> > Clicking on it in that disabled state should try reloading the tracks.
> > Would this make sense?
> 
> In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action?

What happens right now if a track wasn't loaded because the machine serving
it is down, or there are communication issues? Does it retry indefinitely?

I was thinking at these cases particularly; the track failed loading, but
then the machine serving it becomes available and the user can force a retry
toggling the disabled button.

[Eric Carlson]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > On a second thought, I think showing a button in a disabled state would be 
> > > better. This would indicate that <track> elements are present and are supposed
> > > to be shown, but something went wrong.
> > > 
> > > Clicking on it in that disabled state should try reloading the tracks.
> > > Would this make sense?
> > 
> > In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action?
> 
> What happens right now if a track wasn't loaded because the machine serving
> it is down, or there are communication issues? Does it retry indefinitely?
> 
  The same thing that happens when any other resource (eg. image, script, etc) doesn't load - nothing.

> I was thinking at these cases particularly; the track failed loading, but
> then the machine serving it becomes available and the user can force a retry
> toggling the disabled button.
>
  I think just reloading the page makes sense, that way all resources from the same server will reload.

[Silvia Pfeiffer]

Hmm I think I agree with Eric - you can always hit the browser "reload" button to get the captions. YouTube doesn't do a reload either.

Also note that the resources in a list of <track> elements may not all come from the same server. If they all don't load, it's probably not necessary to display a button.

[Victor Carbune]

Created attachment 185287 [details]
Patch

Updated behavior in several places

[Eric Carlson]

Comment on attachment 185287 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:1336
> +        if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading
> +            || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded)
>              return false;

Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load?

[Victor Carbune]

(In reply to comment #8)
> (From update of attachment 185287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185287&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1336
> > +        if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading
> > +            || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded)
> >              return false;
> 
> Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load?

I will look closer in the code, but do you have a specific case in which you
would expect to fail (i.e., visually / behavior?)

[Eric Carlson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 185287 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185287&action=review
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:1336
> > > +        if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading
> > > +            || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded)
> > >              return false;
> > 
> > Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load?
> 
> I will look closer in the code, but do you have a specific case in which you
> would expect to fail (i.e., visually / behavior?)

HTMLMediaElement::mediaPlayerReadyStateChanged has this:

    if (tracksAreReady)
        m_readyState = newState;
    else {
        // If a media file has text tracks the readyState may not progress beyond HAVE_FUTURE_DATA until
        // the text tracks are ready, regardless of the state of the media file.
        if (newState <= HAVE_METADATA)
            m_readyState = newState;
        else
            m_readyState = HAVE_CURRENT_DATA;
    }

So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA.

[Victor Carbune]

(In reply to comment #10)
> So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA.

I think we're safe: if it fails to load, the readinessState changes to FailedToLoad and doesn't remain NotLoaded (as this is the check I added)
 
It's following the spec comment from ::textTracksAreReady - "[...] have a text track readiness state of loaded or failed to load." so it should be fine.

I manually tested, but I could add the case in a layout test as well if you
prefer.

[Eric Carlson]

(In reply to comment #11)
> (In reply to comment #10)
> > So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA.
> 
> I think we're safe: if it fails to load, the readinessState changes to FailedToLoad and doesn't remain NotLoaded (as this is the check I added)
> 
> It's following the spec comment from ::textTracksAreReady - "[...] have a text track readiness state of loaded or failed to load." so it should be fine.
> 
  You are correct, of course. 

> I manually tested, but I could add the case in a layout test as well if you
> prefer.
>
  I think this is already covered by the existing tests, if it actually was a problem a <video> with an unload able <track> would never reach canplaythrough.

[Victor Carbune]

Created attachment 185897 [details]
Extra method

Added extra method for refreshing the cc button visibility

[Radar WebKit Bug Importer]

<rdar://problem/13131121>

[WebKit Review Bot]

Comment on attachment 185897 [details]
Extra method

Clearing flags on attachment: 185897

Committed r141531: <http://trac.webkit.org/changeset/141531>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.