1329193 - More overhaul PeerConnection.js with modern JavaScript

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

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

A follow-up to bug 1322274, got inspired to clean up a little more.

Comment 1

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

Attachment: Bug 1329193: Use es6 classes in PeerConnection.js.

Review commit: https://reviewboard.mozilla.org/r/102926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102926/

Comment 2

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

Green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8de13211326092f0e6536ca70ada75d5cdf698c

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8824413 [details]
Bug 1329193: Use es6 classes in PeerConnection.js.

https://reviewboard.mozilla.org/r/102926/#review103686

::: dom/media/PeerConnection.js:53
(Diff revision 1)
>    let console = Cc["@mozilla.org/consoleservice;1"].
>    getService(Ci.nsIConsoleService);
>    console.logMessage(scriptError);
>  };
>  
> +let classInit = (_class, dict) => {

I wonder if this is the best name.  "init" implies actions on an instance.  setupPrototype might be better.

::: dom/media/PeerConnection.js
(Diff revision 1)
> +  constructor() {
> -  this._senders = [];
> +    this._senders = [];
> -  this._receivers = [];
> +    this._receivers = [];
>  
> -  this._pc = null;
> +    this._pc = null;
> -  this._observer = null;

So rather than an explicit initialization, you are leaving these undefined on construction, is that the intent?

::: dom/media/PeerConnection.js:529
(Diff revision 1)
>    // correct line-numbers in errors, provided that methods validate their inputs
>    // before putting themselves on the pc's operations chain.
>    //
>    // It also serves as guard against settling promises past close().
>  
> -  _async: function(func) {
> +  _async(func) {

You might want to consider a new name for this too.  Adding an underscore to a keyword isn't especially clear.

Comment 4

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

Comment on attachment 8824413 [details]
Bug 1329193: Use es6 classes in PeerConnection.js.

https://reviewboard.mozilla.org/r/102926/#review103686

> I wonder if this is the best name.  "init" implies actions on an instance.  setupPrototype might be better.

I had setStatic initially.

> So rather than an explicit initialization, you are leaving these undefined on construction, is that the intent?

Yes. These values are never encountered uninitialized.

> You might want to consider a new name for this too.  Adding an underscore to a keyword isn't especially clear.

Would a comment help? It's basically impossible to use async directly in JSImplemented code without wrapping, because the implicit promise is of the wrong type for content otherwise. _wrap?

Comment 5

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> I had setStatic initially.

It's not really static stuff, it's assigning things directly to the prototype, usually for XPCOM.  setupPrototype might work.

> > You might want to consider a new name for this too.  Adding an underscore to a keyword isn't especially clear.
> 
> Would a comment help? It's basically impossible to use async directly in
> JSImplemented code without wrapping, because the implicit promise is of the
> wrong type for content otherwise. _wrap?

Yeah, a comment should do.  I think that the fact that we can't transparently pass promises is a major failing of the API ergonomics.  We should have someone look into that.

Comment 6

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

Comment on attachment 8824413 [details]
Bug 1329193: Use es6 classes in PeerConnection.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102926/diff/1-2/

Comment 7

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

Comment on attachment 8824413 [details]
Bug 1329193: Use es6 classes in PeerConnection.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102926/diff/2-3/

Comment 8

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

Rebased.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58f35b43e9f8
Use es6 classes in PeerConnection.js. r=mt

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58f35b43e9f8