Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add success and error callbacks to setLocalDescription and setRemoteDesc... #74

Closed
wants to merge 9 commits into from

Conversation

fippo
Copy link
Collaborator

@fippo fippo commented Jun 17, 2014

as discussed in issue #73.

@juberti
Copy link
Contributor

juberti commented Jun 17, 2014

I see what you meant before about reusing functions - I suggest adding some sort of webrtc.noop() and webrtc.error() (which calls console.error, at least for now) functions to adapter.js that we can use to simplify this code.

@fippo
Copy link
Collaborator Author

fippo commented Jun 20, 2014

I just moved the errors two adapter.js -- setRemoteDescriptionFailure and setLocalDescriptionFailure.
I also noticed that error callbacks for createOffer and createAnswer are missing sometimes. I fixed this in one instance, added createOfferFailure and createAnswerFailure but haven't grepped yet.

(style) question about the success callbacks, e.g.
https://github.com/GoogleChrome/webrtc/blob/master/samples/web/content/datachannel/js/main.js#L118 call setLocalDescription-setRemoteDescription-createAnswer sequentially, but shouldn't setRemoteDescription be called in the success callback of setLocalDescription and likewise createAnswer in the callback of setRemoteDescription? videopipe.js does the latter at least.

Arguably, errors are very unlikely when two peerconnection objects on the same page talk to each outher.

@fippo
Copy link
Collaborator Author

fippo commented Jul 11, 2014

moved the success callbacks to adapter.js as well.

@juberti
Copy link
Contributor

juberti commented Jul 11, 2014

I don't like the idea of dumping functions called setRDFailure/setLDFailure into the global namespace, as they might collide with other real names in application code. It also isn't clear what these functions do from their names - hence I prefer the simpler and more explicit webrtc.noop and webrtc.error suggested above.

@fippo
Copy link
Collaborator Author

fippo commented Jul 12, 2014

mh... but dumping a global webrtc object into adapter.js isn't ideal either.
How about putting this into a common.js which is shared by all demos but not pulled in by anyone that just wants adapter.js as a shim that hides browser differences?

@juberti
Copy link
Contributor

juberti commented Aug 25, 2014

I hate to make a new file for just these little nibs of code. Are you worried about a webrtc object stomping on apps' webrtc objects? We could just pick a different name.

@fippo
Copy link
Collaborator Author

fippo commented Aug 26, 2014

Right, it doesn't make sense for these little nibs of code. We could bold and see if anyone complains about this webrtc object :-)

@fippo
Copy link
Collaborator Author

fippo commented Sep 8, 2014

ping. Apparently FF32 has a regression (https://bugzil.la/1063971) which causes it not to work for at least the data channel example (https://groups.google.com/forum/#!topic/discuss-webrtc/u5NHYR-4rpc) when the mandatory callbacks are missing.

@@ -197,3 +197,11 @@ if (navigator.mozGetUserMedia) {
} else {
console.log("Browser does not appear to be WebRTC-capable");
}

var webrtc = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment about that this does

@@ -157,12 +157,24 @@ function createPeerConnection() {
};
localPeerConnection.createOffer(function (desc) {
console.log('localPeerConnection offering');
localPeerConnection.setLocalDescription(desc);
remotePeerConnection.setRemoteDescription(desc);
localPeerConnection.setLocalDescription(desc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to keep these args on the same line, as long line is < 80 chars (here and below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hitting 82 chars in some places... reduce it to a single line where possible and two lines elsewhere or two lines (putting webrtc.noop and webrtc.error on the second) everywhere?

(no preference, anything that makes jshint happy is fine with me :-)

@fippo
Copy link
Collaborator Author

fippo commented Nov 6, 2014

too many merge conflicts... and it looks this has been taken care of already in the meantime.

@fippo fippo closed this Nov 6, 2014
@fippo fippo deleted the SLD-SRD-callbacks branch October 12, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants