Closed Bug 955507 Opened 10 years ago Closed 10 years ago

Add Support for Buddy Icons

Categories

(Chat Core :: Yahoo! Messenger, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: qheaden, Assigned: qheaden)

References

Details

Attachments

(2 files, 6 obsolete files)

*** Original post on bio 2070 at 2013-07-26 15:36:00 UTC ***

Both libpurple and the official Yahoo! chat client support showing the icons your buddies have set for themselves. We need to support this in the JS Yahoo! plug-in.
Attached patch Get Icons - Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2659 at 2013-07-30 21:54:00 UTC ***

This is a patch which adds code to get buddy icons. It isn't complete, which is why I'm asking for feedback and not a review. I just want to get feedback on my general approach to implementing it.

The icons are requested when the buddy list packet is received, and buddies are being processed. At this point, buddy icons are not being saved as locally cached files. Should they be?
Attachment #8354428 - Flags: feedback?(clokep)
Comment on attachment 8354428 [details] [diff] [review]
Get Icons - Patch 1

*** Original change on bio 2070 attmnt 2659 at 2013-07-31 01:26:09 UTC ***

I think this approach looks fine.

// "1" means a request, "2" means reply.

When do we ever use the "2"?
Attachment #8354428 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2070 at 2013-08-01 17:03:30 UTC ***

Looking at libpurple's code, the upload process for a buddy icon seems a bit complicated. Perhaps we should split the buddy icon code into a module? I'm thinking of something like a YahooBuddyIcon class that can handle uploading, and checksum creation.
Status: NEW → ASSIGNED
*** Original post on bio 2070 at 2013-08-01 17:33:50 UTC ***

(In reply to comment #3)
> Looking at libpurple's code, the upload process for a buddy icon seems a bit
> complicated. Perhaps we should split the buddy icon code into a module?
I think we should worry about this after we write the code. Don't get too hung up on these organization details first.
Attached patch Set Icon - Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2683 at 2013-08-07 02:07:00 UTC ***

This patch allows the user to set his Yahoo! Messenger profile icon using the Instantbird UI. I recommend that you make sure the "Get Icons" patch is applied before this "Set Icons" patch to ensure smooth patching.
Attachment #8354452 - Flags: review?(clokep)
Attached patch Set Icon - Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2688 at 2013-08-08 19:02:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354457 - Flags: review?(clokep)
Comment on attachment 8354452 [details] [diff] [review]
Set Icon - Patch 1

*** Original change on bio 2070 attmnt 2683 at 2013-08-08 19:02:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354452 - Attachment is obsolete: true
Attachment #8354452 - Flags: review?(clokep)
Attached patch Get Icons - Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2689 at 2013-08-08 19:03:00 UTC ***

I'm only asking for feedback on this patch because I still haven't added caching yet.
Attachment #8354458 - Flags: feedback?(clokep)
Comment on attachment 8354428 [details] [diff] [review]
Get Icons - Patch 1

*** Original change on bio 2070 attmnt 2659 at 2013-08-08 19:03:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354428 - Attachment is obsolete: true
Comment on attachment 8354458 [details] [diff] [review]
Get Icons - Patch 2

*** Original change on bio 2070 attmnt 2689 at 2013-08-11 18:27:35 UTC ***

>+  // Buddy icon checksum. This is not used (for now).
>+  0xbd: function(aPacket) {},
Shouldn't this be XXX or TODO? Why is it not used? What is this?

>+  // Buddy icon request reply.
>+  0xbe: function(aPacket) {

>+  // Buddy avatar (icon) update.
>+  0xc7: function(aPacket) {
I guess I'm a bit confused at when we get this opposed to 0xbe?

Can you please set up Mercurial to use 8 context lines in diffs? Thanks
Attachment #8354458 - Flags: feedback?(clokep) → feedback-
Comment on attachment 8354457 [details] [diff] [review]
Set Icon - Patch 2

*** Original change on bio 2070 attmnt 2688 at 2013-08-11 18:40:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354457 - Flags: review?(clokep) → review-
*** Original post on bio 2070 at 2013-08-11 18:40:26 UTC ***

Comment on attachment 8354457 [details] [diff] [review] (bio-attmnt 2688)
Set Icon - Patch 2

>diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
>@@ -15,6 +15,16 @@ Cu.import("resource:///modules/socket.js
> XPCOMUtils.defineLazyGetter(this, "_", function()
>   l10nHelper("chrome://chat/locale/yahoo.properties")
> );
>+XPCOMUtils.defineLazyGetter(this, "FileUtils", function() {
>+  Components.utils.import("resource://gre/modules/FileUtils.jsm");
>+  return FileUtils;
>+});
>+ XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
>+  Components.utils.import("resource://gre/modules/NetUtil.jsm");
>+  return NetUtil;
>+});
I believe Florian said on IRC that we don't need to lazily get when using Cu.import?

>@@ -22,6 +32,8 @@ const kVendorId = 0;
>+const kProfileIconWidth = 96;
>+const kProfileIconHeight = 72;
That's a fun size. :)

>@@ -283,6 +302,23 @@ YahooSession.prototype = {
>+  setProfileIcon: function(aFileName) {
>+    // Try to get a handle to the icon file.
>+    let file = FileUtils.getFile("ProfD", [aFileName]);
>+    var type = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService)
>+                                        .getTypeFromFile(file);
Why var?

>@@ -390,11 +426,9 @@ YahooLoginHelper.prototype = {
>-  // Crumb and cookie strings sent from Yahoo!'s login server, and used in
>+  // Crumb sent from Yahoo!'s login server, and used in
>   // the final authentication request to the pager server.
Nit: This comment should probably be reformatted.

>@@ -824,6 +860,20 @@ const YahooPacketHandler = {
>+  // Picture upload.
>+  0xc2: function(aPacket) {
>+    let onlineBuddies = this.getOnlineBuddies();
>+    // Send a notification to each online buddy that your icon has changed.
>+    // Those offline will automatically pick up the change when they log in.
>+    for each(let buddy in onlineBuddies) {
Nit: Space before (.

>@@ -916,3 +966,72 @@ const YahooPacketHandler = {
>+/* The YahooProfileIconUploader class is specifically designed to set a profile
>+ * image on a Yahoo! Messenger account. The reason this functionality is split
>+ * into a separate class is because of the complexity of the operation. Because
>+ * of special protocol requirements, it is easier to use raw TCP communication
>+ * instead of the doXHRequest() method.

>+ As a result, this class is also a child
>+ * of the Socket class, so it provides the TCP operations natively. */
This part of the comment is unnecessary, please remove it.

>+function YahooProfileIconUploader(aAccount, aSession, aFileName, aImageStream)
>+{
>+  this._account = aAccount;
>+  this._session = aSession;
>+  this._fileName = aFileName;
>+  this._imageStream = aImageStream;
>+}
>+YahooProfileIconUploader.prototype = {
>+  __proto__: Socket,
>+  _account: null,
>+  _session: null,
>+  _fileName: null,
>+  _imageStream: null,
>+  _requestData: null,
>+
>+  uploadIcon: function() {
>+    // Scale the image down, and make it a PNG.
>+    let scaledImage = imgTools.encodeScaledImage(this._imageStream, "image/png",
>+                                                 kProfileIconHeight,
>+                                                 kProfileIconWidth);
Do we know how this scaling is done or is it magic?

>+    // Build the Yahoo packet.
>+    let packet = new YahooPacket(kPacketType.Picture, 0, this.sessionId);
>+    packet.addValue(1, this._account.cleanUsername);
>+    packet.addValue(38, "604800"); // Expiration time (in seconds) of icon.
This is only 7 days? Do we need to resend this every 7 days or something?

>+    // Build the request header.
>+    let host = this._account.getString("xfer_host");
>+    let port = this._account.getString("xfer_port");
I think Florian wants an explanation of why this is using file transfer settings. :)

>+    let headers = [
>+      ["User-Agent", "Mozilla/5.0"],
Should this be our real user agent? Or why Mozilla /5.0?

>+      ["Cookie", "T=" + this._session.tCookie + "; Y=" + this._session.yCookie],
>+      ["Host", host + ":" + port],
>+      ["Content-Length", packetBuffer.byteLength + 4 + imageData.length],
>+      ["Cache-Control", "no-cache"],
>+    ];
>+    let headerString = "POST /notifyft HTTP/1.1\r\n";
>+    headers.forEach(function(header) {
>+      headerString += header[0] + ": " + header[1] + "\r\n";
>+    });
I would've used a map() and join() here, but this is probably faster. :)

>+    // Build the complete POST request data.
>+    this._requestData = headerString + "\r\n" +
>+                        ArrayBufferToString(packetBuffer) + "29" +
>+                        kPacketDataDelimiter + imageData;
What's the 29?

>+  // Socket callbacks.
>+  onConnection: function() {
>+    this.sendData(this._requestData);
Why don't we build the data here and send it instead of storing it?

>diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
>@@ -330,6 +330,15 @@ YahooAccount.prototype = {
>+  getOnlineBuddies: function() {
>+    let onlineBuddies = [];
>+    for (let buddy of this._buddies) {
>+      if (buddy[1].statusType != Ci.imIStatusInfo.STATUS_OFFLINE)
Should this be > STATUS_OFFLINE to also ignore UNKNOWN buddies? I know they're not currently used...
*** Original post on bio 2070 as attmnt 2700 at 2013-08-12 23:56:00 UTC ***

This patch addresses the feedback given above on v2 of the patch. It also fixes scaling of the image based on original aspect ratio, which prevents distortion.
Attachment #8354469 - Flags: review?(clokep)
Comment on attachment 8354457 [details] [diff] [review]
Set Icon - Patch 2

*** Original change on bio 2070 attmnt 2688 at 2013-08-12 23:56:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354457 - Attachment is obsolete: true
Comment on attachment 8354469 [details] [diff] [review]
Set Icon - Patch 3

*** Original change on bio 2070 attmnt 2700 at 2013-08-13 00:23:06 UTC ***

Looks good. Let's try this out in the nightlies. A few things I noticed that I think we'll want to eventually fix:
- I feel like we should not try to stretch the icon if its width <= 92 pixels.
- We probably want to abstract this shared code so we can easily handle things like the above.
Attachment #8354469 - Flags: review?(clokep) → review+
Attached patch Get Icons - Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2708 at 2013-08-13 18:33:00 UTC ***

Although this patch allows us to see buddy icons, it does not contain any code for caching. Caching will be added in another bug or patch.
Attachment #8354477 - Flags: review?(clokep)
Comment on attachment 8354458 [details] [diff] [review]
Get Icons - Patch 2

*** Original change on bio 2070 attmnt 2689 at 2013-08-13 18:33:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354458 - Attachment is obsolete: true
Comment on attachment 8354477 [details] [diff] [review]
Get Icons - Patch 3

*** Original change on bio 2070 attmnt 2708 at 2013-08-13 18:53:41 UTC ***

>+  // Buddy avatar (icon) update.
>+  0xc7: function(aPacket) {
>+    if (!aPacket.hasKey(4))
>+      return;
>+    // Obtain the new icon.
>+    this._session.requestBuddyIcon(aPacket.getValue(4));
>+  },
Is there some situation where we expect there to not be a key here? What is that? Is that if the user clears their icon or something?
Attachment #8354477 - Flags: review?(clokep) → review-
*** Original post on bio 2070 at 2013-08-13 19:58:26 UTC ***

Okay, here is something a bit strange. After testing, I would first like to say that libpurple's buddy icon code doesn't work with the official Yahoo! Messenger client. The reason for this is because when setting a profile icon using the official client, the correct icon URL is sent within the checksum packet, not the avatar update packet. The URL contained in the avatar update packet is a URL to a placeholder image.

This leads to a slight problem with our code. In order to correctly handle the official Yahoo client as well as other messengers, Instantbird, Pidgin, etc. we must update the icon in both the avatar update handler (service 0xbe) as well as the checksum handler (service 0xbd).

My solution is to simply set the buddy icon in both handlers, but only update the buddy image if the checksum is different, which signifies the image changed.
Attached patch Get Icons - Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2070 as attmnt 2709 at 2013-08-13 21:32:00 UTC ***

This version of the patch includes a few changes over v3.

First, I removed the check for the existence of key 4 in handler 0xc7, and replaced it with code that only accepts packets with a Server Ack status (the in-code comment explains why).

Second, I was able to remove the YahooAccount.setBuddyIcon method, since it was no longer needed.

Third, I added code to extract the checksum from the buddy icon URL, and use this checksum to prevent downloading of identical buddy icons. These checksums can be used in the future for icon caching.

Finally, I added code to download buddy icons when they log on. I discovered that last patch only requested buddy icons if a buddy changes their icon, or if they where logged in already when you logged in.
Attachment #8354478 - Flags: review?(clokep)
Comment on attachment 8354477 [details] [diff] [review]
Get Icons - Patch 3

*** Original change on bio 2070 attmnt 2708 at 2013-08-13 21:32:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354477 - Attachment is obsolete: true
*** Original post on bio 2070 as attmnt 2713 at 2013-08-14 01:53:00 UTC ***

Just a small change simplifying the if statement located in the 0xbd packet handler.
Attachment #8354482 - Flags: review?(clokep)
Comment on attachment 8354478 [details] [diff] [review]
Get Icons - Patch 4

*** Original change on bio 2070 attmnt 2709 at 2013-08-14 01:53:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354478 - Attachment is obsolete: true
Attachment #8354478 - Flags: review?(clokep)
Comment on attachment 8354482 [details] [diff] [review]
Get Icons - Patch 5

*** Original change on bio 2070 attmnt 2713 at 2013-08-14 02:00:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354482 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2070 at 2013-08-14 13:50:00 UTC ***

http://hg.instantbird.org/instantbird/rev/38a4fa8c9eaf
http://hg.instantbird.org/instantbird/rev/8094f80f354c

Please file a follow-up for the caching of files.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
*** Original post on bio 2070 at 2013-08-14 15:41:08 UTC ***

I think this is causing tests to fail: http://buildbot.instantbird.org/builders/macosx-onCommit/builds/442/steps/shell_1/logs/stdio

Please look at this ASAP.
Depends on: 955546
You need to log in before you can comment on or make changes to this bug.