953999 - Skype protocol plugin

Status: RESOLVED FIXED

(Chat Core :: Skype, enhancement)

Description

[Benedikt Pfeifer]

*** Original post on bio 563 at 2010-10-26 17:56:00 UTC ***

Even though that Skype requires to be running in background at the moment, it would be nice to have support for it since it would allow you to have all your chat logs in one place (i.e. Instantbird).

The first problem could be solved once SkypeKit is available. For information see:
>  http://blogs.skype.com/developer/2010/06/skypekit_beta.html

At the moment we would need to use the normal Skype API:

General information:
>  http://developer.skype.com/accessories

API Documentation:
>  http://developer.skype.com/resources/public_api_ref.zip

A Windows based tool that offers a command line to interactively work with the 
API (textbased as it seems):
>  http://developer.skype.com/resources/Tracer.exe

Skype Developer Community, public forums:
>  http://forum.skype.com/index.php?showforum=16&s=8c74b03ea134b41b613b585d1ccdee2c


Here's an implementation for Pidgin:
>  http://eion.robbmob.com/
>  http://code.google.com/p/skype4pidgin/
>  http://code.google.com/p/skype4pidgin/source/browse/trunk

Misc: original smilies of Skype, among others smilies available from this page:
>  http://wiki.andreineculau.com/Original_Smileys_Theme_for_Pidgin

Comment 1

[Benedikt Pfeifer]

*** Original post on bio 563 at 2010-10-26 17:58:51 UTC ***

It could be a great example for javascript based protocols and how to use js-ctypes if this could be implemented using the above mentioned techniques.

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 563 at 2010-10-26 18:37:06 UTC ***

Unfortunately there doesn't seem to be a multi-platform way to do this:
"Communication Layer provides a mechanism for external application to communicate with Skype. This layer is platform-d[e]pendant - a transport mechanism to exchange data with Skype is different on Windows, Linux and Mac operating systems."

Linux uses X11/DBus and Mac uses Cocoa, so not sure we can do that via JavaScript. I'm not sure of Windows communication layer.

Also some of the examples in seem to link to a webpage that cannot be accessed without SkypeKit clearance, which isn't very useful.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 563 at 2010-11-17 04:20:49 UTC ***

Some sample code (in C#) about connecting to Skype on Windows: http://www.codeproject.com/KB/cs/skypecontrolapicsharp.aspx

Comment 4

[Patrick Cloke [:clokep]]

*** Original post on bio 563 at 2011-01-25 19:47:24 UTC ***

Just for completeness I'll include this. It contains information about the Skype protocol and reverse-engineering it:
http://www1.cs.columbia.edu/~salman/skype/index.html

Comment 5

[Instantbot]

*** Original post on bio 563 by OLLI_S <oliver AT sahr-net.com> at 2011-12-06 20:30:11 UTC ***

Hello,

I support this feature request.
It would be cool to see Skype contacts in Instantbird.

By the way: Skypekit is published.
See here: http://developer.skype.com/public/skypekit

Greetings

OLLI

Comment 6

[Patrick Cloke [:clokep]]

*** Original post on bio 563 at 2011-12-06 20:57:25 UTC ***

(In reply to comment #5)
> I support this feature request.
> It would be cool to see Skype contacts in Instantbird.
I agree.

> By the way: Skypekit is published.
> See here: http://developer.skype.com/public/skypekit
Yes, I (we?) checked it out when it was released; but the SDK does not agree with our license (at least it's not GPL compatible, I'm not sure about MPL compatible), there's also something about redistributing binaries in there (and a non-competition clause I think). So I'm not sure how feasible it really is. (Also they want $10.00 to have the SDK and you need API keys and bleh.) Hopefully they'll open things up a bit more at some point!

I've done a bit of work on integrating the skype4pidgin code though, although it requires running Skype too. :( Hopefully I'll finish this up soon!

Comment 7

[Instantbot]

*** Original post on bio 563 by OLLI_S <oliver AT sahr-net.com> at 2011-12-06 21:11:42 UTC ***

(In reply to comment #6)
> I've done a bit of work on integrating the skype4pidgin code though, although
> it requires running Skype too. :( Hopefully I'll finish this up soon!

Hello,

this sounds fantastic.
Having to run Skype in the background is no problem (of cause a fill integration would be much better).

Greetings

OLLI

Comment 9

[Patrick Cloke [:clokep]]

I'm close to closing this bug as WONTFIX:
- Skype no longer supports the SkypeKit SDK [1]
- Skype no longer supports the Skype Desktop API [2][3]

Before fully closing this I want to check with the author of skype4pidgin [4] about whether it still works or not.

[1] https://support.skype.com/en/faq/FA12322/what-is-skypekit
[2] https://support.skype.com/en/faq/FA214/what-is-the-desktop-api
[3] https://support.skype.com/en/faq/FA12349/skype-says-my-application-will-stop-working-with-skype-in-december-2013-why-is-that
[4] http://code.google.com/p/skype4pidgin/

Comment 10

[Patrick Cloke [:clokep]]

Eion Robb told me on IRC that this still works for Linux, at least. I'll keep this open for now.

Comment 11

[Patrick Cloke [:clokep]]

There seems to be no reasonable way to do this right now. It's not a matter of just figuring out how to do the work. If there becomes a reasonable way to add Skype support we should look into that and reopen this.

Comment 12

[Patrick Cloke [:clokep]]

Attachment: WIP v1

Skype has added a web API per http://blogs.skype.com/2014/11/14/please-welcome-skype-for-web-beta/, it's in closed beta, but...you just need to set a field properly and it lets you in.

This WIP connects, downloads the contact list and requests status for the buddies and sends/receive messages from individual conversations. I'm sure there is a lot of error checking missing but I want some people to bash on this and see what breaks for them. (And give some feedback on the code.)

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 8534783 [details] [diff] [review]
WIP v1

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

I'd appreciate some feedback on this as I continue down the path. Note that I've already added a few more features (profiles, e.g. real names and buddy icons, are downloaded now, but I haven't uploaded a new patch yet).

Comment 14

[Patrick Cloke [:clokep]]

Attachment: Patch v1

I believe that this is enough of the protocol to land, although we'd want some way to pref this off first. (Let's consider that a review comment, but please don't let that stop you from reviewing the rest of the code.)

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8547221 [details] [diff] [review]
Patch v1

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

Thanks for working on this! :-)

I won't pretend I reviewed this, but here are some drive-by comments I had after opening the attachment out of curiosity:

::: chat/locales/en-US/skype.properties
@@ +6,5 @@
> +authenticating=Authenticating
> +registrationToken=Getting registration token
> +
> +error.pie=Failed getting pie
> +error.etm=Failed getting etm

These 2 strings are neither user friendly not localizer friendly.

::: chat/modules/BigInteger.js
@@ +23,5 @@
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * For more information, please refer to <http://unlicense.org>
> + */
> +

Why are you not adding a const EXPORTED_SYMBOLS = ["bigInt"]; here?

::: chat/protocols/skype/moz.build
@@ +1,1 @@
> +# vim: set filetype=python:

I don't think we have this mode line in other chat/ moz.build files.

@@ +9,5 @@
> +    'skype.js',
> +    'skype.manifest',
> +]
> +
> +#EXTRA_JS_MODULES += []

remove :)

::: chat/protocols/skype/skype.js
@@ +185,5 @@
> +
> +  let sha256Parts = [];
> +  let newHashParts = [];
> +  for (let i = 0; i < 4; ++i) {
> +      // Ensure little-endianness is used.

nit: Fix the indent

@@ +260,5 @@
> +  };
> +  let sign;
> +  let timezone = new Date().getTimezoneOffset() * (-1);
> +  if (timezone >= 0)
> +      sign = "+";

fix indent.

@@ +318,5 @@
> +    let options = {
> +      onLoad: this._onPieResponse.bind(this),
> +      onError: this._onHttpFailure("error.pie"),
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

Should this logger object be created when connecting the account, and saved on the account to avoid 2 closure creation per HTTP request? I'm not sure how frequently we need to send http requests, if it's just 2 requests for the whole lifetime of the account connection, that probably doesn't save much.

@@ +332,5 @@
> +    return (function(aError, aResponse, aXhr) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _(aErrorStr))
> +      this.reportDisconnected();
> +    }).bind(this);

nit: This looks like a place where => could look better than the bind call.

Comment 16

[aleth [:aleth]]

Comment on attachment 8547221 [details] [diff] [review]
Patch v1

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

First pass - lots of nits!

::: chat/locales/en-US/skype.properties
@@ +9,5 @@
> +error.pie=Failed getting pie
> +error.etm=Failed getting etm
> +error.token=Failed getting Skype Token
> +error.registrationToken=Failed getting Registration Token
> +error.subscription=Subscription failed

Maybe we could improve the wording of this a bit too.

::: chat/modules/BigInteger.js
@@ +23,5 @@
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * For more information, please refer to <http://unlicense.org>
> + */
> +

nit: Shouldn't this file be called BigInteger.jsm?

@@ +25,5 @@
> + * For more information, please refer to <http://unlicense.org>
> + */
> +
> +"use strict";
> +var bigInt = (function () {

Do we still need this function wrapper in a module?

@@ +682,5 @@
> +})();
> +
> +if (typeof module !== "undefined") {
> +    module.exports = bigInt;
> +}

Not sure what this does, but I'm pretty sure we don't need it?

::: chat/protocols/skype/skype.js
@@ +9,5 @@
> +Cu.import("resource:///modules/imServices.jsm");
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource:///modules/jsProtoHelper.jsm");
> +
> +Services.scriptloader.loadSubScript("resource:///modules/BigInteger.js", this);

Cu.import please

@@ +15,5 @@
> +const kMaxMsgRetry = 2;
> +
> +const kLockAndKeyAppId = "[e-mail]";
> +const kLockAndKeySecret = "Q1P7W2E4J9R8U3S5";
> +const kClientId = "578134";

All these consts need comments.

@@ +44,5 @@
> + *
> + * Note that some contacts might have a /1: in them (instead of a /8:), that's
> + * for MSN linked contacts.
> + */
> +function contactUrlToName(aUrl) {

Should this be a private method of the account?
Can't it use your extractString helper function below?

@@ +45,5 @@
> + * Note that some contacts might have a /1: in them (instead of a /8:), that's
> + * for MSN linked contacts.
> + */
> +function contactUrlToName(aUrl) {
> +  let start = aUrl.indexOf("/8:");

What about /1: ? Probably the comment should explain more clearly why we ignore those contacts.
"8:" is also used below, should it be in a constant?

@@ +53,5 @@
> +  let end = aUrl.indexOf("/", start + "/8:".length);
> +  if (end == -1)
> +    end = undefined;
> +
> +  return aUrl.slice(start + "/8:".length, end);

Duplication of "/8:".length

@@ +57,5 @@
> +  return aUrl.slice(start + "/8:".length, end);
> +}
> +
> +function SkypeConversation(aAccount, aName) {
> +  this.buddy = aAccount._buddies.get(aName);

Assumes nobody who isn't a buddy talks to us, is this true?

@@ +87,5 @@
> +      },
> +      onError: this._account._onHttpFailure("error.message"),
> +      postData: JSON.stringify(message),
> +      logger: {log: this._account.LOG.bind(this),
> +               debug: this._account.DEBUG.bind(this)}

Duplication, see flo's comment below.

@@ +115,5 @@
> +    if (!this._info)
> +      return EmptyEnumerator;
> +
> +    let tooltipInfo = [];
> +    for (let info in this._info) {

Not sure what sets the property names, but a Map would be safer.

@@ +125,5 @@
> +      tooltipInfo.push(new TooltipInfo(info, this._info[info]));
> +    }
> +    if (this.mood)
> +      tooltipInfo.push(new TooltipInfo("Mood", this.mood));
> +

THe tooltipInfo should also contain the status and status message (cf the IRC implementation). Add a TODO at least...

@@ +157,5 @@
> + * A magic method (originally from MSN) to stop 3rd parties from connecting.
> + * Differs from MSN by swapping MD5 for SHA256.
> + *
> + * A pre-emptive apology is necessary for those about to embark on the journey
> + * of understanding this code. I wish you luck and God's speed.

I'm not going to pretend I understand this function.

@@ +159,5 @@
> + *
> + * A pre-emptive apology is necessary for those about to embark on the journey
> + * of understanding this code. I wish you luck and God's speed.
> + */
> +function magicSha256(aInput) {

as above, should this be a method of the account or protocol?

@@ +196,5 @@
> +  // Make a new string and pad with '0' to a length that's a multiple of 8.
> +  let buf = aInput + productId;
> +  let len = buf.length;
> +  if ((len % 8) != 0) {
> +    let fix = 8 - (len % 8);

(len % 8) is duplicated.

@@ +202,5 @@
> +    len += fix;
> +  }
> +
> +  // Split into integers.
> +  let chlStringParts = StringToArrayBuffer(buf);

chl = ?

@@ +203,5 @@
> +  }
> +
> +  // Split into integers.
> +  let chlStringParts = StringToArrayBuffer(buf);
> +  view = new DataView(chlStringParts);

Lets get rid of the problem by combining these two lines to |view = new DataView(StringToArrayBuffer(buf));|

@@ +209,5 @@
> +  // This is magic.
> +  let nHigh = bigInt(0);
> +  let nLow = bigInt(0);
> +  for (let i = 0; i < (len / 4); i += 2) {
> +    // TODO This is long long in C, will this work?

I guess you know if it works now?

@@ +231,5 @@
> +
> +  // Make a string of the parts and convert to hexadecimal.
> +  let output = "";
> +  for (let i = 0; i < 4; ++i) {
> +    let temp = newHashParts[i];

Is "part" a better variable name?

@@ +239,5 @@
> +
> +    // JavaScript likes to use signed numbers, force this to give us the
> +    // unsigned representation.
> +    if (temp < 0)
> +      temp += 0xFFFFFFFF + 1;

fun!

@@ +241,5 @@
> +    // unsigned representation.
> +    if (temp < 0)
> +      temp += 0xFFFFFFFF + 1;
> +
> +    temp = temp.toString(16);

I'd prefer we use a different variable for the string and for the bigInt.

@@ +249,5 @@
> +
> +  return output;
> +}
> +
> +// This function is copied from the returned response.

I don't understand this comment. I guess it means "I didn't write this"?

@@ +251,5 @@
> +}
> +
> +// This function is copied from the returned response.
> +function getTimezone() {
> +  function pad(n, c) {

Can we use aSomethingIntelligible? Also please add a comment with an example or something.

@@ +252,5 @@
> +
> +// This function is copied from the returned response.
> +function getTimezone() {
> +  function pad(n, c) {
> +    if ((n = n + "").length < c)

Using n.toString and putting the length of the resulting string in a temporary variable is clearer and avoids duplication.

@@ +253,5 @@
> +// This function is copied from the returned response.
> +function getTimezone() {
> +  function pad(n, c) {
> +    if ((n = n + "").length < c)
> +      return new Array(++c - n.length).join("0") + n;

This reads like an obfuscated code contest entry. Isn't it just "0".repeat(++c - n.length - 1) + n ? And that's still not pretty...

@@ +255,5 @@
> +  function pad(n, c) {
> +    if ((n = n + "").length < c)
> +      return new Array(++c - n.length).join("0") + n;
> +    else
> +      return n;

This function reads like it wants to be Haskell ;)

@@ +258,5 @@
> +    else
> +      return n;
> +  };
> +  let sign;
> +  let timezone = new Date().getTimezoneOffset() * (-1);

Date.now()

@@ +276,5 @@
> +
> +function SkypeAccount(aProtoInstance, aImAccount) {
> +  this._init(aProtoInstance, aImAccount);
> +
> +  // Initialize some maps.

I don't think we really need this comment.

@@ +312,5 @@
> +    this.reportConnecting(_("connecting"));
> +
> +    this.LOG("STARTING Login");
> +
> +    // Perform the request to get the pie/etm values.

What are pie/etm values? ;)

@@ +330,5 @@
> +   */
> +  _onHttpFailure: function(aErrorStr) {
> +    return (function(aError, aResponse, aXhr) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _(aErrorStr))

Missing ;. Also, does this automatically log the error string to the debug log too?

@@ +346,5 @@
> +    // Parse the pie/etm and do the actual login.
> +    let loginUrl = "https://" + kLoginHost + "/login";
> +
> +    let params = [["client_id", "578134"],
> +                  ["redirect_uri", "https://web.skype.com"]];

Aren't these in consts at the top of the file?

@@ +358,5 @@
> +    let pie = extractString(aResponse, "=\"pie\" value=\"", "\"");
> +    if (!pie) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _("error.pie"))
> +      this.reportDisconnected();

Duplicates code from _onHttpFailure, seems worth abstracting as it appears a couple of times.

@@ +365,5 @@
> +    let etm = extractString(aResponse, "=\"etm\" value=\"", "\"");
> +    if (!etm) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _("error.etm"))
> +      this.reportDisconnected();

same here.

@@ +387,5 @@
> +                // there are important headers that need to be plucked before
> +                // the redirect happens.
> +                ["BehaviorOverride", "redirectAs404"]],
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

See above.

@@ +398,5 @@
> +    let refreshToken = extractString(aResponse, "=\"skypetoken\" value=\"", "\"");
> +    if (!refreshToken) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _("error.token"))
> +      this.reportDisconnected();

See above.

@@ +410,5 @@
> +    if (this._registrationToken) {
> +      this._subscribe();
> +
> +      // Do some stuff to start some stuff.
> +      return;

Really does lots of stuff?

@@ +423,5 @@
> +    let response = magicSha256(curTime);
> +    let options = {
> +      onLoad: this._onRegistrationTokenReceived.bind(this),
> +      onError: this._onHttpFailure("error.registrationToken"),
> +      postData: "{}", // Empty JSON object.

nit: JSON.Stringify({}) would document itself?

@@ +433,5 @@
> +                ["BehaviorOverride", "redirectAs404"],
> +                ["LockAndKey", "appId=" + kLockAndKeyAppId +
> +                               "; time=" + curTime +
> +                               "; lockAndKeyResponse=" + response],
> +                ["ClientInfo", "os=Windows; osVer=8.1; proc=Win32; lcid=en-us; deviceType=1; country=n/a; clientName=swx-skype.com; clientVer=908/1.0.0.20"],

nit: split into multiple lines using +

@@ +436,5 @@
> +                               "; lockAndKeyResponse=" + response],
> +                ["ClientInfo", "os=Windows; osVer=8.1; proc=Win32; lcid=en-us; deviceType=1; country=n/a; clientName=swx-skype.com; clientVer=908/1.0.0.20"],
> +                ["Authentication", "skypetoken=" + this._skypeToken]],
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

ditto

@@ +441,5 @@
> +    }
> +    httpRequest(messagesUrl, options);
> +  },
> +  _onRegistrationTokenReceived: function(aResponse, aXhr) {
> +    this.LOG("Reg token received: " + aResponse);

nit: can we use "Registration token" everywhere?

@@ +443,5 @@
> +  },
> +  _onRegistrationTokenReceived: function(aResponse, aXhr) {
> +    this.LOG("Reg token received: " + aResponse);
> +
> +    // registration_token = skypeweb_string_get_chunk(url_text, len, "Set-RegistrationToken: ", ";");

Can this be removed?

@@ +449,5 @@
> +    this.LOG("regToken: " + registrationToken);
> +    if (!registrationToken) {
> +      this.reportDisconnecting(Ci.prplIAccount.ERROR_AUTHENTICATION_FAILED,
> +                               _("error.registrationToken"))
> +      this.reportDisconnected();

and another one...

@@ +461,5 @@
> +  // Subscribe to the events we want to see.
> +  _subscribe: function() {
> +    this.LOG("Sending subscription.");
> +
> +    // Request the registration token.

umm, no.

@@ +477,5 @@
> +      onLoad: this._onSubscription.bind(this),
> +      onError: this._onHttpFailure("error.subscription"),
> +      postData: JSON.stringify(subscriptions),
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

...

@@ +506,5 @@
> +  _onContactsList: function(aResponse, aXhr) {
> +    this.LOG("Contacts list: " + aResponse);
> +
> +    let obj = JSON.parse(aResponse);
> +    if (!obj) {

if (!obj || (obj isn't what we expect it to be) } ...

@@ +511,5 @@
> +      this.ERROR("Unable to parse JSON response: " + aResponse);
> +      return;
> +    }
> +
> +    for (let buddyJson of obj) {

why is this called buddyJson? It seems to be an object, not json.

@@ +524,5 @@
> +        // Notify the UI of the buddy.
> +        Services.contacts.accountBuddyAdded(buddy);
> +      }
> +
> +      // TODO There's also fullname.

Seems you handle this?

@@ +526,5 @@
> +      }
> +
> +      // TODO There's also fullname.
> +      if (buddyJson.display_name)
> +        buddy.displayName = buddyJson.display_name;

I'm not sure you're meant to set the displayName directly on an accountbuddy?

@@ +528,5 @@
> +      // TODO There's also fullname.
> +      if (buddyJson.display_name)
> +        buddy.displayName = buddyJson.display_name;
> +      if (buddyJson.fullname)
> +        buddy.serverAlias = buddyJson.fullname;

This needs comments. What's the difference between display_name, skypename, and fullname?
NB The serverAlias setter will send a displayname-changed notification.

@@ +533,5 @@
> +      // Store the buddy info into the object for tooltips.
> +      buddy._info = buddyJson;
> +
> +      // Set the buddy's status to offline until we get an update.
> +      buddy.setStatus(Ci.imIStatusInfo.STATUS_OFFLINE, "");

should it be UNKNOWN?

@@ +537,5 @@
> +      buddy.setStatus(Ci.imIStatusInfo.STATUS_OFFLINE, "");
> +    }
> +
> +    if (!obj.length)
> +      return;

Move this above the loop.

@@ +542,5 @@
> +
> +    // Download profiles.
> +    let profilesUrl = "https://" + kContactsHost + "/users/self/contacts/profiles";
> +    let options = {
> +      postData: obj.map((b) => ["contacts[]", b.skypename]),

no JSON?

@@ +546,5 @@
> +      postData: obj.map((b) => ["contacts[]", b.skypename]),
> +      onLoad: this._onProfiles.bind(this),
> +      onError: this._onHttpError.bind(this),
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

...

@@ +561,5 @@
> +      onLoad: (aResponse, aXhr) =>
> +        this.LOG("Successfully subscribed to contacts."),
> +      onError: this._onHttpError.bind(this),
> +      logger: {log: this.LOG.bind(this),
> +               debug: this.DEBUG.bind(this)}

...
Hmm, maybe onError is the same in each of these too, so maybe it would even make sense to have a helper method that generates the options object?
Something like _options({"contacts": contacts}, aOnLoadFunction, "error.whatever", ...) Just something to consider.
Or maybe a defaultOptions that gets extended with Object.create?

@@ +569,5 @@
> +
> +  _onProfiles: function(aResponse, aXhr) {
> +    this.LOG("Profiles: " + aResponse);
> +
> +    let contacts = JSON.parse(aResponse);

no failure check here?

@@ +571,5 @@
> +    this.LOG("Profiles: " + aResponse);
> +
> +    let contacts = JSON.parse(aResponse);
> +
> +    for (let contact of contacts) {

maybe s/contact/skypeContact to avoid confusion with IB contacts.

@@ +579,5 @@
> +      if (!buddy)
> +        continue;
> +
> +      // Set some properties on the buddy.
> +      buddy.displayName = contact.displayname;

See question above.

@@ +594,5 @@
> +    }
> +  },
> +
> +  /*
> +   * Download the actual messages, this will recurse through it's callback.

it's -> its

@@ +611,5 @@
> +
> +  _onMessages: function(aResponse, aXhr) {
> +    this.LOG("Messages: " + aResponse);
> +
> +    // Poll.

Please expand this comment

@@ +613,5 @@
> +    this.LOG("Messages: " + aResponse);
> +
> +    // Poll.
> +    this._request = null;
> +    this._poller = setTimeout(this._getMessages.bind(this), 1000);

Would it make sense to use the socket sendPing mechanism for this? Especially as empty responses are pongs.

@@ +643,5 @@
> +        let clientMessageId = resource.clientmessageid;
> +        let messageType = resource.messagetype;
> +        let from = contactUrlToName(resource.from);
> +        let composeTime = resource.composetime;
> +        let conversationLink = resource.conversationLink;

Please check you're actually using all of these more than once, and maybe move the let to where it's first needed.

@@ +646,5 @@
> +        let composeTime = resource.composetime;
> +        let conversationLink = resource.conversationLink;
> +
> +        if (!from)
> +          return;

Should this this.WARN?

@@ +658,5 @@
> +
> +        // Get or create the conversation.
> +        let conversationName = contactUrlToName(conversationLink);
> +        let conv = this._conversations.get(conversationName);
> +        // TODO This will create a conversation even for typing state updates.

Just move the following if clause further down?

@@ +673,5 @@
> +            typingState = Ci.prplIConvIM.NOT_TYPING;
> +          if (typingState !== null)
> +            conv.updateTyping(typingState);
> +          // TODO There doesn't seem to be a "typed" state.
> +        } else if (messageType == "RichText" || messageType == "Text") {

are we using K-R } else { style in this file?

@@ +686,5 @@
> +          conv.writeMessage(from, resource.content, options);
> +        }
> +
> +      } else if (resourceType == "UserPresence") {
> +        let selfLink = resource.selfLink;

only used once.

@@ +687,5 @@
> +        }
> +
> +      } else if (resourceType == "UserPresence") {
> +        let selfLink = resource.selfLink;
> +        let status = this.mapStatusString(resource.status);

move further down to where it's used.

@@ +698,5 @@
> +        // Get the buddy and update the status.
> +        let buddy = this._buddies.get(from);
> +        if (buddy)
> +          buddy.setStatus(status, "");
> +      } else if (resourceType == "EndpointPresence") {

Please fix indentation etc

@@ +744,5 @@
> +    headers = headers.concat([
> +      ["RegistrationToken", this._registrationToken],
> +      ["Referer", "https://web.skype.com/main"],
> +      ["Accept", "application/json; ver=1.0;"],
> +      ["ClientInfo", "os=Windows; osVer=8.1; proc=Win32; lcid=en-us; deviceType=1; country=n/a; clientName=swx-skype.com; clientVer=908/1.0.0.20"],

line length...

@@ +763,5 @@
> +
> +    this._request = null;
> +    this._poller = null;
> +
> +    // TODO Clean up conversations.

What needs to be done here?

@@ +779,5 @@
> +  // TODO
> +  remove: function() {},
> +
> +  // TODO
> +  unInit: function() {},

I think you should fill in remove and unInit before landing this.

@@ +787,5 @@
> +    this._conversations.set(aName, conv);
> +    return conv;
> +  },
> +
> +  deleteConversation: function(aName) {},

Does this method even exist?

@@ +794,5 @@
> +  addBuddy: function(aTag, aName) {},
> +
> +  loadBuddy: function(aBuddy, aTag) {
> +    let buddy = new SkypeAccountBuddy(this, aBuddy, aTag);
> +    this._buddies.set(buddy.userName, buddy);

Is there something left to do here like e.g. ensure they are on the server-side roster and subscribe to presence?

Comment 17

[Patrick Cloke [:clokep]]

Attachment: Patch v2

I've replied to the parts that I felt warranted replying to. Other comments I likely just took care of.

(Also, thank you both for taking a look at this!)

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> ::: chat/protocols/skype/moz.build
> @@ +1,1 @@
> > +# vim: set filetype=python:
>
> I don't think we have this mode line in other chat/ moz.build files.

We seem to have it in every file I've checked...

(In reply to aleth [:aleth] from comment #16)
> @@ +44,5 @@
> > + *
> > + * Note that some contacts might have a /1: in them (instead of a /8:), that's
> > + * for MSN linked contacts.
> > + */
> > +function contactUrlToName(aUrl) {
>
> Should this be a private method of the account?

I don't think so, it doesn't need the account object at all. So it seems to make more sense (to me) to be a separate method.

> Can't it use your extractString helper function below?

They're not really the same, extractString requires an end string (which could be made optional, I suppose); contactUrlToName lazily tries to find an end to the string or just returns the rest of the URL.

> @@ +45,5 @@
> > + * Note that some contacts might have a /1: in them (instead of a /8:), that's
> > + * for MSN linked contacts.
> > + */
> > +function contactUrlToName(aUrl) {
> > +  let start = aUrl.indexOf("/8:");
>
> What about /1: ? Probably the comment should explain more clearly why we
> ignore those contacts.

We ignore them because they're MSN linked contacts, not Skype contacts.

> "8:" is also used below, should it be in a constant?

I don't think this would be a useful constant.

> @@ +57,5 @@
> > +function SkypeConversation(aAccount, aName) {
> > +  this.buddy = aAccount._buddies.get(aName);
>
> Assumes nobody who isn't a buddy talks to us, is this true?

I don't see this assumption, it will just set buddy to null if we receive something from a non-buddy. This is the same as the IRC protocol.

> @@ +115,5 @@
> > +    let tooltipInfo = [];
> > +    for (let info in this._info) {
>
> Not sure what sets the property names, but a Map would be safer.

I'm not really sure what this is referring to? this._info is the parsed JSON response.

> @@ +125,5 @@
> THe tooltipInfo should also contain the status and status message (cf the
> IRC implementation). Add a TODO at least...

We have to add those manaully? That's lame? (I added a TODO for now.)

> @@ +159,5 @@
> > + *
> > + * A pre-emptive apology is necessary for those about to embark on the journey
> > + * of understanding this code. I wish you luck and God's speed.
> > + */
> > +function magicSha256(aInput) {
>
> as above, should this be a method of the account or protocol?

I don't really see why it should be, it needs no context.

> Also please add a comment with an example
> or something.

There should likely be tests. I'll make a comment to add them (but I'm not going to add them right this second).

> @@ +258,5 @@
> > +    else
> > +      return n;
> > +  };
> > +  let sign;
> > +  let timezone = new Date().getTimezoneOffset() * (-1);
>
> Date.now()

Why? How will this help?

> @@ +506,5 @@
> > +  _onContactsList: function(aResponse, aXhr) {
> > +    this.LOG("Contacts list: " + aResponse);
> > +
> > +    let obj = JSON.parse(aResponse);
> > +    if (!obj) {
>
> if (!obj || (obj isn't what we expect it to be) } ...

This error checking is pretty hard (and I think we do a bad job of it in e.g. Twitter).

> @@ +526,5 @@
> > +      }
> > +
> > +      // TODO There's also fullname.
> > +      if (buddyJson.display_name)
> > +        buddy.displayName = buddyJson.display_name;
>
> I'm not sure you're meant to set the displayName directly on an accountbuddy?

> @@ +533,5 @@
> > +      // Store the buddy info into the object for tooltips.
> > +      buddy._info = buddyJson;
> > +
> > +      // Set the buddy's status to offline until we get an update.
> > +      buddy.setStatus(Ci.imIStatusInfo.STATUS_OFFLINE, "");
>
> should it be UNKNOWN?

We don't get an "offline" response, I don't think.

> @@ +561,5 @@
> > +      onLoad: (aResponse, aXhr) =>
> > +        this.LOG("Successfully subscribed to contacts."),
> > +      onError: this._onHttpError.bind(this),
> > +      logger: {log: this.LOG.bind(this),
> > +               debug: this.DEBUG.bind(this)}
>
> ...
> Hmm, maybe onError is the same in each of these too, so maybe it would even
> make sense to have a helper method that generates the options object?
> Something like _options({"contacts": contacts}, aOnLoadFunction,
> "error.whatever", ...) Just something to consider.
> Or maybe a defaultOptions that gets extended with Object.create?

We only use this in a few places, I'm not sure it's worth the effort.

>
> @@ +569,5 @@
> > +
> > +  _onProfiles: function(aResponse, aXhr) {
> > +    this.LOG("Profiles: " + aResponse);
> > +
> > +    let contacts = JSON.parse(aResponse);
>
> no failure check here?

I added a TODO.

> @@ +613,5 @@
> > +    this.LOG("Messages: " + aResponse);
> > +
> > +    // Poll.
> > +    this._request = null;
> > +    this._poller = setTimeout(this._getMessages.bind(this), 1000);
>
> Would it make sense to use the socket sendPing mechanism for this?
> Especially as empty responses are pongs.

I need to think about this a bit more, but at first glance it looks like it'd make sense.

> @@ +779,5 @@
> > +  // TODO
> > +  remove: function() {},
> > +
> > +  // TODO
> > +  unInit: function() {},
>
> I think you should fill in remove and unInit before landing this.

Duly noted. I need to check again what needs to go in here.

> @@ +794,5 @@
> > +  addBuddy: function(aTag, aName) {},
> > +
> > +  loadBuddy: function(aBuddy, aTag) {
> > +    let buddy = new SkypeAccountBuddy(this, aBuddy, aTag);
> > +    this._buddies.set(buddy.userName, buddy);
>
> Is there something left to do here like e.g. ensure they are on the
> server-side roster and subscribe to presence?

So addBuddy notifies the server. We probably don't want loadBuddy to do that or it would keep adding buddies back to the server on people. I don't know how we normally handle syncing though.

Comment 18

[aleth [:aleth]]

Comment on attachment 8555486 [details] [diff] [review]
Patch v2

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

Looks good! You've already pointed out the things you haven't addressed yet in your previous comment.

::: chat/protocols/skype/skype.js
@@ +128,5 @@
> +    }
> +    if (this.mood)
> +      tooltipInfo.push(new TooltipInfo("Mood", this.mood));
> +
> +    // TODO Add status / status message.

Sorry about the confusion here. This does not apply to account buddies, it's only needed for MUC participants, so you can remove this.

@@ +592,5 @@
> +      if (!buddy)
> +        continue;
> +
> +      // Set some properties on the buddy.
> +      buddy.serverAlias = skypeContact.displayname;

display_name? (or was that a bug in the previous patch?)

@@ +593,5 @@
> +        continue;
> +
> +      // Set some properties on the buddy.
> +      buddy.serverAlias = skypeContact.displayname;
> +      // TODO There's also firstname and lastname fields.

also fullname (was in the last patch at least), but maybe that's just a concatenation.

@@ +701,5 @@
> +          else
> +            options.incoming = true;
> +          conv.writeMessage(from, resource.content, options);
> +        }
> +

nit: excess blank line

Comment 19

[Patrick Cloke [:clokep]]

Attachment: Patch v3

This should meet the review comments + disables the prpl.

Comment 20

[aleth [:aleth]]

Comment on attachment 8561021 [details] [diff] [review]
Patch v3

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

Looks good! It will be great to have this in nightlies :-)

>> Would it make sense to use the socket sendPing mechanism for this?
>> Especially as empty responses are pongs.
>
>I need to think about this a bit more, but at first glance it looks like it'd make sense.

What was the outcome of the thinking?

::: chat/protocols/skype/skype.js
@@ +558,5 @@
> +      onLoad: this._onProfiles.bind(this),
> +      onError: this._onHttpError.bind(this),
> +      logger: this._logger
> +    };
> +    this.LOG(JSON.stringify(options));

How about adding the logging of options to _contactsRequest and _messagesRequest? (You don't need another review for this)

Comment 21

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #20)
> >> Would it make sense to use the socket sendPing mechanism for this?
> >> Especially as empty responses are pongs.
> >
> >I need to think about this a bit more, but at first glance it looks like it'd make sense.
> 
> What was the outcome of the thinking?

There's no socket. This isn't possible.

I also realized that I didn't do packaging, so I'll need to put up another patch.

Comment 22

[Patrick Cloke [:clokep]]

Attachment: Patch v4

This adds packaging, keeping the r+ from aleth. Florian mentioned he wants to take a look at the strings, at least.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8564613 [details] [diff] [review]
Patch v4

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

::: chat/locales/en-US/skype.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +connecting=Connecting

Do we really need this string? I thought the account manager was automatically showing it until the prpl sends a more specific string to replace it to indicate progress.

@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +connecting=Connecting
> +authenticating=Authenticating

This could do with either a prefixed string id, or a localization note to indicate where this is going to be used (I assume it's to indicate connection progress in the account manager).

@@ +7,5 @@
> +registrationToken=Getting registration token
> +
> +error.pie=Failed getting session token name
> +error.etm=Failed getting session token
> +error.token=Failed getting Skype Token

These messages are all confusing from a user perspective, and probably impossible to meaningfully localize. I think they should be debug log messages, and we should have a more generic error message to display in the account manager for connection failures.

Comment 24

[Patrick Cloke [:clokep]]

Attachment: Patch v5 (updated strings)

Florian please take a quick look at the strings. Thanks!

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8637470 [details] [diff] [review]
Patch v5 (updated strings)

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

::: chat/locales/en-US/skype.properties
@@ +6,5 @@
> +# progress during a connection.
> +connecting.authenticating=Authenticating
> +connecting.registrationToken=Getting registration token
> +
> +# LOCALIZATION NOTE: If an error occurs while

Please fix the comment here :)

Comment 26

[Patrick Cloke [:clokep]]

Landed this with updated comments.

https://hg.mozilla.org/comm-central/rev/30b50e6fa1f9

Comment 27

[Patrick Cloke [:clokep]]

Bustage fix for a broken test: https://hg.mozilla.org/comm-central/rev/dd68254da258

Comment 28

[Patrick Cloke [:clokep]]

Updating milestone from 1.6 to 42.