1018060 - Video calls via XMPP/Jingle and WebRTC

Status: NEW

(Chat Core :: XMPP, enhancement)

Description

[Mayank Kumar [:mayanktg]]

Attachment: Florian's WebRTC support Patch for Ib

This bug is to keep a track on the project to add Video and voice call support to XMPP protocol using WebRTC.
There is a patch made by Florian, I'm including this here for reference.

*The initial patch will include adding voice call support and notification option for user to accept/reject call. A user would be able to make call using the /call command or by accessing UI button for the same.

Comment 1

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: send SDP offer

To test the patch you need to do a full mach build. The WIP contains:
* toolbarbutton to initiate video call (used from work of Bug 1004930)
* Create a SDP offer in the converastion.xml binding.
* sdp2xml() in xml-jingle.jsm is used to convert the offer into XML IQ Stanza.
* The startCall() in xmpp.jsm sends this IQ Stanza to the callee.

Comment 2

[aleth [:aleth]]

Please attach your tests as well.

Comment 3

[aleth [:aleth]]

Comment on attachment 8436934 [details] [diff] [review]
(WIP) v1: send SDP offer

Review of attachment 8436934 [details] [diff] [review]:
-----------------------------------------------------------------

Some drive-by comments.

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +75,5 @@
> +  //jingle
> +  action_initiate           : "session-initiate",
> +  creator                   : "initiator",
> +  id_initiate               : "zid615d9",
> +  sid                       : "a73sjjvkla37jfea",

These two ids should not be hardcoded.

::: chat/protocols/xmpp/xmpp.jsm
@@ +283,5 @@
> +                                            initiator: this.from,
> +                                            sid: Stanza.NS.sid});
> +    let iqStanza = jingle.sdp2xml(aOffer);
> +
> +    for(let i=0; i<aOffer.match(/m=/g).length; i++)

Better something like: 
for (let contentNode of iqStanza) item.addChild(contentNode);

Also sdp2xml doesn't return an iq stanza so the variable name makes no sense.

@@ +286,5 @@
> +
> +    for(let i=0; i<aOffer.match(/m=/g).length; i++)
> +      item.addChild(iqStanza[i]);
> +
> +    let s = Stanza.iq("set", Stanza.NS.id_initiate, this.to + "/Instantbird", item);

Please investigate why the resource is not included by the to getter (i.e. when is the resource set, and why doesn't that happen here?)

Comment 4

[aleth [:aleth]]

Global nit: We put a space between the keyword and the bracket in for (...) and if (...).

Comment 5

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: test sdp2xml

Comment 6

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: send SDP offer

*Fixed nits mentioned by aleth.
*added generated id ans sid.
*Fixed error caused earlier while adding resource to JID.

Comment 7

[Mayank Kumar [:mayanktg]]

Comment on attachment 8437498 [details] [diff] [review]
(WIP) v1: send SDP offer

Review of attachment 8437498 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +282,5 @@
> +    let id   = "callid" + Math.floor(Date.now() / 100000);
> +    let sId  = "sid" + Math.floor(Date.now());
> +    let item = Stanza.node("jingle", null, {xmlns: Stanza.NS.xmlnsJ,
> +                                            action: Stanza.NS.action_initiate,
> +                                            initiator: "", // FIXME: initiator fullJID here

How to get the initiator FullJID?

@@ +288,5 @@
> +    let iqContent = jingle.sdp2xml(aOffer);
> +
> +    // Using the below for...of returns
> +    // Error: iqContent['@@iterator'] is not a function
> +    // for (let contentNode of iqContent) item.addChild(contentNode);

iqContent is an array. Still I'm getting this error.

@@ +293,5 @@
> +
> +    for (let i=0; i<aOffer.match(/m=/g).length; i++)
> +      item.addChild(iqContent[i]);
> +
> +    let toJID = (!this._targetResource) ? (this.to + "/Instantbird") : this.to;

The resource would now be added only if _targetResource is an empty String.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8437498 [details] [diff] [review]
(WIP) v1: send SDP offer

Review of attachment 8437498 [details] [diff] [review]:
-----------------------------------------------------------------

I did I drive by review of the backend XMPP code.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +1,1 @@
> +

Please add the proper license block.

@@ +17,5 @@
> +    let itemPayload     = {};
> +    let itemTransport   = {};
> +    let itemFingerprint = {};
> +    let dataCandidate   = {};
> +    let itemCandidate   = {};

You seem to declare these all as objects and then use them as arrays. Maybe you meant []? Generally we don't align equal signs like you've done.

@@ +21,5 @@
> +    let itemCandidate   = {};
> +    let media           = offer.match(/m=/g); // Matches number of media present.
> +    let name            = ["voice", "face"]; //content name
> +
> +    for (let i=0; i<media.length; i++) {

Nit: Spaces around operators (=, <). ++i, not i++. (Everywhere in the file.)

@@ +22,5 @@
> +    let media           = offer.match(/m=/g); // Matches number of media present.
> +    let name            = ["voice", "face"]; //content name
> +
> +    for (let i=0; i<media.length; i++) {
> +      //content

Nit (everywhere in the file): A space after //, please use full sentences in comments. I think we've discussed this a few times previously. Try to keep this in mind when writing code!

@@ +51,5 @@
> +      }
> +
> +      // payload-type [a=rtpmap:8 PCMA/8000]
> +      // Finds the group containing a=rtpmap
> +      dataPayload[i] = offer.match(/(a=rtpmap[\s\S]*?)(?=a=candidate)/g)[i] + "";

Would it make sense to do this regular expression only once and keep all the matches in the dataPayload array? Otherwise I'd think you want to only look over a smaller part of the offer or something.

@@ +69,5 @@
> +                                           name: (payloadAttr[1] + "").split("/")[0],
> +                                           clockrate: (payloadAttr[1] + "").split("/")[1] });
> +          else
> +            itemPayload[j] = Stanza.node("payload-type", null,
> +                                         { id: (payloadAttr[0] + "").split(":")[1],

Nit: No space after { (Or before } a few lines below this)

@@ +72,5 @@
> +            itemPayload[j] = Stanza.node("payload-type", null,
> +                                         { id: (payloadAttr[0] + "").split(":")[1],
> +                                           name: [0],
> +                                           clockrate: (payloadAttr[1] + "").split("/")[1],
> +                                           channels: channelVal });

Nit: Use { } whenever a statement is multiple lines. (This also happens in other places in the file.)

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +80,5 @@
> +  xmlnsJ                    : "urn:xmpp:jingle:1",
> +  xmlnsD                    : "urn:xmpp:jingle:apps:rtp:1",
> +  xmlnsS                    : "urn:xmpp:jingle:apps:sdp",
> +  xmlnsT                    : "urn:xmpp:jingle:transports:ice-udp:1",
> +  xmlnsF                    : "urn:xmpp:jingle:apps:dtls:0"

Please name these constants in a way that's similar to the other ones, i.e. none of those include "xmlns", they use fairly descriptive variable names.

::: chat/protocols/xmpp/xmpp.jsm
@@ +17,5 @@
>  Cu.import("resource:///modules/jsProtoHelper.jsm");
>  Cu.import("resource:///modules/socket.jsm");
>  Cu.import("resource:///modules/xmpp-xml.jsm");
>  Cu.import("resource:///modules/xmpp-session.jsm");
> +Cu.import("resource:///modules/xmpp-jingle.jsm");

Nit: Keep these in alphabetical order.

@@ +293,5 @@
> +
> +    for (let i=0; i<aOffer.match(/m=/g).length; i++)
> +      item.addChild(iqContent[i]);
> +
> +    let toJID = (!this._targetResource) ? (this.to + "/Instantbird") : this.to;

This doesn't make sense, we should NEVER be hard coding the resource like this!

Comment 9

[aleth [:aleth]]

Comment on attachment 8437498 [details] [diff] [review]
(WIP) v1: send SDP offer

Review of attachment 8437498 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +282,5 @@
> +    let id   = "callid" + Math.floor(Date.now() / 100000);
> +    let sId  = "sid" + Math.floor(Date.now());
> +    let item = Stanza.node("jingle", null, {xmlns: Stanza.NS.xmlnsJ,
> +                                            action: Stanza.NS.action_initiate,
> +                                            initiator: "", // FIXME: initiator fullJID here

Isn't the initiator the sender? All the required info should be in properties of the account.

@@ +293,5 @@
> +
> +    for (let i=0; i<aOffer.match(/m=/g).length; i++)
> +      item.addChild(iqContent[i]);
> +
> +    let toJID = (!this._targetResource) ? (this.to + "/Instantbird") : this.to;

This won't work as the resource can be anything. It doesn't have to be "Instantbird" ;)

The right way to fix this is to use this.to only. Then you have to ensure we obtain the targetResource immediately when starting a conversation. Take a look in the debug log to see what is exchanged when a new conversation is opened. If there's nothing useful there, is there a stanza you can send that will get the receiver to send you the resource? Maybe a requestBuddyInfo?

Comment 10

[aleth [:aleth]]

Comment on attachment 8437498 [details] [diff] [review]
(WIP) v1: send SDP offer

Review of attachment 8437498 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +9,5 @@
> +const jingle = {
> +
> +  sdp2xml: function sdp2xml(offer) {
> +    // Using XEP-0320: Jingle RTP Session
> +    let itemContent     = {};

You want this to be an array, not an object!

@@ +24,5 @@
> +
> +    for (let i=0; i<media.length; i++) {
> +      //content
> +      itemContent[i] = Stanza.node("content", null, {creator: Stanza.NS.creator,
> +                                                     name: name[i]});

To add entries to an array, use itemContent.push().

Note your existing code did not throw any errors because you can use [x] to access the properties of JS objects.

If you don't understand some of this, ask and read up a bit on JS objects and arrays!

Comment 11

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: send SDP offer

Comment 12

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: test sdp2xml

Comment 13

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: Convert to SDP and receive video call offer

This patch contains:
* xml2sdp() which converts the given XML stanza into an equivalent SDP "offer" or "answer".
* notifyObservers sends the SDP data to the conversation binding.
* webrtcCallRequest method which takes the SDP offer as an argument and forms a suitable answer.

Comment 14

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: Send, receive SDP answer and establish video call.

This patch contains:
* Sends the SDP "answer" generated to the prpl which then converts it to suitable XML stanza and send the answer back to caller.
* Answer converted back to SDP and sent to the conversation binding.
* webRTCAnswercall method is called with SDP answer and video call session is established.

Comment 15

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: Merged test for xml2sdp and sdp2xml.

This patch contains:
* Test for sdp2xml and xml2sdp. (It modifies the patch written earlier for sdp2xml. I'll merge to two tests and update here soon.)

Comment 16

[Mayank Kumar [:mayanktg]]

Serial order in which patch should be applied:
1. Apply patch for Bug 1004930.
2. (WIP) v1: send SDP offer.
3. (WIP) v1: test sdp2xml.
4. (WIP) v1: Convert to SDP and receive video call offer.
5. (WIP) v1: Send, receive SDP answer and establish video call.
6. (WIP) v1: Merged test for xml2sdp and sdp2xml.

Comment 17

[Mayank Kumar [:mayanktg]]

NOTE: (Present issues and improvements which needs to be done)
1. Currently there is no specified format given in XEPs for the semantic a:rtcp-mux (indicates the desire to multiplex RTP and RTCP onto a single port). Since most of the devices contains this semantic I have taken this as default parameter to be added at end of each media type format definition.
2. The call is auto answering. We need to add a accept/reject notification for the callee.
3. Add call disconnect support.
3. The _targetResource is set to "" when no conversation has been carried out before. It requires full JID to send the IQ Stanza to the callee.
4. More sample SDP offers and answers are needed to improve the conversion accuracy (Maybe this can be useful for example offers http://tools.ietf.org/id/draft-nandakumar-rtcweb-sdp-01.html#rfc.section.5).
5. The video call can only be established if message exchange has happened before the clients. One should be able to start the call as soon as conversation window opens.
6. The current UI is not as per the prototype.
7. Add capabilities discovery (XEP 0030: Service Discovery). Add video call support to only those clients who supports WebRTC.

Comment 18

[aleth [:aleth]]

Comment on attachment 8439457 [details] [diff] [review]
(WIP) v1: Merged test for xml2sdp and sdp2xml.

Review of attachment 8439457 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/test/test_sdp2xml.js
@@ +99,5 @@
> +            ''];
> +xml2 = xml2.join("\n");
> +
> +let xml  = [xml1, xml2];
> +let offer = jingle.sdp2xml(sdp);

Please place this call inside the two tests.

Comment 19

[aleth [:aleth]]

Comment on attachment 8439455 [details] [diff] [review]
(WIP) v1: Send, receive SDP answer and establish video call.

Review of attachment 8439455 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/public/prplIConversation.idl
@@ +72,5 @@
>    /* Send a SDP offer when user makes a call */
>    void startCall(in AUTF8String aOffer);
>  
> +  /* Send a SDP answer when a user request a video call */
> +  void webrtcAnswer(in AUTF8String aAnswer);

How about "acceptCall"  or "answerCall" rather than webrtcAnswer?

::: chat/protocols/xmpp/xmpp.jsm
@@ +297,5 @@
>  
> +  /* Called when the user sends an SDP answer for a video call. */
> +  webrtcAnswer: function(aAnswer) {
> +    let id   = "callid" + Math.floor(Date.now() / 100000);
> +    let sId  = "sid" + Math.floor(Date.now());

What's the XEP for the generation of ids?

@@ +304,5 @@
> +                                            initiator: this._account._connection._jid.jid,
> +                                            sid: sId});
> +    let iqContent = jingle.sdp2xml(aAnswer);
> +
> +    for (let contentNode of iqContent) item.addChild(contentNode);

nit: Put the loop content on its own line.

@@ +306,5 @@
> +    let iqContent = jingle.sdp2xml(aAnswer);
> +
> +    for (let contentNode of iqContent) item.addChild(contentNode);
> +
> +    let toJID = (!this._targetResource) ? (this.to + "/Instantbird") : this.to;

Let's drop this hack and use this.to. At the moment it's necessary to exchange some messages before calling anyway.

::: im/content/conversation.xml
@@ +377,5 @@
> +        <![CDATA[
> +          let answer = {type: "answer", sdp: aAnswer};
> +          answer = new mozRTCSessionDescription(answer);
> +          this.pc.setRemoteDescription(answer);
> +          return;

You don't need to add a return at the end of a function if you're not returning a value.

Comment 20

[aleth [:aleth]]

Comment on attachment 8439451 [details] [diff] [review]
(WIP) v1: Convert to SDP and receive video call offer

Review of attachment 8439451 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +206,5 @@
> +      media.push(("m=" + descData.attributes["media"]));
> +
> +      // Connection element (IP is first IP of candidate) [c=IN IP4 106.51.45.18].
> +      let firstCandidateData = (cNode.getElement(["transport"])).getElement(["candidate"]);
> +      media[1] = firstCandidateData.attributes["port"];

This will fail for the second, third, ... cNode in the loop, won't it?

@@ +287,5 @@
> +                        + " typ " + candNode.attributes["type"];
> +        if (candNode.attributes["relport"]) {
> +          candidate += " raddr " + candNode.attributes["reladdr"]
> +                       + " rport " + candNode.attributes["relport"];
> +          media[1] = candNode.attributes["relport"];

media[1] again? are we overwriting intentionally what we did above?

@@ +294,5 @@
> +        nodeContent.push(candidate);
> +      }
> +
> +      nodeContent.splice(indexConnection, 0, media.join(" "));
> +      media.length = 0;

What's this for? Do you mean media = [] ? If so, put the let media =[] at the beginning on the loop (let has block scope)

@@ +300,5 @@
> +      nodeContent.push("a=rtcp-mux");
> +    }
> +
> +    sdp.push(nodeContent.join("\r\n"));
> +    return (sdp.join("\r\n"));

nit: you don't need the outer brackets

Comment 21

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v2: 1. Send SDP offer

Comment 22

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v2: 2. Test sdp2xml

Comment 23

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: 3. Convert to SDP and receive video call offer

Comment 24

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: 4. Merged test for xml2sdp and sdp2xml.

Comment 25

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: 5. Send, receive SDP answer and establish video call.

Comment 26

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v1: 3. Disconnect Call.

Comment 27

[Mayank Kumar [:mayanktg]]

(In reply to aleth [:aleth] from comment #20)
> Comment on attachment 8439451 [details] [diff] [review]
> (WIP) v1: Convert to SDP and receive video call offer
> 
> Review of attachment 8439451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/xmpp/xmpp-jingle.jsm
> @@ +287,5 @@
> > +                        + " typ " + candNode.attributes["type"];
> > +        if (candNode.attributes["relport"]) {
> > +          candidate += " raddr " + candNode.attributes["reladdr"]
> > +                       + " rport " + candNode.attributes["relport"];
> > +          media[1] = candNode.attributes["relport"];
> 
> media[1] again? are we overwriting intentionally what we did above?
If there is no relport present in any candidate element the media should
bind to the port of the first candidate. Else it should bind to the first
relport it finds. When you made the comment, I made a mistake there
and it was taking the last relport. Its fixed now.

Comment 28

[Mayank Kumar [:mayanktg]]

(In reply to aleth [:aleth] from comment #19)
> Comment on attachment 8439455 [details] [diff] [review]
> (WIP) v1: Send, receive SDP answer and establish video call.
> 
> Review of attachment 8439455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/components/public/prplIConversation.idl
> @@ +72,5 @@
> >    /* Send a SDP offer when user makes a call */
> >    void startCall(in AUTF8String aOffer);
> >  
> > +  /* Send a SDP answer when a user request a video call */
> > +  void webrtcAnswer(in AUTF8String aAnswer);
> 
> How about "acceptCall"  or "answerCall" rather than webrtcAnswer?
I've changed it to answerCall().
> 
> ::: chat/protocols/xmpp/xmpp.jsm
> @@ +297,5 @@
> >  
> > +  /* Called when the user sends an SDP answer for a video call. */
> > +  webrtcAnswer: function(aAnswer) {
> > +    let id   = "callid" + Math.floor(Date.now() / 100000);
> > +    let sId  = "sid" + Math.floor(Date.now());
> 
> What's the XEP for the generation of ids?
http://xmpp.org/extensions/xep-0124.html#security-sidrid states that the
SIDs should be uniquely generated, MUST be unpredictable and non repeating.
Keeping "sid<number>" would suffice our purpose?

Comment 29

[Mayank Kumar [:mayanktg]]

NOTE:
Design Changes -
* Add CSS and icon to "call disconnect" button. Use "command" to disconnect
  call instead of onclick.
* Add CSS to notification bar.
* Fix how the size of remoteVideo and localVideo changes when width and 
  height of window is changed (the remote video should fit the width).

Comment 30

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v2: WebRTC Video Call Patch

Merged patch for the bug.

Comment 31

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v2: Test patch

Test patch for functions xml2sdp() and sdpxml().

Comment 32

[aleth [:aleth]]

Comment on attachment 8442896 [details] [diff] [review]
(WIP) v2: WebRTC Video Call Patch

Review of attachment 8442896 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial commemnts. Looks like this is making good progress!

Please file followup bugs for mute/hold, fullscreen, audio-only calls, the "what if the no messages have been exchanged yet" issue, and anything else that shouldn't be forgotten.

::: chat/components/public/prplIConversation.idl
@@ +75,5 @@
> +  /* Send a SDP answer when a user request a video call */
> +  void answerCall(in AUTF8String aAnswer);
> +
> +  /* Request a video call disconnect */
> +  void disconnectCall();

endCall() would fit better with startCall().

::: chat/modules/jsProtoHelper.jsm
@@ +512,5 @@
>        this.typingState = aState;
>      this.notifyObservers(null, "update-typing", null);
>    },
>  
> +  startCall: function(aOffer) { throw Cr.NS_ERROR_NOT_IMPLEMENTED; },

Nit: if you don't leave blank lines between these, they form a sensible group together.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +21,5 @@
> +    let itemTransport = [];
> +    let itemFingerprint = [];
> +    let dataCandidate = [];
> +    let itemCandidate = [];
> +    let media = offer.match(/m=/g); // Matches number of media present.

This file needs much more explanatory comments. For example here, "Each media entry describes a ..."

@@ +22,5 @@
> +    let itemFingerprint = [];
> +    let dataCandidate = [];
> +    let itemCandidate = [];
> +    let media = offer.match(/m=/g); // Matches number of media present.
> +    let name = ["voice", "face"]; // Content name.

Again as an example, explain in the comment what this is used for.

@@ +41,5 @@
> +                                 setup: (offer.match(/(a=setup)[\S]*/g)[i] + "").split(":")[1]});
> +      itemContent[i].addChild(itemDesc[i]);
> +
> +      // XEP: 2094 Jingle RTP Header Extensions Negotiation.
> +      // rtp-hdrext [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].

Can we have a comment explaining 1) what header extensions negotiation does 2) what that second cryptic line means or is an example for.

@@ +43,5 @@
> +
> +      // XEP: 2094 Jingle RTP Header Extensions Negotiation.
> +      // rtp-hdrext [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].
> +      let dataRtpHdrext = dataDesc[i].match(/(a=extmap[\s\S]*?)(?=a=)/);
> +      if (dataRtpHdrext != null) {

if (dataRtpHdrext) is enough.

@@ +44,5 @@
> +      // XEP: 2094 Jingle RTP Header Extensions Negotiation.
> +      // rtp-hdrext [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].
> +      let dataRtpHdrext = dataDesc[i].match(/(a=extmap[\s\S]*?)(?=a=)/);
> +      if (dataRtpHdrext != null) {
> +        dataRtpHdrext = (dataRtpHdrext + "").split(" ");

Why the +"" ? Do you need .toString() here? Why?

I have a suspicion you need to read the MDN pages on what match() returns, and there will be a much nicer way to do this here and below.

@@ +53,5 @@
> +        itemDesc[i].addChild(itemRtpHdrext);
> +      }
> +
> +      // payload-type [a=rtpmap:8 PCMA/8000].
> +      // Finds the group containing a=rtpmap.

Comment explaining what this group is for.

@@ +82,5 @@
> +
> +          itemDesc[i].addChild(itemPayload[j]);
> +
> +          // XEP: 2093 Jingle RTP Feedback Negotiation.
> +          // rtcp-fb [a=rtcp-fb:98 nack rpsi].

Without more comments, you are not going to understand what this does and why in a few months.

@@ +106,5 @@
> +          }
> +        }
> +
> +        // This is additional information provided to the payload-type.
> +        else if (p == "a=fmtp") {

Please no newlines between if and else if, it makes it hard to tell where the whole if block ends.

@@ +199,5 @@
> +    sdp.push("a=ice-pwd:" + eleTransport.attributes["pwd"]);
> +    sdp.push("a=fingerprint:" + eleFingerprint.attributes["hash"]
> +             + " " + eleFingerprint.innerText);
> +
> +    // Content node.

Again, this comment is too short. Did you mean "Process all the content nodes." maybe?

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +71,5 @@
>    avatar_metadata_notify    : "urn:xmpp:avatar:metadata+notify",
>    pubsub                    : "http://jabber.org/protocol/pubsub",
> +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> +
> +  //jingle

nit: comment style

::: chat/protocols/xmpp/xmpp.jsm
@@ +279,5 @@
>  
> +  /* Called when the user requests a video call */
> +  startCall: function(aOffer) {
> +    let id   = "callid" + Math.floor(Date.now() / 100000);
> +    let sId  = "sid" + Math.floor(Date.now());

Can we put helper functions on the account object that generate ids, since this seems to come up again and again?

Refer to http://tools.ietf.org/html/rfc6120 section 4.7.3 and 8.2.3.

@@ +871,5 @@
>            this._onRosterItem(item, true);
>          return;
>        }
> +
> +      let jStanza = aStanza.getElement(["jingle"]);

You should put all the following if clauses in an if (jStanza) block in case there is no jingle element.

@@ +872,5 @@
>          return;
>        }
> +
> +      let jStanza = aStanza.getElement(["jingle"]);
> +      if (jStanza.attributes["action"] == "session-initiate") {

Do you know if there MUST be an action attribute in every jingle stanza? Your code assumes this.

@@ +879,5 @@
> +        this.createConversation(this.normalize(aStanza.attributes["from"]))
> +            .notifyObservers(this, "webrtc-call-request", sdp);
> +      }
> +
> +      if (jStanza.attributes["action"] == "session-accept") {

else if

@@ +886,5 @@
> +        this.createConversation(this.normalize(aStanza.attributes["from"]))
> +            .notifyObservers(this, "webrtc-call-answer", sdp);
> +      }
> +
> +      if (jStanza.attributes["action"] == "session-terminate") {

else if 
or use switch(jStanza.attributes["action"]))

@@ +888,5 @@
> +      }
> +
> +      if (jStanza.attributes["action"] == "session-terminate") {
> +        this.createConversation(this.normalize(aStanza.attributes["from"]))
> +            .notifyObservers(this, "call-disconnect");

Lots of duplication here. this.createConversation(this.normalize(aStanza.attributes["from"]) appears in *every one* of these cases. Pull it outside.

::: im/content/conversation.xml
@@ +277,5 @@
> +        <parameter name="aSuccessCallback"/>
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let doc    = this.browser.contentDocument;

We usually use document for this, unless document is already defined of course.

@@ +278,5 @@
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let doc    = this.browser.contentDocument;
> +          let body   = doc.getElementById("ibcontent");

document.querySelector("body") is better

@@ +296,5 @@
> +          // FIXME command and oncommand attribute doesn't seem to work here.
> +          callDisconnectButton.onclick = function() {
> +                                           conversation.callDisconnect();
> +                                           conversation._conv.disconnectCall();
> +                                         };

Why doesn't conversation.callDisconnect() call this.conv.disconnectCall? 

The two different function names are also confusing. Lets make them match (I think I suggested endCall somewhere).

Then you can do callDisconnectButton.onclick = conversation.endCall;

@@ +299,5 @@
> +                                           conversation._conv.disconnectCall();
> +                                         };
> +          body.appendChild(callDisconnectButton);
> +
> +          doc.getElementById("Chat").setAttribute("style", "position: fixed; left: 0; width: 100%; overflow: auto !important;");

This is not the way we set CSS from JS. CSS properties are mapped to JS properties (dashes in css style names get converted to camelCase style names) on DOM elements. Use those.

Why this change to the Chat element CSS? 

Can't these styles be more cleanly applied via setting a "video-call" attribute on the CHat or body element and making changes to conv.css?

@@ +319,5 @@
> +        <parameter name="aSuccessCallback"/>
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let win = this.browser.contentWindow;

this.contentWindow

@@ +346,5 @@
> +          let doc = this.browser.contentDocument;
> +
> +          let remoteVideo = doc.getElementById("remoteVideo");
> +          remoteVideo.setAttribute("width", width);
> +          remoteVideo.setAttribute("height", height * 0.67);

Can this be done more easily via CSS calc() ? Then it would adjust automatically if the window is resized etc.

@@ +366,5 @@
> +        <body>
> +        <![CDATA[
> +          let conv = this._conv;
> +          let fail = function(err) {
> +            conv.systemMessage("Video call failed: " + err);

This must be localised. Maybe using the notification bar would be better than a system message.

@@ +398,5 @@
> +        <body>
> +        <![CDATA[
> +        let conv = this._conv;
> +        let fail = function(err) {
> +          conv.systemMessage("failed to call: " + err);

Needs some improvement, as above.

@@ +412,5 @@
> +              }, fail);
> +            }, fail);
> +          }, fail);
> +        }).bind(this), fail);
> +        return;

return is not needed here

@@ +440,5 @@
> +            {
> +              label: convBundle.GetStringFromName("rejectCall.label"),
> +              callback: function() {
> +                conversation.callDisconnect();
> +                conversation._conv.disconnectCall();

See above on unifying this.

@@ +1184,5 @@
>                                            (convTopHeight - convTopMinHeight);
>              }
>            }
>  
> +          if ("pc" in this)

Needs a comment - what is this for?

Comment 33

[Mayank Kumar [:mayanktg]]

(In reply to aleth [:aleth] from comment #32)
> Comment on attachment 8442896 [details] [diff] [review]
> (WIP) v2: WebRTC Video Call Patch
> 
> Review of attachment 8442896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some initial commemnts. Looks like this is making good progress!
> 
> Please file followup bugs for mute/hold, fullscreen, audio-only calls, the
> "what if the no messages have been exchanged yet" issue, and anything else
> that shouldn't be forgotten.
Thanks for taking a look into this patch. :)
I'm answering some of the questions below.
> ::: chat/components/public/prplIConversation.idl
> @@ +75,5 @@
> > +  /* Send a SDP answer when a user request a video call */
> > +  void answerCall(in AUTF8String aAnswer);
> > +
> > +  /* Request a video call disconnect */
> > +  void disconnectCall();
> 
> endCall() would fit better with startCall().
Yes, changed to endCall.

> @@ +44,5 @@
> > +      // XEP: 2094 Jingle RTP Header Extensions Negotiation.
> > +      // rtp-hdrext [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].
> > +      let dataRtpHdrext = dataDesc[i].match(/(a=extmap[\s\S]*?)(?=a=)/);
> > +      if (dataRtpHdrext != null) {
> > +        dataRtpHdrext = (dataRtpHdrext + "").split(" ");
> 
> Why the +"" ? Do you need .toString() here? Why?
> 
> I have a suspicion you need to read the MDN pages on what match() returns,
> and there will be a much nicer way to do this here and below.
match() returns an array. I needed a toString() to convert the array into strings.
In the upcoming patch wherever possible I have tried to keep it as an array and 
retrieve the content but at some places I needed to use split() so I have used 
toString() there.

> ::: chat/protocols/xmpp/xmpp-xml.jsm
> @@ +71,5 @@
> >    avatar_metadata_notify    : "urn:xmpp:avatar:metadata+notify",
> >    pubsub                    : "http://jabber.org/protocol/pubsub",
> > +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> > +
> > +  //jingle
> 
> nit: comment style
I have changed the nit style. I used it that way because it was the format
the xmpp-xml.jsm followed. For eg. //addressing, //discovering etc.
> ::: chat/protocols/xmpp/xmpp.jsm
> @@ +279,5 @@
> >  
> > +  /* Called when the user requests a video call */
> > +  startCall: function(aOffer) {
> > +    let id   = "callid" + Math.floor(Date.now() / 100000);
> > +    let sId  = "sid" + Math.floor(Date.now());
> 
> Can we put helper functions on the account object that generate ids, since
> this seems to come up again and again?
> 
> Refer to http://tools.ietf.org/html/rfc6120 section 4.7.3 and 8.2.3.
I have created a new function generateId which forms id and sid for the same.
Thanks for pointing me to these RFCs :) I guess using sid + date.Now() will make
the sid unique. How to use the id btw. Doesn't using the id the way it is solves
our purpose? The same id is returned in the form of type result/error.

> @@ +872,5 @@
> >          return;
> >        }
> > +
> > +      let jStanza = aStanza.getElement(["jingle"]);
> > +      if (jStanza.attributes["action"] == "session-initiate") {
> 
> Do you know if there MUST be an action attribute in every jingle stanza?
> Your code assumes this.
If the IQ stanza with action attr session-initiate/accept/terminate is received
then accordingly the notifyObservers would send the notification to the binding.
A user won't receive notification for any other Jingle Stanza. Correct me if I'm
not making any sense here.

Comment 34

[aleth [:aleth]]

(In reply to Mayank Kumar [:mayanktg] from comment #33)
> > > +      let dataRtpHdrext = dataDesc[i].match(/(a=extmap[\s\S]*?)(?=a=)/);
> > > +      if (dataRtpHdrext != null) {
> > > +        dataRtpHdrext = (dataRtpHdrext + "").split(" ");
> > 
> > Why the +"" ? Do you need .toString() here? Why?
> > 
> > I have a suspicion you need to read the MDN pages on what match() returns,
> > and there will be a much nicer way to do this here and below.
> match() returns an array. I needed a toString() to convert the array into
> strings.
> In the upcoming patch wherever possible I have tried to keep it as an array
> and 
> retrieve the content but at some places I needed to use split() so I have
> used 
> toString() there.

Do you really want to turn an array [a,b,c] into a comma-separated string, and then split it on spaces? I doubt it.

My guess is you should be using dataRtpHdrext[0] here.

Are you sure there will be only one match?

> > nit: comment style
> I have changed the nit style. I used it that way because it was the format
> the xmpp-xml.jsm followed. For eg. //addressing, //discovering etc.

OK, that makes sense, I didn't realize the existing style wasn't correct ;)

> > > +      let jStanza = aStanza.getElement(["jingle"]);
> > > +      if (jStanza.attributes["action"] == "session-initiate") {
> > 
> > Do you know if there MUST be an action attribute in every jingle stanza?
> > Your code assumes this.
> If the IQ stanza with action attr session-initiate/accept/terminate is
> received
> then accordingly the notifyObservers would send the notification to the
> binding.
> A user won't receive notification for any other Jingle Stanza. Correct me if
> I'm
> not making any sense here.

The problem is your code assumes jStanza.attributes["action"] is not undefined. Is that assumption correct or should you check this before comparing the values?

Comment 35

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v3: WebRTC Video Call Patch.

Changeset:
* Disable call button during running call. (add a green button showcasing call).
* notification bar for failed calls.
* call timeout support. Call is rejected if unanswered for 30 sec.
* generation of id and sid from a separate function.
* "call wait" message display until call is answered.
* deleted resizeVideo method, now using calc() to resize video elements and chat box.
* descriptive comments for xml2sdp() and sdp2xml().

Comment 36

[aleth [:aleth]]

Comment on attachment 8446726 [details] [diff] [review]
(WIP) v3: WebRTC Video Call Patch.

Review of attachment 8446726 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't looked at all of this patch here.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +27,5 @@
> +       with attribute name = "face" is created. */
> +    let media = offer.match(/m=/g);
> +    let name = ["voice", "face"]; // Content name.
> +
> +    for (let i = 0; i < media.length; ++i) {

There's nicer ways to iterate over an array...

@@ +30,5 @@
> +
> +    for (let i = 0; i < media.length; ++i) {
> +      // Content node is added to itemContent.
> +      itemContent.push(Stanza.node("content", null, {creator: Stanza.NS.creator,
> +                                                     name: name[i]}));

name[i]? So media.length is at most 2?

@@ +36,5 @@
> +      /* The description node contains description about the media type.
> +         i.e the rtp-hdrext and payload type. */
> +      /* Creates group of device type. i.e. set of all a=candidate elements in
> +         the media group is matched. */
> +      dataDesc[i] = offer.match(/(m=[\s\S]*?)(?=a=candidate)/g)[i];

If you're repeatedly running this same regex inside the loop, pull it outside the loop.

If you're adding an entry to an array here, use .push(). If you need to modify it after adding it, then consider instead building the entry in a local variable and then pushing it to the array when it is complete.

Roughly speaking, you should try to use much less [i] which would make all this much more readable.

@@ +38,5 @@
> +      /* Creates group of device type. i.e. set of all a=candidate elements in
> +         the media group is matched. */
> +      dataDesc[i] = offer.match(/(m=[\s\S]*?)(?=a=candidate)/g)[i];
> +      // Set attributes for the description node.
> +      itemDesc[i] = Stanza.node("description", null,

Ditto

@@ +40,5 @@
> +      dataDesc[i] = offer.match(/(m=[\s\S]*?)(?=a=candidate)/g)[i];
> +      // Set attributes for the description node.
> +      itemDesc[i] = Stanza.node("description", null,
> +                                {xmlns: Stanza.NS.description,
> +                                 //gets audio, video

Comment style

@@ +41,5 @@
> +      // Set attributes for the description node.
> +      itemDesc[i] = Stanza.node("description", null,
> +                                {xmlns: Stanza.NS.description,
> +                                 //gets audio, video
> +                                 media: (offer.match(/(m=)[\S]*/g)[i].toString()).split("=")[1],

Why are we still doing a match on m= here, over and over again? Why doesn't the entire loop content only refer to the substring picked out for a given m= line?

I'd like this code (now it works) to be tidied up so it's clear what we're iterating over in this loop, and that each loop iteration only uses the data corresponding to the i'th media entry.

See below for the toString mess.

@@ +43,5 @@
> +                                {xmlns: Stanza.NS.description,
> +                                 //gets audio, video
> +                                 media: (offer.match(/(m=)[\S]*/g)[i].toString()).split("=")[1],
> +                                 //setup=actpass
> +                                 setup: (offer.match(/(a=setup)[\S]*/g)[i].toString()).split(":")[1]});

You're assuming the i'th m= hit will have a corresponding a=setup line?

@@ +55,5 @@
> +         Here 1 is the local ID and the next is URI for the extension.
> +         It is to be mapped with the rtp-hdrext node. */
> +      let dataRtpHdrext = dataDesc[i].match(/(a=extmap[\s\S]*?)(?=a=)/);
> +      if (dataRtpHdrext) {
> +        dataRtpHdrext = (dataRtpHdrext.toString()).split(" ");

Please look at my previous comment regarding regex results. You shouldn't be using toString on an array unless you need a comma-separated string, in which case you would need to add an explanatory comment. Most of these toStrings and +""'s should not be needed at all.

@@ +58,5 @@
> +      if (dataRtpHdrext) {
> +        dataRtpHdrext = (dataRtpHdrext.toString()).split(" ");
> +        let itemRtpHdrext = Stanza.node("rtp-hdrext", null,
> +                                        {xmlns: Stanza.NS.rtpHdrext,
> +                                         id: (dataRtpHdrext[0]).split(":")[1],

Nit: first brackets not needed

@@ +70,5 @@
> +         Here 8 is the ID, PCMA is name of the payload and 8000 is clockrate. */
> +      // Creates group containing payload type. i.e. all the a=rtpmap elements.
> +      dataPayload[i] = offer.match(/(a=rtpmap[\s\S]*?)(?=a=candidate)/g)[i].toString();
> +      // Seperates each payload.
> +      let subPayload = (dataPayload[i]).split("\r\n");

Nit: unnecessary ()

@@ +72,5 @@
> +      dataPayload[i] = offer.match(/(a=rtpmap[\s\S]*?)(?=a=candidate)/g)[i].toString();
> +      // Seperates each payload.
> +      let subPayload = (dataPayload[i]).split("\r\n");
> +
> +      for (let j = 0; j < subPayload.length-1; ++j) {

subPayload is an array, so you can use for...of or .forEach

@@ +73,5 @@
> +      // Seperates each payload.
> +      let subPayload = (dataPayload[i]).split("\r\n");
> +
> +      for (let j = 0; j < subPayload.length-1; ++j) {
> +        let payloadAttr = (subPayload[j] + "").split(" ");

What are these +"" for?

@@ +76,5 @@
> +      for (let j = 0; j < subPayload.length-1; ++j) {
> +        let payloadAttr = (subPayload[j] + "").split(" ");
> +        let p = (payloadAttr[0] + "").split(":")[0];
> +          let channelVal = (payloadAttr[1] + "").split("/")[2];
> +          let payloadID = (payloadAttr[0] + "").split(":")[1];

Nit: indentation

Duplication (the same split() is performed twice)

@@ +82,5 @@
> +        if (p == "a=rtpmap") {
> +          if (!channelVal) {
> +            itemPayload[j] = Stanza.node("payload-type", null,
> +                                         {id: payloadID,
> +                                          name: (payloadAttr[1].toString()).split("/")[0],

toString()? why?

@@ +83,5 @@
> +          if (!channelVal) {
> +            itemPayload[j] = Stanza.node("payload-type", null,
> +                                         {id: payloadID,
> +                                          name: (payloadAttr[1].toString()).split("/")[0],
> +                                          clockrate: (payloadAttr[1].toString()).split("/")[1]});

Duplication (you do the split() twice).

@@ +89,5 @@
> +          else {
> +            itemPayload[j] = Stanza.node("payload-type", null,
> +                                         {id: (payloadAttr[0].toString()).split(":")[1],
> +                                          name: (payloadAttr[1].toString()).split("/")[0],
> +                                          clockrate: (payloadAttr[1].toString()).split("/")[1],

Duplication with the previous if clause.

@@ +300,5 @@
> +      // Setup attribute of description [a=setup:actpass].
> +      nodeContent.push("a=setup:" + descData.attributes["setup"]);
> +
> +      // Candidate elements [a=candidate:0 1 UDP 2122252543 106.51.45.18 60055 typ host].
> +      let candidateData = (cNode.getElement(["transport"])).getElements(["candidate"]);

Nit: brackets around cNode... not needed

::: chat/protocols/xmpp/xmpp.jsm
@@ +275,5 @@
>        flags.delayed = true;
>      }
>      this.writeMessage(from, aMsg, flags);
>    },
> +  /* Generate id and sid for the stanza */

Add a reference to the XEP sections please

@@ +279,5 @@
> +  /* Generate id and sid for the stanza */
> +  generateId: function() {
> +    let id = [];
> +    id.push("callid" + Math.floor(Date.now() / 100000));
> +    id.push("sid" + Math.floor(Date.now()));

A timestamp is probably fairly unique, but does the id need to be unpredictable?

Comment 37

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v4: WebRTC Video Call Patch.

I have changed the way sdp2xml() generates the XML stanza from the SDP. It now divides the SDP into three main parts. i.e. the header information and the two media information.

Changeset:
* Using date.now() for sid wasn't unique as in future when we will implement entity capabilities, it might lead to collision during simultaneous generation of stanza between two accounts. Its better to use a similar sid generation method. So now we are using fairly unique sid's. Also as per RFC 6120 (Section 8.1.3) the id doesn't need to be exclusively generated here.
* Better for...of loop and using toString() only where it was unavoidable.
* Added an if else condition for generating "c=.." SDP line since some clients uses IPv6 address too. Though Mozilla currently doesn't support this.
* More comments for the sdp2xml() to make things more clear.

Comment 38

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v5: WebRTC Video Call Patch.

Removed toString() usage from the sdp2xml().

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8459551 [details] [diff] [review]
(WIP) v5: WebRTC Video Call Patch.

Review of attachment 8459551 [details] [diff] [review]:
-----------------------------------------------------------------

I pretty much only looked at the backend XMPP code.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Cc and Ci are unused, please remove them.

@@ +9,5 @@
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource:///modules/xmpp-xml.jsm");
> +
> +const jingle = {
> +

Nit: Remove this blank line.

@@ +13,5 @@
> +
> +  sdp2xml: function sdp2xml(aOffer) {
> +    // NOTE: The SDP offer folows the RFC-4566: Session Description Protocol.
> +    // Using XEP-0320: Jingle RTP Session.
> +    let contentNode = [];

It might help to say that this is the output variable. :)

@@ +17,5 @@
> +    let contentNode = [];
> +
> +    // Add a=rtcp-mux line at the end of the offer to detect the media groups.
> +    // It will be removed later.
> +    aOffer += "a=rtcp-mux";

Why do we need to do this?

@@ +31,5 @@
> +
> +    // For each media a content node containing description and transport child
> +    // node is to be created.
> +    for (let mediaNode of media) {
> +      let mediaLine = mediaNode.split("\r\n");

Are you just using this to remove the "\r\n" at the end? That's really confusing in my opinion, maybe you can rstrip or something instead.

@@ +39,5 @@
> +      let nameAttr;
> +      if (mediaName == "audio")
> +        nameAttr = "voice";
> +      else if (mediaName == "video")
> +        nameAttr = "face";

What if it's neither audio or video? Can that happen?

@@ +47,5 @@
> +                                                      name: nameAttr});
> +
> +      // Set attributes for the description node.
> +      let itemDesc = [];
> +      let setup = mediaNode.match(/ /);

Is this used anywhere?

@@ +59,5 @@
> +
> +      let ufrag;
> +      let pwd;
> +      let shaHash;
> +      let fingerprintData;

It might be helpful to include brief comments for each of these.

@@ +65,5 @@
> +      for (let headerLineNode of headerLine) {
> +        let params = headerLineNode.split(" ");
> +        let pNode = params[0].split(":");
> +
> +        // [a=ice-ufrag:0c838e8e] It is the ufrag attribute of transport node.

What is [a=ice-ufrag:0c838e8e]? An example?

@@ +79,5 @@
> +          shaHash = pNode[1];
> +          fingerprintData = params[1];
> +        }
> +      }
> +

You should ensure that ufrag, pwd, shaHash and fingerprintData were all set.

@@ +109,5 @@
> +          extension elements in a single RTP packet. This header extension
> +          applies to RTP/AVP (Audio/Visual Profile) and its extensions.
> +          [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].
> +          Here 1 is the local ID and the next is URI for the extension.
> +          It is to be mapped with the rtp-hdrext node. */

Please format your comments like below:
/*
 * This is a multiline
 * comment.
 */

Each line should go out to 80 characters, please ask me if you're unsure what I mean by this. (And this applies to the whole changeset, so please check over all your comments!)

@@ +124,5 @@
> +        else if (pNode[0] == "a=rtpmap") {
> +          let payloadAttr = params[1].split("/");
> +
> +          // If channel information is missing in the payload type pass
> +          // without channel attribute.

Reformat the following code to build the attributes object, something like (please fix the formatting):
let attr = {id: pNode[1],
name: payloadAttr[0],
clockrate: payloadAttr[1]};
if (payloadAttr.length >= 3)
  attr.channels = payloadAttr[2];
itemPayload.push(Stanza.node("payload-type", null, attr);

@@ +146,5 @@
> +           above concerned payload-type we add it as children to the payload
> +           node created just before. */
> +        else if (pNode[0] == "a=ptime") {
> +          let itemParameter = Stanza.node("parameter", null, {ptime: pNode[1]});
> +          itemPayload[itemPayload.length - 1].addChild(itemParameter);

Are we positive that the previous itemPayload is the one we expect it to be? Or does receiving this imply that it applies to the previous item? (Same goes for the fmtp code below)

@@ +158,5 @@
> +        }
> +        /* XEP: 2093 Jingle RTP Feedback Negotiation. It is uses a modified
> +           RTP profile for audio and video conferences with minimal control
> +           to acieve timely feedback.
> +           For eg. [a=rtcp-fb:98 nack rpsi]

Nit: e.g.

@@ +162,5 @@
> +           For eg. [a=rtcp-fb:98 nack rpsi]
> +           Here rtcp-fb is inserted in the payload-type node with the id 98.
> +           It contains the type "nack" and subtype "rpsi". */
> +        else if (pNode[0] == "a=rtcp-fb") {
> +          let itemRtcpFb;

Do something similar here with the attr like I discussed above. Try to reduce duplicated code.

@@ +182,5 @@
> +           has been explained in RFC-5245: Interactive Connectivity
> +           Establishment (ICE).
> +           For eg. [a=candidate:3 2 UDP 1686044670 106.51.45.18 43127 typ host]. */
> +        else if (pNode[0] == "a=candidate") {
> +          let itemCandidate;

Again, the code duplication.

@@ +212,5 @@
> +      }
> +
> +      // Add the payload nodes as children to the description node.
> +      for (let payloadNode of itemPayload)
> +        itemDesc.addChild(payloadNode);

This can probably just be:
itemPayload.forEach(itemDesc.addChild);

@@ +215,5 @@
> +      for (let payloadNode of itemPayload)
> +        itemDesc.addChild(payloadNode);
> +
> +      // Pop extra a=rtcp-mux added before.
> +      aOffer = aOffer.slice(0,-10);

Nit: Add a space after the ,. Although, why do we care?

@@ +222,5 @@
> +
> +    return contentNode;
> +  },
> +
> +  xml2sdp: function xml2sdp(aXMLOffer) {

Nit: Use proper camelcase aXmlOffer.

@@ +230,5 @@
> +    /* Constant SDP to be added at the header. There is no mention in the XEPs
> +       about how to pass these values. Note that these value don't effect the
> +       call being made. Use as it is in case of providing values for test. */
> +    let header = ['v=0',
> +                  'o=Mozilla-SIPUA-1.6a1pre 27638 0 IN IP4 0.0.0.0',

1.6a1pre sounds like it's Instantbird's version. This shouldn't be hard coded. 27638... do we know what that is? And an empty IP? That seems funky.

@@ +244,5 @@
> +    sdp.push("a=ice-pwd:" + eleTransport.attributes["pwd"]);
> +
> +    if (eleFingerprint)
> +      sdp.push("a=fingerprint:" + eleFingerprint.attributes["hash"]
> +               + " " + eleFingerprint.innerText);

Nit: { } around multi line entities, please. Also move the operator to the end of the previous line.

@@ +256,5 @@
> +      let descData = cNode.getElement(["description"]);
> +      media.push(("m=" + descData.attributes["media"]));
> +
> +      // Connection element (IP is first IP of candidate) [c=IN IP4 106.51.45.18].
> +      let firstCandidateData = (cNode.getElement(["transport"])).getElement(["candidate"]);

Why the () around cNode.getElement? They're unnecessary, please remove them.

@@ +267,5 @@
> +      let IP = firstCandidateData.attributes["ip"];
> +      if (IP.length <= 16)
> +        connection += "IP4 " + IP;
> +      else
> +        connection += "IP6 " + IP;

Are you sure about this check? Can't you have very short IPv6 addresses?

@@ +270,5 @@
> +      else
> +        connection += "IP6 " + IP;
> +
> +      nodeContent.push(connection);
> +      let indexConnection = nodeContent.lastIndexOf(connection);

This indexConnection seems unrelated to the nodeContent.push, you should add a line between these (and remove the line before nodeContent.push). Please add a comment above this line saying what it is.

@@ +279,5 @@
> +      let indexSendrecv;
> +      for (let pNode of payloadData) {
> +        payload = "a=rtpmap:" + pNode.attributes["id"]
> +                  + " " + pNode.attributes["name"]
> +                  + "/" + pNode.attributes["clockrate"];

Is this meant to be payload += or is payload meant to be defined only in the loop?

Also, operators always go at the end of lines! And you should generally try to fill up to the 80 char limit. Check over all your code for this. I did not comment on this everywhere.

@@ +298,5 @@
> +            nodeContent.push("a=ptime:" + parameter.attributes["ptime"]);
> +            indexSendrecv = nodeContent.length;
> +          }
> +          // Additional parameter to be added if specified.
> +          else if (parameter.attributes["value"]) {

Is this definitely supposed to be an else? You cannot have both ptime and value defined?

@@ +305,5 @@
> +            indexSendrecv = nodeContent.length;
> +          }
> +        }
> +
> +        // rtcp-fb element.

Please expand this comment!

@@ +311,5 @@
> +        let rtcpfb;
> +        if (rtcpfbData) {
> +          for (let rNode of rtcpfbData) {
> +            rtcpfb = "a=rtcp-fb:" + pNode.attributes["id"]
> +                     + " " + rNode.attributes["type"];

Again, should this be += or should rtcpfb be defined only in the loop?

@@ +328,5 @@
> +      let rtphdrext;
> +
> +      if (rtpContent) {
> +        rtphdrext = "a=extmap:" + rtpContent.attributes["id"]
> +                    + " " + rtpContent.attributes["uri"];

Define this here, don't define it outside the if statement.

@@ +363,5 @@
> +
> +        nodeContent.push(candidate);
> +      }
> +
> +      nodeContent.splice(indexConnection, 0, media.join(" "));

This needs a comment. Splice is always tricky.

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +72,5 @@
>    pubsub                    : "http://jabber.org/protocol/pubsub",
> +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> +
> +  // Jingle elements.
> +  action_initiate           : "session-initiate",

What is "action"? This should be session_initiate.

@@ +73,5 @@
> +  pubsub_event              : "http://jabber.org/protocol/pubsub#event",
> +
> +  // Jingle elements.
> +  action_initiate           : "session-initiate",
> +  creator                   : "initiator",

initiator

@@ +79,5 @@
> +  rtpHdrext                 : "urn:xmpp:jingle:apps:rtp:rtp-hdrext:0",
> +  xmlnsJingle               : "urn:xmpp:jingle:1",
> +  description               : "urn:xmpp:jingle:apps:rtp:1",
> +  transport                 : "urn:xmpp:jingle:transports:ice-udp:1",
> +  fingerprint               : "urn:xmpp:jingle:apps:dtls:0",

Please prefix these and slugify them:
jingle_apps_rtp_rtcp_fb
jingle_apps_rtp_rtp_hdrext
jingle
jingle_apps_rtp
jingle_transports_ice_udp
jingle_apps_dtls

@@ +80,5 @@
> +  xmlnsJingle               : "urn:xmpp:jingle:1",
> +  description               : "urn:xmpp:jingle:apps:rtp:1",
> +  transport                 : "urn:xmpp:jingle:transports:ice-udp:1",
> +  fingerprint               : "urn:xmpp:jingle:apps:dtls:0",
> +  callDisconnect            : "Call disconnected"

This doesn't look like it belongs here. What is this used for?

Comment 40

[Patrick Cloke [:clokep]]

Comment on attachment 8442898 [details] [diff] [review]
(WIP) v2: Test patch

Review of attachment 8442898 [details] [diff] [review]:
-----------------------------------------------------------------

How were these tests generated? Did you just generate tests, hand convert it to XML and then compare them? Do you need other people to create some? This is a good base test, but doesn't seem to test any edge cases. Can you think about those and add some specific tests for that? Thanks!

::: chat/protocols/xmpp/test/test_sdpOffer.js
@@ +105,5 @@
> +  run_next_test();
> +}
> +
> +function test_sdp2xml() {
> +  let xml  = [xml1, xml2];

Nit: double space

Comment 41

[Mayank Kumar [:mayanktg]]

(In reply to Patrick Cloke [:clokep] from comment #39)
> Comment on attachment 8459551 [details] [diff] [review]
> (WIP) v5: WebRTC Video Call Patch.
> 
> Review of attachment 8459551 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks a lot Patrick for taking a look into the patch. Sorry for the delay as I couldn't upload it earlier.
> @@ +17,5 @@
> > +    let contentNode = [];
> > +
> > +    // Add a=rtcp-mux line at the end of the offer to detect the media groups.
> > +    // It will be removed later.
> > +    aOffer += "a=rtcp-mux";
> 
> Why do we need to do this? 
The SDP session description is divided into two parts session-level section followed by 0 or more media-level section. Now to group the media level-section together I am matching the start of m=... line to the line just above a=rtcp-mux line. Though the audio section does contains this line, but it isn't necessary that the video section ends with this too. I've added this line at the end so that the last section can be grouped too.
> @@ +31,5 @@
> > +
> > +    // For each media a content node containing description and transport child
> > +    // node is to be created.
> > +    for (let mediaNode of media) {
> > +      let mediaLine = mediaNode.split("\r\n");
> 
> Are you just using this to remove the "\r\n" at the end? That's really
> confusing in my opinion, maybe you can rstrip or something instead.
> 
The motive here is to split the description into array of the media lines so that I can extract the payload, candidate and other line attributes from it. Using split("\r\n") also removes the trailing \r\n present in each line.  
> @@ +39,5 @@
> > +      let nameAttr;
> > +      if (mediaName == "audio")
> > +        nameAttr = "voice";
> > +      else if (mediaName == "video")
> > +        nameAttr = "face";
> 
> What if it's neither audio or video? Can that happen?
Yes that can happen but we don't support them at present. Thanks for clearing this. The loop skips such a media-level section now. I can read and add support for other media at a later stage when required.
> @@ +146,5 @@
> > +           above concerned payload-type we add it as children to the payload
> > +           node created just before. */
> > +        else if (pNode[0] == "a=ptime") {
> > +          let itemParameter = Stanza.node("parameter", null, {ptime: pNode[1]});
> > +          itemPayload[itemPayload.length - 1].addChild(itemParameter);
> 
> Are we positive that the previous itemPayload is the one we expect it to be?
> Or does receiving this imply that it applies to the previous item? (Same
> goes for the fmtp code below)
Receiving the ptime and afmtp tells that it belongs to the last payload node added.
For e.g.
c=IN IP6 192.168.1.6
a=rtpmap:96 opus/48000/2
a=fmtp:96 1
a=rtpmap:97 SILK/24000/1
Receiving a=fmtp implies it is added to the payload-type node of id 96 which is the last payload node added.
> This can probably just be:
> itemPayload.forEach(itemDesc.addChild);
This line wasn't able to add the payload node elements as child of description node. However
itemPayload.forEach(function(aPayload) itemDesc.addChild(aPayload))
worked.
> @@ +230,5 @@
> > +    /* Constant SDP to be added at the header. There is no mention in the XEPs
> > +       about how to pass these values. Note that these value don't effect the
> > +       call being made. Use as it is in case of providing values for test. */
> > +    let header = ['v=0',
> > +                  'o=Mozilla-SIPUA-1.6a1pre 27638 0 IN IP4 0.0.0.0',
> 
> 1.6a1pre sounds like it's Instantbird's version. This shouldn't be hard
> coded. 27638... do we know what that is? And an empty IP? That seems funky.
o=  (originator and session identifier : username, id, version number, network address)
http://tools.ietf.org/html/rfc4566#section-5.2
None of the XEP's tells how to send the o,v,t= lines in the XML Stanza :( . We won't be able to predict exactly what SDP header we have received from the sender. o=... helps in serving as global unique identifier.
If we know how can these lines be exported in XML stanza there won't be any need to hard code them. :)
> @@ +267,5 @@
> > +      let IP = firstCandidateData.attributes["ip"];
> > +      if (IP.length <= 16)
> > +        connection += "IP4 " + IP;
> > +      else
> > +        connection += "IP6 " + IP;
> 
> Are you sure about this check? Can't you have very short IPv6 addresses?
Sorry, yes we can have really short IPv6 addresses for eg 2002::/16 or even smaller, but each IPv6 address does contains the ":" element and IPv4 doesn't. So we can use this check to differentiate the addresses.
> @@ +298,5 @@
> > +            nodeContent.push("a=ptime:" + parameter.attributes["ptime"]);
> > +            indexSendrecv = nodeContent.length;
> > +          }
> > +          // Additional parameter to be added if specified.
> > +          else if (parameter.attributes["value"]) {
> 
> Is this definitely supposed to be an else? You cannot have both ptime and
> value defined?
This would be definitely an else since there would be separate parameter node for a=ptime and a=fmtp line.
> @@ +311,5 @@
> > +        let rtcpfb;
> > +        if (rtcpfbData) {
> > +          for (let rNode of rtcpfbData) {
> > +            rtcpfb = "a=rtcp-fb:" + pNode.attributes["id"]
> > +                     + " " + rNode.attributes["type"];
> 
> Again, should this be += or should rtcpfb be defined only in the loop?
If rtcp-fb node is present then the id and type attributes would be added, but all a=rtcp-fb lines doesn't need to have a subtype too. Afaik there is no need to use += there. :-o 
> @@ +80,5 @@
> > +  xmlnsJingle               : "urn:xmpp:jingle:1",
> > +  description               : "urn:xmpp:jingle:apps:rtp:1",
> > +  transport                 : "urn:xmpp:jingle:transports:ice-udp:1",
> > +  fingerprint               : "urn:xmpp:jingle:apps:dtls:0",
> > +  callDisconnect            : "Call disconnected"
> 
> This doesn't look like it belongs here. What is this used for?
"Call disconnected" is the innertext sent with the session-termiante Jingle stanza. Removed it from here.

Comment 42

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v6: WebRTC Video Call Patch.

Comment 43

[aleth [:aleth]]

I haven't looked at the code, just responding to your answers for the moment.

(In reply to Mayank Kumar [:mayanktg] from comment #41)
> (In reply to Patrick Cloke [:clokep] from comment #39)
> > > +    // Add a=rtcp-mux line at the end of the offer to detect the media groups.
> > > +    // It will be removed later.
> > > +    aOffer += "a=rtcp-mux";
> > 
> > Why do we need to do this? 
> The SDP session description is divided into two parts session-level section
> followed by 0 or more media-level section. Now to group the media
> level-section together I am matching the start of m=... line to the line
> just above a=rtcp-mux line. Though the audio section does contains this
> line, but it isn't necessary that the video section ends with this too. I've
> added this line at the end so that the last section can be grouped too.

It seems to me there is a safer way to split the SDP into m=... groups without relying in them ending in an a=rtcp-mux line which may not be present. You've split off the header at this point, and you are left with a string containing a number of m= groups beginning with "m=". Why not simply use .split("m=") to get those groups? You can always add back the m= by hand on each substring if you really need it (I doubt you do).

> > This can probably just be:
> > itemPayload.forEach(itemDesc.addChild);
> This line wasn't able to add the payload node elements as child of
> description node. However
> itemPayload.forEach(function(aPayload) itemDesc.addChild(aPayload))
> worked.

That's strange - after all, addChild is a function. What was the error message when you tried?

> > @@ +230,5 @@
> > > +    /* Constant SDP to be added at the header. There is no mention in the XEPs
> > > +       about how to pass these values. Note that these value don't effect the
> > > +       call being made. Use as it is in case of providing values for test. */
> > > +    let header = ['v=0',
> > > +                  'o=Mozilla-SIPUA-1.6a1pre 27638 0 IN IP4 0.0.0.0',
> > 
> > 1.6a1pre sounds like it's Instantbird's version. This shouldn't be hard
> > coded. 27638... do we know what that is? And an empty IP? That seems funky.
> o=  (originator and session identifier : username, id, version number,
> network address)
> http://tools.ietf.org/html/rfc4566#section-5.2
> None of the XEP's tells how to send the o,v,t= lines in the XML Stanza :( .
> We won't be able to predict exactly what SDP header we have received from
> the sender. o=... helps in serving as global unique identifier.
> If we know how can these lines be exported in XML stanza there won't be any
> need to hard code them. :)

Is the id always 27638 or does this change each time you request a new sdp offer? i.e. is it important that we send it?

clokep's point was that the mozilla code clearly uses the Instantbird version to generate the username for some reason. Can we just leave it out here altogether (i.e. use "Mozilla-SIPUA") or does that break something?  

> > @@ +311,5 @@
> > > +        let rtcpfb;
> > > +        if (rtcpfbData) {
> > > +          for (let rNode of rtcpfbData) {
> > > +            rtcpfb = "a=rtcp-fb:" + pNode.attributes["id"]
> > > +                     + " " + rNode.attributes["type"];
> > 
> > Again, should this be += or should rtcpfb be defined only in the loop?
> If rtcp-fb node is present then the id and type attributes would be added,
> but all a=rtcp-fb lines doesn't need to have a subtype too. Afaik there is
> no need to use += there. :-o 

The point is that if rtcpfb is a "local helper variable" that is only used inside the for loop, then its definition (i.e. the let command) should also be inside the for loop, i.e. 
let rtcpfb = "a=rtcp-fb:" + ...

Comment 44

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v7: WebRTC Video Call Patch.

Comment 45

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v8: WebRTC Video Call Patch.

Forgot to remove the import imXPCOMUtils.jsm line :-|
Check between the v6 and v8 for the diff.

Comment 46

[aleth [:aleth]]

Comment on attachment 8463376 [details] [diff] [review]
(WIP) v8: WebRTC Video Call Patch.

Review of attachment 8463376 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks much improved. Here are some more nits (mainly naming issues)...

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +10,5 @@
> +  sdp2xml: function sdp2xml(aOffer) {
> +    // NOTE: The SDP offer folows the RFC-4566: Session Description Protocol.
> +    // Using XEP-0320: Jingle RTP Session.
> +
> +    // This is the output variable which contains the array of contaent node to

typo.

@@ +12,5 @@
> +    // Using XEP-0320: Jingle RTP Session.
> +
> +    // This is the output variable which contains the array of contaent node to
> +    // be sent with Jingle stanza.
> +    let contentNode = [];

Maybe call this contentNodes (plural) as it's an array?

@@ +14,5 @@
> +    // This is the output variable which contains the array of contaent node to
> +    // be sent with Jingle stanza.
> +    let contentNode = [];
> +
> +    // SDP session descritption consists of a session-level section followed by

typo.

@@ +19,5 @@
> +    // zero or more media-level sections.
> +    let sessionDesc = aOffer.split("m=");
> +    // The session-level section contains the version, user name, type of call,
> +    // timestamp, ufrag, password, fingerprint and other attributes.
> +    let headerLine = sessionDesc[0].split("\r\n");

"headerLines" (there is more than one line), or simply "header"

@@ +28,5 @@
> +    sessionDesc.shift();
> +
> +    // For each media a content node containing description and transport child
> +    // node is to be created.
> +    for (let mediaNode of sessionDesc) {

Don't call this a node, it makes it sounds like an XML thing, i.e. like part of the output of this function, which is confusing. "mediaBlock" or simply "medium" maybe?

@@ +29,5 @@
> +
> +    // For each media a content node containing description and transport child
> +    // node is to be created.
> +    for (let mediaNode of sessionDesc) {
> +      let mediaLine = mediaNode.split("\r\n");

mediaLines?

@@ +44,5 @@
> +      else
> +        continue;
> +
> +      // Content node is added to itemContent.
> +      let itemContent = Stanza.node("content", null,

Why "itemContent" and not just "content"? What is the item this is referring to (itemContent makes it sound like it's the content of an item)?

Maybe you mean contentItem - is "item" a term used elsewhere in the XMPP code?

You could call it a contentNode, would that make sense?

@@ +49,5 @@
> +                                    {creator: Stanza.NS.session_initiate,
> +                                     name: nameAttr});
> +
> +      // Set attributes for the description node.
> +      let itemDesc = [];

Same here.

@@ +74,5 @@
> +       */
> +      let shaHash;
> +      let fingerprintData;
> +
> +      for (let headerLineNode of headerLine) {

Why is this parsing of the header done inside a loop over media blocks? Can't you pull it outside the loop and so only do it once?

@@ +93,5 @@
> +        }
> +      }
> +
> +      if (!(ufrag && pwd && shaHash && fingerprintData))
> +        break;

Needs a comment.

@@ +113,5 @@
> +      itemTransport.addChild(itemFingerprint);
> +
> +      let itemPayload = [];
> +
> +      for (let mediaLineNode of mediaLine) {

Again, let's not use the word "node" for sdp lines. What does the SDP RFC call them?

Please decide on a particular terminology for sdp entries and xml nodes and use it consistently everywhere (in both these functions) ;)

@@ +115,5 @@
> +      let itemPayload = [];
> +
> +      for (let mediaLineNode of mediaLine) {
> +        let params = mediaLineNode.split(" ");
> +        let pNode = params[0].split(":");

This doesn't depend on mediaLineNode, so pull it outside the loop.

@@ +213,5 @@
> +    let nodeContent = [];
> +
> +    /* Constant SDP session-level description to be added at the top since there
> +     * is no mention in the XEPs about how to pass these values. Note that these
> +     * value don't effect the call being made.

effect -> affect

@@ +214,5 @@
> +
> +    /* Constant SDP session-level description to be added at the top since there
> +     * is no mention in the XEPs about how to pass these values. Note that these
> +     * value don't effect the call being made.
> +     * Use as it is in case of providing values for test.

I don't understand this line - can you reformulate it?

@@ +251,5 @@
> +                                    .getElement(["candidate"]);
> +
> +      // The port attribute of first candidate is used if no relport exist. Else
> +      // use the first relport found in other candidate of the media.
> +      media[1] = firstCandidateData.attributes["port"];

media.push would be better here

Comment 47

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v9: WebRTC Video Call Patch.

Comment 48

[Mayank Kumar [:mayanktg]]

(In reply to aleth [:aleth] from comment #46)
> Comment on attachment 8463376 [details] [diff] [review]
> (WIP) v8: WebRTC Video Call Patch.
> 
> Review of attachment 8463376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks much improved. Here are some more nits (mainly naming
> issues)...
Thanks aleth. I have made the changes you've suggested and modified the names used in the sdp2xml so that its clear what they stand for. :)
> ::: chat/protocols/xmpp/xmpp-jingle.jsm
> @@ +113,5 @@
> > +      itemTransport.addChild(itemFingerprint);
> > +
> > +      let itemPayload = [];
> > +
> > +      for (let mediaLineNode of mediaLine) {
> 
> Again, let's not use the word "node" for sdp lines. What does the SDP RFC
> call them?
> 
> Please decide on a particular terminology for sdp entries and xml nodes and
> use it consistently everywhere (in both these functions) ;)
As far as I have seen the SDP refer the lines as fields. For e.g.  The "c=" field contains connection data. So I have used mediaField to refer the lines of media-level section.
@@ +115,5 @@
> +      let itemPayload = [];
> +
> +      for (let mediaLineNode of mediaLine) {
> +        let params = mediaLineNode.split(" ");
> +        let pNode = params[0].split(":");
> This doesn't depend on mediaLineNode, so pull it outside the loop.
I guess it does. The params[0] is taken from the mediaLineNode only. :)

Comment 49

[Florian Quèze [:florian]]

Comment on attachment 8464176 [details] [diff] [review]
(WIP) v9: WebRTC Video Call Patch.

Review of attachment 8464176 [details] [diff] [review]:
-----------------------------------------------------------------

note: I haven't really looked at the xmpp-jingle.jsm file.

::: chat/components/public/prplIConversation.idl
@@ +68,5 @@
>  interface prplIConvIM: prplIConversation {
> +  /* Send a SDP offer when user makes a call */
> +  void startCall(in AUTF8String aOffer);
> +
> +  /* Send a SDP answer when a user request a video call */

The user isn't 'requesting' a call, he's accepting an incoming call.

@@ +71,5 @@
> +
> +  /* Send a SDP answer when a user request a video call */
> +  void answerCall(in AUTF8String aAnswer);
> +
> +  /* Request a video call disconnect */

Terminate a video call.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const EXPORTED_SYMBOLS = ["jingle"];

Name this Jingle so that when using it in the code it's obvious that 'jingle' isn't just a local variable.

@@ +258,5 @@
> +      media.push(firstCandidateData.attributes["port"], "RTP/SAVPF");
> +
> +      let connection = "c=IN ";
> +      let IP = firstCandidateData.attributes["ip"];
> +      if (IP.indexOf(":") != -1)

IP.contains(":")

@@ +326,5 @@
> +      let rtphdrext;
> +
> +      if (rtpContent) {
> +        rtphdrext = "a=extmap:" + rtpContent.attributes["id"]
> +                    + " " + rtpContent.attributes["uri"];

nit: when putting a line break, keep the operator at the end of the previous line.

@@ +373,5 @@
> +    }
> +
> +    sdp.push(nodeContent.join("\r\n"));
> +    return sdp.join("\r\n");
> +   }

nit: indent

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +75,5 @@
> +  // Jingle elements.
> +  session_initiate          : "session-initiate",
> +  session_accept            : "session-accept",
> +  session_terminate         : "session-terminate",
> +  initiator                 : "initiator",

NS is a list of namespaces. Element names shouldn't appear here.

::: chat/protocols/xmpp/xmpp.jsm
@@ +13,5 @@
>  Cu.import("resource:///modules/imStatusUtils.jsm");
>  Cu.import("resource:///modules/imXPCOMUtils.jsm");
>  Cu.import("resource:///modules/jsProtoHelper.jsm");
>  Cu.import("resource:///modules/socket.jsm");
> +Cu.import("resource:///modules/xmpp-jingle.jsm");

Can this be a lazy import?

@@ +284,5 @@
> +    let item = Stanza.node("jingle", null,
> +                           {xmlns: Stanza.NS.jingle,
> +                            action: Stanza.NS.session_initiate,
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: "sid-" + this._account.generateId()});

I'm surprised that you aren't keeping the value of the generated id somewhere. Is it not needed?

@@ +313,5 @@
> +  },
> +
> +  /* called the the user wants to disconnect call. */
> +  endCall: function() {
> +    let id   = "callid" + Math.floor(Date.now() / 100000);

where is this used?

@@ +874,5 @@
> +
> +      let jStanza = aStanza.getElement(["jingle"]);
> +      let stanzaAction = jStanza.attributes["action"];
> +      let conversation =
> +        this.createConversation(this.normalize(aStanza.attributes["from"]));

Creating a conversation without checking first that the stanza is something you care about seems wrong.

@@ +876,5 @@
> +      let stanzaAction = jStanza.attributes["action"];
> +      let conversation =
> +        this.createConversation(this.normalize(aStanza.attributes["from"]));
> +      if (jStanza) {
> +        if (stanzaAction) {

These 2 tests can be merged.

@@ +880,5 @@
> +        if (stanzaAction) {
> +          if (stanzaAction == "session-initiate") {
> +            let content = jStanza.getElements(["content"]);
> +            let sdp = jingle.xml2sdp(content);
> +            conversation.notifyObservers(this, "webrtc-call-request", sdp);

the webrtc term for this is "offer" not "request"

@@ +885,5 @@
> +          }
> +          else if (stanzaAction == "session-accept") {
> +            let content = jStanza.getElements(["content"]);
> +            let sdp = jingle.xml2sdp(content);
> +            conversation.notifyObservers(this, "webrtc-call-answer", sdp);

This is duplicated code. The session-initiate and session-accept handling is the same here except for the name of the notification you are sending.

@@ +888,5 @@
> +            let sdp = jingle.xml2sdp(content);
> +            conversation.notifyObservers(this, "webrtc-call-answer", sdp);
> +          }
> +          else if (stanzaAction == "session-terminate") {
> +            conversation.notifyObservers(this, "call-disconnect");

Before sending this notification to the UI, shouldn't you check that:
1. We have an ongoing call in this conversation?
2. This stanza is coming from the right resource?

::: chat/themes/conv.css
@@ +43,5 @@
>  .ib-nick[left] {
>    color: grey;
>  }
> +
> +/* Video call CSS */

I was thinking you would have all the video call UI in the conversation XBL binding rather than in the HTML document.

Is there any reason why that wouldn't work?

@@ +76,5 @@
> +  background-image: url("chrome://global/skin/icons/close.png");
> +  -moz-image-region: rect(0, 16px, 16px, 0);
> +}
> +
> +@media (min-resolution: 2dppx) {

Is a Mac ifdef needed here?

@@ +95,5 @@
> +#callHeading {
> +  color: #ffffff;
> +  position: absolute;
> +  z-index: 2;
> +  top:  25%;

nit: double space

::: im/content/conversation.xml
@@ +276,5 @@
> +        <parameter name="aSuccessCallback"/>
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let document = this.browser.contentDocument;

There's already a global variable named 'document'. Use a different name here, eg 'doc'.

@@ +281,5 @@
> +          let body = document.querySelector("body");
> +          let convBundle = Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +
> +          // Remote video stream.
> +          let remoteVideo = document.createElement("video");

As I said in my comment above, I was expecting all this UI to be directly in the XBL markup.

@@ +324,5 @@
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let win = this.contentWindow;
> +          let pc = this.pc;

Avoid these 2 variables by using .bind(this) for your success callback to getUserMedia.

@@ +344,5 @@
> +          let conv = this._conv;
> +          // Display error notification for call failure.
> +          let convBundle = Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +          let cnb = this.getElt("convNotificationBox");
> +          let conversation = this;

Try to avoid this wherever possible.

@@ +360,5 @@
> +          let pc = this.initPeerConnection();
> +          this.getVideo(function() {
> +            pc.createOffer(function(offer) {
> +              pc.setLocalDescription(offer, function() {
> +                conv.startCall(offer.sdp);

I have a feeling some of this won't behave exactly like we would want if the call has been canceled before getUserMedia returned.

I don't think this._conv should be cached. If the conversation has been closed, we shouldn't call any method on it.

I'm also hoping that this.pc will be cleared if we receive a notification saying that the other party ended the call. In this case, we should re-use this.pc in each callback instead of caching it. This way we can null check at each point.

@@ +363,5 @@
> +              pc.setLocalDescription(offer, function() {
> +                conv.startCall(offer.sdp);
> +              }, fail);
> +            }, fail);
> +          }, fail);

You will probably want different error messages in each case.
And some logging in the error console.

@@ +368,5 @@
> +
> +          // Set call timeout to 30 sec.
> +          const timer = 30;
> +          this.callTimeout = this.contentWindow
> +                                 .setTimeout(function() {

Why do you need the setTimeout from the contentWindow?

@@ +369,5 @@
> +          // Set call timeout to 30 sec.
> +          const timer = 30;
> +          this.callTimeout = this.contentWindow
> +                                 .setTimeout(function() {
> +                                               conversation.endCall(true);

use bind(conversation, true) and avoid the wrapper function.

@@ +375,5 @@
> +
> +          // Disable video button on call start.
> +          this.getElt("conv-top-info")
> +              .querySelector("#videoCallButton").disabled = true;
> +          this.resetInput();

why resetInput?

@@ +385,5 @@
> +      <method name="videoCallNotification">
> +        <parameter name="aData"/>
> +        <body>
> +        <![CDATA[
> +        let convBundle = Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");

The indent isn't correct here.

@@ +387,5 @@
> +        <body>
> +        <![CDATA[
> +        let convBundle = Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +        let cnb = this.getElt("convNotificationBox");
> +        let conversation = this;

No longer needed after the 2 following changes.

@@ +397,5 @@
> +          [
> +            {
> +              accessKey: convBundle.GetStringFromName("acceptCall.accesskey"),
> +              callback: function() {
> +                conversation.webrtcCallRequest(aData);

Use .bind or an arrow function.

@@ +405,5 @@
> +            },
> +            {
> +              accessKey: convBundle.GetStringFromName("rejectCall.accesskey"),
> +              callback: function() {
> +                conversation.endCall(true);

Same here.

@@ +447,5 @@
> +          pc.setRemoteDescription(offer, (function() {
> +            this.getVideo(function() {
> +              pc.createAnswer(function(answer) {
> +                pc.setLocalDescription(answer, function() {
> +                  conv.answerCall(answer.sdp);

The comments I had above about not caching 'conv' and 'pc' also apply here.

@@ +448,5 @@
> +            this.getVideo(function() {
> +              pc.createAnswer(function(answer) {
> +                pc.setLocalDescription(answer, function() {
> +                  conv.answerCall(answer.sdp);
> +                  JSON.stringify(answer.sdp);

dead code?

@@ +452,5 @@
> +                  JSON.stringify(answer.sdp);
> +                }, fail);
> +              }, fail);
> +            }, fail);
> +          }).bind(this), fail);

You can use an arrow function to avoid this bind call.

@@ +475,5 @@
> +          delete this.callTimeout;
> +
> +          let callHeading = this.browser.contentDocument
> +                                .getElementById("callHeading");
> +          callHeading.parentNode.removeChild(callHeading);

I'm not exactly sure of what the callHeading thing is, but these 3 lines of code are duplicated with the method above.

@@ +488,5 @@
> +          let document = this.browser.contentDocument;
> +          let remoteVideo = document.getElementById("remoteVideo");
> +          if (remoteVideo) {
> +            remoteVideo.pause();
> +            remoteVideo.parentNode.removeChild(remoteVideo);

remoteVideo.remove();

@@ +502,5 @@
> +            localVideo.parentNode.removeChild(localVideo);
> +          }
> +
> +          if (this.pc)
> +            this.pc.close();

You also want to set this.bc back to null.

@@ +2002,3 @@
>            // The toolbarbuttons for different services to be added below.
> +          // Video call button
> +          if (this._conv.account.protocol.id == "prpl-jabber") {

Shouldn't this block be somewhere else so that it's called again if the target conversation changes?

::: im/themes/conversation.css
@@ +151,5 @@
> +#videoCallButton[disabled="true"] {
> +  list-style-image: url("chrome://instantbird/skin/webcam-active.png");
> +}
> +
> +@media (min-resolution: 2dppx) {

Is a Mac ifdef needed?

Comment 50

[Patrick Cloke [:clokep]]

Comment on attachment 8464176 [details] [diff] [review]
(WIP) v9: WebRTC Video Call Patch.

Review of attachment 8464176 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +28,5 @@
> +    let pwd;
> +    /* The fingerprint is the result of a hash function of the certificates
> +     * used in the DTLS-SRTP negotiation. This line creates a binding between
> +     * the signaling and the certificates used in DTLS.
> +     * e.g. [a=fingerprint:sha-256 2B:6E:6A:8D:...]

"This line" clearly just defines a variable, it doesn't DO anything.

@@ +36,5 @@
> +
> +    // The session-level section contains the version, user name, type of call,
> +    // timestamp, ufrag, password, fingerprint and other attributes.
> +    let header = sessionDesc[0].split("\r\n");
> +    for (let headerLines of header) {

What you call "header" here really sounds to me like it should be called "headerLines" and "headerLines" should be called "header".

@@ +51,5 @@
> +      // fingerprint node.
> +      else if (pNode[0] == "a=fingerprint") {
> +        shaHash = pNode[1];
> +        fingerprintData = params[1];
> +      }

The logic for this section seems odd. I'd think you'd wanted to take the headerLine, split it on ":", compare the first part and only split the second part in the fingerprint case. That should make this clearer.

@@ +62,5 @@
> +    sessionDesc.shift();
> +
> +    // If the session descrription lacks session-level section's information the
> +    // media-level section shouldn't be parsed.
> +    if (ufrag && pwd && shaHash && fingerprintData) {

If these do not all exist...what does that mean? Can we go on or is this an error condition?

@@ +65,5 @@
> +    // media-level section shouldn't be parsed.
> +    if (ufrag && pwd && shaHash && fingerprintData) {
> +      // For each media a content node containing description and transport
> +      // child node is to be created.
> +      for (let medium of sessionDesc) {

What does "medium" mean? Is this the plural of "media"? I don't understand this variable naming.

@@ +78,5 @@
> +          nameAttr = "face";
> +        // Do not send the content node if the m=… contains media name other
> +        // than audio or video.
> +        else
> +          continue;

What if there are NO audio or video ones and just other types? Will we send a corrupt packet in that case?

@@ +92,5 @@
> +                                       // Sets media as audio or video.
> +                                       media: mediaName,
> +                                       // setup=actpass / active / passive.
> +                                       setup: medium.match(/(a=setup)[\S]*/)[0]
> +                                                    .split(":")[1]});

This regexp needs to be a comment, and most likely a separate variable.

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +76,5 @@
> +  session_initiate          : "session-initiate",
> +  session_accept            : "session-accept",
> +  session_terminate         : "session-terminate",
> +  initiator                 : "initiator",
> +  jingle_apps_rtcpFb        : "urn:xmpp:jingle:apps:rtp:rtcp-fb:0",

This doesn't match what I asked you to change it to: jingle_apps_rtcp_fb.

::: chat/protocols/xmpp/xmpp.jsm
@@ +314,5 @@
> +
> +  /* called the the user wants to disconnect call. */
> +  endCall: function() {
> +    let id   = "callid" + Math.floor(Date.now() / 100000);
> +    let sId  = "sid" + Math.floor(Date.now());

These don't look right...

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +11,5 @@
> +videoCallButton.label=Video call
> +videoCallButton.accesskey=c
> +
> +# Call Notification
> +videoCallNotification.label=The person in chat is trying to call you.

Instead of "The person in chat" provide their actual name.

@@ +12,5 @@
> +videoCallButton.accesskey=c
> +
> +# Call Notification
> +videoCallNotification.label=The person in chat is trying to call you.
> +callErrorNotification.label=Sorry, the call cannot be established:

Why does this end with a :.

@@ +14,5 @@
> +# Call Notification
> +videoCallNotification.label=The person in chat is trying to call you.
> +callErrorNotification.label=Sorry, the call cannot be established:
> +callWait.label=Waiting for response…
> +startVideoCall.label=Video call has been started.

This should match endVideoCall below: "The video call has been started."

Comment 51

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v10: WebRTC Video Call Patch.

The patch addresses the review comments for the previous patch. Major changes includes:
* The UI now uses XBL binding rather than HTML Document.
* Better error handling.
* Separate error notification for different error types.
* Change in the way video call button is added/updated.
* The video call box is added only when the call has been successful. Until the call is established a notification is displayed.

Comment 52

[Patrick Cloke [:clokep]]

Comment on attachment 8470948 [details] [diff] [review]
(WIP) v10: WebRTC Video Call Patch.

Review of attachment 8470948 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all the extensive comments! I only looked at xmpp-jingle, xmpp-xml and xmpp. Feel free to wait for other feedback before uploading a new patch.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +38,5 @@
> +    // timestamp, ufrag, password, fingerprint and other attributes.
> +    let headerLines = sessionDesc[0].split("\r\n");
> +    for (let header of headerLines) {
> +      let params = header.split(" ");
> +      let pNode = params[0].split(":");

I suggested this last review cycle. Do NOT split this here, just set pNode equal to params[0].

@@ +48,5 @@
> +      else if (pNode[0] == "a=ice-pwd")
> +        pwd = pNode[1];
> +      // It gets the fingerprint data and sha hash information of the
> +      // fingerprint node.
> +      else if (pNode[0] == "a=fingerprint") {

Here check if pNode.startsWith("a=fingerprint"), in this case, split on ":" and set the hash to the second part.

@@ +62,5 @@
> +    sessionDesc.shift();
> +
> +    // If the session descrription lacks session-level section's information the
> +    // media-level section shouldn't be parsed.
> +    if (ufrag && pwd && shaHash && fingerprintData) {

Return early in the negative case here so that we can un-indent everything.

@@ +87,5 @@
> +                                   name: nameAttr});
> +
> +        // Setup attribute of description node. Every media-level description
> +        // contains a=setup field whose value is set as the setup attribute.
> +        setupAttr = medium.match(/(a=setup)[\S]*/)[0].split(":")[1];

This needs to have a let in front of it, but I'm not sure I understand what's actually happening here. Can you explain it to me over IRC please?

@@ +97,5 @@
> +                                       // setup=actpass / active / passive.
> +                                       setup: setupAttr});
> +        content.addChild(description);
> +
> +

Nit: Only one blank line.

@@ +225,5 @@
> +    let header = ['v=0',
> +                  'o=Mozilla-SIPUA 0 0 IN IP4 0.0.0.0',
> +                  's=SIP Call',
> +                  't=0 0'];
> +    sdp.push(header.join("\r\n"));

Are we using single quotes or double quotes? :)

@@ +247,5 @@
> +    for (let cNode of aXmlOffer) {
> +      let media = [];
> +      // Media is of the form [m=video 33680 RTP/SAVPF 120].
> +      let descData = cNode.getElement(["description"]);
> +      media.push(("m=" + descData.attributes["media"]));

Doesn't this need the mapping from audio <-> voice and face <-> video?

@@ +267,5 @@
> +        connection += "IP4 " + IP;
> +      nodeContent.push(connection);
> +
> +      // The c=IN… line must be added just below the m=… line. indexConnection
> +      // stores the index of last occurence of c=… element in the SDP offer.

Nit: Don't use the Unicode ... character in comments.

@@ +297,5 @@
> +          }
> +          // Additional parameter to be added if specified.
> +          else if (parameter.attributes["value"]) {
> +            nodeContent.push("a=fmtp:" + pNode.attributes["id"]
> +                             + " " + parameter.attributes["value"]);

Nit: ALWAYS end lines with operators, don't start them. I mentioned this in my previous review too. Please try to keep this in mind!

@@ +328,5 @@
> +      let rtphdrext;
> +
> +      if (rtpContent) {
> +        rtphdrext = "a=extmap:" + rtpContent.attributes["id"]  + " " +
> +                    rtpContent.attributes["uri"];

rtphdrext is only used in this if statement, don't define it outside of it.

::: chat/protocols/xmpp/xmpp.jsm
@@ +297,5 @@
> +    this._account.sendStanza(s);
> +  },
> +
> +  /* Called when the user sends an SDP answer for a video call. */
> +  answerCall: function(aAnswer) {

This function is almost identical to startCall except for one line, can we combine them somehow?

@@ +302,5 @@
> +    let item = Stanza.node("jingle", null,
> +                           {xmlns: Stanza.NS.jingle,
> +                            action: "session-accept",
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: "sid-" + this._account.generateId()});

Why the "sid-" in front? Is that required?

@@ +323,5 @@
> +                            sid: "sid-" + this._account.generateId()});
> +    let reason = Stanza.node("reason", null);
> +    item.addChild(reason);
> +    reason.addChild(Stanza.node("success", null));
> +    reason.addChild(Stanza.node("text", null, null, "Call disconnected"));

Add the children to the reason first and then add the reason to the item, it's clearer.

@@ +873,5 @@
> +
> +      let jStanza = aStanza.getElement(["jingle"]);
> +      let stanzaAction = jStanza.attributes["action"];
> +
> +      if (jStanza && stanzaAction) {

You check for jStanza here, but you've already assumed it exists two lines above that. This is strange.

@@ +875,5 @@
> +      let stanzaAction = jStanza.attributes["action"];
> +
> +      if (jStanza && stanzaAction) {
> +        let conversation =
> +          this.createConversation(this.normalize(aStanza.attributes["from"]));

Do we really need to normalize before creating the conversation?

@@ +884,5 @@
> +          let sdp = Jingle.xml2sdp(contentNodes);
> +            conversation
> +              .notifyObservers(this, stanzaAction == "session-initiate" ?
> +                                     "webrtc-call-offer" : "webrtc-call-answer",
> +                               sdp);

This spacing makes it pretty confusing. Take the ternary operation out and save it to a separate variable.

Additionally, conversation is misaligned.

@@ +888,5 @@
> +                               sdp);
> +        }
> +        else if (stanzaAction == "session-terminate") {
> +          if (aStanza.attributes["to"] == (this._jid.jid + "/" +
> +                                           this._jid.resource))

What if the cases are different or something, shouldn't those still match? What are you trying to do with this check?

Comment 53

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v11: WebRTC Video Call Patch.

Comment 54

[Florian Quèze [:florian]]

Comment on attachment 8473748 [details] [diff] [review]
(WIP) v11: WebRTC Video Call Patch.

Review of attachment 8473748 [details] [diff] [review]:
-----------------------------------------------------------------

I still haven't looked at the content of xmpp-jingle.jsm. The pieces I looked at are moving in the right direction and are now in a much better shape than last time I looked at them.

::: chat/components/public/prplIConversation.idl
@@ +70,4 @@
>  interface prplIConvIM: prplIConversation {
>  
> +  /* Send a SDP offer/answer when user makes/answers a video call */
> +  void webrtcCall(in AUTF8String aType, in AUTF8String aOffer);

should this method just take an mozRTCSessionDescription object as parameter?

I think having 2 methods named initiateCall (or startCall) and acceptCall (or answerCall) would be less confusing.

::: chat/protocols/xmpp/xmpp.jsm
@@ +285,5 @@
> +  webrtcCall: function(aType, aOffer) {
> +    if (!aType)
> +      return;
> +
> +    if (aType == "session-initiate")

ahah, I expected aType to be either "offer" or "answer"; this shows the need for a more descriptive comment in the idl file.

@@ +286,5 @@
> +    if (!aType)
> +      return;
> +
> +    if (aType == "session-initiate")
> +      this._account._callSid = this._account.generateId();

are you sure there can only be one call per account? Can't you store it instead in the conversation object?
Where is this value cleaned-up after a call is terminated?

@@ +292,5 @@
> +    let item = Stanza.node("jingle", null,
> +                           {xmlns: Stanza.NS.jingle,
> +                            action: aType,
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: this._account._callSid});

Can't Jingle.sdp2xml(aOffer) be given directly to the fourth parameter of Stanza.node?

@@ +302,5 @@
> +
> +    this._account.sendStanza(Stanza.iq("set", null, this.to, item));
> +  },
> +
> +  /* called the the user wants to disconnect call. */

"the the user"

@@ +311,5 @@
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: this._account._callSid});
> +    let reason = Stanza.node("reason", null);
> +    reason.addChild(Stanza.node("success", null));
> +    reason.addChild(Stanza.node("text", null, null, "Call disconnected"));

These child nodes can be directly added using Stanza.node's 4th param, right?

@@ +858,5 @@
>            this._onRosterItem(item, true);
>          return;
>        }
> +
> +      let jStanza = aStanza.getElement(["jingle"]);

not a good variable name.

@@ +859,5 @@
>          return;
>        }
> +
> +      let jStanza = aStanza.getElement(["jingle"]);
> +      let stanzaAction = jStanza.attributes["action"];

name this just "action"

@@ +861,5 @@
> +
> +      let jStanza = aStanza.getElement(["jingle"]);
> +      let stanzaAction = jStanza.attributes["action"];
> +
> +      if (jStanza && stanzaAction) {

Too late to null check jStanza, your code has already caused an exception if it's null.

@@ +870,5 @@
> +        if ((stanzaAction == "session-initiate" ||
> +             stanzaAction == "session-accept") && contentNodes.length > 0) {
> +          this._callSid = jStanza.attributes["sid"];
> +          let sdp = Jingle.xml2sdp(contentNodes);
> +          let aTopic = stanzaAction == "session-initiate" ?

Wrong variable name, the 'a' prefix means it's an argument the function received.

::: im/content/conversation.xml
@@ +288,5 @@
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          let remoteVideo = this.getElt("remoteVideo");
> +          if (!this.pc) {

Should we throw if we are attempting to init a new PC when one already exists?

@@ +289,5 @@
> +        <body>
> +        <![CDATA[
> +          let remoteVideo = this.getElt("remoteVideo");
> +          if (!this.pc) {
> +            this.pc = new mozRTCPeerConnection();

Should we do |new (this.contentWindow.mozRTCPeerConnection)();| to have the PeerConnection attached to the content window rather than the chrome one? I see you are using this.contentWindow.navigator for the gUM call.

@@ +290,5 @@
> +        <![CDATA[
> +          let remoteVideo = this.getElt("remoteVideo");
> +          if (!this.pc) {
> +            this.pc = new mozRTCPeerConnection();
> +            this.pc.onaddstream = function(obj) {

You can use an arrow function and move the this.getElt("remoteVideo"); call here.

@@ +306,5 @@
> +        <parameter name="aSuccessCallback"/>
> +        <parameter name="aErrorCallback"/>
> +        <body>
> +        <![CDATA[
> +          this.contentWindow.navigator.mozGetUserMedia({video: true, audio:true},

space after ':'

@@ +313,5 @@
> +            localVideo.mozSrcObject = stream;
> +            localVideo.play();
> +            if (this.pc)
> +              this.pc.addStream(stream);
> +            aSuccessCallback();

The success callback take the stream as parameter, instead of the getVideo function messing with 'this.pc'.

@@ +322,5 @@
> +
> +      <method name="startCall">
> +        <body>
> +        <![CDATA[
> +          // Display error notification for call failure.

This whole block is duplicated, move it to a separate method.

@@ +330,5 @@
> +
> +          let fail = type => {
> +            cnb.appendNotification(
> +              convBundle.GetStringFromName("callError" +
> +                                           (type || Notification) + ".label"),

What is Notification here? 'fail' never seems to be called without a string as parameter.

@@ +345,5 @@
> +            pc.createOffer(offer => {
> +              pc.setLocalDescription(offer, () => {
> +                if (this._conv)
> +                  this._conv.webrtcCall("session-initiate", offer.sdp);
> +                this.ongoingCall = true;

Are you sure you really want to set this if _conv no longer exists? I thought you would return early if this._conv has been uninitialized.

@@ +346,5 @@
> +              pc.setLocalDescription(offer, () => {
> +                if (this._conv)
> +                  this._conv.webrtcCall("session-initiate", offer.sdp);
> +                this.ongoingCall = true;
> +              }, () => fail("LocalDesc"));

() => this.displayError("LocalDesc")
This way the code in the displayError method will have the correct 'this' value.

@@ +361,5 @@
> +              {
> +                accessKey: convBundle.GetStringFromName("cancelCall.accesskey"),
> +                callback: () => this.endCall(true),
> +                label: convBundle.GetStringFromName("cancelCall.label"),
> +                popup: null

Is this line needed?

@@ +371,5 @@
> +          this.callTimeout = setTimeout(() => this.endCall(true), timer * 1000);
> +
> +          // Disable video button on call start.
> +          let videoCallButton = this.getElt("conv-top-info")
> +                                    .querySelector(".videoCallButton")

This seems a bit complicated as a way to access the video call button. Should we create a getter for it so that you avoid duplicating these 2 lines several times?

@@ +375,5 @@
> +                                    .querySelector(".videoCallButton")
> +          if (videoCallButton)
> +            videoCallButton.disabled = true;
> +
> +          this.resetInput();

Why this line?

@@ +376,5 @@
> +          if (videoCallButton)
> +            videoCallButton.disabled = true;
> +
> +          this.resetInput();
> +          return this.callTimeout;

This return value is never used.

@@ +460,5 @@
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="webrtcAnswerCall">

name this "onVideoCallAnswered"

@@ +466,5 @@
> +        <body>
> +        <![CDATA[
> +          let answer = {type: "answer", sdp: aAnswer};
> +          answer = new mozRTCSessionDescription(answer);
> +          if (this.pc)

I don't think we need this check.

@@ +475,5 @@
> +          delete this.callTimeout;
> +
> +          let callWaitNotification = this.getElt("convNotificationBox");
> +          if (callWaitNotification.getNotificationWithValue("video-call-wait"))
> +            callWaitNotification.removeCurrentNotification();

These lines seem wrong; they could remove a different notification if something else with a higher priority is displayed at the same time.

@@ +477,5 @@
> +          let callWaitNotification = this.getElt("convNotificationBox");
> +          if (callWaitNotification.getNotificationWithValue("video-call-wait"))
> +            callWaitNotification.removeCurrentNotification();
> +
> +          this.getElt("videoCallBox").removeAttribute("hidden");

Wouldn't .hidden = false be enough?

@@ +486,5 @@
> +      <method name="endCall">
> +        <parameter name="aSender"/>
> +        <body>
> +        <![CDATA[
> +          this.getElt("videoCallBox").setAttribute("hidden", "true");

wouldn't .hidden = true be enough?

@@ +497,5 @@
> +          }
> +
> +          let localVideo = this.getElt("localVideo");
> +          if (localVideo) {
> +            if (localVideo.mozSrcObject) {

Put these 2 tests on the same line with an && operator.

@@ +516,5 @@
> +
> +          let videoCallNotification = this.getElt("convNotificationBox");
> +          if (videoCallNotification.getNotificationWithValue("video-call-notification") ||
> +              videoCallNotification.getNotificationWithValue("video-call-wait"))
> +            videoCallNotification.removeCurrentNotification();

Same comment as above.

@@ +1997,5 @@
> +      <method name="updateVideoCallButton">
> +        <body>
> +        <![CDATA[
> +          let bundle =
> +            Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");

Don't create the bundle unless you really need it.

@@ +1998,5 @@
> +        <body>
> +        <![CDATA[
> +          let bundle =
> +            Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +          let videoCallButton = this.getElt("conv-top-info")

You can shorten this variable name to "button", it's unambiguous in the context of this method.

@@ +2002,5 @@
> +          let videoCallButton = this.getElt("conv-top-info")
> +                                .querySelector(".videoCallButton");
> +
> +
> +          if (videoCallButton && this._conv.account.protocol.id != "prpl-jabber")

prpl-jabber shouldn't be hardcoded in the UI code. Instead, add a boolean in prplIProtocol or prplIAccount.

@@ +2014,5 @@
> +            videoCallButton.setAttribute("accesskey",
> +                                         bundle.GetStringFromName("videoCallButton.accesskey"));
> +            videoCallButton.setAttribute("command", "cmd_startCall");
> +            let cti = this.getElt("conv-top-info");
> +            cti.insertBefore(videoCallButton, cti.firstChild);

Wouldn't this.appendChild(button); work instead of getting the cti first?

I wonder if it would be easier to have the button created at the same time as the target switcher, and just set .hidden to true or false.

@@ +2025,5 @@
>        <method name="initConversationUI">
>          <body>
>          <![CDATA[
> +          let bundle =
> +            Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");

Why is the bundle now needed here?

@@ +2187,5 @@
>              this._forgetConv();
>              break;
> +
> +          case "webrtc-call-offer":
> +            this.videoCallNotification(aData);

onVideoCallOffer seems like a better name to me.

@@ +2192,5 @@
> +            break;
> +
> +          case "webrtc-call-answer":
> +            if (this.callTimeout)
> +              this.webrtcAnswerCall(aData);

If we received an answer that we weren't expecting, should we let the prpl know? Should we log something to the error console?

@@ +2196,5 @@
> +              this.webrtcAnswerCall(aData);
> +            break;
> +
> +          case "call-disconnect":
> +            if (this.ongoingCall == true)

the | == true| isn't needed here.

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +24,5 @@
> +callErrorLocalDesc.label=Unable to set local description. Sorry, the call cannot be established.
> +callErrorNotification.label=Sorry, the call cannot be established.
> +callErrorOffer.label=SDP session description offer creation failed. Sorry, the call cannot be established.
> +callErrorRemoteDesc.label=Unable to set remote description. Sorry, the call cannot be established.
> +callErrorVideo.label=Failed to intialize video. Sorry, the call cannot be established.

The error messages don't really look like strings we would want to show users (nor translators actually...). How would you feel about sending the real error message to the error console, and displaying "Sorry, the call could not be established." all the time in the notification bar?

Also, I think most (if not all) webrtc functions give information about the failure in a parameter to the error callback; we should also display that additional information in the error console.

Comment 55

[aleth [:aleth]]

Comment on attachment 8473748 [details] [diff] [review]
(WIP) v11: WebRTC Video Call Patch.

Review of attachment 8473748 [details] [diff] [review]:
-----------------------------------------------------------------

Looking much better!

::: chat/components/public/prplIConversation.idl
@@ +70,4 @@
>  interface prplIConvIM: prplIConversation {
>  
> +  /* Send a SDP offer/answer when user makes/answers a video call */
> +  void webrtcCall(in AUTF8String aType, in AUTF8String aOffer);

Why did you change this from startCall/acceptCall? Some misunderstanding?
I think the parameter should be aSdp, that might be clearer.

::: chat/protocols/xmpp/xmpp.jsm
@@ +286,5 @@
> +    if (!aType)
> +      return;
> +
> +    if (aType == "session-initiate")
> +      this._account._callSid = this._account.generateId();

Do you really need to store these ids? Aren't jingle stanzas IQ stanzas, for which a callback mechanism already exists? i.e. when you send an IQ stanza you can pass a callback which is called if a reply to that id is received.

::: im/content/conversation.xml
@@ +24,5 @@
>          <xul:toolbar class="conv-top-info" anonid="conv-top-info" context="tabContextMenu"/>
>          <xul:hbox class="conv-top" flex="1" anonid="conv-top">
> +          <xul:vbox flex="1">
> +            <xul:vbox class="videoCallBox" anonid="videoCallBox" hidden="true"
> +                      flex="2">

Why do you need the flex to be 2? https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/flex

@@ +33,1 @@
>              </xul:vbox>

Out of interest, if you put a <xul:splitter/> here, what doesn't work?

@@ +330,5 @@
> +
> +          let fail = type => {
> +            cnb.appendNotification(
> +              convBundle.GetStringFromName("callError" +
> +                                           (type || Notification) + ".label"),

I think you intended "Notification" as a default value but I agree with flo's comment.

@@ +449,5 @@
> +            this.getVideo(() => {
> +              pc.createAnswer(answer => {
> +                pc.setLocalDescription(answer, () => {
> +                  if (this._conv)
> +                    this._conv.webrtcCall("session-accept", answer.sdp);

If there isn't a _conv I doubt you want to show the video call box etc here.

::: im/themes/conversation.css
@@ +425,5 @@
>  }
> +
> +/* Video call CSS */
> +.videoCallBox {
> +  position: fixed;

What is this for?

@@ +427,5 @@
> +/* Video call CSS */
> +.videoCallBox {
> +  position: fixed;
> +  width: 100%;
> +  min-height: 50%;

And this? Is this possibly why you need flex=2 in the XBL? ;)

@@ +431,5 @@
> +  min-height: 50%;
> +}
> +
> +.remoteVideo {
> +  position: fixed;

Same here.

@@ +434,5 @@
> +.remoteVideo {
> +  position: fixed;
> +  width: 100%;
> +  height: 50%;
> +  z-index: 1;

Why does this need a z-index? What's it over?

Comment 56

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #55)

> > +    if (aType == "session-initiate")
> > +      this._account._callSid = this._account.generateId();
> 
> Do you really need to store these ids?

You need the jingle session id if you want to send more messages related to the session later (eg. to terminate the session).

> Aren't jingle stanzas IQ stanzas, for
> which a callback mechanism already exists? i.e. when you send an IQ stanza
> you can pass a callback which is called if a reply to that id is received.

That handles acks from the server.


> ::: im/content/conversation.xml
> @@ +24,5 @@
> >          <xul:toolbar class="conv-top-info" anonid="conv-top-info" context="tabContextMenu"/>
> >          <xul:hbox class="conv-top" flex="1" anonid="conv-top">
> > +          <xul:vbox flex="1">
> > +            <xul:vbox class="videoCallBox" anonid="videoCallBox" hidden="true"
> > +                      flex="2">
> 
> Why do you need the flex to be 2?

My guess was to have the video area be twice the size of the chat area by default when both are visible.

Comment 57

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Backend part of WIP v11

Splitting the patch into parts as discussed.

This is the chat/ part of the latest WIP.

Comment 58

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: UI part of WIP v11

Splitting the patch into parts as discussed.

This is the im/ part of the latest WIP.

Comment 59

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Purple part of WIP v11

Splitting the patch into parts as discussed.

This is the purple part of the latest WIP.

Comment 60

[Patrick Cloke [:clokep]]

Attachment: Backend part v12

Comment 61

[Patrick Cloke [:clokep]]

Attachment: UI part v12

Comment 62

[Patrick Cloke [:clokep]]

Attachment: SDP Test v12

Comment 63

[Florian Quèze [:florian]]

Comment on attachment 8507398 [details] [diff] [review]
Backend part v12

Review of attachment 8507398 [details] [diff] [review]:
-----------------------------------------------------------------

r- only because _callSid is not reset when it should. The other issues mentioned in my review comments can be handled in follow-ups after this lands.

We also need to add code to pref-off this feature until we are done polishing it.

::: chat/components/public/prplIConversation.idl
@@ +78,1 @@
>  interface prplIConvIM: prplIConversation {

We really need an API (probably at the conversation level, but maybe also at the protocol or contact level) letting the UI know if video calls are supported.

::: chat/protocols/xmpp/xmpp.jsm
@@ +285,5 @@
>    },
>  
> +  /* Called when the user requests a video call. */
> +  initiateCall: function(aOffer) {
> +    this._callSid = this._account.generateId();

We should consider using the session id from the SDP instead of generating our own randomly. I'm not sure if the formats are compatible though.

@@ +317,5 @@
> +                            action: "session-terminate",
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: this._callSid}, reason);
> +
> +    this._account.sendStanza(Stanza.iq("set", null, this.to, item));

We should reset _callSid here.

@@ +874,5 @@
> +          let contentNodes = jingle.getElements(["content"]);
> +
> +          if ((action == "session-initiate" ||
> +               action == "session-accept") && contentNodes.length > 0) {
> +            this._callSid = jingle.attributes["sid"];

Just overwriting this is probably not good. I'm especially thinking about the case of both people in a conversation calling each other at the same time. We need to figure out how we want to handle that case. Probably reject the incoming call automatically with a "busy" error if we are already waiting for an answer.

@@ +882,5 @@
> +            conversation.notifyObservers(this, topic, sdp);
> +          }
> +          else if (action == "session-terminate") {
> +            if (jingle.attributes["sid"] == this._callSid)
> +              conversation.notifyObservers(this, "call-disconnect");

We should reset the _callSid value here.

Comment 64

[Florian Quèze [:florian]]

Attachment: Purple part v12

Comment 65

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #63)
> ::: chat/components/public/prplIConversation.idl
> @@ +78,1 @@
> >  interface prplIConvIM: prplIConversation {
> 
> We really need an API (probably at the conversation level, but maybe also at
> the protocol or contact level) letting the UI know if video calls are
> supported.

The XMPP prpl part of this is bug 1025150.

Comment 66

[aleth [:aleth]]

Comment on attachment 8507399 [details] [diff] [review]
UI part v12

Review of attachment 8507399 [details] [diff] [review]:
-----------------------------------------------------------------

Just a checklist of things to, well, check.

::: im/content/conversation.xml
@@ +350,5 @@
> +            case "RemoteDesc":
> +              msg = "Unable to set remote description."
> +              break;
> +            case "Video":
> +              msg = "Failed to intialize video.";

Typo.

@@ +553,5 @@
> +            videoCallNotification.getNotificationWithValue("video-call-wait")
> +          if (notification)
> +            videoCallNotification.removeNotification(notification);
> +
> +          // It is used to send the session-terminate stanza to the user to

Remove "it is used"

@@ +561,5 @@
> +
> +          // Call-end system message.
> +          let convBundle =
> +            Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +          if (this._conv)

Move this check further up before the first reference to _conv.

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +22,5 @@
> +cancelCall.accesskey=C
> +rejectCall.label=Reject
> +rejectCall.accesskey=R
> +callError.label=Unable to establish call.
> +callWait.label=Waiting for the response…

"Waiting for a response..." or maybe just "Calling..."

::: im/themes/conversation.css
@@ +445,5 @@
> +}
> +
> +.remoteVideo {
> +  width: 100%;
> +  height: 50%;

Does this line actually do anything?

@@ +446,5 @@
> +
> +.remoteVideo {
> +  width: 100%;
> +  height: 50%;
> +  background: #424F5A;

Should probably be moved to .videoCallBox

@@ +452,5 @@
> +
> +.localVideo {
> +  border: 2px solid rgba(255, 255, 255, 0.25);
> +  border-radius: 5px;
> +  background: black;

This doesn't look great, it produces a 1px black border inside a grey border and the border-radius doesn't really fit. How about removing the background line and instead going for a 1px or 2px black border (which will give a subtle separation from the dark grey background).

@@ +458,5 @@
> +  max-width: 192px;
> +  min-width: 192px;
> +  height: 144px;
> +  max-height: 144px;
> +  min-height: 144px;

Do we need all of these?

@@ +461,5 @@
> +  max-height: 144px;
> +  min-height: 144px;
> +}
> +
> +.endCallButton {

clokep said this looks terrible on Windows. Maybe compare with the similar button in the user icon webcam patch?

Comment 67

[aleth [:aleth]]

Attachment: frontend.diff v13

Addresses comment #66. CSS changes **untested** as nobody was online. In particular, while I copied some rules from the usericon remove button across, I strongly suspect the end call button still isn't styled properly in this patch.

Comment 68

[aleth [:aleth]]

Attachment: frontend.diff v14

Forgot to qref.

Comment 69

[aleth [:aleth]]

Attachment: frontend.diff v15

Comment 71

[aleth [:aleth]]

Attachment: backend.diff v13

Include some error handling, and fix nasty typo in the idl.

Comment 72

[aleth [:aleth]]

Attachment: frontend.diff v16

Handles some additional error pathways (some due to the various orders in which the callbacks can be called). In particular, the endCall functions have to be able to handle being called more than once.

I can't help thinking that it might be possible to simplify a bit the maze of functions we are adding in conversation.xml. Maybe we should draw a flowchart.

Comment 73

[aleth [:aleth]]

Left to do:
* flo's review comment 63
* test/fix end call button CSS

Comment 74

[aleth [:aleth]]

Attachment: backend.diff v14

Comment 75

[aleth [:aleth]]

Attachment: frontend.diff v17

Comment 76

[aleth [:aleth]]

Attachment: backend.diff v15

Fix callsid issues from comment 63

Comment 77

[aleth [:aleth]]

Attachment: frontend.diff v18

Pref off video calls by default

Comment 78

[aleth [:aleth]]

Attachment: backend.diff v16

Missed an instance of _callsid erroneously being referred to on the account rather than the conversation.

Comment 79

[aleth [:aleth]]

Comment on attachment 8509647 [details] [diff] [review]
frontend.diff v18

Needs more testing but it's ready for a first review.

Comment 80

[Patrick Cloke [:clokep]]

Comment on attachment 8509651 [details] [diff] [review]
backend.diff v16

Review of attachment 8509651 [details] [diff] [review]:
-----------------------------------------------------------------

Some fairly trivial comments to this. ALso, someone should fix the message for this patch with the next upload. :)

::: chat/components/public/prplIConversation.idl
@@ +78,2 @@
>  interface prplIConvIM: prplIConversation {
> +  /* Send a SDP offer when user makes a call. */

Nit: "when a user" to match the comment below.

::: chat/protocols/xmpp/xmpp.jsm
@@ +314,5 @@
> +
> +  /* Called when the user wants to disconnect call. */
> +  endCall: function() {
> +    if (!this._callSid) // No current call.
> +      return;

Should this throw a warning or anything or do we not care?

@@ +905,5 @@
> +                this.sendStanza(Stanza.iq("set", null, this.to, item));
> +                return;
> +              }
> +              else
> +                this.WARN("Received a call answer with a different sid to our call offer.");

Don't we want to return here?

Comment 81

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #80)
> > +  endCall: function() {
> > +    if (!this._callSid) // No current call.
> > +      return;
> 
> Should this throw a warning or anything or do we not care?

It's better if you don't have to worry about the state of the call, or whether it's been called before, when you call it from callbacks.
 
> @@ +905,5 @@
> > +                this.sendStanza(Stanza.iq("set", null, this.to, item));
> > +                return;
> > +              }
> > +              else
> > +                this.WARN("Received a call answer with a different sid to our call offer.");
> 
> Don't we want to return here?

I'm not sure. The current code accepts the sid change in the call answer, it's not clear whether that's the right thing to do though. I added the warning so we would be notified if it ever actually happens and can figure it out then.

Comment 82

[Kent James (:rkent)]

"feature" keyword added as key feature for upcoming Thunderbird 38 release.

Comment 83

[Florian Quèze [:florian]]

Comment on attachment 8509647 [details] [diff] [review]
frontend.diff v18

Review of attachment 8509647 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of simple comments, nothing too scary :-).

::: im/app/profile/all-instantbird.js
@@ +314,5 @@
>  // Whether to parse log files for conversation statistics.
>  pref("statsService.parseLogsForStats", true);
> +
> +// Whether to allow video calls.
> +pref("messenger.conversations.enableVideoCalls", false);

I'm surprised this isn't in the chat/ patch. Shouldn't the XMPP code decline code invites differently (and immediately) if the feature isn't enabled?

::: im/content/conversation.xml
@@ +22,5 @@
>      <content>
>        <xul:vbox class="convBox" flex="1">
>          <xul:toolbar class="conv-top-info" anonid="conv-top-info" context="tabContextMenu"/>
>          <xul:hbox class="conv-top" flex="1" anonid="conv-top">
> +          <xul:vbox flex="1">

I'm not sure this new vbox is really useful. Can't the videoCallBox stack be moved to the convBox vbox, between the conv-top-info toolbar and the conv-top hbox?

@@ +25,5 @@
>          <xul:hbox class="conv-top" flex="1" anonid="conv-top">
> +          <xul:vbox flex="1">
> +            <xul:stack class="videoCallBox" anonid="videoCallBox" flex="1" hidden="true">
> +              <html:video class="remoteVideo" anonid="remoteVideo"/>
> +              <xul:vbox>

Should this vbox have a class?

@@ +26,5 @@
> +          <xul:vbox flex="1">
> +            <xul:stack class="videoCallBox" anonid="videoCallBox" flex="1" hidden="true">
> +              <html:video class="remoteVideo" anonid="remoteVideo"/>
> +              <xul:vbox>
> +                <xul:box flex="1"/>

This should be xul:spacer, not xul:box.

Is there a reason why this same result couldn't be achieved using the pack attribute on the parent vbox?

@@ +28,5 @@
> +              <html:video class="remoteVideo" anonid="remoteVideo"/>
> +              <xul:vbox>
> +                <xul:box flex="1"/>
> +                <xul:hbox>
> +                  <xul:box flex="1"/>

Same comments as above:
- should the hbox have a class?
- box -> spacer
- pack attribute instead?

@@ +34,5 @@
> +                </xul:hbox>
> +              </xul:vbox>
> +              <xul:vbox class="flexbox">
> +                <xul:hbox class="flexbox">
> +                  <xul:box flex="1" class="flexbox"/>

box -> spacer. What is the "flexbox" class used for?

I wonder if there was a way to display that button in the top right corner with less wrapping elements.

@@ +44,5 @@
> +            <xul:splitter class="splitter" anonid="videoCall-splitter" hidden="true">
> +              <xul:grippy/>
> +            </xul:splitter>
> +            <xul:notificationbox class="conv-messages" anonid="convNotificationBox" flex="2" xbl:inherits="chat">
> +              <xul:vbox flex="1">

I also have doubts about the usefulness of this vbox, but it was already here before this patch, so let's not touch it for now.

@@ +292,5 @@
>  
> +      <method name="initPeerConnection">
> +        <parameter name="aPeerConnection"/>
> +        <parameter name="aSuccessCallback"/>
> +        <parameter name="aErrorCallback"/>

Where are these 2 parameters used?

@@ +298,5 @@
> +        <![CDATA[
> +          if (!this.peerConnection) {
> +            this.peerConnection = new (this.contentWindow.mozRTCPeerConnection)();
> +            this.peerConnection.onaddstream = obj => {
> +              let remoteVideo = this.getElt("remoteVideo");

I feel there's something missing here.
- What happens if the conversation tab is closed before the onaddstream callback is called?
- What if the tab has been teared off and the contentWindow is now wrapped in a different instance of the conversation binding? (It seems the => callback keeps a reference to "this" here.)
- Should we reset the callback to null after it's been called, to prevent the peerconnection object from holding a reference to this chat window after the conversation (with its ongoing video call) has been moved to another window? (maybe the peerconnection implementation clears that callback after calling it, but in that case we should probably add a comment in our code about it.)

@@ +336,5 @@
> +            Services.strings.createBundle("chrome://instantbird/locale/conversation.properties");
> +          let cnb = this.getElt("convNotificationBox");
> +          cnb.appendNotification(
> +            convBundle.GetStringFromName("callError.label"),
> +            "video-call-error", null, cnb.PRIORITY_INFO_HIGH, null);

appendNotification null checks it's aButtons last parameter, the trailing null here isn't needed.

@@ +352,5 @@
> +              msg = "Unable to set remote description."
> +              break;
> +            case "Video":
> +              msg = "Failed to initialize video.";
> +              break;

should we add:
  default:
     msg = aErrorType;
?
Or just initialize msg to aErrorType instead of "".

@@ +355,5 @@
> +              msg = "Failed to initialize video.";
> +              break;
> +          }
> +          if (msg)
> +            Components.utils.reportError("Unable to establish call:\n" + msg);

Let's remove the \n here, they are sometimes annoying when copy/pasting from the error console.

@@ +375,5 @@
> +            pc.createOffer(offer => {
> +              pc.setLocalDescription(offer,
> +                () => {}, () => this.callError("LocalDesc"));
> +            }, () => this.callError("Offer"));
> +            pc.onicecandidate = event => {

This hack needs a comment explaining that we are disabling/not supporting trickle ICE for now, and if possible should include a bug number.

@@ +396,5 @@
> +            [{
> +              accessKey: convBundle.GetStringFromName("cancelCall.accesskey"),
> +              callback: () => this.endCall(true),
> +              label: convBundle.GetStringFromName("cancelCall.label"),
> +              popup: null

I don't think this "popup: null" line does anything. This comment also applies to the 2 buttons in the video-call-notification notification.

@@ +400,5 @@
> +              popup: null
> +            }]);
> +
> +          // Set call timeout to 30 sec.
> +          const timer = 30;

Either inline this "30" in the next line (explicit enough given the comment IMO), or give the const a much better name (eg. kCallTimeout).

@@ +441,5 @@
> +              }
> +            ]);
> +
> +          // Disable video button as soon as call notification is received so
> +          // that user cannot make another call until he accepts/rejects it.

Grammar: _a_ call notification, _the_ user.

@@ +455,5 @@
> +        <body>
> +        <![CDATA[
> +          let offer = {type: "offer", sdp: aOffer};
> +          let pc = this.initPeerConnection();
> +          offer = new mozRTCSessionDescription(offer);

Can this just be:
  let offer = new mozRTCSessionDescription({type: "offer", sdp: aOffer});
and still fit on one 80 chars line?

@@ +463,5 @@
> +                pc.setLocalDescription(answer,
> +                  () => {}, () => this.callError("LocalDesc"));
> +              }, () => this.callError("Answer"));
> +
> +              pc.onicecandidate = event => {

Same as above, this needs a comment about not supporting trickle ICE, and a bug number.

@@ +468,5 @@
> +                if (event.candidate)
> +                  return;
> +                pc.onicecandidate = null;
> +                if (!this._conv)
> +                  return;

I would like a comment above this null check. I assume it's here to take care of the possible issues I mentioned in the initPeerConnection method, if the user closed or detached the conversation tab. Is this null check enough to take care of all the possible cases? (I currently think it more or less does, but I may have missed some edge cases.)

@@ +518,5 @@
> +        </body>
> +      </method>
> +
> +      <method name="endCall">
> +        <parameter name="aSender"/>

The meaning of this parameter isn't obvious at all. Should this be 2 separate methods with the one covering the case where aSender is currently true calling the other one?

@@ +559,5 @@
> +            videoCallNotification.removeNotification(notification);
> +
> +          // End the call if it is not already terminated.
> +          if (this._conv && aSender == true)
> +            this._conv.endCall();

I wonder if prpls would like to have this call before we close the peer connection.

@@ +2047,5 @@
>  
> +      <method name="updateVideoCallButton">
> +        <body>
> +        <![CDATA[
> +          let button = this.getElt("videoCallButton");

It's not clear to me when the videoCallButton should be null-checked and when it's guaranteed to exist. This doesn't seem to be done consistently throughout the code.

@@ +2049,5 @@
> +        <body>
> +        <![CDATA[
> +          let button = this.getElt("videoCallButton");
> +
> +          // TODO Implement a way to actually check if the conversation supports video chat.

This comment wants a bug number.

@@ +2247,5 @@
> +            if (this.callTimeout) {
> +              this.onVideoCallAnswered(aData);
> +              break;
> +            }
> +            Components.utils.reportError("Unexpected webrtc call answer:\n" + aData);

Should this error somehow bubble to the prpl?

::: im/content/instantbird.xul
@@ +72,5 @@
>               oncommand="var conv = getTabBrowser().selectedConversation;
>                          if (conv) conv.findbar.onFindAgainCommand(true);"/>
>      <command id="cmd_findPrevious"
>               oncommand="var conv = getTabBrowser().selectedConversation;
>                          if (conv) conv.findbar.onFindAgainCommand(false);"/>

I'm tempted to suggest a helper in instantbird.js:

function getConv(aCallback)
{
  let conv = getTabBrowser().selectedConversation;
  if (conv)
    aCallback(conv);
}

So that we can clean this up to:
oncommand="getConv(conv => conv.findbar.onFindAgainCommand(false));"

Probably better in another bug though :-).

@@ +80,5 @@
> +                          return;
> +                        if (conv.ongoingCall)
> +                          conv.endCall();
> +                        else
> +                          conv.startCall();"/>

Isn't this long enough to deserve an onVideoCallButton method on the conversation binding?

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +14,5 @@
> +videoCallButton.label=Video call
> +videoCallButton.accesskey=c
> +
> +# Call Notification
> +# %1$S is the call initiator's identiy.

This should be right above the string using %S. It doesn't need to be %1$S if we have only one replacement. It should also use a "LOCALIZATION NOTE" prefix.

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +158,5 @@
>  <!ENTITY hideOtherAppsCmdMac.label      "Hide Others">
>  <!ENTITY hideOtherAppsCmdMac.commandkey "H">
>  <!ENTITY showAllAppsCmdMac.label        "Show All">
> +
> +<!-- Video call support -->

This comment isn't helpful.

::: im/themes/conversation.css
@@ +150,5 @@
> +.videoCallButton > .toolbarbutton-icon {
> +  max-width: 16px;
> +}
> +
> +.videoCallButton:hover:not([disabled = "true"]) {

nit: no space before and after =

@@ +164,5 @@
> +    list-style-image: url("chrome://instantbird/skin/[e-mail]");
> +    -moz-image-region: rect(0, 32px, 32px, 0);
> +  }
> +
> +  .videoCallButton:hover:not([disabled = "true"]) {

same here.

@@ +456,5 @@
> +  max-width: 192px;
> +  min-width: 192px;
> +  height: 144px;
> +  max-height: 144px;
> +  min-height: 144px;

Do we _really_ need to set width+max-width+min-width? (same for height)

::: im/themes/jar.mn
@@ +34,5 @@
>  	skin/classic/instantbird/userIcon.png
>  	skin/classic/instantbird/userIcon.svg
>  	skin/classic/instantbird/multiUserIcon.png
>  	skin/classic/instantbird/webcam.png		(webRTC-shareDevice-16.png)
> +	skin/classic/instantbird/webcam-active.png     (webRTC-sharingDevice-16.png)

misalignment.

@@ +139,5 @@
>  	skin/classic/aero/instantbird/userIcon.png
>  	skin/classic/aero/instantbird/userIcon.svg
>  	skin/classic/aero/instantbird/multiUserIcon.png
>  	skin/classic/aero/instantbird/webcam.png	(webRTC-shareDevice-16.png)
> +	skin/classic/aero/instantbird/webcam-active.png     (webRTC-sharingDevice-16.png)

misaligned.

Comment 84

[Florian Quèze [:florian]]

Comment on attachment 8509651 [details] [diff] [review]
backend.diff v16

Review of attachment 8509651 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked quickly at the xmpp-jingle.jsm file, as I seem to remember we reviewed it extensively during the TB Summit.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +6,5 @@
> +
> +Components.utils.import("resource:///modules/xmpp-xml.jsm");
> +
> +const Jingle = {
> +  sdp2xml: function sdp2xml(aOffer) {

aOffer isn't a good parameter name here, as it can also be an answer.

@@ +63,5 @@
> +      // one of: actpass / active / passive.
> +      let setupAttr = medium.match(/(a=setup)[\S]*/)[0].split(":")[1];
> +
> +      let description = Stanza.node("description", null,
> +                                    {xmlns: Stanza.NS.jingle_apps_rtp,

The XML namespace should be set using the second parameter of Stanza.node rather than the xmlns attribute. Please fix this throughout the sdp2xml method.

@@ +82,5 @@
> +                                      hash: hashType},
> +                                     fingerprintData));
> +
> +      let lastPayload;
> +

nit: This empty line isn't helpful.

@@ +84,5 @@
> +
> +      let lastPayload;
> +
> +      for (let line of lines) {
> +

neither is this one.

::: chat/protocols/xmpp/xmpp.jsm
@@ +32,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "UuidGenerator",
>                                     "@mozilla.org/uuid-generator;1",
>                                     "nsIUUIDGenerator");
> +XPCOMUtils.defineLazyModuleGetter(this, "Jingle",
> +  "resource:///modules/xmpp-jingle.jsm");

nit: would this be > 80 chars if aligned like the other lines above?

@@ +296,5 @@
> +    this._sendCallStanza(aOffer, "session-initiate");
> +  },
> +
> +  /* Called when the user accepts an incoming video call. */
> +  acceptCall: function(aOffer) {

aOffer is actually an answer here.

@@ +300,5 @@
> +  acceptCall: function(aOffer) {
> +    this._sendCallStanza(aOffer, "session-accept");
> +  },
> +
> +  _sendCallStanza: function(aOffer, aType) {

Change aOffer to a non-confusing name. aSDP? aSessionDescription?

@@ +302,5 @@
> +  },
> +
> +  _sendCallStanza: function(aOffer, aType) {
> +    let item = Stanza.node("jingle", null,
> +                           {xmlns: Stanza.NS.jingle,

Stanza.node("jingle", Stanza.NS.jingle,

(The xml name space is the second parameter of the Stanza.node function; it shouldn't be set as an attribute of the node.)

@@ +306,5 @@
> +                           {xmlns: Stanza.NS.jingle,
> +                            action: aType,
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: this._callSid},
> +                            Jingle.sdp2xml(aOffer));

nit: This last line is misaligned.

Jingle.sdp2xml can return null in error cases. Shouldn't we null check this and log a warning or error if we failed to parse the SDP?

@@ +307,5 @@
> +                            action: aType,
> +                            initiator: this._account._connection._jid.jid,
> +                            sid: this._callSid},
> +                            Jingle.sdp2xml(aOffer));
> +

nit: this empty line doesn't help readability.

@@ +318,5 @@
> +      return;
> +
> +    let reason = Stanza.node("reason", null);
> +    reason.addChild(Stanza.node("success", null));
> +    reason.addChild(Stanza.node("text", null, null, "Call disconnected"));

The 4th parameter of Stanza.node will automatically do the addChild calls.

@@ +321,5 @@
> +    reason.addChild(Stanza.node("success", null));
> +    reason.addChild(Stanza.node("text", null, null, "Call disconnected"));
> +
> +    let item = Stanza.node("jingle", null,
> +                           {xmlns: Stanza.NS.jingle,

Move the namespace to the Stanza.node second param.

@@ +881,5 @@
> +      if (jingle) {
> +        let action = jingle.attributes["action"];
> +        if (action) {
> +          let conversation =
> +            this.createConversation(this.normalize(aStanza.attributes["from"]));

Are we sure we want to create a conversation if we receive session-terminate or a random unknown action?

@@ +893,5 @@
> +              if (isOffer) {
> +                // Automatically decline incoming offers when we already have
> +                // an outgoing call at the same time.
> +                let reason = Stanza.node("reason", null);
> +                reason.addChild(Stanza.node("success", null));

Can we avoid these 2 addChild calls?

@@ +897,5 @@
> +                reason.addChild(Stanza.node("success", null));
> +                reason.addChild(Stanza.node("text", null, null,
> +                  "Received a call offer while busy with an outgoing call."));
> +                let item = Stanza.node("jingle", null,
> +                                       {xmlns: Stanza.NS.jingle,

Fix the xml NS.

@@ +908,5 @@
> +              else
> +                this.WARN("Received a call answer with a different sid to our call offer.");
> +            }
> +            conversation._callSid = sid;
> +            let sdp = Jingle.xml2sdp(contentNodes);

Jingle.xml2sdp can return null in error cases; should we null check and report an error if we failed to convert to an SDP?

@@ +922,5 @@
> +          }
> +        }
> +      }
> +
> +      this.WARN("Unhandled IQ set stanza.");

I think we are supposed to send an iq error in that case, but let's keep that for another bug.

@@ +931,5 @@
> +        let action = jingle.attributes["action"];
> +        if (action) {
> +          if (action == "session-initiate") {
> +            let conversation =
> +              this.createConversation(this.normalize(aStanza.attributes["from"]));

This createConversation call, without verifying first that we have actually tried to start a call, makes me a bit uncomfortable.

Comment 85

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #83)

> ::: im/app/profile/all-instantbird.js
> @@ +314,5 @@
> >  // Whether to parse log files for conversation statistics.
> >  pref("statsService.parseLogsForStats", true);
> > +
> > +// Whether to allow video calls.
> > +pref("messenger.conversations.enableVideoCalls", false);
> 
> I'm surprised this isn't in the chat/ patch. Shouldn't the XMPP code decline
> code invites differently (and immediately) if the feature isn't enabled?

I meant "call invites" here.

Comment 86

[Patrick Cloke [:clokep]]

Attachment: Purple part v13

This applies attachment 8507406 [details] [diff] [review] after attachment 8505695 [details] [diff] [review] and combines them into a single patch.

Comment 87

[aleth [:aleth]]

Attachment: jingle.diff v17

This patch depends on the fixes in bug 1128741.

The backend code has changed quite a bit, because apart from addressing the review comments
- The jingle stanzas sent were not always following the spec (we don't fully implement any of the specs at the moment of course)
- Sends and handles (some) errors according to the spec (e.g. when video chats aren't implemented or disabled with the pref at the other end) and is hopefully a bit more robust.
- SDPs generated by gecko webrtc are more complicated than they used to be
- Added debug logging as no doubt there'll be SDP/XML issues still to fix in the future
- Generally tried to remove duplication in the code

Comment 88

[aleth [:aleth]]

Attachment: sdp-test.diff.diff v13

Updates the test with a current SDP generated by the platform. This is a pretty fragile test, but it will have to do for now.

Comment 89

[aleth [:aleth]]

Attachment: frontend.diff.diff v19

Addresses frontend review comments, prefs off video calls for TB (note this can be done independently of IB), and makes the video call button icon turn orange while a call is ongoing.

I've cleaned up the XUL adding the video to the conversation binding, but there is a problem: The stack containing the html:video elements doesn't size itself correctly. For some reason, the remote video gets a fixed narrow width and the other elements in the containing vbox get that width too. I haven't discovered why this happens or how to fix it without terrible hacks, so tips are welcome.

>Can't the videoCallBox stack be moved to the convBox vbox, between the conv-top-info toolbar and the conv-top hbox?

If one does this, the window doesn't behave as well when it is resized (it's possible to make the input box disappear). It's anyway unrelated to the remote video size problem.

Comment 90

[aleth [:aleth]]

Another outstanding issue is that at least on OSX it doesn't seem possible to call another instance of Instantbird running on the same machine (the remote video doesnt' show up).

Comment 91

[aleth [:aleth]]

Attachment: frontend.diff.diff v20

> I've cleaned up the XUL adding the video to the conversation binding, but
> there is a problem: The stack containing the html:video elements doesn't
> size itself correctly. For some reason, the remote video gets a fixed narrow
> width and the other elements in the containing vbox get that width too. I
> haven't discovered why this happens or how to fix it without terrible hacks,
> so tips are welcome.

The problem we have to work around here is that as mentioned on MDN, the layout of children of a stack can behave strangely when size attributes like width/min-width etc are added. But the localVideo requires that we give it a fixed size, so we need to use this, and at the same time we want the remoteVideo to adapt its size to its surroundings. The latter fails if we use align/pack/right/top XUL attributes to determine the position of the localVideo element. Also, the localVideo element somehow gets a width three times the actual video width if not all of width/min-width/max-width are set.

Anyway, this patch seems to do the trick, by placing the localVideo using spacers in a modified version of mayanktg's original solution. I've also cleaned up the CSS a bit.

Comment 92

[Patrick Cloke [:clokep]]

Comment on attachment 8558211 [details] [diff] [review]
jingle.diff v17

Review of attachment 8558211 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nits + a few minor things I'd like to see changed! Thanks for working on this though, the code is much clearer and simpler.

::: chat/protocols/xmpp/xmpp-jingle.jsm
@@ +5,5 @@
> +const EXPORTED_SYMBOLS = ["Jingle"];
> +
> +Components.utils.import("resource:///modules/xmpp-xml.jsm");
> +
> +// NOTE: Relevant specs:

I don't think you need "NOTE: " here. :) Quite the list of specs here! I hope you enjoyed reading them!

@@ +30,5 @@
> +
> +    // Helper function that returns a key/value pair for each line.
> +    let parseLine = line => {
> +      let i = line.indexOf(":");
> +      if (i < 0)

Nit: It is clearer to check if it is == -1.

@@ +92,5 @@
> +        continue;
> +
> +      // Every media section contains an a=setup line with a value that's
> +      // one of: actpass / active / passive.
> +      let setupAttr = medium.match(/(a=setup)[\S]*/)[0].split(":")[1];

I'm not entirely sure what this line is doing / I feel like it might be fragile? What is this trying to do? Can this just be done when we parse through all the lines?

@@ +98,5 @@
> +      let description = Stanza.node("description", Stanza.NS.jingle_apps_rtp,
> +                                    {media: mediaName,
> +                                     setup: setupAttr});
> +
> +      // Map.get that initializes the value if it the key does not exist yet.

Specify that it is initialized to an array.

@@ +125,5 @@
> +          // e.g. [ a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level].
> +          let [id, uri] = params;
> +          description.addChild(Stanza.node("rtp-hdrext",
> +                                            Stanza.NS.jingle_apps_rtp_hdrext,
> +                                            {id: id, uri: uri}));

Nit: Spacing.

@@ +130,5 @@
> +        }
> +        else if (key =="a=rtpmap") {
> +          // Codec
> +          // e.g. [a=rtpmap:8 PCMA/8000/2]
> +          let id = params[0]; // Must be listed in the m= line

Do we verify it is in the m= line or do we assume?

@@ +170,5 @@
> +          // e.g. [a=candidate:3 2 UDP 1686044670 106.51.45.18 43127 typ host]
> +          let attr = {component: params[0], foundation: params[1],
> +                      protocol: params[2], priority: params[3], ip: params[4],
> +                      port: params[5], type: params[7]};
> +          if (params.length >= 9) {

We likely want to check if it is >= 12 here?

@@ +233,5 @@
> +  },
> +
> +  xml2sdp: function xml2sdp(aContentNodes, aGroupNodes = []) {
> +    if (!aContentNodes.length)
> +      return null;

Is this a case we're expecting?

@@ +285,5 @@
> +      if (!description)
> +        continue;
> +
> +      // Media is of the form [m=video 33680 RTP/SAVPF 120].
> +      let media = "m=" + description.attributes["media"];

I thought we have to map these back to SDP values (e.g. face -> video, voice -> audio)?

@@ +327,5 @@
> +                        node.attributes["protocol"] + " " +
> +                        node.attributes["priority"] + " " +
> +                        node.attributes["ip"] + " " +
> +                        node.attributes["port"] +
> +                        " typ " + node.attributes["type"];

Does only indenting two spaces help the formatting here? I also wonder if a join would somehow help, but I doubt it. :)

@@ +387,5 @@
> +
> +        /* rtcp-fb element defined in XEP-0293 Jingle RTP Feedback Negotiation
> +         * uses a modified  RTP profile for audio and video conferences with
> +         * minimal control to acieve timely feedback.
> +         */

Nit: Funny formatting of this comment.

::: chat/protocols/xmpp/xmpp.jsm
@@ +318,5 @@
> +      xml = Jingle.sdp2xml(aSDP);
> +      if (!xml || !xml.length)
> +        throw("SDP->XML conversion failed.");
> +      this.DEBUG("Converted to XML:\n" +
> +                 xml.map(node => node.convertToString()).join(""));

This debug call looks expensive, but likely useful initially.

@@ +998,5 @@
>        aStanza.attributes.from, error));
>    },
>  
> +  sendJingleStanza(aAction, aData, aCallback, aSid, aTo) {
> +    let attr = { action: aAction, sid: aSid };

Nit: Spaces around { }.

@@ +1047,5 @@
> +            this.sendErrorStanza(aStanza, "not-acceptable", "modify");
> +            return;
> +          }
> +          let conversation =
> +            this.createConversation(this.normalize(aStanza.attributes.from));

I find it confusing that this function creates or just returns a conversation, but not your fault!

@@ +1093,5 @@
> +          return;
> +        }
> +        else if (action == "session-terminate") {
> +          let conversation =
> +            this.createConversation(this.normalize(aStanza.attributes.from));

Weird thing here, but if a session is terminated, we likely don't want to bring up a conversation if one doesn't exist already.

Comment 93

[aleth [:aleth]]

Attachment: jingle.diff v18

(In reply to Patrick Cloke [:clokep] from comment #92)
> > +          let id = params[0]; // Must be listed in the m= line
> 
> Do we verify it is in the m= line or do we assume?

We make all kinds of assumptions about the well-formedness of the SDP which should really be checked. But that's for a followup...

> > +  xml2sdp: function xml2sdp(aContentNodes, aGroupNodes = []) {
> > +    if (!aContentNodes.length)
> > +      return null;
> 
> Is this a case we're expecting?

Just a simple sanity check, more likely to arise due to coding error in xmpp.jsm.

> > +      // Media is of the form [m=video 33680 RTP/SAVPF 120].
> > +      let media = "m=" + description.attributes["media"];
> 
> I thought we have to map these back to SDP values (e.g. face -> video, voice
> -> audio)?

No, those are the default section names, this here is the mediaName.

> > +      this.DEBUG("Converted to XML:\n" +
> > +                 xml.map(node => node.convertToString()).join(""));
> 
> This debug call looks expensive, but likely useful initially.

Yeah, we might want to remove it in a few months when things settle down.

Comment 94

[Patrick Cloke [:clokep]]

Comment on attachment 8558214 [details] [diff] [review]
sdp-test.diff.diff v13

Review of attachment 8558214 [details] [diff] [review]:
-----------------------------------------------------------------

Well, this is a test...I'm unsure how good it is though.

Comment 95

[Patrick Cloke [:clokep]]

Comment on attachment 8564373 [details] [diff] [review]
jingle.diff v18

Review of attachment 8564373 [details] [diff] [review]:
-----------------------------------------------------------------

Doh. We need to package xmpp-jingle.jsm!

Comment 96

[Patrick Cloke [:clokep]]

Comment on attachment 8559158 [details] [diff] [review]
frontend.diff.diff v20

Review of attachment 8559158 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel entirely comfortable reviewing this. Can we ask Florian to review this? I gave you some feedback though!

::: im/content/conversation.xml
@@ +22,5 @@
>      <content>
>        <xul:vbox class="convBox" flex="1">
>          <xul:toolbar class="conv-top-info" anonid="conv-top-info" context="tabContextMenu"/>
>          <xul:hbox class="conv-top" flex="1" anonid="conv-top">
> +          <xul:vbox flex="1">

Did we want to add any comments about the odd issues you had with xul:stack and html:video?

@@ +40,5 @@
> +
> +            <xul:splitter class="splitter" anonid="videoCall-splitter"
> +                          hidden="true" resizebefore="closest"/>
> +
> +            <xul:notificationbox class="conv-messages" anonid="convNotificationBox"

I'd think we'd want the notification box ABOVE the video, but let's ask Florian.

@@ +308,5 @@
> +        <![CDATA[
> +          if (!this.peerConnection) {
> +            this.peerConnection = new (this.contentWindow.mozRTCPeerConnection)();
> +            this.peerConnection.onaddstream = obj => {
> +              this.peerConnection.onaddstream = null;

Why this?

@@ +333,5 @@
> +              // Check conversation binding hasn't been closed in the interim.
> +              if (!this.conv)
> +                return;
> +              let localVideo = this.getElt("localVideo");
> +              localVideo.mozSrcObject = stream;

I'm somewhat surprised we initialize the local video in a different place than the remote video.

@@ +398,5 @@
> +            pc.createOffer(offer => {
> +              pc.setLocalDescription(offer,
> +                () => {},
> +                () => this.callError("LocalDesc"));
> +            }, () => this.callError("Offer"));

Would it make sense to put the descriptive error right here instead of having a switch statement in callError?

@@ +469,5 @@
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="acceptCall">

I feel like there's a good amount of duplicated code from startCall in here, but maybe we can't do anything about it.

@@ +547,5 @@
> +        <body>
> +        <![CDATA[
> +          // Clear the timeout.
> +          if (this.callTimeout) {
> +            clearTimeout(this.callTimeout);

Doesn't clearTimeout null check first so you can just blindly do it?

::: im/content/instantbird.xul
@@ +78,5 @@
>                          if (!panel) return;
>                          if (panel.findbar) panel.findbar.onFindAgainCommand(false);"/>
> +    <command id="cmd_videoCallButton"
> +             oncommand="var conv = getTabBrowser().selectedConversation;
> +                        if (conv) conv.onVideoCallButton();"/>

These names are odd: "onVideoCallButton" and "endCall"...let's normalize these to sound similar (and match the other commands?)

::: im/locales/en-US/chrome/instantbird/conversation.properties
@@ +24,5 @@
> +rejectCall.accesskey=R
> +# LOCALIZATION NOTE:
> +# These appear in the video call notification box.
> +callError.label=Unable to establish call.
> +callWait.label=Calling…

Can we add a name in here? "Calling %S..." (This matches videoCallNotification)

@@ +28,5 @@
> +callWait.label=Calling…
> +endVideoCall.label=The video call has ended.
> +# LOCALIZATION NOTE: (videoCallNotification.label)
> +# %S is the name of the person initiating the video call.
> +videoCallNotification.label=%S is calling you.

Sometimes we seem to say just "call" and other times "videoCall"...which is it?

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +61,5 @@
>  <!ENTITY about.accesskey       "A">
>  
>  <!ENTITY conversation.contextMenu.close "Close Tab">
>  <!ENTITY chat.participants  "Participants:">
> +<!ENTITY endCall.label      "End video call">

Should this just be "End call"?

::: mail/components/im/all-im.js
@@ +12,5 @@
>  
>  // Limit the number of gloda IM results
>  pref("mailnews.database.global.search.im.limit", 1000);
> +
> +// Whether to allow video calls.

We should probably put a note here saying that no UI is implemented for TB yet.

Comment 97

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #95)
> Comment on attachment 8564373 [details] [diff] [review]
> jingle.diff v18
> 
> Doh. We need to package xmpp-jingle.jsm!

I believe it should be packaged automatically.

Comment 98

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #97)
> (In reply to Patrick Cloke [:clokep] from comment #95)
> > Comment on attachment 8564373 [details] [diff] [review]
> > jingle.diff v18
> > 
> > Doh. We need to package xmpp-jingle.jsm!
> 
> I believe it should be packaged automatically.

My bad, you're right. We only need to manually package components.

Comment 99

[aleth [:aleth]]

Attachment: jingle.diff v19

r+ carried forward, r+ bug 1131251 folded in.

Comment 100

[aleth [:aleth]]

Attachment: sdp-test.diff.diff v14

r+ carried forward.

Comment 101

[aleth [:aleth]]

Attachment: frontend.diff.diff v21

r+ Bug 1131255 folded in.

Comment 104

[aleth [:aleth]]

So I unbitrotted these patches. The XMPP ones look OK to me (the test is fragile, but better than nothing) as a first-pass. The frontend one is a bit sad with its callback hell. It's a shame there's no promise-based API for webrtc. But maybe improving on that would be better as a followup.

Still, I'm a bit hesitant about whether these should be landed without someone willing to spend some time working on finishing/polishing this feature after ;)

Comment 105

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #104)
> It's a shame there's no promise-based API for webrtc.

There was no promise-based API at the time, but this was a long while ago.

https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia

Comment 106

[aleth [:aleth]]

Attachment: frontend.diff.diff v22

I added the new getUserMedia, but unfortunately it looks like PeerConnection (which has most of the callbacks) hasn't changed.

Comment 107

[Patrick Cloke [:clokep]]

Resetting assignee due to lack of activity.

Comment 108

[msdosfx]

Is there a bounty I can contribute to for this feature?