1025150 - Implement Entity Capabilities in XMPP (XEP-0115)

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)

Mayank Kumar [:mayanktg]

Updated

•

10 years ago

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

Mayank Kumar [:mayanktg]

Comment 2

•

10 years ago

Attached patch (WIP) v1: Receiving part of entity capabilities. (obsolete) — Details — Splinter Review

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.

Mayank Kumar [:mayanktg]

Comment 3

•

10 years ago

Attached patch (WIP) v1: Test for createVerAttribute function. — Details — Splinter Review

I will update with bigger samples of the Query stanza soon.

aleth [:aleth]

Comment 4

•

10 years ago

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+

Mayank Kumar [:mayanktg]

Comment 5

•

10 years ago

Attached patch (WIP) v2: Entity capabilities. (obsolete) — Details — 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)

Mayank Kumar [:mayanktg]

Updated

•

10 years ago

Depends on: 1015678

Mayank Kumar [:mayanktg]

Updated

•

10 years ago

Depends on: 1011878
No longer depends on: 1015678

aleth [:aleth]

Comment 6

•

10 years ago

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+

aleth [:aleth]

Comment 7

•

10 years ago

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.

aleth [:aleth]

Comment 8

•

10 years ago

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).

aleth [:aleth]

Updated

•

10 years ago

No longer blocks: 1018060

Depends on: 1018060

Mayank Kumar [:mayanktg]

Comment 9

•

10 years ago

Attached patch (WIP) v3: Entity capabilities. — Details — Splinter Review

Attachment #8455549 - Attachment is obsolete: true

Attachment #8473951 - Flags: feedback?(benediktp)

Attachment #8473951 - Flags: feedback?(aleth)

Florian Quèze [:florian]

Comment 10

•

10 years ago

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?

aleth [:aleth]

Comment 11

•

10 years ago

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+

Florian Quèze [:florian]

Comment 12

•

10 years ago

(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.

Patrick Cloke [:clokep]

Comment 13

•

5 years ago

Resetting assignee due to lack of activity.

Assignee: mayanktg → nobody

Patrick Cloke [:clokep]

Updated

•

4 years ago

Blocks: 1616923

Benedikt Pfeifer

Comment 14

•

3 years ago

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)

BMO Automation

Updated

•

2 years ago

Severity: normal → S3

(WIP) v1: Receiving part of entity capabilities. 10 years ago Mayank Kumar [:mayanktg] 12.46 KB, patch	aleth : feedback+	Details \| Diff \| Splinter Review
(WIP) v1: Test for createVerAttribute function. 10 years ago Mayank Kumar [:mayanktg] 2.01 KB, patch		Details \| Diff \| Splinter Review
(WIP) v2: Entity capabilities. 10 years ago Mayank Kumar [:mayanktg] 19.31 KB, patch	aleth : feedback+	Details \| Diff \| Splinter Review
(WIP) v3: Entity capabilities. 10 years ago Mayank Kumar [:mayanktg] 22.21 KB, patch	aleth : feedback+	Details \| Diff \| Splinter Review