Closed Bug 1329193 Opened 3 years ago Closed 3 years ago

More overhaul PeerConnection.js with modern JavaScript

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(1 file)

A follow-up to bug 1322274, got inspired to clean up a little more.
Depends on: 1322274
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.
Attachment #8824413 - Flags: review?(martin.thomson) → review+
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?
Rank: 25
Priority: -- → P2
(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.
Rebased.
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58f35b43e9f8
Use es6 classes in PeerConnection.js. r=mt
https://hg.mozilla.org/mozilla-central/rev/58f35b43e9f8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.