1071166 - Outgoing messages not escaped correctly

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Mook (work)]

Build: Thunderbird 20140922030203

STR:
1. Create a GTalk conversation
2. Type (and send) something with tag-looking things, like <defunct>
3. Read scrollback

Expected results:
Text is preserved

Actual results:
The tag-looking part is missing

Additional information:
JSON log:
{"date":"2014-09-22T17:54:17.000Z","who":"foo@bar","text":"Try `ps -Af f | grep -B10 -F '<defunct>'` ?\n","flags":["outgoing"],"alias":"mook"}

Note missing escape into &lt; / &gt; (... I have no idea if bugzilla will deal with that correctly.)

arlolra points at https://hg.mozilla.org/comm-central/rev/e2c85d70fda2#l10.24 as being possibly relevant.

Comment 1

[arlolra]

Attachment: prepare-purple.patch

Comment 2

[arlolra]

Attachment: prepare.patch

Comment 3

[Patrick Cloke [:clokep]]

So I think IRC does something similar to this by overriding writeMessage [1]. Is this the same?

[1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#286

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8493356 [details] [diff] [review]
prepare.patch

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

IRC also needs to use this per comment 3.

Comment 5

[arlolra]

Attachment: prepare.patch from comment 4

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8493462 [details] [diff] [review]
prepare.patch from comment 4

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

I think this looks pretty good, I do have a couple of questions though before giving a final review.

::: chat/components/src/imConversations.js
@@ +405,5 @@
>        if (aSubject.cancelled)
>          return;
> +      aSubject.conversation.prepareForDisplaying(aSubject);
> +      if (aSubject.cancelled)
> +        return;

So prepareForDisplaying can let a purple cancel displaying a message? Is that useful?

::: chat/protocols/xmpp/xmpp.jsm
@@ +266,5 @@
>    },
>  
> +  /* Perform entity escaping before displaying the message. */
> +  prepareForDisplaying: function(aMsg) {
> +    if (aMsg.outgoing && !aMsg.system)

Just to make sure I understand: This is only for outgoing messages because incoming messages (should) have been properly escaped already?

Comment 7

[arlolra]

Comment on attachment 8493462 [details] [diff] [review]
prepare.patch from comment 4

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

::: chat/components/src/imConversations.js
@@ +405,5 @@
>        if (aSubject.cancelled)
>          return;
> +      aSubject.conversation.prepareForDisplaying(aSubject);
> +      if (aSubject.cancelled)
> +        return;

No, should be removed. We don't let them do it in prepareForSending either.

::: chat/protocols/xmpp/xmpp.jsm
@@ +266,5 @@
>    },
>  
> +  /* Perform entity escaping before displaying the message. */
> +  prepareForDisplaying: function(aMsg) {
> +    if (aMsg.outgoing && !aMsg.system)

That's how it was being applied before it was removed in the previous patch.

It seems like other clients shouldn't rely on our escaping. And indeed, we don't rely on theres.

Comment 8

[arlolra]

Attachment: prepare.patch from comment 7

Updated based on the review.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8493355 [details] [diff] [review]
prepare-purple.patch

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

::: purplexpcom/src/purpleConvChat.h
@@ +65,5 @@
>      return purpleConversation::PrepareForSending(aMsg, aMsgCount, aMsgs);
>    }
> +  NS_IMETHOD PrepareForDisplaying(imIMessage *aMsg) {
> +    return purpleConversation::PrepareForDisplaying(aMsg);
> +  }

Can't this one and the one in purpleConvIM just inherit from purpleConversation instead of overriding it and calling the super?

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8494911 [details] [diff] [review]
prepare.patch from comment 7

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

::: chat/components/src/imConversations.js
@@ +433,5 @@
>    },
>  
> +  // Called above when the conversation service itself is set as the
> +  // conversation for a message. This is case for some system messages.
> +  prepareForDisplaying: function(aMsg) {},

I'm not sure I understand what this implies. Hopefully one of your other r? flags understands it. :)

Comment 11

[aleth [:aleth]]

Comment on attachment 8494911 [details] [diff] [review]
prepare.patch from comment 7

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

This looks good to me, but I'd like the following comments to be improved.

Also, I think you've already started, but the new message pipeline code cries out for some tests.

::: chat/components/public/prplIConversation.idl
@@ +52,5 @@
>    void prepareForSending(in imIOutgoingMessage aMsg,
>                           [optional] out unsigned long aMsgCount,
>                           [retval, array, size_is(aMsgCount)] out wstring aMsgs);
>  
> +  /* Postprocess messages before they are displayed (eg. escaping). */

As this is also documentation, please mention how this works, i.e. that the implementation can set aMessage.displayMessage, otherwise originalMessage is used.

::: chat/components/src/imConversations.js
@@ +432,5 @@
>      }
>    },
>  
> +  // Called above when the conversation service itself is set as the
> +  // conversation for a message. This is case for some system messages.

"some system messages"? Can we be more precise?

::: chat/protocols/xmpp/xmpp.jsm
@@ +266,5 @@
>    },
>  
> +  /* Perform entity escaping before displaying the message. */
> +  prepareForDisplaying: function(aMsg) {
> +    if (aMsg.outgoing && !aMsg.system)

We assume incoming messages don't need escaping, or we are doing it elsewhere? Please add a comment.

Comment 12

[arlolra]

Attachment: prepare.patch from comment 11

Improved the comments as suggested.

And yes, tests are forthcoming. Are they necessary to merge this patch or can I follow up with them?

Comment 13

[arlolra]

Attachment: prepare.patch from comment 11

Whoops, wrong patch :(

Sorry for spamming.

Comment 14

[aleth [:aleth]]

Comment on attachment 8511759 [details] [diff] [review]
prepare.patch from comment 11

I'd like flo to take another look at this too.

Comment 15

[arlolra]

Attachment: test.patch from comment 11

Adding tests for the messaging pipeline.

Comment 16

[Patrick Cloke [:clokep]]

Comment on attachment 8514041 [details] [diff] [review]
test.patch from comment 11

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

Overall this looks great! Thanks for writing these. Were the fixes in imConversations.js found as results of these tests or were they just changes you had locally or what?

::: chat/components/src/test/test_conversations.js
@@ +1,3 @@
> +/* 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/. */

We usually license tests under CC0: http://dxr.mozilla.org/comm-central/source/chat/protocols/irc/test/test_ctcpColoring.js#1

@@ +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/. */
> +
> +const { interfaces: Ci, utils: Cu } = Components;

Nit: No spaces inside of { and }. Additionally, Ci is unused so probably just const Cu = Components.utils; is enough.

@@ +6,5 @@
> +
> +let jsProtoHelper = {}, imConversations = {};
> +
> +Cu.import("resource:///modules/imServices.jsm");
> +Cu.import("resource:///modules/jsProtoHelper.jsm", jsProtoHelper);

Any particular reason to import this into an object?

@@ +13,5 @@
> +  "resource:///components/imConversations.js", imConversations
> +);
> +
> +// Fake prplMessage
> +function Message(aWho, aMessage, aObject) {

It looks to me like aWho and aObject just arbitrary in the tests below. Maybe make this constructor ONLY take a message and pass some constants for aWho and aObject. This will make it more obvious below what is being tested.

@@ +32,5 @@
> +Conversation.prototype = {
> +  __proto__: jsProtoHelper.GenericConvIMPrototype,
> +  _account: {
> +    imAccount: {
> +      protocol: { name: "Fake Protocol" },

Nit: Spaces inside of { }. Fix this everywhere.

@@ +36,5 @@
> +      protocol: { name: "Fake Protocol" },
> +      alias: "",
> +      name: "Fake Account"
> +    },
> +    ERROR: function(e) { throw e }

Is your goal just to fail the test if this is called? I usually do_check_true(false) or whatever to do that, not sure if we have a particular style we follow here.

@@ +37,5 @@
> +      alias: "",
> +      name: "Fake Account"
> +    },
> +    ERROR: function(e) { throw e }
> +  } 

Nit: Trailing space.

@@ +40,5 @@
> +    ERROR: function(e) { throw e }
> +  } 
> +};
> +
> +// Ensure blank messages don't return original message.

This says "Ensure blank messages don't return original message", but then in the test is looks like you're testing exactly the opposite of that: iMsg.originalMessage =? originalMessage.

@@ +57,5 @@
> +
> +// Example transformation
> +function rot13(aString)
> +  aString.replace(/[a-zA-Z]/g, function(c)
> +    String.fromCharCode(c.charCodeAt(0) + (c.toLowerCase() < "n" ? 1 : -1) * 13))

Usually we put { } around multi-line statements, but I see why you did this and I'm OK with it. One of the other reviews can feel free to disagree. :)

@@ +59,5 @@
> +function rot13(aString)
> +  aString.replace(/[a-zA-Z]/g, function(c)
> +    String.fromCharCode(c.charCodeAt(0) + (c.toLowerCase() < "n" ? 1 : -1) * 13))
> +
> +// Test message transformation path

This comment doesn't add much.

@@ +61,5 @@
> +    String.fromCharCode(c.charCodeAt(0) + (c.toLowerCase() < "n" ? 1 : -1) * 13))
> +
> +// Test message transformation path
> +let test_message_transformation = function() {
> +  let pConv = new Conversation();

Why pConv? What's the p?

@@ +68,5 @@
> +  };
> +
> +  let uiConv = new imConversations.UIConversation(pConv);
> +  let message = "Hello!";
> +  let receivedMsg = false, newTxt = false;

Are you using these to ensure the observers are called in the correct order?

@@ +73,5 @@
> +
> +  uiConv.addObserver({
> +    observe: function(aObject, aTopic, aMsg) {
> +      switch(aTopic) {
> +      case "sending-message":

I believe we usually indent our case statements an additional level.

@@ +104,5 @@
> +
> +  ok(newTxt);
> +};
> +
> +// Test cancelling a message before sending

Nit: Comments should be full sentences. This goes for everywhere.

Comment 17

[arlolra]

Comment on attachment 8514041 [details] [diff] [review]
test.patch from comment 11

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

> Were the fixes in imConversations.js found as results of these tests or were they just changes you had locally or what?

They were the result of writing these tests.

::: chat/components/src/test/test_conversations.js
@@ +6,5 @@
> +
> +let jsProtoHelper = {}, imConversations = {};
> +
> +Cu.import("resource:///modules/imServices.jsm");
> +Cu.import("resource:///modules/jsProtoHelper.jsm", jsProtoHelper);

Nope ... just copied from another file.

@@ +36,5 @@
> +      protocol: { name: "Fake Protocol" },
> +      alias: "",
> +      name: "Fake Account"
> +    },
> +    ERROR: function(e) { throw e }

There's a try/catch wrapping observer notification
https://github.com/mozilla/releases-comm-central/blob/master/chat/modules/jsProtoHelper.jsm#L449

that calls this._account.ERROR
https://github.com/mozilla/releases-comm-central/blob/master/chat/modules/jsProtoHelper.jsm#L479

While debugging, it came up that ERROR wasn't defined, which was masking the actual issue. I put the throw in cause I wanted to propagate the initial error.

@@ +40,5 @@
> +    ERROR: function(e) { throw e }
> +  } 
> +};
> +
> +// Ensure blank messages don't return original message.

My commenting should be clearer,

"Ensure that when iMsg.message is set to a blank message, the blank message is returned by iMsg.message and not the original message. This prevents regressions due to JS coercions."

@@ +57,5 @@
> +
> +// Example transformation
> +function rot13(aString)
> +  aString.replace(/[a-zA-Z]/g, function(c)
> +    String.fromCharCode(c.charCodeAt(0) + (c.toLowerCase() < "n" ? 1 : -1) * 13))

You wrote this line of code :)
http://hg.instantbird.org/addons/file/546b657cdcbe/rot13/bootstrap.js

@@ +61,5 @@
> +    String.fromCharCode(c.charCodeAt(0) + (c.toLowerCase() < "n" ? 1 : -1) * 13))
> +
> +// Test message transformation path
> +let test_message_transformation = function() {
> +  let pConv = new Conversation();

prplConv ... I guess just conv suffices.

@@ +68,5 @@
> +  };
> +
> +  let uiConv = new imConversations.UIConversation(pConv);
> +  let message = "Hello!";
> +  let receivedMsg = false, newTxt = false;

More that they are called at all.

At one point I misspelled the topic and the event wasn't ever observed.

Comment 18

[arlolra]

Attachment: test.patch from comment 17

Hopefully fixed all the nits.

Comment 19

[Patrick Cloke [:clokep]]

Comment on attachment 8514721 [details] [diff] [review]
test.patch from comment 17

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

::: chat/components/src/test/test_conversations.js
@@ +187,5 @@
> +    let prepared = false;
> +
> +    let conv = new Conversation();
> +    conv.sendMsg = function(aMsg) {
> +      msgCount += 1;

Nit: ++msgCount;

Comment 20

[aleth [:aleth]]

Comment on attachment 8514721 [details] [diff] [review]
test.patch from comment 17

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

Thanks for adding these tests!

::: chat/components/src/imConversations.js
@@ +127,5 @@
>      if (!(id in this._prplConv)) {
>        this._prplConv[id] = aPrplConversation;
> +      aPrplConversation.addObserver({
> +        observe: this.observeConv.bind(this, id)
> +      });

Why this change?

@@ +361,5 @@
>      // sent (eg. split long messages). If a message is split here, the split
>      // will be visible in the UI.
>      let messages = this.target.prepareForSending(om);
>  
>      // Protocols can return null if they don't need to make any changes.

Is this also mentioned in the IDL documentation for this function?

::: chat/components/src/test/test_conversations.js
@@ +14,5 @@
> +let _id = 0;
> +function Conversation(aName) {
> +  this._name = aName;
> +  this._observers = [];
> +  this._date = new Date() * 1000;

nit: Date.now() * 1000 avoids creating the object.

@@ +72,5 @@
> +          ok(!receivedMsg);
> +          ok(aObject instanceof imConversations.OutgoingMessage);
> +          aObject.message = rot13(aObject.message);
> +          break;
> +        case "received-message":

I'm confused why this is received before "new-text", since it's sent by a new-text observer. Please expand the comment at the top of the test to explain what the expected sequence of events is.

Actually I think I'm a bit confused by received-message itself, since in the existing (landed) code nothing seems to be observing it, and its name makes it sounds like it's for incoming messages, but it actually is just "received by the uiConv" I guess. What is its purpose? Can we document it somewhere?

Comment 21

[arlolra]

Comment on attachment 8514721 [details] [diff] [review]
test.patch from comment 17

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

::: chat/components/src/imConversations.js
@@ +127,5 @@
>      if (!(id in this._prplConv)) {
>        this._prplConv[id] = aPrplConversation;
> +      aPrplConversation.addObserver({
> +        observe: this.observeConv.bind(this, id)
> +      });

To use the UIConversation constructor in the JS tests, in which case the function being passed in to addObserver isn't coerced to an nsIObserver. ie. notifying observers complains that the function doesn't have a method `observe`.

@@ +361,5 @@
>      // sent (eg. split long messages). If a message is split here, the split
>      // will be visible in the UI.
>      let messages = this.target.prepareForSending(om);
>  
>      // Protocols can return null if they don't need to make any changes.

Yup, it says,

  /* Preprocess messages before they are sent (eg. split long messages).
     Can return null if no changes are to be made. */

::: chat/components/src/test/test_conversations.js
@@ +14,5 @@
> +let _id = 0;
> +function Conversation(aName) {
> +  this._name = aName;
> +  this._observers = [];
> +  this._date = new Date() * 1000;

Ok, do you want me to fix this in jsProtoHelper.jsm as well?

https://github.com/mozilla/releases-comm-central/blob/master/chat/modules/jsProtoHelper.jsm#L384
https://github.com/mozilla/releases-comm-central/blob/master/chat/modules/jsProtoHelper.jsm#L455

@@ +72,5 @@
> +          ok(!receivedMsg);
> +          ok(aObject instanceof imConversations.OutgoingMessage);
> +          aObject.message = rot13(aObject.message);
> +          break;
> +        case "received-message":

Perhaps "displaying-message" is a more accurate name for the event. Thoughts on that? The point is to give extensions a chance to swap or cancel a message before it gets displayed. There's a comment above the imMessage interface:

// A cancellable message to be displayed. When the conversation service is
// notified of a new-text (ie. an incoming or outgoing message to be displayed),
// it in turn notifies observers (again, typically add-ons), which have the
// opportunity to swap or cancel the message.

Is that enough documentation?

Comment 22

[aleth [:aleth]]

Comment on attachment 8514721 [details] [diff] [review]
test.patch from comment 17

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

::: chat/components/src/imConversations.js
@@ +127,5 @@
>      if (!(id in this._prplConv)) {
>        this._prplConv[id] = aPrplConversation;
> +      aPrplConversation.addObserver({
> +        observe: this.observeConv.bind(this, id)
> +      });

OK, in that case it needs a comment.

@@ +361,5 @@
>      // sent (eg. split long messages). If a message is split here, the split
>      // will be visible in the UI.
>      let messages = this.target.prepareForSending(om);
>  
>      // Protocols can return null if they don't need to make any changes.

Great!

::: chat/components/src/test/test_conversations.js
@@ +14,5 @@
> +let _id = 0;
> +function Conversation(aName) {
> +  this._name = aName;
> +  this._observers = [];
> +  this._date = new Date() * 1000;

No, it's just a nit.

@@ +72,5 @@
> +          ok(!receivedMsg);
> +          ok(aObject instanceof imConversations.OutgoingMessage);
> +          aObject.message = rot13(aObject.message);
> +          break;
> +        case "received-message":

I think "displaying-message" would be clearer, but more importantly mention the notification name in the IDL comment (which is otherwise fine).

Comment 23

[aleth [:aleth]]

The last comment is best viewed in splinter ;)

Comment 24

[Patrick Cloke [:clokep]]

_date is returned by startDate from prplIConversation as a PRTime: http://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#40 By multiplying by 1000 we coerce back to an integer, I assume.

Comment 25

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #22)

> > +        case "received-message":
> 
> I think "displaying-message" would be clearer

Unless this isn't the code I remember looking at, "displaying" would be misleading because it could very well be that we are receiving a message for a conversation that's on hold.

Comment 26

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> Unless this isn't the code I remember looking at, "displaying" would be
> misleading because it could very well be that we are receiving a message for
> a conversation that's on hold.

It's more confusing that we "receive" outgoing messages imho.

Comment 27

[arlolra]

Attachment: test.patch from comment 26

Updated fixing the nits and expanding the comment for the transformation pipeline test.

I'll leave it to you to debate renaming the event. That can come in a follow up patch if necessary.

Comment 28

[aleth [:aleth]]

Comment on attachment 8516180 [details] [diff] [review]
test.patch from comment 26

Thanks for adding the comments! This looks OK to me, but I would prefer to rename "received-message". Let's see what the others think ;)

Comment 29

[Florian Quèze [:florian]]

Comment on attachment 8516180 [details] [diff] [review]
test.patch from comment 26

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

Thanks for taking the time to write tests here!

::: chat/components/src/imConversations.js
@@ +126,5 @@
>  
>      if (!(id in this._prplConv)) {
>        this._prplConv[id] = aPrplConversation;
> +      // Pass an object here, instead of just a function, because coercion
> +      // to an nsIObserver won't happen in the JS tests.

Is there a reason why this can't be worked around by overriding the addObserver method of the conversation object in the test? eg.
  if (!(aObserver instanceof Ci.nsIObserver))
     aObserver = {observe: aObserver};

::: chat/components/src/test/test_conversations.js
@@ +41,5 @@
> +  let iMsg = new imConversations.imMessage(pMsg);
> +  equal(iMsg.message, originalMessage);
> +  // Setting the message should prevent a fallback to the original.
> +  iMsg.message = "";
> +  equal(iMsg.message, "");

The last parameter of the equal and ok functions is a string that will be displayed in the terminal and is helpful to figure out what the line was intended to verify, if the test ever fails in the future.

@@ +90,5 @@
> +      switch(aTopic) {
> +        case "sending-message":
> +          ok(!newTxt);
> +          ok(!receivedMsg);
> +          ok(aObject instanceof imConversations.OutgoingMessage);

This tests an implementation detail. I think you really want to test:
  aObject.QueryInterface(Ci.imIOutgoingMessage);

This comment also applies to the other tests for "instanceof imConversations.something" later in this file.

@@ +120,5 @@
> +};
> +
> +// A test that cancels a message before it can be sent.
> +let test_cancel_send_message = function() {
> +    let conv = new Conversation();

Why is this (and the next few tests) 4-space intended?

@@ +134,5 @@
> +          aObject.cancelled = true;
> +        }
> +      }
> +    });
> +    uiConv.sendMsg("Hi!");

I know the test_message_transformation test above verifies it, but shouldn't we check here that the sending-message observer has actually been called?

Should we also verify that the observer is NOT called with any of the other topics? eg.
  default:
    ok(false, "no other notification should be fired for a cancelled message");

@@ +208,5 @@
> +    let prepared = false;
> +
> +    let conv = new Conversation();
> +    conv.sendMsg = function(aMsg) {
> +      ++msgCount;

Should we also verify that the split messages are sent in the correct order?

Comment 30

[Florian Quèze [:florian]]

Comment on attachment 8511759 [details] [diff] [review]
prepare.patch from comment 11

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

::: chat/components/src/imConversations.js
@@ +435,5 @@
>      }
>    },
>  
> +  // Used above when notifying of new-texts originating in the
> +  // UIConversation. The happens when this.systemMessage() is called. The

Typo: "The happens"

@@ +436,5 @@
>    },
>  
> +  // Used above when notifying of new-texts originating in the
> +  // UIConversation. The happens when this.systemMessage() is called. The
> +  // conversation for the message is set as the UIConversation. 

Trailing space.

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8516180 [details] [diff] [review]
test.patch from comment 26

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

::: chat/components/src/test/test_conversations.js
@@ +192,5 @@
> +    uiConv.addObserver({
> +      observe: function(aObject, aTopic, aMsg) {
> +        if (aTopic === "new-text") {
> +          equal(aObject.displayMessage, msg);
> +          receivedMsg = true;

Should we test here that |prepared| is true, to prevent the test from passing if both prepareForSending and prepareForDisplaying are called, but after the new-text notification is fired?

Comment 32

[arlolra]

Comment on attachment 8516180 [details] [diff] [review]
test.patch from comment 26

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

Thanks for the review :)

I'll have this cleaned up momentarily.

::: chat/components/src/imConversations.js
@@ +126,5 @@
>  
>      if (!(id in this._prplConv)) {
>        this._prplConv[id] = aPrplConversation;
> +      // Pass an object here, instead of just a function, because coercion
> +      // to an nsIObserver won't happen in the JS tests.

Nope. Happy to change it if you prefer it that way.

::: chat/components/src/test/test_conversations.js
@@ +41,5 @@
> +  let iMsg = new imConversations.imMessage(pMsg);
> +  equal(iMsg.message, originalMessage);
> +  // Setting the message should prevent a fallback to the original.
> +  iMsg.message = "";
> +  equal(iMsg.message, "");

Ok, I can populate them.

@@ +90,5 @@
> +      switch(aTopic) {
> +        case "sending-message":
> +          ok(!newTxt);
> +          ok(!receivedMsg);
> +          ok(aObject instanceof imConversations.OutgoingMessage);

Ah, yes.

@@ +120,5 @@
> +};
> +
> +// A test that cancels a message before it can be sent.
> +let test_cancel_send_message = function() {
> +    let conv = new Conversation();

Just a mistake.

@@ +134,5 @@
> +          aObject.cancelled = true;
> +        }
> +      }
> +    });
> +    uiConv.sendMsg("Hi!");

Yes, good point (on both accounts). Fixing.

@@ +192,5 @@
> +    uiConv.addObserver({
> +      observe: function(aObject, aTopic, aMsg) {
> +        if (aTopic === "new-text") {
> +          equal(aObject.displayMessage, msg);
> +          receivedMsg = true;

Right; the `ok(prepared);` should be moved here.

@@ +208,5 @@
> +    let prepared = false;
> +
> +    let conv = new Conversation();
> +    conv.sendMsg = function(aMsg) {
> +      ++msgCount;

Seems reasonable; I'll add that.

Comment 33

[arlolra]

Attachment: test.patch from comment 32

Ok, all fixed up.

Comment 34

[arlolra]

Attachment: prepare.patch from comment 30

Fixes up the typos.

Comment 35

[Florian Quèze [:florian]]

Comment on attachment 8543699 [details] [diff] [review]
test.patch from comment 32

Thanks! :-)

Comment 36

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/90bc651566be
https://hg.mozilla.org/comm-central/rev/d6f2f1a42c74

Comment 37

[aleth [:aleth]]

http://hg.mozilla.org/users/florian_queze.net/purple/rev/d44192a91176

Thanks!