[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 #90 from Jer Noble <jer.noble at apple.com>  2013-04-17 15:42:36 PST ---
(From update of attachment 198390)
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.

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