Bug 169841 – Add libwebrtc backend support for RTCRtpSender::replaceTrack

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

replaceTrack might be handy in various cases.

[youenn fablet]

Created attachment 304840 [details]
Patch

[youenn fablet]

Created attachment 305119 [details]
Patch

[Alex Christensen]

Comment on attachment 305119 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSender.idl:42
> -    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack withTrack);
> +    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack? withTrack);

Why are we deviating from the spec here?

[youenn fablet]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 305119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305119&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.idl:42
> > -    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack withTrack);
> > +    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack? withTrack);
> 
> Why are we deviating from the spec here?

Probably a typo, I need to file an issue.
Description at https://www.w3.org/TR/webrtc/#dom-rtcrtpsender-replacetrack expects withTrack to be nullable.

[Jon Lee]

Comment on attachment 305119 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:69
>  {

Are we missing step 4 of replaceTrack?

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:75
> +    if (!withTrack) {

shouldn't this be "if (withTrack)"?

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:85
>          return Exception { TypeError };

Should this be promise.reject(TypeError) instead?

[youenn fablet]

(In reply to Jon Lee from comment #5)
> Comment on attachment 305119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305119&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:69
> >  {
> 
> Are we missing step 4 of replaceTrack?

The implementation order is not matching the spec order.
It is true that it might cause subtle differences between implementation.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:75
> > +    if (!withTrack) {
> 
> shouldn't this be "if (withTrack)"?

Again, we are not matching the spec step by step.
It seems to me the spec should branch early on when withTrack is null.
That would be more readable and easier to implement anyway.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:85
> >          return Exception { TypeError };
> 
> Should this be promise.reject(TypeError) instead?

Oh, that is a left over when we were catching exceptions and rejecting promises.
replaceTrack should not have MayThrowException anymore.

[youenn fablet]

Created attachment 305221 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305221 [details]
Patch

Clearing flags on attachment: 305221

Committed r214357: <http://trac.webkit.org/changeset/214357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.