[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 #36 from Jer Noble <jer.noble at apple.com>  2013-04-09 00:31:43 PST ---
(From update of attachment 196979)
View in context: https://bugs.webkit.org/attachment.cgi?id=196979&action=review

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?

> Source/WebCore/html/HTMLMediaElement.cpp:3038
> +    m_videoTracks->remove(track);

Ditto.

> 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));

It would be even easier if there were a convenience "lastItem()" or "firstItem()" method on TrackListBase.

> Source/WebCore/html/HTMLMediaElement.cpp:3073
> +    for (int i = m_videoTracks->length() - 1; i >= 0; --i) {
> +        VideoTrack* track = m_videoTracks->item(i);
> +        removeVideoTrack(track);

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:56
> +class AudioTrackList;

Shouldn't this be in the #if ENABLE(VIDEO_TRACK)?

> Source/WebCore/html/HTMLMediaElement.h:65
> +class VideoTrackList;

Ditto.

> Source/WebCore/html/track/AudioTrack.cpp:152
> +void AudioTrack::setKind(const AtomicString& kind)
> +{
> +    String oldKind = m_kind;
> +
> +    if (isValidKindKeyword(kind))
> +        m_kind = kind;
> +    else
> +        m_kind = "";
> +}

It seems like this can be moved into TrackBase, since it's identical to the code in VideoTrack.cpp.

> 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.

> 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.

> Source/WebCore/html/track/AudioTrack.h:61
> +    AtomicString kind() const { return m_kind; }
> +    void setKind(const AtomicString&);

Move up into TrackBase().

> 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.

> Source/WebCore/html/track/AudioTrack.h:75
> +    AtomicString label() const { return m_label; }
> +    void setLabel(const AtomicString& label) { m_label = label; }
> +
> +    AtomicString language() const { return m_language; }
> +    void setLanguage(const AtomicString& language) { m_language = language; }

These are all common across track types, so move this up.

> Source/WebCore/html/track/AudioTrack.h:83
> +    int inbandTrackIndex();

Ditto.

> Source/WebCore/html/track/AudioTrack.h:92
> +    AtomicString m_id;
> +    AtomicString m_kind;
> +    AtomicString m_label;
> +    AtomicString m_language;

Ditto.

> Source/WebCore/html/track/VideoTrack.cpp:152
> +void VideoTrack::setKind(const AtomicString& kind)
> +{
> +    String oldKind = m_kind;
> +
> +    if (isValidKindKeyword(kind))
> +        m_kind = kind;
> +    else
> +        m_kind = "";
> +}

Move into TrackBase.

> Source/WebCore/html/track/VideoTrack.cpp:169
> +int VideoTrack::inbandTrackIndex()
> +{
> +    ASSERT(m_private);
> +    return m_private->videoTrackIndex();
> +}

Ditto.

> Source/WebCore/html/track/VideoTrack.h:61
> +    AtomicString id() const { return m_id; }
> +    void setId(const AtomicString& id) { m_id = id; }
> +
> +    AtomicString kind() const { return m_kind; }
> +    void setKind(const AtomicString&);

Ditto.

> Source/WebCore/html/track/VideoTrack.h:75
> +    AtomicString label() const { return m_label; }
> +    void setLabel(const AtomicString& label) { m_label = label; }
> +
> +    AtomicString language() const { return m_language; }
> +    void setLanguage(const AtomicString& language) { m_language = language; }

Ditto.

> Source/WebCore/html/track/VideoTrack.h:83
> +    int inbandTrackIndex();

Ditto.

> Source/WebCore/html/track/VideoTrack.h:92
> +    AtomicString m_id;
> +    AtomicString m_kind;
> +    AtomicString m_label;
> +    AtomicString m_language;

Ditto.

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