Bug 114330 – Refactor TextTrack and TextTrackList to make it easier to add audio and video tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brendan Long]

Refactor TextTrack and TextTrackList to make it easier to add audio and video tracks

[Brendan Long]

This bug is for some refactoring work we're doing before adding audio and video tracks in bug #113965.

[Brendan Long]

Created attachment 197201 [details]
Patch

[Brendan Long]

Created attachment 197203 [details]
Patch

[Brendan Long]

Created attachment 197205 [details]
Patch

[Build Bot]

Comment on attachment 197205 [details]
Patch

Attachment 197205 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17739013

[Build Bot]

Comment on attachment 197205 [details]
Patch

Attachment 197205 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17671034

[Build Bot]

Comment on attachment 197205 [details]
Patch

Attachment 197205 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17624051

[Brendan Long]

Created attachment 197316 [details]
Patch

[Brendan Long]

I'm trying to get the Mac and Windows builds to work, but I'm not really sure where to add the .idl files to get them picked up.

[Brendan Long]

(In reply to comment #9)
> I'm trying to get the Mac and Windows builds to work, but I'm not really sure where to add the .idl files to get them picked up.

Ignore this. This applies to the audio/video tracks, not this change.

[Build Bot]

Comment on attachment 197316 [details]
Patch

Attachment 197316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17705099

New failing tests:
media/track/track-user-preferences.html
media/track/track-node-add-remove.html

[Build Bot]

Created attachment 197415 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Brendan Long]

Created attachment 197460 [details]
Patch

[Brendan Long]

Created attachment 197466 [details]
Patch

[Brendan Long]

Comment on attachment 197466 [details]
Patch

This doesn't build since I added the removetrack event, and I need some time to figure out how to handle some things better.

The most annoying part of this is that Vector<RefPtr<TrackBase> > can't be cast to Vector<RefPtr<TextTrack> >, which makes TextTrackList.cpp significantly more complicated than it needs to be. I'm thinking I can just make m_elementTracks and m_addTrackTracks(?) into Vector<RefPtr<TrackBase> > also, but it will make the code more complicated. I think if I move the track index and isRendered() code into TrackBase it will work, I just need to make sure that those make sense for audio and video tracks.

[Brendan Long]

The track index and isRendered() code only makes sense for text tracks, because it's all related to how to render subtitles.

[Brendan Long]

Created attachment 197865 [details]
Patch

[Brendan Long]

Let me know if you'd prefer that these patches be broken up differently.

[Jer Noble]

I really don't like all the static_cast<TextTrack*> going on in TextTrackList.  Perhaps add the following to TextTrack.h?:

TextTrack* toTextTrack(TrackBase* track)
{
    ASSERT(track->type() == TrackBase::TextTrack);
    return static_cast<TextTrack*>(track);
}

Similar functions could go into AudioTrack.h and VideoTrack.h.

Then replace all the explicit casts with calls to toTextTrack(), and if ever another type sneaks into a TextTrackList, it'll be caught by this assert.

[Eric Carlson]

Comment on attachment 197865 [details]
Patch

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

This is really close! 

Thanks for taking this on, refactoring isn't a lot of fun :-)

> Source/WebCore/ChangeLog:19
> +        (WebCore::HTMLMediaElement::mediaPlayerDidAddTextTrack):
> +        (WebCore::HTMLMediaElement::mediaPlayerDidRemoveTextTrack):
> +        (WebCore::HTMLMediaElement::addTextTrack):
> +        (WebCore::HTMLMediaElement::removeTextTrack):
> +        (WebCore::HTMLMediaElement::removeAllInbandTracks):
> +        (WebCore::HTMLMediaElement::didAddTextTrack):
> +        (WebCore::HTMLMediaElement::didRemoveTextTrack):

Nit: I think it is useful to have brief comments in the ChangeLog detailing what changed in each method.

> Source/WebCore/ChangeLog:21
> +        (WebCore):

Nit: all of these empty entries added by the script can be removed.

> Source/WebCore/html/track/TextTrack.h:87
> +    virtual const AtomicString& defaultKindKeyword() const { return subtitlesKeyword(); }

Nit: OVERRIDE.

> Source/WebCore/html/track/TextTrack.h:169
> +    virtual void refEventTarget() { ref(); }
> +    virtual void derefEventTarget() { deref(); }

OVERRIDE

> Source/WebCore/html/track/TextTrackList.cpp:58
> +    TextTrack* textTrack = static_cast<TextTrack*>(track);

This would be better if you had a toTextTrack() function that ASSERTs when passed the wrong type (see the various toXXX functions in Render classes).

[Brendan Long]

Created attachment 197877 [details]
Patch

[Brendan Long]

I added toTextTrack() and put some comments in the ChangeLog. I'm not sure if I was too detailed, or not detailed enough. Almost all of the changes involve renaming or changing a TextTrack* to a TrackBase*.

[Jer Noble]

Comment on attachment 197877 [details]
Patch

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

r+, with nit:

> Source/WebCore/html/track/TextTrack.cpp:574
> +TextTrack* toTextTrack(TrackBase* track)
> +{
> +    ASSERT(track->type() == TrackBase::TextTrack);
> +    return static_cast<TextTrack*>(track);
>  }

Nit: please put the body of this function into the header file, so it can be inlined.

(If you don't have committer rights, just upload a new patch named "Patch for Committing" and either Eric or I will cq+ it.)

[Brendan Long]

Created attachment 197881 [details]
Patch for committing

I needed to make it inline TextTrack* toTextTrack() to make it compile.

[WebKit Commit Bot]

Comment on attachment 197881 [details]
Patch for committing

Clearing flags on attachment: 197881

Committed r148305: <http://trac.webkit.org/changeset/148305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.