Separate signal types from peerconnection

[iislucas]

To be reviewed after: #145 is merged and this pull request is updated to be against the latest head of dev (this pull request builds on that one).

There were a number of places (in uproxy-networking) where peerconnection was being imported just for signaling types; this was bad because it results in huge amounts of confused code in the browserified outputs (e.g. module-environment dependent code in a core env). So, I moved the signal types to their own file and now import only that when needed (done in: UWNetworksLab/uProxy-networking#224)

Note: In merge; noticed that coverage was broken, so that has been fixed too.

[iislucas]

Thanks!

Renamed to signals I think I like that better too.

Sadly your comment on JSON.stringify didn't have a location attached to it (maybe reviewable is better at that), but I'm guessing you're referring to log statements... I've removed JSON.stringify from those locations. I also noticed two potential bugs, fixed one, and added a TODO for the other.

PTAL.

[jpevarnek]

Strange, it's showing up on the line for me...

I think you may have missed another comment I had above:

General note: I would rename all the places you have used s as a variable name to message, I think that is a lot more descriptive (I might also do the same for some of the places the variable is still called signal actually)

[iislucas]

ooo, I did miss that, could call. fixed. PTAL.

[trevj]

I think this is the diff we're looking for:
iislucas-coverageRuleFixes...iislucas-separate-signal

[trevj]

You've made this signal:signals.Message in some places, not in others. I prefer signal but we should be consistent.

[iislucas]

I think I'm now using message::signals.Message everywhere.

[jpevarnek]

LGTM

[trevj]

"signal message" reads weird..."signalling message" would be better (or "signal" for short).

[iislucas]

done.

[iislucas]

@trevj: ping?

[trevj]

Sorry! Two easy comments, then 👍.

---

---

^{src/webrtc/peerconnection.ts, line 392 [r4] (raw file):}
This TODO does not read well :-)

---

^{src/webrtc/peerconnection.ts, line 447 [r4] (raw file):}
Indentation, semi-colons, and quotes need attention:

 var descriptionString :string;
 try {
  descriptionString = JSON.stringify(description);
} catch (e) {
  descriptionString = '[unstringifiable object]';
}
this.closeWithError_('Failed to set remote description: ' +
    descriptionString + '; Error: ' + e.toString());

---

---

Comments from the review on Reviewable.io

[iislucas]

---

^{src/webrtc/peerconnection.ts, line 392 [r4] (raw file):}
oops, that comment was confused/wrong (only parse needs a try-catch). good catch. removed.

---

^{src/webrtc/peerconnection.ts, line 447 [r4] (raw file):}
oops, same error here: stringify doesn't throw so this isn't needed. cleaned up.

---

---

Comments from the review on Reviewable.io

[iislucas]

Thanks for your diligent reviewing; that caught two confusions that nearly crept into the code!

---

---

---

Comments from the review on Reviewable.io

[jpevarnek]

---

^{src/webrtc/peerconnection.ts, line 444 [r4] (raw file):}
stringify will throw in some cases (at the very least if it is given a recursive data structure)

---

---

Comments from the review on Reviewable.io

[iislucas]

---

^{src/webrtc/peerconnection.ts, line 444 [r4] (raw file):}
good point. For these data structures, we are never going to get exceptions; so I'm fine without the runtime-check (we get compile time checking goodness here)

---

---

Comments from the review on Reviewable.io