Bug 113965 – Add interfaces and stubs for audio and video tracks

• Component: Media
• Status: RESOLVED FIXED

[Brendan Long]

Add interfaces and stubs for audio and video tracks

[Glenn Adams]

(In reply to comment #0)
> Add interfaces and stubs for audio and video tracks

What does that mean?

[Brendan Long]

Sorry, I meant to upload a patch immediately after the bug was created, but I noticed something when I was looking through the diff and now I'm waiting for the build to finish again.

I meant for this bug to be an AudioTrack/VideoTrack version of bug #60379.

I'm also working on a GStreamer implementation, but it seemed like it would be easier to give you the generic (non-port specific) part first, then the GStreamer implementation. I can do it as one giant patch if you'd prefer.

[Brendan Long]

Basically, this patch will:

  * Add AudioTrack, AudioTrackPrivate, AudioTrackList, VideoTrack, VideoTrackPrivate, and VideoTrackList classes (with various clients).
  * Add IDL for the tracks and tracklists.
  * Add an AudioTrackList and VideoTrackList to HTMLMediaElment.

The classes are based on TextTrack, InbandTextTrack, TextTrackPrivate and TextTrackList. All audio and video tracks are in-band, so there's no need for an AudioTrack + InbandAudioTrack -- it's just called AudioTrack.

I hid it behind ENABLE_VIDEO_TRACK, like TextTracks are. I could create another feature define if that seems useful.

[Glenn Adams]

(In reply to comment #2)
> Sorry, I meant to upload a patch immediately after the bug was created, but I noticed something when I was looking through the diff and now I'm waiting for the build to finish again.
> 
> I meant for this bug to be an AudioTrack/VideoTrack version of bug #60379.

Do you have a class diagram and design doc similar to what was referenced at [1]?

[1] https://bugs.webkit.org/show_bug.cgi?id=60379#c3

> 
> I'm also working on a GStreamer implementation, but it seemed like it would be easier to give you the generic (non-port specific) part first, then the GStreamer implementation. I can do it as one giant patch if you'd prefer.

The larger the patch the less likely it will get reviewed or reviewed in a timely manner. I would suggest you use a meta bug and then a number of blocking bugs that introduce small well defined patches.

[Glenn Adams]

(In reply to comment #3)
> Basically, this patch will:
> 
>   * Add AudioTrack, AudioTrackPrivate, AudioTrackList, VideoTrack, VideoTrackPrivate, and VideoTrackList classes (with various clients).
>   * Add IDL for the tracks and tracklists.
>   * Add an AudioTrackList and VideoTrackList to HTMLMediaElment.
> 
> The classes are based on TextTrack, InbandTextTrack, TextTrackPrivate and TextTrackList. All audio and video tracks are in-band, so there's no need for an AudioTrack + InbandAudioTrack -- it's just called AudioTrack.

Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding?

> 
> I hid it behind ENABLE_VIDEO_TRACK, like TextTracks are. I could create another feature define if that seems useful.

[Brendan Long]

(In reply to comment #5)
> Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding?

What is MSE?

We will need to be able to dynamically add in-band audio and video tracks, but they will all come from the MediaPlayer. There's no concept of out-of-band audio and video tracks in the spec.

[Brendan Long]

(In reply to comment #4)
> Do you have a class diagram and design doc similar to what was referenced at [1]?
> 
> [1] https://bugs.webkit.org/show_bug.cgi?id=60379#c3

I'll look into making those. The changes I'm making are basically identical to the ones for text tracks (but they're simpler since I don't need to deal with cues or parsing WebVTT).

[Glenn Adams]

(In reply to comment #6)
> (In reply to comment #5)
> > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding?
> 
> What is MSE?
> 
> We will need to be able to dynamically add in-band audio and video tracks, but they will all come from the MediaPlayer. There's no concept of out-of-band audio and video tracks in the spec.

Please review MSE [1] to make certain.

[1] https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html

[Eric Carlson]

(In reply to comment #5)
> 
> Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding?
> 
No, this is to implement an existing part of the spec, media.audioTracks and media.videoTracks [1]. Anything required for MSE will have to be added to that spec, and will be part of that feature.

[1] http://dev.w3.org/html5/spec-author-view/video.html#media-resources-with-multiple-media-tracks

[Glenn Adams]

(In reply to comment #9)
> (In reply to comment #5)
> > 
> > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding?
> > 
> No, this is to implement an existing part of the spec, media.audioTracks and media.videoTracks [1]. Anything required for MSE will have to be added to that spec, and will be part of that feature.
> 
> [1] http://dev.w3.org/html5/spec-author-view/video.html#media-resources-with-multiple-media-tracks

OK. I just wanted to make sure we were not making an assumption (about always in-band) that was going to be reverted in short order.

[Brendan Long]

Created attachment 196560 [details]
Patch

[Brendan Long]

This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt.

[Glenn Adams]

Comment on attachment 196560 [details]
Patch

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

> Source/WebCore/ChangeLog:9


How about test(s) for presence of the JS signatures introduced here?

> Source/WebCore/ChangeLog:12
> +        * Target.pri:

Where is build support for other platforms?

[Glenn Adams]

Comment on attachment 196560 [details]
Patch

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

> Source/WebCore/ChangeLog:9


How about test(s) for presence of the JS signatures introduced here?

> Source/WebCore/ChangeLog:12
> +        * Target.pri:

Where is build support for other platforms?

[Glenn Adams]

(In reply to comment #14)
> (From update of attachment 196560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196560&action=review
> 
> > Source/WebCore/ChangeLog:9
> 
> 
> How about test(s) for presence of the JS signatures introduced here?
> 
> > Source/WebCore/ChangeLog:12
> > +        * Target.pri:
> 
> Where is build support for other platforms?

Ignore this duplicate set of comments.

[Eric Carlson]

(In reply to comment #12)
> This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt.

No, you should just reattach the patch once bug #113522 has landed so the bots can use it.

[Glenn Adams]

(In reply to comment #12)
> This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt.

For record keeping, also add 113522 to the "Depend on" field.

[Glenn Adams]

Also, when you upload a patch, you might want to use:

webkit-patch upload --no-review

And then add the r? flag only after the patch is building/testing properly on EWS.

[Eric Carlson]

I won't have a chance to look at this in detail before next week, but a high level comment is that it makes me sad to see so much boilerplate code duplicated in all three *Track class. Would it be possible to have all three Track types share a base class? If so, you may also be able to have a single mediaPlayerDidAdd*Track, mediaPlayerDidRemove*Track, etc. 

It may also be possible to have the *TrackPrivate classes share a base class.

[Brendan Long]

(In reply to comment #19)
> Would it be possible to have all three Track types share a base class?

The track classes already share a TrackBase class. I think it has most of their shared code. Looking into this more closely made me realize that TrackBase extends EventTarget, but AudioTrack and VideoTrack aren't EventTargets, so fixing that should simplify things a little bit.

I'm also looking at making a TrackListBase since their interfaces are extremely similar. I might be able to make a TrackList<T> class that has almost all of the code in it.

> If so, you may also be able to have a single mediaPlayerDidAdd*Track, mediaPlayerDidRemove*Track, etc. 

Do you mean just having a mediaPlayerDidAddTrack(TrackBase)? I could look into that. 

> It may also be possible to have the *TrackPrivate classes share a base class.

I'll look into that too.

[Brendan Long]

When making the template for TrackListBase<T>, should I:

  * Include templated member functions in TrackListBase.h

Or

  * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp

There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is.

[Jer Noble]

(In reply to comment #21)
> When making the template for TrackListBase<T>, should I:
> 
>   * Include templated member functions in TrackListBase.h
> 
> Or
> 
>   * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp
> 
> There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is.

Please don't use a templated base class. (In reply to comment #21)
> When making the template for TrackListBase<T>, should I:
> 
>   * Include templated member functions in TrackListBase.h
> 
> Or
> 
>   * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp
> 
> There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is.

While I haven't seen your proposed templated base class yet, it seems like it will be much more complicated than it has to be.  Why not just make a (non templated) TrackListBase, where all the methods take TrackBase* arguments, add specialized {Video,Audio,Text}TrackList classes with non-virtual index accessors, and add "ASSERT(type() == VideoTrack)" checks where necessary?

[Jer Noble]

(In reply to comment #22)
> Please don't use a templated base class.

Let me quickly expand on this: templated container classes make sense when the template arguments are unrelated, e.g. Vector<int> and Vector<String>.  But when the set of possible arguments are all similar types (e.g., VideoTrack, AudioTrack, and TextTrack all derive from TrackBase), using a templated container is usually the wrong solution.

[Brendan Long]

(In reply to comment #22)
> While I haven't seen your proposed templated base class yet, it seems like it will be much more complicated than it has to be.  Why not just make a (non templated) TrackListBase, where all the methods take TrackBase* arguments, add specialized {Video,Audio,Text}TrackList classes with non-virtual index accessors, and add "ASSERT(type() == VideoTrack)" checks where necessary?

Ok, I'll do it that way.

[Brendan Long]

Created attachment 196979 [details]
Patch

[Brendan Long]

This patch:

  * Moved as much code as I could into TrackListBase (not templated). This makes TextTrackList slightly more complicated, but audio and video track lists are much simpler.
  * Made TextTrack extend EventTarget instead of TrackBase. AudioTrack and VideoTrack are not EventTargets.

I looked at the *TrackPrivate classes, but they don't really share any code. Would it be worth it to make them inherit from something just so we could have fewer callbacks? In the callback we'd need to immediately cast them back to their real type.

One thing worth looking at is the JS*Custom files, since I don't really understand how these work. I based these on JSTextTrackCustom and JSTextTrackListCustom, but I'm not sure if they're necessary.

I'll at at adding this to other ports now.

[Build Bot]

Comment on attachment 196979 [details]
Patch

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

[Build Bot]

Comment on attachment 196979 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 196979 [details]
Patch

Attachment 196979 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17686006

[Brendan Long]

Created attachment 196990 [details]
Patch

[Brendan Long]

This should theoretically make EFL and GTK builds work, but I can't get EFL to build at all on Fedora, and I got started on GTK too late. I'll try to figure out the Mac version tomorrow.

[Eric Carlson]

I don't have time to do a proper review tonight, but this looks like it is definitely headed in the right direction!

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.

[EFL EWS Bot]

Comment on attachment 196990 [details]
Patch

Attachment 196990 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17603014

[Build Bot]

Comment on attachment 196990 [details]
Patch

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

[Build Bot]

Comment on attachment 196990 [details]
Patch

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

[Jer Noble]

Comment on attachment 196979 [details]
Patch

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.

[Brendan Long]

Created attachment 197143 [details]
Patch

[Brendan Long]

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.

[Build Bot]

Comment on attachment 197143 [details]
Patch

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

[Brendan Long]

Created attachment 197147 [details]
Patch

This should fix the CMake build (I missed some cpp files)

[Jer Noble]

(In reply to comment #38)
> 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.

Sounds like you need an ASSERT here then.

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

That does seem better. :)

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

Right, but you could move the member into TrackBase and leave the code which modifies it in TextTrack.

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

Eh, that's fine.

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

Ah, that's interesting. I'd just call it "isValidKind()" and put it in protected: in TextBase and private: in all the subclasses.

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

Yeah, I guess there's an opportunity to make the graph more complicated and introduce an AudioVisualTrackBase.  Seems like the increase in boilerplate overwhelm the reduction in duplication though. :-/

[EFL EWS Bot]

Comment on attachment 197147 [details]
Patch

Attachment 197147 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17676037

[Build Bot]

Comment on attachment 197147 [details]
Patch

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

[Brendan Long]

Created attachment 197154 [details]
Patch

Fix unused variable warning in EFL build

[Eric Carlson]

(In reply to comment #32)
> 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 still think it would be helpful to split this into two patches, one for the refactoring of the existing code and another to add the new track types.

[Build Bot]

Comment on attachment 197154 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 197154 [details]
Patch

Attachment 197154 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17673059

[Build Bot]

Comment on attachment 197154 [details]
Patch

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

[Brendan Long]

Created attachment 197158 [details]
Patch

Fix some review issues and add to DerivedSources.make/cpp, JSBindingsAllInOne.cpp and UseJSC.make

[Brendan Long]

Alright, I have no clue how to get WebKitGTK to generate the JS*.h files. It generates the .cpp ones now, so why not the .h's?

[Build Bot]

Comment on attachment 197158 [details]
Patch

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

[Build Bot]

Comment on attachment 197158 [details]
Patch

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

[Brendan Long]

I created bug #114330 so we can do the refactoring as one patch and the audio and video tracks as another.

[Brendan Long]

Created attachment 197206 [details]
Patch

[Brendan Long]

This new patch is much simpler, but it won't build until bug #114330 is resolved.

[Brendan Long]

Created attachment 197313 [details]
Patch

[Brendan Long]

Created attachment 197896 [details]
Patch

[Brendan Long]

I updated this patch based on comments in #114330:

  * Add toAudioTrack() and toVideoTrack()
  * Added some comments in the ChangeLog

There are only two things that concern me about this patch:

  * I'm still not sure how to get the GTK and Mac builds to pick up the new .idl files.
  * I basically just copied the JS*Custom files and have no idea why they're necessary (but the build won't work without them).

[Build Bot]

Comment on attachment 197896 [details]
Patch

Attachment 197896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/37192

[Build Bot]

Comment on attachment 197896 [details]
Patch

Attachment 197896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/9038

[Build Bot]

Comment on attachment 197896 [details]
Patch

Attachment 197896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2203

[Philippe Normand]

If you rebase the patch against trunk I can test it and fix the GTK build...

[Brendan Long]

Created attachment 197940 [details]
Patch

[Brendan Long]

I rebased it again. Sorry, I didn't notice that the GTK build failed because the patch didn't apply.

I'm also working on some tests, but almost all of them will fail at first because there's no way to add audio and video tracks by JavaScript.

[Build Bot]

Comment on attachment 197940 [details]
Patch

Attachment 197940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/97171

[Build Bot]

Comment on attachment 197940 [details]
Patch

Attachment 197940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/102202

[kov's GTK+ EWS bot]

Comment on attachment 197940 [details]
Patch

Attachment 197940 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/148155

[Build Bot]

Comment on attachment 197940 [details]
Patch

Attachment 197940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/143187

[Philippe Normand]

Add this in the webcore_built_sources variable of GNUmakefile.list.am

	DerivedSources/WebCore/JSAudioTrack.cpp \
	DerivedSources/WebCore/JSAudioTrack.h \
	DerivedSources/WebCore/JSVideoTrack.cpp \
	DerivedSources/WebCore/JSVideoTrack.h \
	DerivedSources/WebCore/JSVideoTrackList.cpp \
	DerivedSources/WebCore/JSVideoTrackList.h \

[Philippe Normand]

And:

	DerivedSources/WebCore/JSAudioTrackList.cpp \
	DerivedSources/WebCore/JSAudioTrackList.h \

[Brendan Long]

Created attachment 198010 [details]
Patch

[Brendan Long]

(In reply to comment #69)
> Add this in the webcore_built_sources variable of GNUmakefile.list.am
> 
>     DerivedSources/WebCore/JSAudioTrack.cpp \
>     DerivedSources/WebCore/JSAudioTrack.h \
>     DerivedSources/WebCore/JSVideoTrack.cpp \
>     DerivedSources/WebCore/JSVideoTrack.h \
>     DerivedSources/WebCore/JSVideoTrackList.cpp \
>     DerivedSources/WebCore/JSVideoTrackList.h \

Thanks!

[Build Bot]

Comment on attachment 198010 [details]
Patch

Attachment 198010 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/74216

[Build Bot]

Comment on attachment 198010 [details]
Patch

Attachment 198010 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/28247

[Brendan Long]

Created attachment 198174 [details]
Patch

[Build Bot]

Comment on attachment 198174 [details]
Patch

Attachment 198174 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/42437

[Brendan Long]

Created attachment 198185 [details]
Patch

[Brendan Long]

Created attachment 198191 [details]
Patch

[Brendan Long]

Somehow I'm still missing somewhere where AudioTrackPrivate.h and TextTrackPrivate.h are supposed to be in the Xcode project..

[Build Bot]

Comment on attachment 198191 [details]
Patch

Attachment 198191 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/154437

[Build Bot]

Comment on attachment 198191 [details]
Patch

Attachment 198191 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22257

[Build Bot]

Comment on attachment 198191 [details]
Patch

Attachment 198191 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/132048

[Eric Carlson]

I will fix the Mac build problems and update the patch.

[Eric Carlson]

Created attachment 198336 [details]
Updated to fix Mac builds.

[Build Bot]

Comment on attachment 198336 [details]
Updated to fix Mac builds.

Attachment 198336 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/71129

[Build Bot]

Comment on attachment 198336 [details]
Updated to fix Mac builds.

Attachment 198336 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/21297

[Build Bot]

Comment on attachment 198336 [details]
Updated to fix Mac builds.

Attachment 198336 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/169088

[Eric Carlson]

Created attachment 198390 [details]
Export VideoTrackPrivate.h in private framework headers.

[Eric Carlson]

JSCustomIsReachable needs to be changed to CustomIsReachable after r148638

[Jer Noble]

Comment on attachment 198390 [details]
Export VideoTrackPrivate.h in private framework headers.

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

> Source/WebCore/bindings/js/JSAudioTrackCustom.cpp:47
> +bool JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
> +{
> +    JSAudioTrack* jsAudioTrack = jsCast<JSAudioTrack*>(handle.get().asCell());
> +    AudioTrack* audioTrack = static_cast<AudioTrack*>(jsAudioTrack->impl());\
> +
> +    // If the track has no custom properties, it is not reachable.
> +    if (!jsAudioTrack->hasCustomProperties())
> +        return false;
> +
> +    return visitor.containsOpaqueRoot(root(audioTrack));
> +}

It's unnecessary to do this with a custom function.  Adding "GenerateIsReachable" in the IDL file will do the right thing here.

> Source/WebCore/bindings/js/JSAudioTrackListCustom.cpp:54
> +bool JSAudioTrackListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
> +{
> +    JSAudioTrackList* jsAudioTrackList = jsCast<JSAudioTrackList*>(handle.get().asCell());
> +    AudioTrackList* audioTrackList = static_cast<AudioTrackList*>(jsAudioTrackList->impl());
> +
> +    // If the list is firing event listeners, its wrapper is reachable because
> +    // the wrapper is responsible for marking those event listeners.
> +    if (audioTrackList->isFiringEventListeners())
> +        return true;
> +
> +    // If the list has no event listeners and has no custom properties, it is not reachable.
> +    if (!audioTrackList->hasEventListeners() && !jsAudioTrackList->hasCustomProperties())
> +        return false;
> +
> +    // It is reachable if the media element parent is reachable.
> +    return visitor.containsOpaqueRoot(root(audioTrackList->owner()));
> +}

This could be generated with "GenerateIsReachable", if the generator was updated to add the "isFiringEventListeners()" check for classes which inherited from EventTarget. :-D

> Source/WebCore/bindings/js/JSVideoTrackCustom.cpp:47
> +bool JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
> +{
> +    JSVideoTrack* jsVideoTrack = jsCast<JSVideoTrack*>(handle.get().asCell());
> +    VideoTrack* videoTrack = static_cast<VideoTrack*>(jsVideoTrack->impl());
> +
> +    // If the track has no custom properties, it is not reachable.
> +    if (!jsVideoTrack->hasCustomProperties())
> +        return false;
> +
> +    return visitor.containsOpaqueRoot(root(videoTrack));
> +}

Ditto: "GenerateIsReachable".

> Source/WebCore/bindings/js/JSVideoTrackListCustom.cpp:54
> +bool JSVideoTrackListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
> +{
> +    JSVideoTrackList* jsVideoTrackList = jsCast<JSVideoTrackList*>(handle.get().asCell());
> +    VideoTrackList* videoTrackList = static_cast<VideoTrackList*>(jsVideoTrackList->impl());
> +
> +    // If the list is firing event listeners, its wrapper is reachable because
> +    // the wrapper is responsible for marking those event listeners.
> +    if (videoTrackList->isFiringEventListeners())
> +        return true;
> +
> +    // If the list has no event listeners and has no custom properties, it is not reachable.
> +    if (!videoTrackList->hasEventListeners() && !jsVideoTrackList->hasCustomProperties())
> +        return false;
> +
> +    // It is reachable if the media element parent is reachable.
> +    return visitor.containsOpaqueRoot(root(videoTrackList->owner()));
> +}

Ditto with the generator change.

> Source/WebCore/html/HTMLMediaElement.cpp:2828
> +    RefPtr<AudioTrack> track = AudioTrack::create(this, prpTrack);
> +
> +    addAudioTrack(track.get());

This seems unfortunate.  If addAudioTrack took a PassRefPtr<AudioTrack>, this could be:

addAudioTrack(AudioTrack::create(this, prpTrack));

> Source/WebCore/html/HTMLMediaElement.cpp:2873
> +    RefPtr<VideoTrack> track = VideoTrack::create(this, prpTrack);
> +
> +    addVideoTrack(track.get());

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2888
> +    // This cast is safe because we created the AudioTrack with the AudioTrackPrivate
> +    // passed to mediaPlayerDidAddAudioTrack.
> +    RefPtr<AudioTrack> track = static_cast<AudioTrack*>(prpTrack->client());
> +    if (!track)
> +        return;

I still don't like this. There should be a better way to get from an AudioTrackPrivate to an AudioTrack.  Perhaps the notification needs to come through the AudioTrackPrivate, like so:

void AudioTrackPrivate::willBeRemoved()
{
  client()->willRemoveAudioTrackPrivate(this);
}

void AudioTrack::willRemoveAudioTrackPrivate(AudioTrackPrivate* trackPrivate)
{
  ASSERT(trackPrivate == m_private);
  mediaElement()->removeAudioTrack(this);
}

> Source/WebCore/html/HTMLMediaElement.cpp:2922
> +    // This cast is safe because we created the VideoTrack with the VideoTrackPrivate
> +    // passed to mediaPlayerDidAddVideoTrack.
> +    RefPtr<VideoTrack> track = static_cast<VideoTrack*>(prpTrack->client());
> +    if (!track)
> +        return;

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:3003
> +void HTMLMediaElement::addAudioTrack(AudioTrack* track)

Since this is taking ownership, this could take a PassRefPtr<AudioTrack>.

> Source/WebCore/html/HTMLMediaElement.cpp:3005
> +    audioTracks()->append(track);

Add a null check: audioTracks() can return null if tracks are disabled at runtime.

> Source/WebCore/html/HTMLMediaElement.cpp:3015
> +void HTMLMediaElement::addVideoTrack(VideoTrack* track)

Ditto w.r.t. PassRefPtr.

> Source/WebCore/html/HTMLMediaElement.cpp:3017
> +    videoTracks()->append(track);

Ditto w.r.t null check.

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

Ditto w.r.t. null check.

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

Ditto w.r.t. null check.

> Source/WebCore/html/HTMLMediaElement.cpp:3046
> +        while (unsigned length = m_audioTracks->length())
> +            removeAudioTrack(m_audioTracks->item(length - 1));

This would be a lot less confusing if AudioTrackList had a "lastItem()" method.  And the "if" could be rolled into the while:

while (m_audioTracks && m_audioTracks->length())
  removeAudioTrack(m_audioTracks->lastItem());

> Source/WebCore/html/HTMLMediaElement.cpp:3060
> +    if (m_videoTracks)
> +        while (unsigned length = m_videoTracks->length())
> +            removeVideoTrack(m_videoTracks->item(length - 1));

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:245
> +    void addAudioTrack(AudioTrack*);
>      void addTextTrack(TextTrack*);
> +    void addVideoTrack(VideoTrack*);

As before, these can be PassRefPtrs.

> Source/WebCore/html/track/AudioTrack.idl:29
> +    JSCustomIsReachable

As before, this can be GenerateIsReachable.

> Source/WebCore/html/track/AudioTrackList.idl:31
> +    JSCustomIsReachable

With a change to the generator, this could be GenerateIsReachable.

> Source/WebCore/html/track/VideoTrack.idl:29
> +    JSCustomIsReachable

Ditto.

> Source/WebCore/html/track/VideoTrackList.idl:31
> +    JSCustomIsReachable

Ditto.

[Brendan Long]

If I switch to GenerateIsReachable, I get these errors:

generated/JSAudioTrack.cpp: In member function 'virtual bool WebCore::JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)':
generated/JSAudioTrack.cpp:233:18: error: 'root' is not a member of 'WebCore'
generated/JSVideoTrack.cpp: In member function 'virtual bool WebCore::JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)':
generated/JSVideoTrack.cpp:233:18: error: 'root' is not a member of 'WebCore'

Even though root(AudioTrack*) and root(VideoTrack*) are unchanged and still in JS*TrackCustom.h

[Jer Noble]

(In reply to comment #91)
> If I switch to GenerateIsReachable, I get these errors:
> 
> generated/JSAudioTrack.cpp: In member function 'virtual bool WebCore::JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)':
> generated/JSAudioTrack.cpp:233:18: error: 'root' is not a member of 'WebCore'
> generated/JSVideoTrack.cpp: In member function 'virtual bool WebCore::JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)':
> generated/JSVideoTrack.cpp:233:18: error: 'root' is not a member of 'WebCore'
> 
> Even though root(AudioTrack*) and root(VideoTrack*) are unchanged and still in JS*TrackCustom.h

Ah, yes, the generator won't add a #include "JSAudioTrackCustom.h" directive.  It will add a #include "AudioTrack.h", so you could add the WebCore::root() declaration there, instead.

The relevant code in the generator is in Source/WebCore/bindings/js/CodeGeneratorJS.pm, line ~2648. It shows what the various directive parameters will generate.  One option that might make sense would be to have an element() accessor (in addition to mediaElement()) in TrackBase, and use "GenerateIsReachable(ImplElementRoot)". 

The reason I'm pushing for getting rid of the JS*TrackCustom files is that when improvements are made to the generator, classes with custom directives don't automatically benefit; they need to be improved manually, and they may just get forgotten about.  So it's best not to have custom directives unless strictly necessary.

[Jer Noble]

Filed https://bugs.webkit.org/show_bug.cgi?id=114784 to track adding a generated "isFiringEventListeners()" check.

[Brendan Long]

(In reply to comment #90)
> > Source/WebCore/html/HTMLMediaElement.cpp:2888
> > +    // This cast is safe because we created the AudioTrack with the AudioTrackPrivate
> > +    // passed to mediaPlayerDidAddAudioTrack.
> > +    RefPtr<AudioTrack> track = static_cast<AudioTrack*>(prpTrack->client());
> > +    if (!track)
> > +        return;
> 
> I still don't like this. There should be a better way to get from an AudioTrackPrivate to an AudioTrack.  Perhaps the notification needs to come through the AudioTrackPrivate, like so:
> 
> void AudioTrackPrivate::willBeRemoved()
> {
>   client()->willRemoveAudioTrackPrivate(this);
> }
> 
> void AudioTrack::willRemoveAudioTrackPrivate(AudioTrackPrivate* trackPrivate)
> {
>   ASSERT(trackPrivate == m_private);
>   mediaElement()->removeAudioTrack(this);
> }

Is there any reason that the AudioTrackPrivate shouldn't know its parent AudioTrack? I could add AudioTrack* m_track (basically identical to m_client), then:

removeAudioTrack(privateTrack->audioTrack());

The only weird thing I can think of for this is that m_client and m_track would be the same object, so it's kind of weird.

[Brendan Long]

Created attachment 198774 [details]
Updated based on review

[Brendan Long]

I'm not able to obsolete the previous patch (I incorporated the changes to WebCore.xcodeproj).

Making root() work without the TrackCustom.h files was surprisingly difficult, but I was able to combine everything into root(TrackBase*), so that's nice.

I didn't notice that bug #114784 was finished, so I'm including the changes related to that now.

[Build Bot]

Comment on attachment 198774 [details]
Updated based on review

Attachment 198774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/116089

[Brendan Long]

To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not.

[Build Bot]

Comment on attachment 198774 [details]
Updated based on review

Attachment 198774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/96380

[Glenn Adams]

(In reply to comment #98)
> To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not.

I notice that bug 79822 failed to build on any EWS bot. In any case, someone (such as the reviewer) would need to set cq+ before it would be landed by the cq.

[Jer Noble]

(In reply to comment #100)
> (In reply to comment #98)
> > To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not.
> 
> I notice that bug 79822 failed to build on any EWS bot. In any case, someone (such as the reviewer) would need to set cq+ before it would be landed by the cq.

It will need to be rebaselined and run past the EWS bots again. I'll take that on.

[Brendan Long]

Created attachment 198891 [details]
Add UNUSED_PARAM to willRemove*TrackPrivate

[Build Bot]

Comment on attachment 198891 [details]
Add UNUSED_PARAM to willRemove*TrackPrivate

Attachment 198891 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/95389

[Eric Carlson]

(In reply to comment #103)
> (From update of attachment 198891 [details])
> Attachment 198891 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/95389

InbandTextTrackPrivateClient.h needs to be add to the framework's private interfaces so it is visible to WebKit.

[Brendan Long]

Created attachment 198913 [details]
Generate isReachableFromOpaqueRoots() and fix mac build again

[Jer Noble]

Comment on attachment 198913 [details]
Generate isReachableFromOpaqueRoots() and fix mac build again

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

> Source/WebCore/html/HTMLMediaElement.cpp:2889
> +void HTMLMediaElement::mediaPlayerDidRemoveAudioTrack(PassRefPtr<AudioTrackPrivate> prpTrack)
> +{
> +    prpTrack->willBeRemoved();
> +}
> +
> +void HTMLMediaElement::mediaPlayerDidRemoveTextTrack(PassRefPtr<InbandTextTrackPrivate> prpTrack)
> +{
> +    prpTrack->willBeRemoved();
> +}
> +
> +void HTMLMediaElement::mediaPlayerDidRemoveVideoTrack(PassRefPtr<VideoTrackPrivate> prpTrack)
> +{
> +    prpTrack->willBeRemoved();
>  }

This looks much better.  It seems like, in theory, MediaPlayerPrivates could remove the middle-man and just call VideoTrackPrivate::willBeRemoved() directly, but there's nothing keeping both approaches from working with this patch.

[Brendan Long]

(In reply to comment #106)
It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention.

[Eric Carlson]

(In reply to comment #107)
> (In reply to comment #106)
> It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention.

Yes, r+ means that any committer is free to commit. You can mark it cq? if you don't have commit access and someone else can change it to cq+.

[Brendan Long]

(In reply to comment #108)
> (In reply to comment #107)
> > (In reply to comment #106)
> > It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention.
> 
> Yes, r+ means that any committer is free to commit. You can mark it cq? if you don't have commit access and someone else can change it to cq+.

I see. I had been assuming the commit bot was commiting anything that was r+ if it was cq?, but I see now that the reviewers set the cq+ also.

[WebKit Commit Bot]

Comment on attachment 198913 [details]
Generate isReachableFromOpaqueRoots() and fix mac build again

Clearing flags on attachment: 198913

Committed r148785: <http://trac.webkit.org/changeset/148785>