Open Bug 1025150 Opened 10 years ago Updated 2 years ago

Implement Entity Capabilities in XMPP (XEP-0115)

Categories

(Chat Core :: XMPP, defect)

Product:
Chat Core
Component:
XMPP
Type:
defect

Tracking

(Not tracked)

People

(Reporter: sawrubh, Unassigned)

References

(Depends on 1 open bug, Blocks 2 open bugs,
URL
)

Details

Attachments

(2 files, 2 obsolete files)

(WIP) v1: Test for createVerAttribute function.
(deleted), patch
Details | Diff | Splinter Review
(WIP) v3: Entity capabilities.
(deleted), patch
aleth
: feedback+
Details | Diff | Splinter Review
Implement Service Discovery (XEP-0030).
Summary: Implement Service Discover in XMPP (XEP-0030) → Implement Service Discovery in XMPP (XEP-0030)
Blocks: 1024023
Depends on: 1018060
Blocks: 1018060
No longer depends on: 1018060
We decided to implement Entity Capabilities instead of Service Discovery since the latter is prone to disco/version floods with a large roster : http://xmpp.org/extensions/xep-0030.html#impl-info
Summary: Implement Service Discovery in XMPP (XEP-0030) → Implement Entity Capabilities in XMPP (XEP-0115)
Assignee: nobody → mayanktg
URL: http://xmpp.org/extensions/xep-0030.html → http://xmpp.org/extensions/xep-0115.html
OS: Linux → All
Hardware: x86_64 → All
We have started with the receiving part of the entity capabilities. When a presence stanza containing c node is received from a buddy it checks whether the entity is already received earlier during the session or not. If yes it gathers the capabilities and proceeds. Else it sends a service discovery request IQ stanza to the buddy. On receiving service discovery response it regenerates the ver attribute received earlier and extract capabilities info from the same. This info is sent in form of bitmask integer to the binding where it is used to enable/disable the respective features.
I will update with bigger samples of the Query stanza soon.
Comment on attachment 8453934 [details] [diff] [review] (WIP) v1: Receiving part of entity capabilities. Review of attachment 8453934 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIStatusInfo.idl @@ +38,5 @@ > + /* Gives more detail to compare the availability of two buddies with the same > + status type. > + Example: 2 buddies may have been idle for various amounts of times. > + Bitmasked value used to assign entity capability info for the buddy. > + video = 1, audio = 2, fileTransfer = 4 */ Define your constants right here in the interface so you can use them from everywhere. ::: chat/protocols/xmpp/xmpp.jsm @@ +560,5 @@ > + let varAttr = featureNode.attributes["var"]; > + > + // For video call support. > + if (varAttr == Stanza.NS.webrtcVideo) > + video = 1; Use constants here too. Define the constants in an interface (imiStatusInfo maybe?) so you only need to do it once for all these files. @@ +602,5 @@ > + > + if (identityNode.attributes["xml:lang"]) > + identityString.push(identityNode.attributes["xml:lang"]); > + else > + identityString.push(""); You can shorten this to identityString.push(identityNode.attributes["xml:lang"] || ""); @@ +607,5 @@ > + > + if (identityNode.attributes["name"]) > + identityString.push(identityNode.attributes["name"]); > + else > + identityString.push(""); Here too. @@ +709,5 @@ > + let ext = capability.attributes["ext"]; > + > + if (capability.attributes["hash"] == Stanza.NS.sha1Hash) { > + if (ver) { > + let verFound = 0; This should be a boolean. ::: im/content/conversation.xml @@ +1989,5 @@ > > + <method name="onServiceDiscovery"> > + <body> > + <![CDATA[ > + let entities = this._conv.buddy.availabilityDetails; I think we should call this "capabilities", not "entities". The 'entity' is the buddy, these are his capabilities. @@ +1995,5 @@ > + const audio = 2; > + const fileTransfer = 4; > + let buddyEntities = {video:false, audio:false, fileTransfer:false}; > + > + if (entities != 0 || entities != undefined || entities != null) { You can shorten this to if (entities) ;) @@ +2024,5 @@ > + // List of supported entities. > + let buddyEntities = this.onServiceDiscovery(); > + > + // Video call button. > + if ((buddyEntities.video == true) && (buddyEntities.audio == true)) { I realize it's probably part of the 'send' work, but you have to check the user's computer has these capabilities as well ;)
Attachment #8453934 - Flags: feedback+
Attached patch (WIP) v2: Entity capabilities. (obsolete) (deleted) — — Splinter Review
Added the sending part of the XEP: 0115 - Entity Capability. User A sends a capability node with the presence stanza and on receive of presence the User B requests for the service discovery. User A then creates a service discovery Stanza and sends it to the user B. This is how the sending part works. Now the user A needs to know about its own service capabilities for which it uses the detectVideoCapabilty method in the conversation binding. If the video and audio devices are detected the call is proceeded.
Attachment #8453934 - Attachment is obsolete: true
Attachment #8455549 - Flags: feedback?(aleth)
Depends on: 1015678
Depends on: 1011878
No longer depends on: 1015678
Comment on attachment 8455549 [details] [diff] [review] (WIP) v2: Entity capabilities. Review of attachment 8455549 [details] [diff] [review]: ----------------------------------------------------------------- If an accountbuddy has more than one resource, do we receive different capabilities for each resource? One resource may be for a computer with a webcam, and one for a computer without a webcam, so I would expect this to be the case. If this is the case (check the XEPs and experiment with multiple clients), your code should make sure it stores the capabilities just like the status info: *separately for each resource*. Then set availabilityDetails based on the current preferred resource. ::: chat/protocols/xmpp/xmpp.jsm @@ +543,5 @@ > + /* Map entities discovered to the ver attribute. */ > + _verArray: [], > + > + /* Generate unique ids. */ > + generateId: function() { Do we really need a separate id generator here or can we use the one you are adding on the account in an earlier patch?
Attachment #8455549 - Flags: feedback?(aleth) → feedback+
Comment on attachment 8455549 [details] [diff] [review] (WIP) v2: Entity capabilities. Review of attachment 8455549 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIStatusInfo.idl @@ +38,5 @@ > + /* Gives more detail to compare the availability of two buddies with the same > + status type. > + Example: 2 buddies may have been idle for various amounts of times. > + Bitmasked value used to assign entity capability info for the buddy. > + video = 1, audio = 2, fileTransfer = 4 */ The example is no longer correct, so remove it. Also remove the constants from the comment. @@ +43,5 @@ > readonly attribute long availabilityDetails; > > + const short video = 1; > + const short audio = 2; > + const short fileTransfer = 4; Constants in IDL files are given names in CAPITAL LETTERS by convention. Also, as these belong together, they should start with the same word, e.g. CAPABILITY_VIDEO, CAPABILITY_AUDIO etc.
Comment on attachment 8455549 [details] [diff] [review] (WIP) v2: Entity capabilities. Review of attachment 8455549 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIStatusInfo.idl @@ +43,5 @@ > readonly attribute long availabilityDetails; > > + const short video = 1; > + const short audio = 2; > + const short fileTransfer = 4; Actually, since higher bits should correspond to more impressive capabilities (since we will prefer buddys with larger availabilityDetails values), how about ordering them by how rare it is to find support for it? This would make fileTransfer = 1 (most commmon), audio = 2, video = 4 (least common).
No longer blocks: 1018060
Depends on: 1018060
Attached patch (WIP) v3: Entity capabilities. (deleted) — — Splinter Review
Attachment #8455549 - Attachment is obsolete: true
Attachment #8473951 - Flags: feedback?(benediktp)
Attachment #8473951 - Flags: feedback?(aleth)
Comment on attachment 8473951 [details] [diff] [review] (WIP) v3: Entity capabilities. Review of attachment 8473951 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-xml.jsm @@ +83,5 @@ > + // Entity Discovery elements. > + jingle_apps_rtp_video : "urn:xmpp:jingle:apps:rtp:video", > + jingle_apps_rtp_audio : "urn:xmpp:jingle:apps:rtp:audio", > + file_transfer : "http://jabber.org/protocol/si/profile/file-transfer", > + node : "http://instantbird.com", "node" is not a good constant name. ::: chat/protocols/xmpp/xmpp.jsm @@ +524,5 @@ > + _verArray: [], > + > + /* Called on receiving a Service Discovery IQ Stanza from the buddy. */ > + onServiceDiscoveryStanza: function(aStanza) { > + let qStanza = aStanza.getElement(["query"]); should that variable be named "query"? "qStanza" and "aStanza" are very close and it's easy to confuse them. @@ +525,5 @@ > + > + /* Called on receiving a Service Discovery IQ Stanza from the buddy. */ > + onServiceDiscoveryStanza: function(aStanza) { > + let qStanza = aStanza.getElement(["query"]); > + let identity = qStanza.getElements(["identity"]); This will throw if qStanza is null. @@ +555,5 @@ > + } > + > + // Assign bitmask value to the capabilities. > + let capabilities = video | audio | fileTransfer; > + this._notifyObservers("availability-changed"); You are sending the notification before saving the change. That can't work. @@ +556,5 @@ > + > + // Assign bitmask value to the capabilities. > + let capabilities = video | audio | fileTransfer; > + this._notifyObservers("availability-changed"); > + this._verArray.push({ver: this._account.createVerAttribute(qStanza), Looks like you are modifying the _verArray of the prototype. If so, the capabilities will be shared by ALL buddies. @@ +566,5 @@ > + let resource = > + this._account._parseJID(aStanza.attributes["from"]).resource || ""; > + if (capabilities > this._resources[resource].capabilities) { > + this._availabilityDetails = this._resources[resource].capabilities = > + capabilities; This is where the availability-changed notification should be sent. @@ +567,5 @@ > + this._account._parseJID(aStanza.attributes["from"]).resource || ""; > + if (capabilities > this._resources[resource].capabilities) { > + this._availabilityDetails = this._resources[resource].capabilities = > + capabilities; > + this._preferredResource = resource; Why are you unconditionally changing the preferred resource?
Comment on attachment 8473951 [details] [diff] [review] (WIP) v3: Entity capabilities. Review of attachment 8473951 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +556,5 @@ > + > + // Assign bitmask value to the capabilities. > + let capabilities = video | audio | fileTransfer; > + this._notifyObservers("availability-changed"); > + this._verArray.push({ver: this._account.createVerAttribute(qStanza), Can you explain what you need to store here and why? @@ +563,5 @@ > + fileTransfer: fileTransfer}); > + > + // If needed assign bitmask value to the availabilityDetails. > + let resource = > + this._account._parseJID(aStanza.attributes["from"]).resource || ""; Wait, can it happen we don't have a resource here? And do we use a "" resource elsewhere in XMPP? @@ +564,5 @@ > + > + // If needed assign bitmask value to the availabilityDetails. > + let resource = > + this._account._parseJID(aStanza.attributes["from"]).resource || ""; > + if (capabilities > this._resources[resource].capabilities) { I'm not sure what you are intending with this. But shouldn't you store the capabilities in this._resources[resource].capabilites now you have received them? @@ +641,5 @@ > + > + if (capability.attributes["hash"] == "sha-1") { > + if (ver) { > + let verFound = false; > + for (let verNode of this._verArray) { _verArray should be a Set or a Map, not an Array. Then lookups are easy and you don't need a loop! @@ +647,5 @@ > + // the entity and assign to availabilityDetails. > + if (ver == verNode.ver) { > + capabilities > + = verNode.video | verNode.audio | verNode.fileTransfer; > + this._notifyObservers("availability-changed"); This is wrong. You only want to notify the observers if the capabilities of the preferred resource changes, either because the preferred resource changes or because *its* capabilities change. @@ +696,5 @@ > this._resources[r].statusType > this._resources[preferred].statusType) > // FIXME also compare priorities... > preferred = r; > + > + if (this._resources[r].capabilities > 0) { This isn't what you want. This loop determines the preferred resource. You don't want the last resource with capabilities to be preferred in all cases. Instead you need to add to the previous if clause the circumstance in which a capability comparison means we've found a better resource to use. @@ +698,5 @@ > preferred = r; > + > + if (this._resources[r].capabilities > 0) { > + preferred = r; > + this._availabilityDetails = capabilities; This shouldn't be here inside the loop. ::: im/content/conversation.xml @@ +2013,5 @@ > ]]> > </body> > </method> > > + <method name="onEntityCapabilities"> This should probably be a <property>, not a method, and called "buddyCapabilities". Look at existing properties for examples. Needs a comment explaining what it is for. @@ +2021,5 @@ > + {video:false, audio:false, fileTransfer:false}; > + let capabilities = this._conv.buddy.availabilityDetails; > + if (capabilities) { > + if (capabilities & this._conv.buddy.CAPABILITY_VIDEO) > + buddyCapabilities.video = true; You can simplify this enormously by let availability = this._conv.buddy.availabilityDetails; let capabilities = { video: availability & this._conv.buddy.CAPABILITY_VIDEO; audio: availability & this._conv.buddy.CAPABILITY_AUDIO; ... } return capabilities; @@ +2049,3 @@ > > + if (videoCallButton && buddyCapabilities.video != true && > + buddyCapabilities.audio != true) This has some duplication and also looks wrong. Use a helper boolean outside the if clause like let canVideoCall = capabilities.video && capabilities.audio;
Attachment #8473951 - Flags: feedback?(aleth) → feedback+
(In reply to aleth [:aleth] from comment #11) > @@ +563,5 @@ > > + fileTransfer: fileTransfer}); > > + > > + // If needed assign bitmask value to the availabilityDetails. > > + let resource = > > + this._account._parseJID(aStanza.attributes["from"]).resource || ""; > > Wait, can it happen we don't have a resource here? And do we use a "" > resource elsewhere in XMPP? We do this in other places because the Facebook XMPP gateway doesn't support resources. I'm not sure if it's a behavior allowed by the standard or a quirk of the Facebook gateway.

Resetting assignee due to lack of activity.

Assignee: mayanktg → nobody
Blocks: 1616923

Comment on attachment 8473951 [details] [diff] [review]
(WIP) v3: Entity capabilities.

I'm not familiar with this code anymore. Clearing feedback request.

Attachment #8473951 - Flags: feedback?(benediktp)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: