Closed Bug 954988 Opened 8 years ago Closed 8 years ago

Download the user's vCard before uploading an avatar

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1556 at 2012-06-27 16:37:00 UTC ***

http://xmpp.org/extensions/xep-0153.html#bizrules-vcard says:
"A client MUST NOT advertise an avatar image without first downloading the current vCard. Once it has done this, it MAY advertise an image. However, a client MUST advertise an image if it has just uploaded the vCard with a new avatar image. In this case, the client MAY choose not to redownload the vCard to verify its contents."

The code I added in bug 954816 (bio 1381) doesn't respect this.
Blocks: 954816
*** Original post on bio 1556 at 2012-06-27 16:42:22 UTC ***

See also http://xmpp.org/extensions/xep-0054.html#sect-id229678 (3.1 Retrieving One's vCard) and http://xmpp.org/extensions/xep-0054.html#sect-id229776 (3.2 Updating One's vCard)
Whiteboard: [1.2-blocking?]
Blocks: 955019
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 1556 as attmnt 1755 at 2012-07-25 18:00:00 UTC ***

I haven't tested this at all (the unindented debug code is meant to be removed as soon as I'll have verified that things work as expected).
Patrick, I'm requesting feedback now so that you know what to expect in your review queue tomorrow ;). (and of course so that you have a chance to tell me if I'm doing something stupid before I waste more time on it :))
Attachment #8353516 - Flags: feedback?(clokep)
Assignee: nobody → florian
*** Original post on bio 1556 at 2012-07-25 18:06:50 UTC ***

Comment on attachment 8353516 [details] [diff] [review] (bio-attmnt 1755)
WIP

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -584,20 +584,22 @@ const XMPPAccountPrototype = {
>+      this._sendVCard(false, true);
>     }
>     else if (aTopic == "user-display-name-changed")
>-      this._sendVCard();
>+      this._forceUserDisplayNameUpdate = true;
>+      this._sendVCard(true);

>@@ -1118,35 +1138,86 @@ const XMPPAccountPrototype = {
>   _sendVCard: function() {

Am I missing something or does this function not take any parameters?
*** Original post on bio 1556 at 2012-07-25 20:33:51 UTC ***

(In reply to comment #3)
> Comment on attachment 8353516 [details] [diff] [review] (bio-attmnt 1755) [details]

> >+      this._sendVCard(false, true);

> Am I missing something or does this function not take any parameters?

Until a few minutes before I attached this patch, the function was taking 2 parameters aForceDisplayNameUpdate and aForceUserIconUpdate.

I changed to using member variables (eg. this._forceUserDisplayNameUpdate) because keeping the values of these 2 parameters so that the code fetching the vCard from the server and fetching the icon from the disk could provide them when calling _sendVCard back was challenging.

I'll clean this up for the next attachment, thanks for pointing it out!
Attached patch PatchSplinter Review
*** Original post on bio 1556 as attmnt 1758 at 2012-07-26 16:33:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353519 - Flags: review?(clokep)
Comment on attachment 8353516 [details] [diff] [review]
WIP

*** Original change on bio 1556 attmnt 1755 at 2012-07-26 16:33:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353516 - Attachment is obsolete: true
Attachment #8353516 - Flags: feedback?(clokep)
Comment on attachment 8353519 [details] [diff] [review]
Patch

*** Original change on bio 1556 attmnt 1758 at 2012-07-26 16:40:17 UTC ***

>+    // Send the vCard only if it has really changed.
>+    if (this._userVCard.getXML() != existingVCard)
>+      this._connection.sendStanza(Stanza.iq("set", null, null, this._userVCard));
>+    else
>+      LOG("Not sending the vcard to the server because it already has the same");

Maybe "Not sending the vcard to the server because the server vcard is identical." Or something...it took me a few times to understand what this meant. :) r+ with this changed to be readable.
Attachment #8353519 - Flags: review?(clokep) → review+
*** Original post on bio 1556 at 2012-07-26 17:36:21 UTC ***

https://hg.instantbird.org/instantbird/rev/4cdab92b4cc0
Status: NEW → RESOLVED
Closed: 8 years ago
OS: Other → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [1.2-blocking?] → [1.2-blocking]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.