1098015 - Need to not fail on seeing RTCAnswerOptions in CreateAnswer

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P4)

Description

[Byron Campen [:bwc]]

Word is that W3C is moving toward having an RTCAnswerOptions param on createAnswer.

Comment 1

[Byron Campen [:bwc]]

I think this boils down to support for VoiceActivityDetection.

Comment 2

[Philipp Hancke [:fippo]]

there is a more immediate problem, see https://github.com/webrtc/apprtc/pull/162#discussion-diff-34814044

When calling this.pc_.createAnswer({mandatory:..., optional: ...})
Firefox errors out since it interprets this as a callback and does not find an error callback.
From discussion with jib on a similar issue in createOffer a while ago we concluded that webidl can't tell the difference between function and object.

Even if it's a no-op, not throwing an error in that case would be good.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The error in comment 2 is:

  TypeError: Not enough arguments to mozRTCPeerConnection.createAnswer.

I'll submit a patch here to support the optional (no-op) argument to createAnswer.

I've opened Bug 1184712 to track voiceActivityDetection.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8634935 [details]
MozReview Request: Bug 1098015 - make createAnswer tolerate (no-op) options argument.

https://reviewboard.mozilla.org/r/13477/#review12095

Ship It!

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8634935 [details]
MozReview Request: Bug 1098015 - make createAnswer tolerate (no-op) options argument.

https://reviewboard.mozilla.org/r/13477/#review12097

::: dom/media/PeerConnection.js:697
(Diff revision 1)
> -  createAnswer: function(onSuccess, onError) {
> +  createAnswer: function(optionsOrOnSuccess, onError) {
> +    let onSuccess, options;
> +    if (typeof optionsOrOnSuccess == "function") {
> +      onSuccess = optionsOrOnSuccess;
> +    } else {
> +      options = optionsOrOnSuccess;
> +    }

I think it would be helpful to comment why we're doing this.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/13477/#review12097

> I think it would be helpful to comment why we're doing this.

It's similar to what we're already doing on createOffer, but I'll add a comment to both.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8634935 [details]
MozReview Request: Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1098015 - modify a test to make sure createAnswer({}) works. r=bwc

Bug 1098015 - modify a test to make sure createAnswer({}) works.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8634976 [details]
MozReview Request: Bug 1098015 - modify a test to make sure createAnswer({}) works. r=bwc

https://reviewboard.mozilla.org/r/13493/#review12181

::: dom/media/tests/mochitest/test_peerConnection_promiseSendOnly.html:41
(Diff revision 1)
> -    .then(() => pc1.createOffer())
> +    .then(() => pc1.createOffer({}))
>      .then(offer => pc1.setLocalDescription(offer))
>      .then(() => pc2.setRemoteDescription(pc1.localDescription))
> -    .then(() => pc2.createAnswer())
> +    .then(() => pc2.createAnswer({}))

Couldn't hurt to have a comment here explaining why you're doing this.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8634935 [details]
MozReview Request: Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Bug 1098015 - make createAnswer tolerate (no-op) options argument.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8634976 [details]
MozReview Request: Bug 1098015 - modify a test to make sure createAnswer({}) works. r=bwc

Bug 1098015 - modify a test to make sure createAnswer({}) works. r=bwc

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/382c04062988
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8c8896957f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/382c04062988
https://hg.mozilla.org/mozilla-central/rev/7d8c8896957f