1171691 - Improve the participant tooltips for XMPP

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

requestBuddyInfo isn't implemented yet.

Comment 1

[aleth [:aleth]]

XMPP already sends presence stanzas for MUC participants, so using the info in those would be the place to start.

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - implement requestBuddyInfo

Comment 3

[aleth [:aleth]]

Comment on attachment 8638613 [details] [diff] [review]
rev 1 - implement requestBuddyInfo

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +82,5 @@
>                           kRoles.indexOf(item.attributes["affiliation"]));
> +
> +    let jid = item.attributes["jid"];
> +    if (jid)
> +      this._userName = jid;

If it's the jid, we should probably call it this.jid...

No underscore if it's going to be non-private (i.e. accessed from outside the MUCParticipant).

@@ +1077,5 @@
>      this.sendStanza(s);
>      this.removeBuddyRequest(aRequest);
>    },
>  
> +  requestBuddyInfo: function(aBuddyName) {

aBuddyName is actually the result of a getNormalizedChatBuddyName call, so it's a normalized participant jid. So you can call it aJid for clarity.

@@ +1078,5 @@
>      this.removeBuddyRequest(aRequest);
>    },
>  
> +  requestBuddyInfo: function(aBuddyName) {
> +    if (!this._connection)

Use this.connected

@@ +1104,5 @@
> +      statusType = buddy.statusType;
> +      statusText = buddy.statusText;
> +    }
> +    else {
> +      // Otherwise the participant is available as he/she is in MUC.

That's not necessarily true - they can be AWAY, and maybe IDLE if they have been inactive for a bit. Why didn't you use the presence stanzas we receive for each participant? You can store the statustype and text in the MUCParticipant. Then you probably also don't need to special-case buddies and the user.

@@ +1111,5 @@
> +    }
> +    tooltipInfo.push(new TooltipInfo(statusType, statusText, true));
> +
> +    let iq = Stanza.iq("get", null,
> +                       userName ? this.normalize(userName) : aBuddyName,

Doesn't this.normalize(userName) strip the participant name? How can this work?

@@ +1128,5 @@
> +  },
> +
> +  // Parses vCard information in aResult parameter through adding properties
> +  // and returns it.
> +  parseVCardInfo: function(aNode, aResult) {

Should/could this be merged with onVCard() in a useful way?

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - implement requestBuddyInfo

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #3)
> Doesn't this.normalize(userName) strip the participant name? How can this
> work?

The userName refers to jid of the participant (e.g username@domain/resource), but not in all cases we can get it. The normalization is for stripping the resource as it can cause error.
BTW, I removed it and let the query use only the jid of the participant (e.g room@domain/nick).

> Should/could this be merged with onVCard() in a useful way?

Yes, this is done in this patch.

Comment 6

[aleth [:aleth]]

Comment on attachment 8638929 [details] [diff] [review]
rev 2 - implement requestBuddyInfo

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +61,5 @@
>  MUCParticipant.prototype = {
>    __proto__: ClassInfo("prplIConvChatBuddy", "XMPP ConvChatBuddy object"),
>  
>    buddy: false,
> +  jid: null,

Duplicates _jid (which, incorrectly, is not stated in the prototype). I'm OK with making that property non-private, but then you have to change the references to this._jid.

@@ +68,5 @@
>    get alias() this.name,
>  
>    role: 2, // "participant" by default
>    set stanza(aStanza) {
>      this._stanza = aStanza;

Why are we storing this? Is it actually used?

I'm also not a huge fan of parsing a presence stanza in a setter, it's confusing (at least to me). So unless the stanza getter is actually used, let's change the setter to an onPresenceStanza method, consistent with the rest of the code, and call that when appropriate instead.

@@ +71,5 @@
>    set stanza(aStanza) {
>      this._stanza = aStanza;
>  
>      let x =
>        aStanza.getChildren("x").filter(function (c) c.uri == Stanza.NS.muc_user);

Could use getChildrenByNS here.

@@ +84,5 @@
>                           kRoles.indexOf(item.attributes["affiliation"]));
> +
> +    let jid = item.attributes["jid"];
> +    if (jid)
> +      this.jid = jid;

We should already have the jid. Or can it change (nick change) without that change already being stored in this.jid?

@@ +96,5 @@
> +      else if (show == "dnd")
> +        statusType = Ci.imIStatusInfo.STATUS_UNAVAILABLE;
> +      else if (show == "xa")
> +        statusType = Ci.imIStatusInfo.STATUS_IDLE;
> +    }

Could this be deduplicated with the code in onPresenceStanza for buddies?

The code there also parses idleSince, maybe also take a look at XMPPAccountBuddy::getTooltipInfo.

@@ +1102,5 @@
> +    let userName;
> +    let tooltipInfo = [];
> +    let jid = this._parseJID(aJid);
> +    let muc = this._mucs.get(jid.node + "@" + jid.domain);
> +    if (muc && muc._participants.has(jid.resource)) {

If muc isn't undefined but the if clause is false, shouldn't the status diplayed in the tooltip be STATUS_OFFLINE or STATUS_UNKNOWN?

@@ +1114,5 @@
> +
> +    let iq = Stanza.iq("get", null, aJid, Stanza.node("vCard", Stanza.NS.vcard));
> +    this.sendStanza(iq, aStanza => {
> +      const kTooltipInfo = ["userName", "fullName", "nickname", "email",
> +                            "birthday"];

Move this down above the for loop where it is used, and add a comment ("vCard fields we want to display in the tooltip")
The name could be improved, e.g. kTooltipFields, as it's not actually a prplITooltipInfo.

@@ +1117,5 @@
> +      const kTooltipInfo = ["userName", "fullName", "nickname", "email",
> +                            "birthday"];
> +      let vCardInfo = {};
> +      if (userName)
> +        vCardInfo.userName = this.normalize(userName);

Needs a comment.

I don't see how for MUC participants it makes sense to strip the resource. And for general buddies, the current resource might be interesting to keep too.

@@ +1118,5 @@
> +                            "birthday"];
> +      let vCardInfo = {};
> +      if (userName)
> +        vCardInfo.userName = this.normalize(userName);
> +      if (aStanza.attributes["type"] == "result")

Shouldn't we check this at the top of the handler and return false if it's not the result stanza we expect?

Are there possible type=errors that need to be handled?

@@ +1119,5 @@
> +      let vCardInfo = {};
> +      if (userName)
> +        vCardInfo.userName = this.normalize(userName);
> +      if (aStanza.attributes["type"] == "result")
> +        vCardInfo = this.parseVCard(aStanza, vCardInfo);

You don't need the vCardInfo = as objects are passed as references.

@@ +1122,5 @@
> +      if (aStanza.attributes["type"] == "result")
> +        vCardInfo = this.parseVCard(aStanza, vCardInfo);
> +
> +      for (let index in kTooltipInfo) {
> +        let field = kTooltipInfo[index];

for (let field of kTooltipInfo) { ...

@@ +1131,5 @@
> +                                   "user-info-received", aJid);
> +    });
> +  },
> +
> +  // Parses vCard information in aResult parameter through adding properties

// Parses the vCard into properties of the aResult object.

@@ +1133,5 @@
> +  },
> +
> +  // Parses vCard information in aResult parameter through adding properties
> +  // and returns it.
> +  parseVCard: function(aNode, aResult) {

aVCardNode

@@ +1135,5 @@
> +  // Parses vCard information in aResult parameter through adding properties
> +  // and returns it.
> +  parseVCard: function(aNode, aResult) {
> +    // XEP-0054: vcard-temp.
> +    for each (let c in aNode.children) {

for each is deprecated, use for...of.

And while I know some of the older JS-XMPP code does this, we try to avoid single-letter variable names when they are not very readable.
e.g. here this is probably better:
for (let node of aNode.children.filter(child => child.type == "node"))

(You will have noticed some of the review comments in this and previous bugs are to clean up some of the existing code too along the way ;) )

@@ +1144,5 @@
> +      else if (c.localName == "NICKNAME")
> +        aResult.nickname = c.innerText;
> +      else if (c.localName == "BDAY")
> +        aResult.birthday = c.innerText;
> +      else if (c.localName == "USERID")

if (...== "EMAIL") {... get the USERID child node...}

@@ +1147,5 @@
> +        aResult.birthday = c.innerText;
> +      else if (c.localName == "USERID")
> +        aResult.email = c.innerText;
> +      else if (c.localName == "PHOTO")
> +        aResult.photo = c;

I think TITLE, ORGNAME, and maybe JABBERID for MUC participants would be nice to have too, what do you think?

@@ +1149,5 @@
> +        aResult.email = c.innerText;
> +      else if (c.localName == "PHOTO")
> +        aResult.photo = c;
> +      else if (c.children)
> +        aResult = this.parseVCard(c, aResult);

This shouldn't be recursive, a vCard is not a recursive structure.

Add a TODO here to parse the vcard completely in the future and display it in system messages in response to /whois.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - implement requestBuddyInfo

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

> Duplicates _jid (which, incorrectly, is not stated in the prototype). I'm OK
> with making that property non-private, but then you have to change the
> references to this._jid.

> We should already have the jid. Or can it change (nick change) without that
> change already being stored in this.jid?
I changed the name of the new one and added comments to explain the differences.

> Why are we storing this? Is it actually used?
No, it's not used. I removed in this patch and used the method |onPresenceStanza|

> I'm also not a huge fan of parsing a presence stanza in a setter, it's
> confusing (at least to me). So unless the stanza getter is actually used,
> let's change the setter to an onPresenceStanza method, consistent with the
> rest of the code, and call that when appropriate instead.

> Could use getChildrenByNS here.
Yes, this is done in this patch.

> Could this be deduplicated with the code in onPresenceStanza for buddies?
Yes, this is done in this patch.

> If muc isn't undefined but the if clause is false, shouldn't the status
> diplayed in the tooltip be STATUS_OFFLINE or STATUS_UNKNOWN?
Yes, you are right.

> Shouldn't we check this at the top of the handler and return false if it's
> not the result stanza we expect?
No, as we can provide the status info. and also may be the real jid of the participant in the tooltip.

> Are there possible type=errors that need to be handled?
No, as the errors mean that no vCard exists or the user does not exist.

> I think TITLE, ORGNAME, and maybe JABBERID for MUC participants would be
> nice to have too, what do you think?
yes, I added them too.

Comment 8

[aleth [:aleth]]

Comment on attachment 8640622 [details] [diff] [review]
rev 3 - implement requestBuddyInfo

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

Works well! Nice to see XMPP participant tooltips. You're right about them being slower than IRC though.

To also see the vcard info added to contact tooltips, you should add XMPP to https://dxr.mozilla.org/comm-central/source/chat/content/imtooltip.xml#230 (but make sure to check the pref to make sure JS-XMPP is enabled.) Make a note in the "enable js-xmpp by default" bug to remove the pref check later.

::: chat/protocols/xmpp/xmpp.jsm
@@ +41,5 @@
>    let cs = Cc["@mozilla.org/txttohtmlconv;1"].getService(Ci.mozITXTToHTMLConv);
>    return function(aTxt) cs.scanTXT(aTxt, cs.kEntities);
>  });
>  
> +// Parses status into object of statusType, statusText and idleSince.

Parses the status from a presence stanza into an object...

@@ +88,5 @@
>   */
>  const kRoles = ["outcast", "visitor", "participant", "member", "moderator",
>                  "admin", "owner"];
>  
>  function MUCParticipant(aNick, aName, aStanza)

Let's call the parameter aPresenceStanza to make it clear what kind of stanza is expected in the constructor, and similarly replace aName with aJid.

@@ +103,5 @@
> +  // The occupant jid of the participant which is of the form room@domain/nick.
> +  _jid: null,
> +
> +  // The real jid of the participant which is of the form local@domain/resource.
> +  accountJID: null,

accountJid (as I think we use lowercase jid pretty much everywhere else for variable names)

@@ +118,3 @@
>      if (x.length == 0)
>        return;
>      x = x[0];

Do you have any clue what the point of these three lines of (existing) code is? (I don't.) Is there something in the spec about multiple x elements? Can we get rid of it or at least add a comment, while we are touching this code anyway?

@@ +130,5 @@
> +      this.accountJID = accountJID;
> +
> +    let statusInfo = parseStatus(aStanza);
> +    this.statusType = statusInfo.statusType;
> +    this.statusText = statusInfo.statusText;

Shouldn't this status part be above the let item =... with its possible early return?

@@ +1096,5 @@
>    },
>  
> +  requestBuddyInfo: function(aJid) {
> +    if (!this.connected)
> +      return;

You should still call notifyObservers in this case as the UI is waiting for a notification.

@@ +1110,5 @@
> +        userName = participant.accountJID;
> +      statusType = participant.statusType;
> +      statusText = participant.statusText;
> +    }
> +    else {

This else only applies if (muc) (remember requestBuddyInfo can be called for any jid, not just MUC jids)

@@ +1115,5 @@
> +      // You have left the room or the participant does not exist.
> +      statusType = Ci.imIStatusInfo.STATUS_UNKNOWN;
> +      statusText = "";
> +    }
> +    tooltipInfo.push(new TooltipInfo(statusType, statusText, true));

Only do this if you actually have status info to add (i.e. if (muc)).

Tip: move the if (muc) above let statusType...

@@ +1116,5 @@
> +      statusType = Ci.imIStatusInfo.STATUS_UNKNOWN;
> +      statusText = "";
> +    }
> +    tooltipInfo.push(new TooltipInfo(statusType, statusText, true));
> +

A nice improvement for the future would be to change the return value of requestBuddyInfo to prplITooltipInfo, as we could then return the status info early and display it immediately without waiting for the vcard to arrive first.

@@ +1126,5 @@
> +      if (userName)
> +        vCardInfo.userName = userName;
> +
> +      let vCardNode = aStanza.getElement(["vCard"]);
> +      if (aStanza.attributes["type"] == "result" && vCardNode)

Add a comment saying that in the case of an error response, we just notify the observers with what info we already have.

@@ +1131,5 @@
> +        this.parseVCard(vCardNode, vCardInfo);
> +
> +      // vCard fields we want to display in the tooltip.
> +      const kTooltipFields = ["userName", "fullName", "nickname", "title",
> +                              "orgName", "email", "birthday"];

Looking at some sample tooltips, I think it would be nice to include CTRY and LOCALITY as well, as those also seem fairly common. It won't be too much as most people won't have all of these fields set.

In the future it would be nice if we could also handle PHOTO, but that's for a followup as prplITooltipInfo doesn't handle that at the moment (I don't think any existing prpl can get photos for muc participants).

@@ +1144,5 @@
> +  },
> +
> +
> +  // Parses the vCard into properties of the aResult object.
> +  parseVCard: function(aVCardNode, aResult) {

As you decided to make it non-recursive, you don't need the aResult parameter:
// Parses the vCard into the properties of the returned object.

@@ +1147,5 @@
> +  // Parses the vCard into properties of the aResult object.
> +  parseVCard: function(aVCardNode, aResult) {
> +    // XEP-0054: vcard-temp.
> +    for (let node of aVCardNode.children.filter(child => child.type == "node")) {
> +      if (node.localName == "FN")

To avoid duplication,

let localName = node.localName;
if (localName == "FN") ...

or

switch (node.localName) { ...

Comment 9

[aleth [:aleth]]

Comment on attachment 8640622 [details] [diff] [review]
rev 3 - implement requestBuddyInfo

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1152,5 @@
> +        aResult.fullName = node.innerText;
> +      else if (node.localName == "NICKNAME")
> +        aResult.nickname = node.innerText;
> +      else if (node.localName == "TITLE")
> +        aResult.title = node.innerText;

Turns out there are servers that send stanzas like

<vCard xmlns="vcard-temp">
<FN xmlns="vcard-temp"/>
<N xmlns="vcard-temp">
<GIVEN xmlns="vcard-temp"/>
<MIDDLE xmlns="vcard-temp"/>
<FAMILY xmlns="vcard-temp"/>
</N>
<NICKNAME xmlns="vcard-temp">
L
</NICKNAME>
<BDAY xmlns="vcard-temp"/>
<GENDER xmlns="vcard-temp"/>
<ADR xmlns="vcard-temp">
<HOME xmlns="vcard-temp"/>
<STREET xmlns="vcard-temp"/>
<EXTADR xmlns="vcard-temp"/>
<EXTADD xmlns="vcard-temp"/>
<LOCALITY xmlns="vcard-temp"/>
<REGION xmlns="vcard-temp"/>
<PCODE xmlns="vcard-temp"/>
<CTRY xmlns="vcard-temp"/>
<COUNTRY xmlns="vcard-temp"/>
</ADR>
<ADR xmlns="vcard-temp">
<WORK xmlns="vcard-temp"/>
<STREET xmlns="vcard-temp"/>
<EXTADR xmlns="vcard-temp"/>
<EXTADD xmlns="vcard-temp"/>
<LOCALITY xmlns="vcard-temp"/>
<REGION xmlns="vcard-temp"/>
<PCODE xmlns="vcard-temp"/>
<CTRY xmlns="vcard-temp"/>
<COUNTRY xmlns="vcard-temp"/>
</ADR>
<ORG xmlns="vcard-temp">
<ORGNAME xmlns="vcard-temp"/>
<ORGUNIT xmlns="vcard-temp"/>
</ORG>
<TITLE xmlns="vcard-temp"/>
<ROLE xmlns="vcard-temp"/>
<URL xmlns="vcard-temp"/>
<DESC xmlns="vcard-temp"/>

so you'll have to check innerText exists.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - implement requestBuddyInfo

(In reply to aleth [:aleth] from comment #8)
> Do you have any clue what the point of these three lines of (existing) code
> is? (I don't.) Is there something in the spec about multiple x elements? Can
> we get rid of it or at least add a comment, while we are touching this code
> anyway?

Yes, I added comments explain that.

> Shouldn't this status part be above the let item =... with its possible
> early return?

Yes, right.

> A nice improvement for the future would be to change the return value of
> requestBuddyInfo to prplITooltipInfo, as we could then return the status
> info early and display it immediately without waiting for the vcard to
> arrive first.

I think this needs to be in a follow up bug as we need to change the interface for |requestBuddyInfo| to be able to return |nsISimpleEnumerator|
(https://dxr.mozilla.org/comm-central/source/chat/components/public/imIAccount.idl#113)

Comment 11

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #10)
> > A nice improvement for the future would be to change the return value of
> > requestBuddyInfo to prplITooltipInfo, as we could then return the status
> > info early and display it immediately without waiting for the vcard to
> > arrive first.
> 
> I think this needs to be in a follow up bug as we need to change the
> interface for |requestBuddyInfo| to be able to return |nsISimpleEnumerator|
> (https://dxr.mozilla.org/comm-central/source/chat/components/public/
> imIAccount.idl#113)

Yes, "for the future" kind of means "we should file a followup if you agree with the idea" ;-)

Comment 12

[aleth [:aleth]]

Comment on attachment 8645461 [details] [diff] [review]
rev 4 - implement requestBuddyInfo

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

Next one should be r+ ;)

::: chat/content/imtooltip.xml
@@ +222,5 @@
>             this.addRow(this.bundle.GetStringFromName("buddy.username"), name);
>  
>           this.addRow(this.bundle.GetStringFromName("buddy.account"), account.name);
>  
>           // Technically there is no reason to restrict this to JS-IRC. But

You can remove the "to JS-IRC" now :-)

@@ +227,5 @@
>           // there is currently no filtering mechanism in place to select which
>           // of the returned fields are to be added to the tooltip for the other
>           // protocols (libpurple in particular).
> +         let forcePurple = Services.prefs.getCharPref("chat.prpls.forcePurple");
> +         let ProtocolId = account.protocol.id;

ProtocolId -> protocolId

@@ +229,5 @@
>           // protocols (libpurple in particular).
> +         let forcePurple = Services.prefs.getCharPref("chat.prpls.forcePurple");
> +         let ProtocolId = account.protocol.id;
> +         if (ProtocolId == "prpl-irc" ||
> +             (forcePurple != "prpl-jabber" && ProtocolId == "prpl-jabber"))

!forcePurple.includes("prpl-jabber")

::: chat/locales/en-US/xmpp.properties
@@ +82,5 @@
> +tooltip.email=Email
> +tooltip.birthday=Birthday
> +tooltip.userName=Username
> +tooltip.title=Title
> +tooltip.orgName=Organization Name

Let's shorten this to "Organization", it should be clear anyway.

::: chat/protocols/xmpp/xmpp.jsm
@@ +118,5 @@
> +    let statusInfo = parseStatus(aStanza);
> +    this.statusType = statusInfo.statusType;
> +    this.statusText = statusInfo.statusText;
> +
> +    // We get an array of elements that match muc_user namespace.

You don't need this comment as it just explains what getChildrenByNS does ;)

@@ +119,5 @@
> +    this.statusType = statusInfo.statusType;
> +    this.statusText = statusInfo.statusText;
> +
> +    // We get an array of elements that match muc_user namespace.
> +    let elements = aStanza.getChildrenByNS(Stanza.NS.muc_user);

You need to filter for x elements.

@@ +125,5 @@
>        return;
> +
> +    // XEP-0045 (16.3.5): We get the first element as if a MUC service receives
> +    // such extended presence information from an occupant (e.g. multiple <x/>
> +    // elements), it MUST NOT reflect it to other occupants.

I think that section refers to what servers should do, but I'm not sure. We can just say

// XEP-0045 (7.2.3): We only expect a single <x/> element of this namespace, so we ignore any others.

@@ +1113,5 @@
> +        if (participant.accountJid)
> +          userName = participant.accountJid;
> +        let statusType =
> +          muc.left ? Ci.imIStatusInfo.STATUS_UNKNOWN : participant.statusType;
> +        let statusText = participant.statusText;

If muc.left, we can't know if the statusText has changed either. So you can wrap the whole status part here in an (if (!muc.left))

@@ +1124,5 @@
> +      // not request user's info.
> +      Services.obs.notifyObservers(new nsSimpleEnumerator(tooltipInfo),
> +                                   "user-info-received", aJid);
> +      return;
> +    }

Move this to the top as if we're not connected, we can't trust any information we may still have stored.

You can use EmptyEnumerator instead of creating one.

@@ +1127,5 @@
> +      return;
> +    }
> +    let iq = Stanza.iq("get", null, aJid, Stanza.node("vCard", Stanza.NS.vcard));
> +    this.sendStanza(iq, aStanza => {
> +      let vCardInfo;

let vCardInfo = {} in case you don't receive a vcard.

@@ +1159,5 @@
> +    // XEP-0054: vcard-temp.
> +    let aResult = {};
> +    for (let node of aVCardNode.children.filter(child => child.type == "node")) {
> +      let localName = node.localName;
> +      if (localName == "FN" && node.innerText)

Probably more readable to do
let innerText = node.innerText;
if (innerText) {
  if (localName = ...) 
   ... put all the localName checks where you need to test innerText here.
}
if (localName...)

@@ +1187,5 @@
> +        if (country && country.innerText)
> +          aResult.country = country.innerText;
> +      }
> +      else if (localName == "JABBERID" && node.innerText &&
> +               !aResult.hasOwnProperty("userName"))

Where would this property come from?

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - implement requestBuddyInfo

(In reply to aleth [:aleth] from comment #12)
> Where would this property come from?

The check was used when aResult passed by reference, but now there is no need for this check here.

Comment 14

[aleth [:aleth]]

Comment on attachment 8645683 [details] [diff] [review]
rev 5 - implement requestBuddyInfo

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1134,5 @@
> +      if (aStanza.attributes["type"] == "result" && vCardNode)
> +        vCardInfo = this.parseVCard(vCardNode);
> +
> +      // The real jid of participant which is of the form local@domain/resource.
> +      if (userName && !vCardInfo.hasOwnProperty("userName"))

Doesn't userName come from the server? Is it more likely to be correct than what's in the vcard (is that set by the user?)? If yes, then you don't want the hasOwnProperty check.

(Not a rhetorical question, I don't know the right answer.)

@@ +1160,5 @@
> +      let localName = node.localName;
> +      let innerText = node.innerText;
> +      if (innerText) {
> +        if (localName == "FN")
> +          aResult.fullName = node.innerText;

... = innerText;
and same below

Comment 15

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 6 - implement requestBuddyInfo

(In reply to aleth [:aleth] from comment #14)
> Doesn't userName come from the server? Is it more likely to be correct than
> what's in the vcard (is that set by the user?)? If yes, then you don't want
> the hasOwnProperty check.

Yes, you are right. I removed the check and added a comment there.

Comment 16

[aleth [:aleth]]

Comment on attachment 8645719 [details] [diff] [review]
rev 6 - implement requestBuddyInfo

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

Thanks, works great now! Looks like the hardest parts of MUC support are done :-)

Comment 17

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/c06a2023ecf3d130de6a3f1768336bde4ab75ff2
changeset:  c06a2023ecf3d130de6a3f1768336bde4ab75ff2
user:       Abdelrhman Ahmed <[e-mail]>
date:       Mon Aug 10 07:12:00 2015 +0200
description:
Bug 1171691 - Implement requestBuddyInfo for JS-XMPP. r=aleth