835712 - Stop hand-rolling sequence conversion code

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 834463 comment 5.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Both part of converting to WebIDL and a spec bug to bring up in W3 WebRTC.

Comment 2

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

Attachment: Uses more webidl for safer JS traversal

Partly needed by Bug 835370 which I built on-top of this one, and partly a good-will patch to make up for what I wasn't able to do with webIDL in that bug :-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 713422 [details] [diff] [review]
Uses more webidl for safer JS traversal

>+  RTCIceServer RTCIceServer();

Is this still needed?  If RTCConfiguration includes RTCIceServer somewhere inside it, then it shouldn't be.

The patch doesn't have an RTCConfiguration.webidl.  Forgot to hg add?

> +  if (!(config.Init(aCx, nullptr, aSrc)) && config.mIceServers.WasPassed()) {

That looks wrong.  It should either be:

  if (!(config.Init(aCx, nullptr, aSrc) && config.mIceServers.WasPassed())) {

or:

  if (!config.Init(aCx, nullptr, aSrc) || !config.mIceServers.WasPassed()) {

but the patch as written will fail to return early if mIceServers was in fact not passed, afaict.

>-    if (!server.mUrl.WasPassed()) {
>-      return NS_ERROR_FAILURE;
>-    }

I don't think you can remove this check.

r=me with the above fixed, though I'd like to see the new WebIDL file too.  Thank you for doing this!

Comment 4

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

Attachment: Uses more webidl for safer JS traversal

>>+  RTCIceServer RTCIceServer();
>
> Is this still needed?  If RTCConfiguration includes RTCIceServer somewhere inside it,
> then it shouldn't be.
I did that because without it, RTCIceServer got defined AFTER its use in the bindings, and I got errors. But when I remove it now it works, so not sure what happened. Removed.

> The patch doesn't have an RTCConfiguration.webidl.  Forgot to hg add?
Yes :*) Now added!

>> +  if (!(config.Init(aCx, nullptr, aSrc)) && config.mIceServers.WasPassed()) {
>
> That looks wrong.  It should either be:
>
>  if (!(config.Init(aCx, nullptr, aSrc) && config.mIceServers.WasPassed())) {
Good catch, thanks!

>>-    if (!server.mUrl.WasPassed()) {
>>-      return NS_ERROR_FAILURE;
>>-    }
>
> I don't think you can remove this check.
Agree. Now back. Side-note with validation done in JS now, perhaps all these check could be squeaky NS_ENSURE_SUCCESS()?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 713541 [details] [diff] [review]
Uses more webidl for safer JS traversal

r=me

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a21768b10f0

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2a21768b10f0