*** Original post on bio 1982 at 2013-05-29 21:13:00 UTC *** Currently, Instantbird supports the Yahoo! Messenger protocol via libpurple's C implementation. A Google Summer of Code project is underway to re-implement Yahoo! support by creating a native Instantbird protocol plug-in (prpl) in JavaScript. Doing so will result in cleaner, native code that is easier to maintain.

*** Original post on bio 1982 at 2013-05-29 23:00:14 UTC *** Assigning.

*** Original post on bio 1982 as attmnt 2475 at 2013-06-05 19:53:00 UTC *** This is a alpha-stage patch. I'm submitting it so that I can gain some feedback on the overall design, and where things can be improved. This patch allows the plug-in to log into the Yahoo! Messenger service, and log out again. It currently has no other functionality.

Comment on attachment 8354242 [details] [diff] [review] Patch 0.1 *** Original change on bio 1982 attmnt 2475 at 2013-06-06 01:05:12 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 at 2013-06-06 01:05:12 UTC *** Comment on attachment 8354242 [details] [diff] [review] (bio-attmnt 2475) Patch 0.1 This includes the patch from bug 955419 (bio 1982), which needs to land separately. >diff -r bcd11f419854 -r 5d1989d027c2 chat/protocols/yahoo/yahoo-session.jsm >+Cu.import("resource:///modules/socket.jsm"); >+Cu.import("resource:///modules/http.jsm"); >+Cu.import("resource:///modules/ArrayBufferUtils.jsm"); Nit: Alphabetical order please. >+const kPagerRequestURL = "http://vcs1.msg.yahoo.com/capacity"; >+const kLoginTokenGetURL = "https://login.yahoo.com/config/pwtoken_get"; >+const kLoginTokenLoginURL = "https://login.yahoo.com/config/pwtoken_login"; How are you planning to switch these for Yahoo JAPAN? >+const kYahooBuildID = "4194239"; >+const kYahooProtocolVersion = 16; Are these from a certain version? :) >+const kPacketDataDelimiter = "\xC0\x80"; >+const kPacketIdentifier = 0x594D5347; // Spells "YMSG". For some reason I want this constant to be "YMSG", but there's no reason for it... >+YahooSession.prototype = { >+ sessionID: null, Nit: sessionId. >+ // Public methods. >+ login: function(aUsername, aPassword, aCallback) { >+ this._account.reportConnecting(); >+ let loginHelper = new YahooLoginHelper(this); >+ >+ loginHelper.login(aUsername, aPassword, aCallback); As we discussed, I think passing through aCallback here is very confusing, you're already directly calling reportConnecting() above, let's just call connected() after the connection is finished. I also wonder if this can just be the connect method on the account. >+ logout: function(aCallback) { >+ this._account.reportDisconnecting(Components.interfaces.prplIAccount.NO_ERROR, ""); Nit: Ci.prplIAccount. >+function YahooLoginHelper(aSession) >+{ >+ this._session = aSession; >+ this._sessionMethods = {}; >+} >+YahooLoginHelper.prototype = { Maybe we can put a comment above this which goes through the steps of logging in? >+ _session: null, >+ _finishedCallback: null, >+ _username: null, >+ _password: null, >+ _challengeString: null, >+ _loginToken: null, >+ _crumb: null, >+ _yCookie: null, >+ _tCookie: null, >+ _sessionMethods: null, Let's put some comments here! I have no idea what these mean, give me a hint! >+ // Public methods. >+ login: function(aUsername, aPassword, aCallback) { >+ this._finishedCallback = aCallback; >+ this._saveSessionMethods(); We already discussed callbacks a bit, but this is super confusing. >+ // The protocol doesn't want @yahoo.com in the username, but other email >+ // domains are fine. >+ this._username = aUsername.replace("@yahoo.com", ""); Maybe something like "Strip yahoo.com domains, but other email address are fine."? >+ _getPagerAddress: function() { >+ doXHRequest(kPagerRequestURL, null, null, >+ function(aResponse, aXHR) { >+ this._session.pagerAddress = >+ aResponse.substring(aResponse.lastIndexOf("=") + 1); >+ this._getChallengeString(); We probably need some error checking here? Or at least some debug statements! >+ function (aError, aStatusText, aXHR) { >+ this._done(true); >+ }, this, "GET", null); My guess is that you can make an error function on this object and pass the handle to that in all the time. >+ this._session.onBinaryDataReceived = function(aData) { I don't like this way of constantly changing onBinaryDataReceived, is there an identifier which can be used to always identify the type of message? >+ this._session.connect(this._session.pagerAddress, 5050); The port here should be a constant. >+ doXHRequest(url, null, null, >+ function(aResponse, aXHR) { >+ // TODO - Handle error codes here. >+ let responseParams = aResponse.split("\r\n"); >+ this._loginToken = responseParams[1].replace("ymsgr=", ""); Looks more like we want to split on = and make sure it's ymsgr and then take the second value. >+ _getCookies: function() { >+ // TODO - Simplify this using map and join. >+ let url = kLoginTokenLoginURL; >+ url += "?src=ymsgr&"; >+ url += "token=" + this._loginToken; >+ >+ doXHRequest(url, null, null, >+ function(aResponse, aXHR) { >+ let responseParams = aResponse.split("\r\n"); >+ >+ let status = responseParams[0]; >+ if (status != "0") { What does this mean? >+ this._crumb = responseParams[1].replace("crumb=", ""); Again, I think these could be done more clearly. >+ this._yCookie = responseParams[2].substring(2); // Remove the "Y=" bit. >+ this._tCookie = responseParams[3].substring(2); // Remove the "T=" bit. >+ this._sendPagerAuthResponse(); >+ _sendPagerAuthResponse: function() { >+ this._session.onBinaryDataReceived = function(aData) { >+ this._done(false); >+ }.bind(this); >+ >+ let response = this._calculatePagerResponse(); >+ let packet = new YahooPacket(kPacketType.AuthResponse, 0, >+ this._session.sessionID); >+ // Build the key/value pairs. >+ packet.addValue(1, this._username); >+ packet.addValue(0, this._username); >+ packet.addValue(277, this._yCookie); >+ packet.addValue(278, this._tCookie); >+ packet.addValue(307, response); We only use this variable (response) once, we should just inline the call. >+ packet.addValue(244, kYahooBuildID); >+ packet.addValue(2, this._username); >+ packet.addValue(2, "1"); >+ packet.addValue(98, "us"); Really the username gets sent three times? Could be nice to have these constants named... >+ _calculatePagerResponse: function() { >+ let crypt = this._crumb + this._challengeString; >+ let hash = this._md5Hash(crypt, false); >+ let response = btoa(hash); >+ response = response.replace(/\+/g, ".").replace(/\//g, "_"). >+ replace(/=/g, "-"); These don't need to be regular expressions, you can just do: response = response.replace("+", ".").replace("/", "_"). replace("=", "-"); Nit: The replace on the second line should line up with the response on the line above (and the period should be on the second line). >+ _md5Hash: function(aString, aUTF8) { We should probably abstract this somewhere if you and XMPP will use it. >+function YahooPacket(aService, aStatus, aSessionID) >+{ >+ this.service = aService; >+ this.status = aStatus; >+ this.sessionID = aSessionID; >+ this._keyValuePairs = []; Would this make more sense as an object/Map instead of an array? >+ toArrayBuffer: function() { >+ let dataString = ""; >+ for (let i = 0; i < this._keyValuePairs.length; i++) { >+ let pair = this._keyValuePairs[i]; >+ dataString += pair.key + kPacketDataDelimiter; >+ dataString += pair.value + kPacketDataDelimiter; >+ } let dataString = this._keyValuePairs.map(function(aPairs) aPair.key + kPacketDataDelimiter + aPair.value + kPacketDataDelimiter).join(""); >+ let packetLength = dataString.length; >+ let buffer = new ArrayBuffer(20 + packetLength); // Header is 20 bytes. 20 should be a constant. >diff -r bcd11f419854 -r 5d1989d027c2 chat/protocols/yahoo/yahoo.js >+function YahooConversation(aAccount) >+{ >+ this._init(aAccount); >+} >+YahooConversation.prototype = { >+ _disconnected: false, >+ _setDisconnected: function() { >+ this._disconnected = true; >+ }, >+ close: function() { >+ if (!this._disconnected) >+ this.account.disconnect(true); >+ }, >+ sendMsg: function (aMsg) { >+ if (this._disconnected) { >+ this.writeMessage("yahootest", "This message could not be sent because the conversation is no longer active: " + aMsg, {system: true, error: true}); This needs to be localized. :) >+ this.writeMessage("You", aMsg, {outgoing: true}); I assume this is mostly a place holder, but we should be about to use this._account.name, or whatever it is to get the username. >+ options: null, // TODO - Fill this with meaningful options. These need to match the ones from libpurple so migrations work, see http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libyahoo.c#311 >+ usernameSplits: [ >+ {label: "Server", separator: "@", defaultValue: "yahoo.com", >+ reverse: true} I don't think we want a username split for Yahoo? (libpurple does not use one, it just asks for a "Yahoo ID".)

*** Original post on bio 1982 as attmnt 2518 at 2013-06-25 20:15:00 UTC *** This is an updated, overall patch of my protocol plug-in so far. This version has login support fully working, with tests of YahooPacket and YahooLoginHelper. It also supports the buddy list, but it does have a couple of bugs and some more improvement to make in that area. As for buddies, authorization requests have a small bug in which the user is asked for permission twice. Also, extra tooltip information needs to be added to the plug-in.

Comment on attachment 8354242 [details] [diff] [review] Patch 0.1 *** Original change on bio 1982 attmnt 2475 at 2013-06-25 20:15:29 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 as attmnt 2604 at 2013-07-16 16:28:00 UTC *** This is a far updated patch. It includes support for private chat, conference chat, and Yahoo! JAPAN. It allows creation and joining of conferences, and inviting others via a command.

Comment on attachment 8354286 [details] [diff] [review] Patch 0.2 *** Original change on bio 1982 attmnt 2518 at 2013-07-16 16:28:33 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354373 [details] [diff] [review] Patch 0.3 *** Original change on bio 1982 attmnt 2604 at 2013-07-16 18:00:12 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 at 2013-07-16 18:00:12 UTC *** Comment on attachment 8354373 [details] [diff] [review] (bio-attmnt 2604) Patch 0.3 >diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties >+login.error.missingCredentials=Username or password wasn't provided. Can this happen? >+login.error.stripSuffix=Username contains @yahoo.com suffix. Don't we strip this automatically? This is just one of the cases that we get back from the server? >+# In this message, %S is replaced with the username of the user who left. >+system.message.conferenceLogoff=%S has left the conference. Is there no join message? >diff --git a/chat/protocols/yahoo/test/test_yahooLoginHelper.js b/chat/protocols/yahoo/test/test_yahooLoginHelper.js >+// Random values. I don't like "random values" here, maybe there's a better phrase here "Precalculated values" or something? >+/* In each test, we override the function that would normally be called next in >+ * the login process. We do this so that we can intercept the login process, >+ * preventing calls to real Yahoo! servers, and do equality testing. */ >+ >+function run_test() Nit: No blank line between these. >+++ b/chat/protocols/yahoo/yahoo-session.jsm >@@ -0,0 +1,848 @@ >+const kPacketType = { We should check libpurple and see if there are types we're missing, probably some error situations and stuff, you might have to "break" code to try to get the responses. >+const kBuddyStatuses = { I think there are more statuses than this, one of the documents I pointed you to had them. >+function YahooSession(aAccount) So what is a YahooSession anyway? :) (Please add a comment here, I should be able to read that and the comment that should be above YahooSocket and tell why they both exist and what separates them.) >+ _username: null, >+ pagerAddress: null, >+ sessionId: null, You're going to get tired of this: Comments! >+ addBuddyToServer: function(aBuddy) { >+ let packet = new YahooPacket(kPacketType.AddBuddy, 0, this.sessionId); >+ packet.addValue(14, ""); // Messsage to send. Is this supposed to be empty, what does it mean? >+ packet.addValue(65, aBuddy.tag.name); >+ packet.addValue(97, "1"); // UTF-8 encoding. >+ packet.addValue(302, "319"); >+ packet.addValue(300, "319"); >+ packet.addValue(334, "0"); >+ packet.addValue(301, "319"); >+ packet.addValue(303, "319"); Do we know what these mean? >+ setStatus: function(aStatus, aMessage) { >+ // Anytime your status is anything other than available, key 47 must be set >+ // to 1, but it is 0 when you are available. Also when a custom status >+ // message is used, key 97 must be defined as 1, and key 10 must be 99. >+ >+ let packet = new YahooPacket(kPacketType.StatusUpdate, 0, this.sessionId); >+ if (aMessage && aMessage.length > 0) { >+ packet.addValue(10, "99"); >+ packet.addValue(97, "1"); >+ } else { >+ // Perform a reverse lookup for the status code using the status. >+ for (let statusNumber in kBuddyStatuses) { >+ if (kBuddyStatuses[statusNumber] == aStatus) { >+ packet.addValue(10, statusNumber); >+ break; >+ } >+ } >+ } >+ >+ packet.addValue(19, aMessage); Should this only be set in the if aMessage block above? Does this mean you can't be away and have a custom message at once? Are we positive about this? >+ sendChatMessage: function(aName, aMessage) { >+ let packet = new YahooPacket(kPacketType.Message, 0, this.sessionId); >+ // Key 0 is the user ID, and key 1 is the active ID. We need to find the >+ // difference between these two. Alias maybe? Sounds like this is an XXX comment? >+ packet.addValue(1, this._account.cleanUsername); >+ packet.addValue(3, this._account.cleanUsername); I'm seeing a lot of this, I have a feeling one of these can be an alias or "real name" or something...please look into this. >+ // Private methods. >+ _onBinaryDataReceived: function(aData) { >+ let packets = this._extractPackets(aData); This should probably have a try-catch around it? Should this return the number of bytes instead of counting it later on? >+ // aPattern is a >+ // normal array of hex values. >+ _extractPackets: function(aData) { aPattern seems to be left over from old comments. I have a feeling this function is way more complicated than it needs to be, but I do not have time to look it right now. >+/* The purpose of YahooLoginHelper is to separate the complicated login logic Nice comment. :) >+ // Public methods. >+ login: function(aAccount) { >+ this._account = aAccount; Should the account be given during in >+ _calculatePagerResponse: function() { >+ let hasher = Cc["@mozilla.org/security/hash;1"] >+ .createInstance(Ci.nsICryptoHash); . should align with the [ above it. >+ let crypt = this._crumb + this._challengeString; >+ let cryptData = [crypt.charCodeAt(i) for (i in crypt)]; Use StringToBytes. >+ // The protocol requires replacing + with ., / with _, and = with - within >+ // the base64 response string. >+ return btoa(hasher.finish(false)).replace(/\+/g, ".").replace(/\//g, "_") >+ .replace(/=/g, "-"); Align the . with one of the ones on the line above it. >+ // HTTP & TCP Callbacks. >+ _onHttpResponse: function(aResponse, aXHR) { >+ switch (this._stage) { I don't love this being a switch statement, it always adds that extra level of indentation. Instead of different stages, should we different callbacks at each stage? (Also for onTcpResponse.) >+function YahooPacket(aService, aStatus, aSessionId) Comments! What do each of these mean? >+ // Add a single key/value pair. >+ addValue: function(aKey, aValue) { >+ let pair = { >+ "key": aKey.toString(), >+ "value": aValue The " aren't necessary. >+ // This method returns the first value found with the given key. >+ getValue: function(aKey) { >+ for (let i = 0; i < this.keyValuePairs.length; i++) { >+ let pair = this.keyValuePairs[i]; >+ if (pair.key == aKey.toString()) What is the toString() for? Aren't keys numbers? >+ toArrayBuffer: function() { >+ // Build header. >+ let view = new DataView(buffer); >+ let idBytes = StringToBytes(kPacketIdentifier); Can kPacketIdentifier use StringToBytes when defined? >+ // Copy in data. >+ copyBytes(buffer, BytesToArrayBuffer(StringToBytes(dataString)), kPacketHeaderSize); Something about this being a string really scares me. >+ fromArrayBuffer: function(aBuffer) { >+ let dataString = ArrayBufferToString(aBuffer).substring(kPacketHeaderSize); Again, something about this being a string scares me, maybe at least slice aBuffer first? >+const YahooPacketHandler = { >+ // Buddy logoff. >+ 0x02: function(aPacket) { Weird spacing above. >+ // Incoming chat message. >+ 0x06: function(aPacket) { >+ let messageId; >+ if (aPacket.hasKey(429)) >+ messageId = aPacket.getValue(429); >+ if (messageId) { Can this just be if (aPacket.hasKey(429)) and then messageId is only inside of this if statement? >+ let packet = new YahooPacket(kPacketType.MessageAck, 0, aPacket.sessionId); >+ // Some keys have an unknown purpose, so we set a constant value. >+ packet.addValue(1, to); >+ packet.addValue(5, from); >+ packet.addValue(302, "430"); >+ packet.addValue(430, messageId); >+ packet.addValue(303, 430); >+ packet.addValue(450, 0); Do we know what these other fields are at all? >+ // Conference additional invitation. NOTE: Due to limitations of JavaScript, >+ // you cannot assign one function to multiple keys in an object literal. >+ // For now, we will just redefine this function the same as above, but >+ // perhaps we can find a way to do this without having to add extra code. I think I already gave feedback on how to get around this, you define the function outside the object and just do 0x1c: confInvite. >+ this._account.receiveTypingNotification(name, isTyping); It looks like almost all of these use this._account not this, maybe we should use call() with the account object, not the session? >+ 0xd6: function(aPacket) { >+ let authRequest = { Should this be in jsProtoHelper? Is similar code used in XMPP? >+ 0xf0: function (aPacket) { >+ let buddyNames = aPacket.getValues(7); >+ let buddyStatuses = aPacket.getValues(10); >+ let statusMessages; >+ if (aPacket.hasKey(19)) >+ statusMessages = aPacket.getValues(19); >+ else >+ statusMessages = []; Leave out the else here. >+ let status = kBuddyStatuses[buddyStatuses[i]]; What if this key does not exist? >+ let message = statusMessages[i] ? statusMessages[i] : ""; Do statusMessages ? statusMessages[i] : "" >+ 0xf1: function(aPacket) { >+ let tagName = ""; >+ for each (let pair in aPacket.keyValuePairs) { >+ if (pair.key == "65") { >+ tagName = pair.value; Fix the spacing here. >+ this._account.addBuddyFromServer(Services.tags.createTag(tagName), Does this just return the tag if it already exists? >diff --git a/chat/protocols/yahoo/yahoo-socket.jsm b/chat/protocols/yahoo/yahoo-socket.jsm >diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js >diff --git a/chat/protocols/yahoo/yahoo.jsm b/chat/protocols/yahoo/yahoo.jsm I have not reviewed these yet, but we don't need yahoo.jsm anymore since both protocols are in yahoo.js. >diff --git a/chat/protocols/yahoo/yahoo.manifest b/chat/protocols/yahoo/yahoo.manifest >+contract @mozilla.org/chat/yahootest;1 {50ea817e-5d79-4657-91ae-aa0a52bdb98c} >+category im-protocol-plugin prpl-yahootest @mozilla.org/chat/yahootest;1 >+contract @mozilla.org/chat/yahoojptest;1 {5f6dc733-ec0d-4de8-8adc-e4967064ed38} >+category im-protocol-plugin prpl-yahoojptest @mozilla.org/chat/yahoojptest;1 Please change these to the real IDs now. Things to check everywhere: - All line endings are UNIX style. - All files have an extra line at the end. - Cu, Ci, Cc, and Cr are used when defined.

*** Original post on bio 1982 at 2013-07-16 23:35:24 UTC *** Comment on attachment 8354373 [details] [diff] [review] (bio-attmnt 2604) Patch 0.3 >+const kLoginStatusErrors = { >+ "100" : [_("login.error.missingCredentials"), >+ Ci.prplIAccount.ERROR_AUTHENTICATION_IMPOSSIBLE], - You don't want to cache the localized versions of all these error messages when your JS file is parsed. - All the "login.error" in there seem duplicated. You can write this instead: + "100" : ["missingCredentials", + Ci.prplIAccount.ERROR_AUTHENTICATION_IMPOSSIBLE], and get the localized string only when you are about to display it. >+ case kLoginStages.LoginTokenRequest: { >+ let responseParams = aResponse.split("\r\n"); >+ >+ // Status code "0" means success. >+ if (responseParams[0] != "0") { >+ let errorInfo = kLoginStatusErrors[responseParams[0]]; >+ aErrorMessage = errorInfo[0]; >+ aError = errorInfo[1]; >+ this._session.onLoginError(aError, aErrorMessage); >+ return; >+ } >+ ... >+ } break; >+ >+ case kLoginStages.LoginCookiesRequest: { >+ let responseParams = aResponse.split("\r\n"); >+ >+ // Status code "0" means success. >+ if (responseParams[0] != "0") { >+ let errorInfo = kLoginStatusErrors[responseParams[0]]; >+ aErrorMessage = errorInfo[0]; >+ aError = errorInfo[1]; >+ this._session.onLoginError(aError, aErrorMessage); >+ return; >+ } >+ ... >+ } break; This smells like code duplication. Could you create a single function doing the error handling for both these cases? >+ options: { >+ port : {label: _("options.pagerPort"), default: 5050}, >+ xfer_host: {label: _("options.transferHost"), default: "filetransfer.msg.yahoo.com"}, >+ xfer_port: {label: _("options.transferPort"), default: 80}, >+ room_list_locale: {label: _("options.chatLocale"), default: "us"}, >+ local_charset: {label: _("options.chatEncoding"), default: "UTF-8"}, >+ ignore_invites: {label: _("options.ignoreInvites"), default: false}, >+ proxy_ssl: {label: _("options.proxySSL"), default: false} I already said on IRC that this should use getters for the localized strings. Example: + port : {get label() _("options.pagerPort"), default: 5050},

*** Original post on bio 1982 as attmnt 2608 at 2013-07-18 18:36:00 UTC *** This patch implements almost all of the feedback given in the two comments above. Below is some of the feedback I haven't implemented copied verbatim. >+const kBuddyStatuses = { I think there are more statuses than this, one of the documents I pointed you to had them. >+++ b/chat/protocols/yahoo/yahoo-session.jsm >@@ -0,0 +1,848 @@ >+const kPacketType = { We should check libpurple and see if there are types we're missing, probably some error situations and stuff, you might have to "break" code to try to get the responses. >+ // Private methods. >+ _onBinaryDataReceived: function(aData) { >+ let packets = this._extractPackets(aData); This should probably have a try-catch around it? Should this return the number of bytes instead of counting it later on? NOTE: This following suggestion is mainly an aesthetic change to the code, we will probably leave this alone for the first patch landing, as changing it can cause delays. >+ // HTTP & TCP Callbacks. >+ _onHttpResponse: function(aResponse, aXHR) { >+ switch (this._stage) { I don't love this being a switch statement, it always adds that extra level of indentation. Instead of different stages, should we different callbacks at each stage? (Also for onTcpResponse.) >+ // Build header. >+ let view = new DataView(buffer); >+ let idBytes = StringToBytes(kPacketIdentifier); Can kPacketIdentifier use StringToBytes when defined? >+ // Copy in data. >+ copyBytes(buffer, BytesToArrayBuffer(StringToBytes(dataString)), kPacketHeaderSize); Something about this being a string really scares me. >+ fromArrayBuffer: function(aBuffer) { >+ let dataString = ArrayBufferToString(aBuffer).substring(kPacketHeaderSize); Again, something about this being a string scares me, maybe at least slice aBuffer first?

Comment on attachment 8354373 [details] [diff] [review] Patch 0.3 *** Original change on bio 1982 attmnt 2604 at 2013-07-18 18:36:39 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354377 [details] [diff] [review] Patch 0.4 *** Original change on bio 1982 attmnt 2608 at 2013-07-18 19:11:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 at 2013-07-18 19:11:55 UTC *** Comment on attachment 8354377 [details] [diff] [review] (bio-attmnt 2608) Patch 0.4 (In reply to comment #8) > Created attachment 8354377 [details] [diff] [review] (bio-attmnt 2608) [details] > Patch 0.4 > NOTE: This following suggestion is mainly an aesthetic change to the code, we > will probably leave this alone for the first patch landing, as changing it can > cause delays. > > >+ // HTTP & TCP Callbacks. > >+ _onHttpResponse: function(aResponse, aXHR) { > >+ switch (this._stage) { > I don't love this being a switch statement, it always adds that extra level of > indentation. Instead of different stages, should we different callbacks at each > stage? (Also for onTcpResponse.) This is not what my comment on IRC meant. I meant don't worry about this for attaching a patch. I'll want this changed before landing. >+login.error.accountLockedGeneral=Account logked due to too many login attempts. Spelling. >diff --git a/chat/protocols/yahoo/yahoo-socket.jsm b/chat/protocols/yahoo/yahoo-socket.jsm In general I dislike how empty this file is, it makes me say "Why is this a separate object from YahooSession? What does it do differently?" >+const {classes: Cc, interfaces: Ci, utils: Cu} = Components; >+ >+Cu.import("resource:///modules/socket.jsm"); You're only using Cu, just inline tthat here and get rid of the other definitions. >+function YahooSocket() >+{ >+ this.binaryMode = true; >+} >+YahooSocket.prototype = { >+ __proto__: Socket, >+ >+ // Socket events. >+ LOG: function(aString) {}, >+ DEBUG: function(aString) {}, These should point to the account. >+ // Called when a connection is established. >+ onConnection: function() {}, Really, this is empty? :( Nothing to do here at all? I find that unlikely. >+ // Called when a socket is accepted after listening. >+ onConnectionHeard: function() {}, Leave this out entirely. >+ // Called when a connection times out. >+ onConnectionTimedOut: function() {}, This probably needs to inform the account that you have no connection. >+ // Called when a socket request's network is reset. >+ onConnectionReset: function() {}, Here too. >+ // Called when the certificate provided by the server didn't satisfy NSS. >+ onBadCertificate: function(aNSSErrorMessage) {}, I'm not sure Yahoo can have bad certificates or not reasonably, but check out how IRC/XMPP handle this. >+ // Called when the other end has closed the connection. >+ onConnectionClosed: function() {}, Again, you probably need to notify the account. >+ // Called when binary data is available. >+ onBinaryDataReceived: function(/* ArrayBuffer */ aData) {} I think this gets overwritten in various points...this at least needs a comment saying that. >diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js >+function YahooConversation(aAccount, aName) >+ finishedComposing: function() { >+ _refreshTypingTimer: function() { >+ _cancelTypingTimer: function() { Seems like fodder for jsProtoHelper. >+ get name() this._buddyUserName You could probably do this.buddy.name if you don't want to store this separately. Actually, does this work properly if my name is FoO, but you add me to your buddy list as "foo"? What shows up? >+ removeParticipant: function(aName) { >+ if (!this._participants[aName]) >+ return; Do you expect this to happen? Why? >+function YahooConferenceBuddy(aName, aConference) Do things like double clicking on these names work? >+function YahooAccount(aProtoInstance, aImAccount) >+ disconnect: function(aSilent) { >+ this._session.logout(); >+ // buddy[1] is the actual object. >+ for (let buddy of this._buddies) >+ buddy[1].setStatus(Ci.imIStatusInfo.STATUS_UNKNOWN, ""); Don't you have to set the account status here? >+ chatRoomFields: { >+ }, Put these on the same line. >+ joinChat: function(aComponents) { >+ // Create a random number sequence for the room name. >+ let roomName = this.cleanUsername + "-" + >+ Math.floor(Math.random() * 5000).toString(); Any reason this is from 0 to 4999? >+YahooProtocol.prototype = { The below comments also go for YahooJapanProtocol >+ get name() "Yahoo", Do we know if this is ever localized? >+ options: { >+ port: {get label() _("options.pagerPort"), default: 5050}, >+ xfer_host: {get label() _("options.transferHost"), default: "filetransfer.msg.yahoo.com"}, >+ xfer_port: {get label() _("options.transferPort"), default: 80}, >+ room_list_locale: {get label() _("options.chatLocale"), default: "us"}, >+ local_charset: {get label() _("options.chatEncoding"), default: "UTF-8"}, >+ ignore_invites: {get label() _("options.ignoreInvites"), default: false}, >+ proxy_ssl: {get label() _("options.proxySSL"), default: false} Any of these options that are not currently in use need to be commented out.

*** Original post on bio 1982 as attmnt 2613 at 2013-07-19 20:32:00 UTC *** This patch fixes the things mentioned in the previous review. I didn't touch the handling of ArrayBuffer data as a string in YahooPacket.toArrayBuffer() and YahooPacket.fromArrayBuffer(). Patrick mentioned possible problems with encoding, but from what I understand, encoding is used to manipulate the presentation, not the data itself. As a result, I think we should be okay using data strings in these methods since YahooPacket doesn't care about presentation, but only manipulates and stores data for transmission.

Comment on attachment 8354377 [details] [diff] [review] Patch 0.4 *** Original change on bio 1982 attmnt 2608 at 2013-07-19 20:32:02 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 at 2013-07-19 21:34:03 UTC *** There's still a reference to yahoo-socket.jsm, make sure we get rid of all of those.

*** Original post on bio 1982 as attmnt 2623 at 2013-07-22 18:23:00 UTC *** This patch fixes a major bug that can cause repeated logins (resulting in a locked account) when the account is given the wrong password. I also took the opportunity to add tests for the new YahooPacket methods.

Comment on attachment 8354382 [details] [diff] [review] Patch 0.5 *** Original change on bio 1982 attmnt 2613 at 2013-07-22 18:23:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 as attmnt 2626 at 2013-07-22 22:13:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354392 [details] [diff] [review] Patch 0.6 *** Original change on bio 1982 attmnt 2623 at 2013-07-23 02:12:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1982 at 2013-07-23 02:12:08 UTC *** Comment on attachment 8354392 [details] [diff] [review] (bio-attmnt 2623) Patch 0.6 >diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties >+login.error.missingCredentials=Username or password wasn't provided. Can this ever happen? >+login.error.stripSuffix=Username contains @yahoo.com suffix. Would we ever expect a user to actually see this error? Do they care why this happened? >+options.pagerPort=Pager port >+options.transferHost=File transfer server >+options.transferPort=File transfer port >+options.chatLocale=Chat room locale >+options.chatEncoding=Encoding >+options.ignoreInvites=Ignore conference and chatroom invitations >+options.proxySSL=Use account proxy for HTTP and HTTPS connections Are all of these currently used or are some commented out? I don't know what we want to do with those. >+const kLoginStages = { >+ PagerAddressRequest: 0, >+ ChallengeStringRequest: 1, >+ LoginTokenRequest: 2, >+ LoginCookiesRequest: 3, >+ FinalLoginRequest: 4 >+}; I don't think we use this anymore. >+const kBuddyStatuses = { >+ "0" : Ci.imIStatusInfo.STATUS_AVAILABLE, >+ "2" : Ci.imIStatusInfo.STATUS_UNAVAILABLE, >+ "99" : Ci.imIStatusInfo.STATUS_AWAY, >+ "999" : Ci.imIStatusInfo.STATUS_IDLE >+}; Aren't there other statuses? I've asked this previously, but don't seem to have received a satisfactory answer. >+ login: function() { >+ this._account.reportConnecting(); >+ let loginHelper = new YahooLoginHelper(this); >+ loginHelper.login(this._account); Should loginHelper be stored anywhere? I feel like it's weird that it's just chilling here. >+ logout: function() { >+ this._account.reportDisconnecting(Ci.prplIAccount.NO_ERROR, ""); >+ if (this.isConnected) >+ this.disconnect(); >+ this._account.reportDisconnected(); >+ }, You call disconnect in logout...what's the difference? >+ addBuddyToServer: function(aBuddy) { >+ let packet = new YahooPacket(kPacketType.AddBuddy, 0, this.sessionId); >+ // TODO - Add a buddy invite message on key 14 whenever such messages are >+ // supported by the Instantbird UI. For now, leave it empty. >+ packet.addValue(14, ""); Nit: You can't refer to "Instantbird" in chat/ code. Why does it matter if our UI supports it though, doesn't this show up to the remote user? >+ setStatus: function(aStatus, aMessage) { >+ // Anytime your status is anything other than available, key 47 must be set >+ // to 1, but it is 0 when you are available. Also when a custom status >+ // message is used, key 97 must be defined as 1, and key 10 must be 99. I find this comment to be very confusing, maybe we can separate it into a bulleted list? >+ let packet = new YahooPacket(kPacketType.StatusUpdate, 0, this.sessionId); >+ if (aMessage && aMessage.length > 0) { >+ packet.addValue(10, "99"); >+ packet.addValue(97, "1"); So if there is any status you have to be available? Is this correct? This logic seems wrong. >+ } else { >+ // Perform a reverse lookup for the status code using the status. >+ for (let statusNumber in kBuddyStatuses) { >+ if (kBuddyStatuses[statusNumber] == aStatus) { >+ packet.addValue(10, statusNumber); >+ break; >+ } >+ } >+ } >+ // This method looks for individual YMSG packets in an ArrayBuffer, and >+ // returns them in an array. aData is an ArrayBuffer. >+ _extractPackets: function(aData) { >+ let view = new Uint8Array(aData); >+ let pattern = StringToBytes(kPacketIdentifier); Should the constant kPacketIdentifier already be run through StringToBytes? (I've asked this before.) >+ let patternIndex = 0; >+ let sliceBegin = 0; >+ let sliceEnd = 0; >+ let occurences = 0; >+ let bytesHandled = 0; >+ let packets = []; >+ >+ for (let i = 0; i < aData.byteLength; i++) { >+ // If the current byte matches the next expected byte in the pattern, >+ // look for the following byte in the pattern. >+ if (view[i] == pattern[patternIndex]) >+ patternIndex++; >+ else >+ patternIndex = 0; >+ >+ // If we have found all of the bytes in the given pattern, do some split >+ // work to separate the packet. Else, if we are on the final byte, and >+ // there is nothing to split, just place the remaining data in a packet. >+ if (patternIndex == pattern.length) { >+ if (occurences > 0) { >+ sliceEnd = (i - pattern.length) + 1; >+ let packet = new YahooPacket(); >+ packet.fromArrayBuffer(aData.slice(sliceBegin, sliceEnd)); >+ packets.push(packet); >+ bytesHandled += sliceEnd - sliceBegin; >+ sliceBegin = sliceEnd; >+ } else { >+ sliceBegin = (i - pattern.length) + 1; >+ } >+ occurences++; >+ patternIndex = 0; >+ } else if (i == aData.byteLength - 1) { >+ let buf = aData.slice(sliceBegin); >+ // Does the remaining data contain a valid identifier? >+ if (ArrayBufferToString(buf).indexOf(kPacketIdentifier) >= 0) { >+ let packet = new YahooPacket(); >+ packet.fromArrayBuffer(buf); >+ packets.push(packet); >+ } >+ } >+ } >+ >+ return packets; >+ }, Doesn't this code need to check that YMSG packet's length parameter at some point and ensure everything matches up OK? What happens if I send the bytes YMSG inside of a message? Is searching for an identifier really the best way to do this? >+ onBinaryDataReceived: function(aData) { >+ let packets = this._extractPackets(aData); >+ let bytesHandled = 0; I think I asked this before and never received an answer, would it be easier to calculate bytesHandled inside of _extractPackets? Does this already know it? >+ // The username, striped of any occurance of @yahoo.com to compyly with the >+ // protocol. >+ _username: null, *Stripped* of @yahoo.com or @yahoo.co.jp depending on the protocol. Nit: I think you mean "comply" :) >+ // During the login process, we need to override the onBinaryDataReceived >+ // method on the YahooSession object. After login is complete, we need to >+ // restore that method so that YahooSession can listen for data. We store >+ // the original method here to be restored later. >+ _oldBinaryDataReceived: null, I think I'd prefer "original", but can you just set this to YahooSession.prototype.onBinaryDataReceived or whatever? >+ login: function(aAccount) { >+ this._account = aAccount; >+ this._session.onConnection = this._onTcpConnection.bind(this); >+ this._getPagerAddress(); >+ }, Can't this just be the onConnection method? >+ _handleLoginError: function(aErrorCode) { >+ let errorInfo = kLoginStatusErrors[aErrorCode]; >+ aErrorMessage = _("login.error." + errorInfo[0]); >+ aError = errorInfo[1]; Nit: These variable names are all funky, they shouldn't be "aFoo" unless they're parameters. Additionally they need to have a let in front of them. >+ // This method returns the first value found with the given key. >+ getValue: function(aKey) { >+ for (let i = 0; i < this.keyValuePairs.length; i++) { Nit: ++i. >+ hasKey: function(aKey) { >+ for (let i = 0; i < this.keyValuePairs.length; i++) { >+ let pair = this.keyValuePairs[i]; >+ // The server handles keys as ASCII number values. >+ if (pair.key == aKey.toString()) >+ return true; We probably don't need to save this as a separate variable. >+ toArrayBuffer: function() { >+ let dataString = ""; >+ for (let i = 0; i < this.keyValuePairs.length; i++) { Nit: ++i. >+ fromArrayBuffer: function(aBuffer) { >+ let view = new DataView(aBuffer); >+ this.length = view.getUint16(8) + kPacketHeaderSize; >+ this.service = view.getUint16(10); >+ this.status = view.getUint32(12); >+ this.sessionId = view.getUint32(16); >+ >+ let dataString = ArrayBufferToString(aBuffer).substring(kPacketHeaderSize); >+ let delimitedData = dataString.split(kPacketDataDelimiter); >+ for (let i = 0; i < delimitedData.length; i += 2) { Should we explode if delimitedData.length % 2 != 0? >+ // Buddy authorization request. >+ 0xd6: function(aPacket) { >+ let authRequest = { >+ _account: this, >+ _session: this._session, >+ get account() this.imAccount, >+ userName: aPacket.getValue(4), >+ grant: function() { >+ let packet = new YahooPacket(kPacketType.BuddyAuth, 0, >+ this._session.sessionId); >+ packet.addValue(1, this._account.cleanUsername); >+ packet.addValue(5, this.userName); >+ // Misc. Unknown flags. >+ packet.addValue(13, 1); >+ packet.addValue(334, 0); >+ this._session._socket.sendBinaryData(packet.toArrayBuffer()); >+ >+ // TODO - Possibly allow different tags to be used. >+ this._account.addBuddy(Services.tags.createTag("Friends"), >+ this.userName); >+ }, >+ deny: function() { >+ let packet = new YahooPacket(kPacketType.BuddyReqReject, 0, >+ this._session.sessionId); >+ packet.addValue(1, this._account.cleanUsername); >+ packet.addValue(7, this.userName); >+ packet.addValue(14, ""); >+ this._session._socket.sendBinaryData(packet.toArrayBuffer()); >+ }, >+ cancel: function() { >+ Services.obs.notifyObservers(this, >+ "buddy-authorization-request-canceled", >+ null); >+ }, >+ QueryInterface: XPCOMUtils.generateQI([Ci.prplIBuddyRequest]) >+ }; I think I've mentioned this before, should this be in jsProtoHelper? >diff --git a/chat/protocols/yahoo/yahoo-utils.jsm b/chat/protocols/yahoo/yahoo-utils.jsm We got rid of this after this patch was added. >diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js >+ this._participants[aName] = buddy; Do we need to worry about whether aName is upper/lower case? in the IRC prpl we generally "normalize" everything before indexing into objects. This would apply to anywhere we do _participants[foo]. >+ removeParticipant: function(aName) { ... >+ let stringNickname = Cc["@mozilla.org/supports-string;1"] >+ .createInstance(Ci.nsISupportsString); Nit: one extra space. >+ this.writeMessage(this._roomName, >+ _("system.message.conferenceLogoff", aName), >+ {system: true}); Nit: Spacing is wrong. >+ // This removes the buddy locally, and from the Yahoo! servers. >+ remove: function() this._account.removeBuddy(this, true), Nit: No comma here, it's not a list. >+function YahooAccount(aProtoInstance, aImAccount) ... >+ this._converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); Isn't Ci defined? >+ this._roomsCreated = 0; Can't this be in the prototype? >+ createConversation: function(aName) { >+ let conv = new YahooConversation(this, aName); >+ this._conversations.set(aName, conv); Does aName need to be normalized at all? This goes for all accesses of _conversations. >+ receiveConferenceInvite: function(aOwner, aRoom, aParticipants, aMessage) { ... >+ this._conferences.set(aRoom, conf); Does aRoom need to be normalized at all? This goes for all accesses of _conferences. >+ // Strip @yahoo.com or @yahoo.co.jp from username. Other email domains >+ // can be left alone. >+ get cleanUsername() this.imAccount.name.replace(this._protocol.emailSuffix, >+ ""), Can't this just be this.name? >+YahooProtocol.prototype = { ... >+ commands: [ I kind of dislike having this inlined, but leave it since there's only one command. Please make sure to follow all the review comments or to reply to which ones you AREN'T following, with an explanation why. You don't need to reply saying you've directly followed any advice I've given. Thanks! Looks like we're getting close!

*** Original post on bio 1982 at 2013-07-23 15:00:23 UTC *** Comment on attachment 8354392 [details] [diff] [review] (bio-attmnt 2623) Patch 0.6 >+ onBinaryDataReceived: function(aData) { >+ let packets = this._extractPackets(aData); >+ let bytesHandled = 0; >+ for each (let packet in packets) { >+ bytesHandled += packet.length; >+ if (YahooPacketHandler.hasOwnProperty(packet.service)) { >+ try { >+ YahooPacketHandler[packet.service].call(this._account, packet); >+ } catch(e) { >+ this._account.ERROR(e); >+ } >+ } If we don't have a handler we should throw a warning saying that it was an unknown packet type. libpurple does this also, I believe. Pretty much anywhere where you check for a situation and reach an "error" state of some sort, we need to log this. This will allow us to easily look at the account log and see what happened, as well as properly having information displayed to the user.

*** Original post on bio 1982 as attmnt 2632 at 2013-07-24 20:25:00 UTC *** This patch fixes the issues pointed out in the previous review. There are a few things I didn't touch, and I will explain why. > >+ // This removes the buddy locally, and from the Yahoo! servers. > >+ remove: function() this._account.removeBuddy(this, true), > Nit: No comma here, it's not a list. A comma is there because there are other methods below. > >+ this._participants[aName] = buddy; > Do we need to worry about whether aName is upper/lower case? in the IRC prpl we > generally "normalize" everything before indexing into objects. This would apply > to anywhere we do _participants[foo]. > >+ createConversation: function(aName) { > >+ let conv = new YahooConversation(this, aName); > >+ this._conversations.set(aName, conv); > Does aName need to be normalized at all? This goes for all accesses of > _conversations. > >+ receiveConferenceInvite: function(aOwner, aRoom, aParticipants, aMessage) { > ... > >+ this._conferences.set(aRoom, conf); > Does aRoom need to be normalized at all? This goes for all accesses of > _conferences. Normalization is more than likely handled by the Yahoo pager server. More testing is needed in this area. > >+ // Buddy authorization request. > >+ 0xd6: function(aPacket) { > >+ let authRequest = { > >+ _account: this, > >+ _session: this._session, > >+ get account() this.imAccount, > >+ userName: aPacket.getValue(4), > >+ grant: function() { > >+ let packet = new YahooPacket(kPacketType.BuddyAuth, 0, > >+ this._session.sessionId); > >+ packet.addValue(1, this._account.cleanUsername); > >+ packet.addValue(5, this.userName); > >+ // Misc. Unknown flags. > >+ packet.addValue(13, 1); > >+ packet.addValue(334, 0); > >+ this._session._socket.sendBinaryData(packet.toArrayBuffer()); > >+ > >+ // TODO - Possibly allow different tags to be used. > >+ this._account.addBuddy(Services.tags.createTag("Friends"), > >+ this.userName); > >+ }, > >+ deny: function() { > >+ let packet = new YahooPacket(kPacketType.BuddyReqReject, 0, > >+ this._session.sessionId); > >+ packet.addValue(1, this._account.cleanUsername); > >+ packet.addValue(7, this.userName); > >+ packet.addValue(14, ""); > >+ this._session._socket.sendBinaryData(packet.toArrayBuffer()); > >+ }, > >+ cancel: function() { > >+ Services.obs.notifyObservers(this, > >+ "buddy-authorization-request-canceled", > >+ null); > >+ }, > >+ QueryInterface: XPCOMUtils.generateQI([Ci.prplIBuddyRequest]) > >+ }; > I think I've mentioned this before, should this be in jsProtoHelper? I plan on improving jsProtoHelper code later on in development. I will work on this then.

Comment on attachment 8354392 [details] [diff] [review] Patch 0.6 *** Original change on bio 1982 attmnt 2623 at 2013-07-24 20:25:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354401 [details] [diff] [review] Patch 0.7 *** Original change on bio 1982 attmnt 2632 at 2013-07-25 12:01:48 UTC *** >diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties >+login.error.missingCredentials=Username or password wasn't provided. >+login.error.stripSuffix=Username contains @yahoo.com suffix. These are now unused. >diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm >+ setStatus: function(aStatus, aMessage) { >+ let packet = new YahooPacket(kPacketType.StatusUpdate, 0, this.sessionId); >+ // When a custom status message is used, key 10 is set to 99, key 97 is set >+ // to 1, and key 47 is used to determine the actual status. If no custom >+ // message is used, we set key 10 to the correct status code. Maybe I'm super thick, but this code still doesn't look like it matches this comment. I'm reading this comment as "if a custom status message, then set keys 10 = 99, 97 = 1, and 47 = the message; otherwise set 10 = status code." Is this in-correct? It looks like key 47 is being set to 0 or 1, not the message or the status. What's going on here? >+ if (aMessage && aMessage.length > 0) { >+ packet.addValue(10, "99"); >+ packet.addValue(97, "1"); >+ } else { >+ let statusCode; >+ switch(aStatus) { >+ // Available >+ case Ci.imIStatusInfo.STATUS_AVAILABLE: >+ case Ci.imIStatusInfo.STATUS_MOBILE: Yahoo supports mobile, I had a contact signed on this morning that was mobile. Check libpurple's source. >+ onBinaryDataReceived: function(aData) { >+ let packets; >+ let bytesHandled; >+ try { >+ [packets, bytesHandled] = YahooPacket.extractPackets(aData); >+ } catch(e) { >+ this._account.ERROR(e); >+ this.onSessionError(Ci.prplIAccount.NETWORK_ERROR, ""); >+ return 0; >+ } >+ >+ for each (let packet in packets) { >+ if (YahooPacketHandler.hasOwnProperty(packet.service)) { >+ try { >+ YahooPacketHandler[packet.service].call(this._account, packet); >+ } catch(e) { >+ this._account.ERROR(e); >+ } >+ } You seem to have missed the comment where I asked you to throw a warning if there is no handler for the packet. >+/* The YahooPacket class represents a single Yahoo! Messenger data packet. >+ * Using this class allows you to easily create packets, stuff them with >+ * required data, and convert them to/from ArrayBuffer objects. */ >+function YahooPacket(aService, aStatus, aSessionId) >+YahooPacket.extractPackets = function(aData, aOnNetworkError) { I think this should go after the prototype. >+YahooPacket.prototype = { >+ // This method returns all of the values found with the given key. In some >+ // packets, one key is associated with multiple values. If that is the case, >+ // use this method to retrieve all of them instead of just the first one. >+ getValues: function(aKey) { >+ let values = []; >+ for (let i = 0; i < this.keyValuePairs.length; i++) { ++i >+ // If we don't have an even number of delimitedData elements, that means >+ // we are either missing a key or a value. >+ if (delimitedData.length % 2 != 0) >+ throw "Odd number of data elements. Either a key or value is missing. " >+ "Num of elements: " + delimitedData.length; { } around a multiline statement. >diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js (In reply to comment #16) > Normalization is more than likely handled by the Yahoo pager server. More > testing is needed in this area. I highly doubt this, what makes you say this? We'll worry about this in a follow-up.

*** Original post on bio 1982 as attmnt 2643 at 2013-07-25 20:02:00 UTC *** > >+ if (aMessage && aMessage.length > 0) { > >+ packet.addValue(10, "99"); > >+ packet.addValue(97, "1"); > >+ } else { > >+ let statusCode; > >+ switch(aStatus) { > >+ // Available > >+ case Ci.imIStatusInfo.STATUS_AVAILABLE: > >+ case Ci.imIStatusInfo.STATUS_MOBILE: > Yahoo supports mobile, I had a contact signed on this morning that was mobile. > Check libpurple's source. As per our discussion in IRC, we will worry about this in a follow up bug.

Comment on attachment 8354401 [details] [diff] [review] Patch 0.7 *** Original change on bio 1982 attmnt 2632 at 2013-07-25 20:02:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354412 [details] [diff] [review] Patch 0.8 *** Original change on bio 1982 attmnt 2643 at 2013-07-26 10:41:01 UTC *** >diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm >+Cu.import("resource:///modules/ArrayBufferUtils.jsm"); >+Cu.import("resource:///modules/http.jsm"); >+Cu.import("resource:///modules/imServices.jsm"); >+Cu.import("resource:///modules/imXPCOMUtils.jsm"); >+Cu.import("resource:///modules/socket.jsm"); >+Cu.import("resource:///modules/yahoo-socket.jsm"); You still never removed yahoo-socket. I'm removing this before committing.

*** Original post on bio 1982 at 2013-07-26 10:43:19 UTC *** http://hg.instantbird.org/instantbird/rev/8cdc81c96b7d Please file follow ups for other issues and mark them blocking this.