1127596 - First incoming message from someone who is not a contact is dropped on the floor

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

The first incoming message from a non-contact appears in the debug log, but not in a conversation window. The conversation window only opens with the second received message.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - recieve non-contact messages

Comment 2

[aleth [:aleth]]

Comment on attachment 8570971 [details] [diff] [review]
rev 1 - recieve non-contact messages

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

Thanks for looking at this!

::: chat/protocols/xmpp/xmpp.jsm
@@ +198,1 @@
>    },

You can shorten this to 
get buddy() this._account._buddies.get(this._name)
(check the page for Maps on MDN if you don't know why!)

@@ +315,5 @@
>  {
> +  this._init(aAccount, name);
> +  if(this.buddy) {
> +    this.displayName = this.buddy.contact.displayName;
> +    this.contactDisplayName = this.buddy.contactDisplayName || this.displayName;

You don't need both displayName and contactDisplayName as properties of the conversation. Only contactDisplayName is actually used.

@@ +322,5 @@
> +  else {
> +    this.displayName = this._name;
> +    this.contactDisplayName = this._name;
> +    this.userName = this._name;
> +  }

You shouldn't set this in the constructor because it is possible, from the UI, to add a contact for the person you are talking to, in which case this.buddy will suddenly no longer be null. So these three properties should also be getters.

@@ -402,5 @@
>      delete this._serverAlias;
>    },
>  
> -  /* Display name of the buddy */
> -  get contactDisplayName() this.buddy.contact.displayName || this.displayName,

Are you sure this isn't used anywhere else?

@@ +457,5 @@
>  
>      let s = Stanza.iq("set", null, null,
>                        Stanza.node("query", Stanza.NS.roster, null,
>                                    Stanza.node("item", null,
> +                                              {jid: this._name,

Why this change?

@@ +492,5 @@
>  
>      let fileName = this._photoHash + "." + kExt[type];
>      let file = FileUtils.getFile("ProfD", ["icons",
> +                                           this.account.protocol._name,
> +                                           this.account._name,

Why these changes?

@@ +520,5 @@
>  
>      let type = aStanza.attributes["type"];
>  
>      // Reset typing status if the buddy is in a conversation and becomes unavailable.
> +    let conv = this._account._conv.get(this._name);

Why?

@@ +582,5 @@
>      let photo = aStanza.getElement(["x", "photo"]);
>      if (photo && photo.uri == Stanza.NS.vcard_update) {
>        let hash = photo.innerText;
>        if (hash && hash != this._photoHash)
> +        this._account._requestVCard(this._name);

You really don't want to change all these.

I'm not sure what your misunderstanding is - maybe take another look at the way "this" works in Javascript?

@@ +1304,3 @@
>      if (!this._conv.has(aNormalizedName)) {
>        this._conv.set(aNormalizedName,
> +        new this._conversationConstructor(this,aNormalizedName));

nit: space after comma

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - recieve non-contact messages

Sorry, I was confused about classes.
because some methods have the same name in classes,
also properties and inheritance relations.

Comment 4

[aleth [:aleth]]

Comment on attachment 8571006 [details] [diff] [review]
rev 2 - recieve non-contact messages

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

This looks almost ready!

Please change the commit message to include the reviewer, be more specific, and fix the typo, e.g. "Bug 1127596 - Allow XMPP conversations with non-contacts. r=aleth"

::: chat/protocols/xmpp/xmpp.jsm
@@ +307,5 @@
>      delete this.buddy;
>      GenericConvIMPrototype.unInit.call(this);
>    }
>  };
> +function XMPPConversation(aAccount, name)

Nit: function parameters are named like aName, aAccount, etc...

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - recieve non-contact messages

Comment 6

[aleth [:aleth]]

Comment on attachment 8571039 [details] [diff] [review]
rev 3 - recieve non-contact messages

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

Please fix the commit message as I asked in the previous review.

::: chat/protocols/xmpp/xmpp.jsm
@@ +191,5 @@
>    _typingState: "active",
>  
> +  get buddy() this._account._buddies.get(this.name),
> +  get title() this.contactDisplayName,
> +  get normalizedName() this.buddy ? this.buddy.normalizedName : this.name,

You don't need the normalizedName getter as the prototype already has one that is sufficient.

@@ +307,1 @@
>      delete this.buddy;

This line is no longer needed.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - recieve non-contact messages

Comment 8

[aleth [:aleth]]

Comment on attachment 8571057 [details] [diff] [review]
rev 4 - recieve non-contact messages

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

You still didn't fix the commit message, but I can do that on checkin.

Thanks for fixing this bug!

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/21e2455b6b00