Refactor TextTrack and TextTrackList to make it easier to add audio and video tracks
This bug is for some refactoring work we're doing before adding audio and video tracks in bug #113965.
Created attachment 197201 [details] Patch
Created attachment 197203 [details] Patch
Created attachment 197205 [details] Patch
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
Comment on attachment 197205 [details] Patch Attachment 197205 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17671034
Comment on attachment 197205 [details] Patch Attachment 197205 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17624051
Created attachment 197316 [details] Patch
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.
(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.
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
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
Created attachment 197460 [details] Patch
Created attachment 197466 [details] Patch
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.
The track index and isRendered() code only makes sense for text tracks, because it's all related to how to render subtitles.
Created attachment 197865 [details] Patch
Let me know if you'd prefer that these patches be broken up differently.
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.
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).
Created attachment 197877 [details] Patch
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*.
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.)
Created attachment 197881 [details] Patch for committing I needed to make it inline TextTrack* toTextTrack() to make it compile.
Comment on attachment 197881 [details] Patch for committing Clearing flags on attachment: 197881 Committed r148305: <http://trac.webkit.org/changeset/148305>
All reviewed patches have been landed. Closing bug.