[Webkit-unassigned] [Bug 113965] Add interfaces and stubs for audio and video tracks

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113965





--- Comment #38 from Brendan Long <b.long at cablelabs.com>  2013-04-09 10:56:15 PST ---
If I didn't comment on part of this, it means I did it and have nothing to say besides "done".

(In reply to comment #36)
> I found some time to do a review.  Overall it looks really good, but I'd like to see more functionality moved into TrackBase, leaving only those things which are truly different across Track types in each subclass.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3022
> > +    m_audioTracks->remove(track);
> 
> What happens when someone calls removeAudioTrack() before calling addAudioTrack()?  Should that be an ASSERT(), or should it be an early return?

I'm not really sure. I'm not familiar with WebKit's style when handling this yet. Currently, there's no way for JavaScript to remove any kind of track (per the spec, no remove method, even for added tracks), so right now an ASSERT would probably be fine. I assume someone will eventually realize that it should be possible to remove tracks though, but there probably won't be a way to remove audio and video tracks.

> > Source/WebCore/html/HTMLMediaElement.cpp:3046
> > +    for (int i = m_audioTracks->length() - 1; i >= 0; --i) {
> 
> I see why you're doing it this way, but it still seems awkward.  How about:
> 
> while (m_audioTracks->length())
>   removeAudioTrack(m_audioTracks->itemAt(0));

I changed it to:

while (unsigned length = m_audioTracks->length())
    removeAudioTrack(m_audioTracks->item(length - 1));

Does that work?

Since the removeAll*Tracks() methods are all called at the same time (in clearMediaPlayer(), I'm considering if it would be better to just make it removeAllInbandTracks() again and have it remove all three types of tracks.

> > Source/WebCore/html/track/AudioTrack.cpp:169
> > +int AudioTrack::inbandTrackIndex()
> > +{
> > +    ASSERT(m_private);
> > +    return m_private->audioTrackIndex();
> > +}
> 
> This too can get moved into TrackBase.  Just move the m_trackIndex member variable from TextTrack to TrackBase and set the value in the AudioTrack's constructor.

This is actually different in TextTracks vs Audio/VideoTrack, since TextTracks aren't necessarily in-band. I could create an InbandTrackBase for InbandTextTrack, AudioTrack and VideoTrack to extend, but I'm not sure if you want me to go that far.

TextTrack's trackIndex() does something different (to be honest, I don't know why the text tracks know their own index in the TextTrackList, but existing code uses it so I assume it's there for a reason).

> > Source/WebCore/html/track/AudioTrack.h:58
> > +    AtomicString id() const { return m_id; }
> > +    void setId(const AtomicString& id) { m_id = id; }
> 
> This is common across Audio and VideoTracks, but not TextTracks.  I'd move it up into TrackBase anyway.

I'd really prefer not to. I think it would be confusing in the future for TextTrack to have an id() function when it's not supposed to. I can do this though if you think it's necessary.

> > Source/WebCore/html/track/AudioTrack.h:69
> > +    static bool isValidKindKeyword(const AtomicString&);
> 
> Turn this into a virtual method, which overrides a pure virtual in TrackBase.

I did this, but it caused some complication because in TextTrack, it's used as a static method in HTMLMediaElement:

    // 1. If kind is not one of the following strings, then throw a SyntaxError exception and abort these steps
    if (!TextTrack::isValidKindKeyword(kind)) {
        ec = SYNTAX_ERR;
        return 0;
    }

Audio and video tracks won't need to do this though, so it worked fine. I had to name the virtual method isValidKindKeywordProtected() because it couldn't have the same name as the static function. I doubt that's what I should name it though. Any suggestions?

> I think it would be useful to split the refactoring out into a separate patch from the new functionality. This will make it much easier to review, but I suspect it will make it easier to figure out build issues on the various bots as well.

I'll look into it. There's a big list of commits at this point so it'll take time to rearrange it.

---

I also want to mention the TextTrackPrivate/AudioTrackPrivate/VideoTrackPrivate classes. I could create a TrackPrivateBase class, but those classes don't actually share any functionality except the label() and language() functions.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.