983347 - Need different paths for displaying to the screen and sending over the wire

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[arlolra]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.149 Safari/537.36



Actual results:

Sending a message follows the same path for both.

Comment 1

[arlolra]

pasting the current plan from irc

flo-retina:

what I have in mind right now: the API letting 'stuff' modify messages before they are sent will have in its return value an object that will contain the modified message, but also a flag indicating if the modified version or the original one should be shown in the conversation.

If the original message should be shown, then the conversation service prints the message itself. If the modified message should be shown, then it's up to the prpl to display the message.

btw, I find this stuff easier to think about if I have concrete examples of use cases in mind. So the examples I use are OTR and a pastebin add-on/feature.

in the case of a pastebin feature, we would want to show the modified message

it seems over complicated. But I can't come up with something nicer right now

Comment 2

[Florian Quèze [:florian]]

Two other relevant messages before what's pasted in comment 1:

clokep: in http://mxr.mozilla.org/comm-central/source/chat/components/src/imConversations.js#273 you'll want to 1. display the text to the UI conversation, 2. encrypt the text, 3. pass the encrypted text to this.target.sendMsg

flo-retina: what clokep said is a very high level version of what needs to happen. The step 2 ("encrypt the text") is a bit more complicated.
You need to add there an API that will let other components (eg. your OTR code) modify messages.

Comment 3

[arlolra]

Attachment: display.patch

As a first step, this splits displaying and sending a message in the conversation.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8398176 [details] [diff] [review]
display.patch

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

I'm sure Florian will have some comments too. Many of mine are style type things which aren't a big deal but will eventually need to get fixed (and hopefully pointing them out now will keep you from having to redo work later on).

Also, can you turn on git style diffs with 8-lines of context for your next diff please? Thanks! (See https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Basic_configuration for the configuration we normally use.)

::: chat/components/src/imConversations.js
@@ +271,5 @@
>    get title() this.target.title,
>    get startDate() this.target.startDate,
> +  sendMsg: function (aMsg) {
> +	  this.target.sendMsg(aMsg);
> +	  this.target.displayMsg(aMsg);

Nit: Please use spaces, not tabs. We do 2-space indent.

::: chat/modules/jsProtoHelper.jsm
@@ +479,4 @@
>    },
>    sendTyping: function(aString) Ci.prplIConversation.NO_TYPING_LIMIT,
>  
> +  displayMsg: function (aMsg) {

Please move this between sendMsg and sendTyping. It logically belongs next to sendMsg.

::: chat/protocols/irc/irc.js
@@ +171,5 @@
>        this._pendingMessage = true;
>      }
>    },
> +
> +  displayMsg: function (aMessage) {

Nit: No space between function and (. (This is done in a few other places too, I didn't point them out.)

@@ +173,5 @@
>    },
> +
> +  displayMsg: function (aMessage) {
> +    // Since the server doesn't send us a message back, just assume the
> +    // message was received and immediately show it.

This comment makes me wonder if there should be a way for a prpl to tell the UI NOT display a message until a response is acknowledged. I'm unsure what (if any) protocols do that. (Maybe Twitter?)

::: chat/protocols/irc/test/test_splitLongMessages.js
@@ +23,5 @@
>    prefix: "!user@host",
>    maxMessageLength: 51, // For convenience.
> +  sendMessage: function(aCommand, aParams) {
> +    irc.GenericIRCConversation.messages.push(aParams[1]);
> +    return true

Nit: End lines in a ;.

Comment 5

[arlolra]

Attachment: display.patch

Re: the comment, XMPP MUC and Twitter seems to be fire and forget.

Comment 6

[arlolra]

twitter and xmpp muc needs a no-op displayMsg with a comment  "We re-receive this messag efrom the server so it is displayed there."

Comment 7

[arlolra]

Attachment: display.patch

Add the changes from comment 6.

Comment 8

[arlolra]

Attachment: display.patch

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8398229 [details] [diff] [review]
display.patch

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

I don't really see how this patch relates to the discussion we have had before. Hopefully the comments I've left will help further define what needs to happen:

::: chat/components/src/imConversations.js
@@ +269,5 @@
>    get name() this.target.name,
>    get normalizedName() this.target.normalizedName,
>    get title() this.target.title,
>    get startDate() this.target.startDate,
> +  sendMsg: function(aMsg) {

Where are you integrating the encryption? You need a hook before calling the prpl's sendMsg method.

@@ +270,5 @@
>    get normalizedName() this.target.normalizedName,
>    get title() this.target.title,
>    get startDate() this.target.startDate,
> +  sendMsg: function(aMsg) {
> +    this.target.sendMsg(aMsg);

Maybe sendMsg could return a string with the message that should be displayed; or null if the message will be re-received from the server before it should be displayed (ie. the Twitter and XMPP MUC cases). Or maybe an object if we need to pass around more than just the displayable message text.

@@ +271,5 @@
>    get title() this.target.title,
>    get startDate() this.target.startDate,
> +  sendMsg: function(aMsg) {
> +    this.target.sendMsg(aMsg);
> +    this.target.displayMsg(aMsg);

I'm wondering if displaying the message should be handled in imConversations.js instead of having some code doing it in each protocol plugin.

Comment 10

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Comment on attachment 8398229 [details] [diff] [review]
> display.patch
> 
> Review of attachment 8398229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really see how this patch relates to the discussion we have had
> before.

It accomplishes what's stated in the bug report. I was trying to take things one step at a time.


> Hopefully the comments I've left will help further define what needs
> to happen:
> 
> ::: chat/components/src/imConversations.js
> @@ +269,5 @@
> >    get name() this.target.name,
> >    get normalizedName() this.target.normalizedName,
> >    get title() this.target.title,
> >    get startDate() this.target.startDate,
> > +  sendMsg: function(aMsg) {
> 
> Where are you integrating the encryption? You need a hook before calling the
> prpl's sendMsg method.

That will be in the next patch.

Sometimes OTR will need to send a message in response to a received message that is transparent to the user (particularly during the authenticated key exchange). The changes in this patch allow me to call sendMsg on the prpls without displaying anything to the user.


> 
> @@ +270,5 @@
> >    get normalizedName() this.target.normalizedName,
> >    get title() this.target.title,
> >    get startDate() this.target.startDate,
> > +  sendMsg: function(aMsg) {
> > +    this.target.sendMsg(aMsg);
> 
> Maybe sendMsg could return a string with the message that should be
> displayed; or null if the message will be re-received from the server before
> it should be displayed (ie. the Twitter and XMPP MUC cases). Or maybe an
> object if we need to pass around more than just the displayable message text.

It would need to be an object with the to, msg, and alias fields, which differ between prpls.

If that's what you'd prefer, I'm happy to do that. But the reason I did it this way is that some prpls do some processing before displaying the message that's logically separate from sending it. An example is the txttohtml conversion in xmpp.

> 
> @@ +271,5 @@
> >    get title() this.target.title,
> >    get startDate() this.target.startDate,
> > +  sendMsg: function(aMsg) {
> > +    this.target.sendMsg(aMsg);
> > +    this.target.displayMsg(aMsg);
> 
> I'm wondering if displaying the message should be handled in
> imConversations.js instead of having some code doing it in each protocol
> plugin.

All the displayMsg functions move the code related to displaying messages out of sendMsg and call writeMessage. If sendMsg returns an object as you'd defined above, then yes, that should be possible.

Comment 11

[Florian Quèze [:florian]]

(In reply to arlolra from comment #10)
> (In reply to Florian Quèze [:florian] [:flo] from comment #9)

> > I don't really see how this patch relates to the discussion we have had
> > before.
> 
> It accomplishes what's stated in the bug report. I was trying to take things
> one step at a time.

I appreciate the effort to break things down to small patches for easier review; but I want to be sure we are not doing changes that we will discover later don't actually fit our needs.

> Sometimes OTR will need to send a message in response to a received message
> that is transparent to the user (particularly during the authenticated key
> exchange). The changes in this patch allow me to call sendMsg on the prpls
> without displaying anything to the user.

Making sendMsg not display the message to the user seems the right thing to do (although I do wonder how we can get this behavior with libpurple). The part that I dislike is the displayMsg implemented by each prpl.

> > @@ +270,5 @@
> > >    get normalizedName() this.target.normalizedName,
> > >    get title() this.target.title,
> > >    get startDate() this.target.startDate,
> > > +  sendMsg: function(aMsg) {
> > > +    this.target.sendMsg(aMsg);
> > 
> > Maybe sendMsg could return a string with the message that should be
> > displayed; or null if the message will be re-received from the server before
> > it should be displayed (ie. the Twitter and XMPP MUC cases). Or maybe an
> > object if we need to pass around more than just the displayable message text.
> 
> It would need to be an object with the to, msg, and alias fields, which
> differ between prpls.

The conversation and account objects would be accessible, so I don't think we need to pass around the alias. For 'to' (I assume you meant from), I'm not sure but I suspect we could also avoid it.

Comment 12

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> (In reply to arlolra from comment #10)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #9)
> 
> > > I don't really see how this patch relates to the discussion we have had
> > > before.
> > 
> > It accomplishes what's stated in the bug report. I was trying to take things
> > one step at a time.
> 
> I appreciate the effort to break things down to small patches for easier
> review; but I want to be sure we are not doing changes that we will discover
> later don't actually fit our needs.

Fair enough.

> 
> > Sometimes OTR will need to send a message in response to a received message
> > that is transparent to the user (particularly during the authenticated key
> > exchange). The changes in this patch allow me to call sendMsg on the prpls
> > without displaying anything to the user.
> 
> Making sendMsg not display the message to the user seems the right thing to
> do (although I do wonder how we can get this behavior with libpurple). The
> part that I dislike is the displayMsg implemented by each prpl.

Are you happy with having the prpl sendMsg return an object that imConversation
then optionally displays?

> 
> > > @@ +270,5 @@
> > > >    get normalizedName() this.target.normalizedName,
> > > >    get title() this.target.title,
> > > >    get startDate() this.target.startDate,
> > > > +  sendMsg: function(aMsg) {
> > > > +    this.target.sendMsg(aMsg);
> > > 
> > > Maybe sendMsg could return a string with the message that should be
> > > displayed; or null if the message will be re-received from the server before
> > > it should be displayed (ie. the Twitter and XMPP MUC cases). Or maybe an
> > > object if we need to pass around more than just the displayable message text.
> > 
> > It would need to be an object with the to, msg, and alias fields, which
> > differ between prpls.
> 
> The conversation and account objects would be accessible, so I don't think
> we need to pass around the alias. For 'to' (I assume you meant from), I'm
> not sure but I suspect we could also avoid it.

My concern here is that prpls seem to want to use different attributes for the,
as you correctly pointed out, 'from' field. For example, xmpp uses prefers
`this._account._connection._jid.jid`, whereas yahoo defaults to
`this._account.cleanUsername`. The same is true for alias. Casing that in the
imConversations sendMsg seemslike the wrong things to do.

Comment 13

[Florian Quèze [:florian]]

(In reply to arlolra from comment #12)
> (In reply to Florian Quèze [:florian] [:flo] from comment #11)
> > (In reply to arlolra from comment #10)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #9)

> > > Sometimes OTR will need to send a message in response to a received message
> > > that is transparent to the user (particularly during the authenticated key
> > > exchange). The changes in this patch allow me to call sendMsg on the prpls
> > > without displaying anything to the user.
> > 
> > Making sendMsg not display the message to the user seems the right thing to
> > do (although I do wonder how we can get this behavior with libpurple). The
> > part that I dislike is the displayMsg implemented by each prpl.
> 
> Are you happy with having the prpl sendMsg return an object that
> imConversation
> then optionally displays?

Yes. The message could implement the prplIMessage interface.

I'm still wondering if it's something we will manage to implement with libpurple though. Is there an easy way to obtain that behavior from the libpurple code base? Or is it going to just add a message to the conversation without our control...

Do you know how pidgin-otr solves this problem?

> prpls seem to want to use different attributes for the [...]
> 'from' field. For example, xmpp uses prefers
> `this._account._connection._jid.jid`, whereas yahoo defaults to
> `this._account.cleanUsername`.

This may be a bug (not fully sure).

> The same is true for alias.

The alias should be handled by the UI, not by the prpl. It's something that has made me frown a few times already when looking at JS prpls.

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8398229 [details] [diff] [review]
display.patch

I think we need to take a step back and ensure that:
1. We agree on what we're trying to accomplish
2. We agree on the requirements

This should hopefully allow us to design an API that will not need to be changed many times in the ensuing patches.

The requirements as I see them are:
1. prpls can modify text before sending, including sending multiple sub-messages (e.g. for length reasons as on IRC)
2. An API to sit between the UI and the prpl that can modify the text send to a prpl, but display the unmodified text to the user (e.g. OTR or other encryption type stuff)
3. Some prpls get a ack that a message was received and should not be displayed until that point (e.g. on Twitter we get our own tweets sent back to us)
4. Messages must go through an API before being shown to the user (e.g. OTR gets messages that contain no plaintext, only encoded information)
5. Some text might need to be sent without the users knowledge (e.g. the OTR handshake)

Do all of these make sense? Am I missing any?

We have two use-cases in mind:
1. OTR where ciphertext is sent to the network and plaintext is displayed to the user.
2. Pastebin Add-on where the original text is discarded and replaced with a link.

An additional thing to think about, what if I have both the Pastebin Add-on and OTR installed? How do they interact? (Everything should be fine if the Pastebin one is "run" first, but the opposite is not true...)

Comment 15

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #14)

> The requirements as I see them are:
[...]
> Do all of these make sense?

Yes.

> Am I missing any?

6. The API we come up with needs to be reasonable to implement both for js-prpls and libpurple.

Comment 16

[arlolra]

> Do you know how pidgin-otr solves this problem?

It adds a signal handler that listens for "sending-im-msg". When that fires, it intercepts the message and passes it to otr for processing. If otr fragments the encrypted response, it'll inject all but the last message itself. It then swaps in a pointer to the final encrypted message so that the normal course of continues.

> An additional thing to think about, what if I have both the Pastebin Add-on and OTR installed? How do they interact? (Everything should be fine if the Pastebin one is "run" first, but the opposite is not true...)

Until recently, I think you were just expected not to do this. If you're using otr, you probably don't want to be pasting plaintext. However, the latest pidgin api provides priorities to the signal handlers.

Comment 17

[arlolra]

I have two ideas here, neither of which are entirely satisfying.

The api to libpurple is limited to accepting a string to send and then notifying when that's accomplished. If transforms are happening in imConversation, we'd need to buffer tuples of strings (sent, display) set by the transforms and retrieved by the message in the callback.

Either that, or forgo async transforms (like Pastebin) and mimic what pidgin-otr does in libpurple, making similar changes to the js prpls.

Comment 18

[Florian Quèze [:florian]]

(In reply to arlolra from comment #17)
> I have two ideas here, neither of which are entirely satisfying.
> 
> The api to libpurple is limited to accepting a string to send and then
> notifying when that's accomplished. If transforms are happening in
> imConversation, we'd need to buffer tuples of strings (sent, display) set by
> the transforms and retrieved by the message in the callback.

That's unfortunate but not unacceptable. I've also scratched my head while looking at the existing libpurple API and didn't find anything I fully liked to work around the fact that the libpurple prpls directly call write_conv without context.

> Either that, or forgo async transforms (like Pastebin) and mimic what
> pidgin-otr does in libpurple, making similar changes to the js prpls.

I'm not sure I fully understand this solution, but I suspect the changes to the js prpls would be even more unpleasant than the first proposed solution.

Comment 19

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> That's unfortunate but not unacceptable. I've also scratched my head while
> looking at the existing libpurple API and didn't find anything I fully liked
> to work around the fact that the libpurple prpls directly call write_conv
> without context.

In that case, I'll proceed down this road.

prpls will need to fire a new event (text-sent, or something) for the replacer to observe, which will fire the new-text event in turn.

 
> > Either that, or forgo async transforms (like Pastebin) and mimic what
> > pidgin-otr does in libpurple, making similar changes to the js prpls.
> 
> I'm not sure I fully understand this solution, but I suspect the changes to
> the js prpls would be even more unpleasant than the first proposed solution.

The shape would be something like this: the js prpl would need to implement a toWire
that does the final step of sending a msg. the prpl's sendMsg would call a generic
transformMsg that performs the synchronous operations and calls toWire as necessary.

The sync restriction is to conform with running them in a signal handler in libpurple,
as in pidgin-otr.

Comment 20

[Florian Quèze [:florian]]

(In reply to arlolra from comment #19)
> (In reply to Florian Quèze [:florian] [:flo] from comment #18)
> > That's unfortunate but not unacceptable. I've also scratched my head while
> > looking at the existing libpurple API and didn't find anything I fully liked
> > to work around the fact that the libpurple prpls directly call write_conv
> > without context.
> 
> In that case, I'll proceed down this road.
> 
> prpls will need to fire a new event (text-sent, or something) for the
> replacer to observe, which will fire the new-text event in turn.

I'm a bit uncomfortable with the idea of having the text replacer fire events instead of the prpls.

What about having prpls call a method of the conversation, and having the conversation object communicate with the replacer?

Comment 21

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> I'm a bit uncomfortable with the idea of having the text replacer fire
> events instead of the prpls.
> 
> What about having prpls call a method of the conversation, and having the
> conversation object communicate with the replacer?

Yes, that will work. I'll do that.

Comment 22

[Florian Quèze [:florian]]

I wonder if this means we no longer need to have so many different implementations of the prplIMessage interface. If we could have a single implementation of the message object in imConversations.js, I think we would get a nice code simplification.

Comment 23

[arlolra]

Attachment: transform.patch

Here's a rot13 transform using this api: https://gist.github.com/arlolra/11151564

Comment 24

[Patrick Cloke [:clokep]]

Comment on attachment 8409753 [details] [diff] [review]
transform.patch

Florian will hopefully give more specific feedback at some point, but aleth, him and I had a conversation about this that boiled down to a few points:
- The current proposed API probably adds too many interfaces, some of this could be done with observers.
- The changes in jsProtoHelper should most likely be in imConversations.
- Changes to the message interface should actually change prplIMessage, there shouldn't be protocol specific versions of this.

Also, please include comments about what each of these APIs does, etc. I had to read the whole patch before understanding how everything fit together.

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8409753 [details] [diff] [review]
transform.patch

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

::: chat/components/public/prplIConversation.idl
@@ +12,5 @@
>  interface nsIURI;
>  interface nsIDOMDocument;
> +interface prplIConversation;
> +
> +/* A tMsg interface */

Interfaces really need descriptive comments.

@@ +14,5 @@
> +interface prplIConversation;
> +
> +/* A tMsg interface */
> +[scriptable, uuid(b62563d6-c8ff-11e3-b0b0-42251e5d46b0)]
> +interface tMessage : nsISupports {

Is this something that should be part of prplIMessage? What's the purpose of this interface?

@@ +21,5 @@
> +};
> +
> +/* A generic callback */
> +[scriptable, function, uuid(fe7d8cdc-c8e4-11e3-bdf4-1c101e5d46b0)]
> +interface callback : nsISupports {

All the interfaces we define have a prefix:
"im" for things that are implemented by the chat/ core (shared by Instantbird and Thunderbird)
"prpl" for things implemented by each protocol plugin
"ib" for things specific to Instantbird (these shouldn't exist in chat/).

@@ +22,5 @@
> +
> +/* A generic callback */
> +[scriptable, function, uuid(fe7d8cdc-c8e4-11e3-bdf4-1c101e5d46b0)]
> +interface callback : nsISupports {
> +  void invoke([optional] in AUTF8String aMsg);

The parameter being optional here makes me wonder if you could just have used nsIRunnable

@@ +25,5 @@
> +interface callback : nsISupports {
> +  void invoke([optional] in AUTF8String aMsg);
> +};
> +
> +/* A transform */

It's not clear what a "transform" is. What should implement this interface? What's responsible for calling its methods?

@@ +71,5 @@
>  
> +  /* Transform a message */
> +  void transformMsg(in AUTF8String aMsg, in boolean isSending, in callback aCb);
> +  void addTransform(in transform aTransform);
> +  void removeTransform(in transform aTransform);

Conversation objects already support observers (addObserver, removeObserver methods). Could you get what you want by just firing more observer notifications? Maybe fire a notification when a message is about to be sent, with the prplIMessage object as aSubject parameter.

::: chat/modules/jsProtoHelper.jsm
@@ +478,5 @@
>      throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>    },
>    sendTyping: function(aString) Ci.prplIConversation.NO_TYPING_LIMIT,
>  
> +  addTransform: function(aTransform) {

I really think you don't want to have each prpl implement add/removeTransform methods in addition to the existing observers.

@@ +485,5 @@
> +  },
> +  removeTransform: function(aTransform) {
> +    this._transforms = this._transforms.filter(function (o) o !== aTransform);
> +  },
> +  transformMsg: function(aMsg, isSending, aCb) {

I used to think that you also don't want to implement this in the prpls, but I'm increasingly thinking that if we can just simplify this to firing a few observer notifications, it would be acceptable to have an implementation in jsProtoHelper and one in purplexpcom.

Comment 26

[Florian Quèze [:florian]]

Here's a proposal; let me know if it addresses your use case correctly:

I think you want the process of sending/receiving/displaying a message to fire 3 distinct observer notifications:

- sending-message
-- the aSubject parameter is a prplIMessage (or a new interface wrapping a prplIMessage) where the message can still be modified (or maybe even cancelled?) by observers


- receiving-message
-- aSubject would be a prplIMessage or an interface wrapping it. Observers can modify the message, or stop its processing.


- new-text (the current notification)
-- This is fired to tell the UI to display the message in the conversation.


Note: all names in this proposal can be modified/tweaked for better consistency.

Comment 27

[arlolra]

> - sending-message
> -- the aSubject parameter is a prplIMessage (or a new interface wrapping a
> prplIMessage) where the message can still be modified (or maybe even
> cancelled?) by observers

To make this more concrete, you're saying in imConversation.js sendMsg() would first fire "sending-message" that the transformer (otr, etc.) would modify, and then continue doing target.sendMsg(). This doesn't work for the async case (pastebin) that was mentioned above.

Comment 28

[Florian Quèze [:florian]]

(In reply to arlolra from comment #27)
> > - sending-message
> > -- the aSubject parameter is a prplIMessage (or a new interface wrapping a
> > prplIMessage) where the message can still be modified (or maybe even
> > cancelled?) by observers
> 
> To make this more concrete, you're saying in imConversation.js sendMsg()
> would first fire "sending-message" that the transformer (otr, etc.) would
> modify, and then continue doing target.sendMsg(). This doesn't work for the
> async case (pastebin) that was mentioned above.

Observers performing an async processing could cancel the message (ie. stop it) and then reinsert a new message once they get the result from the async operation.

Comment 29

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> Observers performing an async processing could cancel the message (ie. stop
> it) and then reinsert a new message once they get the result from the async
> operation.

Can you elaborate on what you mean by reinsert. Let's say you wanted to run two transformations and the first one is async. When does the second one run and how do you avoid calling the first one twice.

Comment 30

[Florian Quèze [:florian]]

(In reply to arlolra from comment #29)
> When does the second one run
> and how do you avoid calling the first one twice.

The second (or Nth) runs when no transformation before it cancels the message.

I don't see any generic way to avoid calling the same transformation twice, but I also can't see any real use case of a transformation that would be both async and unable to detect if it needs to be performed again (pastebin would detect that the pastebin url is short and not worth pastebining; other transformations that would fix typos or automatically add links would just do nothing if they have already transformed the message before (+ these are sync operations)).

Comment 31

[arlolra]

Attachment: transform.patch

This patch implements Florian's above proposal. Because of the libpurple limitations, there's an implicit contract that whatever message the UIConversation passes to a prpl, it'll get back as is when "new-text" is notified. This requires making a few changes to the js prpls to make use of the new API to apply their transforms (think irc message splitting). The one caveat is that observers need to be fired by priority for it to work.

Comment 32

[Patrick Cloke [:clokep]]

Comment on attachment 8438931 [details] [diff] [review]
transform.patch

arlo and flo are pair programmed on this a bit today at the Tor conference (or whatever it's called) in Paris. Clearing feedback on this as I'm sure arlo got good feedback in person! (And we discussed the API a bit over IRC after flo and arlo had discussed.)

Comment 33

[arlolra]

I started implementing the newly proposed API but ran into a few issues. Here's the patch so you can follow along, https://gist.github.com/14d38c35c65b555677ec

1) JS arrays do *not* implement the nsIMutableArray interface (ie. queryElementAt is not an available method), so we'll need to introduce a bunch of helpers (not present in the above patch at the time of writing) for converting back-and-forth, which is tedious.

2) We will need the converse of prepareForSending (ie. prepareForDisplaying) because the irc prpl wants to apply ctcpFormatToHTML before displaying.

3) We concentrated a lot of outgoing messages, at the neglect of incoming (and displaying) them. The logic I had in the last patch waited for "new-text" to be emitted, but that only worked because I made the originalMessage mutable. Here I put a bunch of stuff in writeMessage, which seems wrong. Maybe the conversation service should expose a displayMsg method, that receive the text, conversation and properties so it can notifyObservers (with cancellableMessages) and then emit "new-text"? Hopefully the issue here is clear, if not the solution.

4) In practice, working with both an array of sending strings and a linked list of cancellableMessages is a little cumbersome. As an example, see prepareForSending in the irc prpl where it splits strings.

5) I've added a "displaying-message" event, for outgoing messages, which is distinct from "receiving-message" for incoming.

Comment 34

[Florian Quèze [:florian]]

Attachment: transform.patch from comment 33

Patch reviews (even WIPs) are easier on bugzilla.

Comment 35

[Florian Quèze [:florian]]

(In reply to arlolra from comment #33)

> 1) JS arrays do *not* implement the nsIMutableArray interface

Do you remember I was unhappy about having to use an nsIMutableArray? I guess now you understand why.

An other solution if we really want JS Arrays could be to have a method returning an array, instead of having an attribute. But then we may need a second method to modify the array, as the arrays returned by methods are copies, not references :-/.

> 2) We will need the converse of prepareForSending (ie. prepareForDisplaying)
> because the irc prpl wants to apply ctcpFormatToHTML before displaying.

Do we actually need this? If the encryption happened on the HTML message, then decrypting the message should output HTML, and I don't think we need to apply ctcp formatting.

I don't know much about ctcp though, so clokep's opinion would be very welcome here.

> 3) We concentrated a lot of outgoing messages, at the neglect of incoming
> (and displaying) them. The logic I had in the last patch waited for
> "new-text" to be emitted, but that only worked because I made the
> originalMessage mutable. Here I put a bunch of stuff in writeMessage, which
> seems wrong. Maybe the conversation service should expose a displayMsg
> method, that receive the text, conversation and properties so it can
> notifyObservers (with cancellableMessages) and then emit "new-text"?
> Hopefully the issue here is clear, if not the solution.

I think the issue is clear, yes. We forgot to discuss the canceling of incoming messages yesterday.

The conversation service already seems to receive all messages in a single place (http://lxr.instantbird.org/instantbird/source/chat/components/src/imConversations.js#251) so I don't see how adding a method would help.

I'm wondering if we shouldn't create an imIMessage interface inheriting from prplIMessage, but with more attributes. This interface would be implemented by the conversation service, and an instance of imMessage would be created for each received prplMessage, and sent to conversation observers.

> 4) In practice, working with both an array of sending strings and a linked
> list of cancellableMessages is a little cumbersome. As an example, see
> prepareForSending in the irc prpl where it splits strings.

That method seems either not finished, or not doing what you want.

> 5) I've added a "displaying-message" event, for outgoing messages, which is
> distinct from "receiving-message" for incoming.

I'm under the impression that these 2 notifications want to be fired from the observeConv method of the conversation service, rather than from jsProtoHelper.

Comment 36

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> (In reply to arlolra from comment #33)
> > 2) We will need the converse of prepareForSending (ie. prepareForDisplaying)
> > because the irc prpl wants to apply ctcpFormatToHTML before displaying.
> 
> Do we actually need this? If the encryption happened on the HTML message,
> then decrypting the message should output HTML, and I don't think we need to
> apply ctcp formatting.
> 
> I don't know much about ctcp though, so clokep's opinion would be very
> welcome here.

I'd think the encryption should apply to the raw CTCP message, not to HTML. But I'm not positive how other clients would receive this.

Comment 37

[arlolra]

Attachment: transform.patch

Comment 38

[arlolra]

Comment on attachment 8450280 [details] [diff] [review]
transform.patch

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

::: chat/components/src/imConversations.js
@@ +337,5 @@
>        this.target = this._prplConv[aTargetId];
> +
> +    if (aTopic == "new-text") {
> +      aSubject = new imMessage(aSubject);
> +      this.notifyObservers(aSubject, "received-message");

if (aSubject.cancelled)
  return;

Comment 39

[aleth [:aleth]]

Comment on attachment 8450280 [details] [diff] [review]
transform.patch

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

This is a much nicer API than the last version I saw - thanks for improving this so much!

The interface file needs some documentatory comments (most of what's needed seems to already exist in comments in imConversations).

I'd like clokep to take a look at the way this will interact with ctcp in IRC.

Comment 40

[Patrick Cloke [:clokep]]

Comment on attachment 8450280 [details] [diff] [review]
transform.patch

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

Please add a lot of comments to the interfaces! (As aleth said earlier.)

The question with CTCP messages is, what should get encrypted? (In all my examples below I'm using ` to be \001, cause it's easier.)

If the user types "/me foobar" to somenick, this becomes PRIVMSG somenick :`ACTION foobar`. Do we expect just "foobar" to be encrypted or `ACTION foobar` to be encrypted? (We should probably match whatever other clients do!)

Encrypting just "foobar" is nice because we can do all the CTCP processing first and just hand the encrypted string to the UI and it'll get decrypted before being shown...

But...

CTCP is also used for formatting (I use * below to be ^B):

The user types "Hello <b>bob</b>" to somenick, becomes "PRIVMSG somenick :Hello *bob*"

The question here is...do you care about the CTCP entities at all or do you just encrypt the entire HTML block: "Hello <b>bob</b>"

::: chat/components/src/imConversations.js
@@ +377,5 @@
> +    // prpls have an opportunity here to preprocess messages before they are
> +    // sent, eg. split long messages. If a message is split here, the split 
> +    // will be visible in the UI.
> +    // prpls can return null if they don't need to make any change
> +    let messages = this.target.prepareForSending(om) || [om.message];

Why the || here? In case this is not implemented by a prpl?

Comment 41

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #40)

> If the user types "/me foobar" to somenick, this becomes PRIVMSG somenick
> :`ACTION foobar`. Do we expect just "foobar" to be encrypted or `ACTION
> foobar` to be encrypted? (We should probably match whatever other clients
> do!)
> 
> Encrypting just "foobar" is nice because we can do all the CTCP processing
> first and just hand the encrypted string to the UI and it'll get decrypted
> before being shown...

I think I would encrypt only "foobar".

> The question here is...do you care about the CTCP entities at all or do you
> just encrypt the entire HTML block: "Hello <b>bob</b>"

The latter, I think it makes sense to just encrypt the HTML message. (We did have a long debate about this in Paris, and IIRC the conclusion was that it was very likely undefined behavior.)

> ::: chat/components/src/imConversations.js
> @@ +377,5 @@
> > +    // prpls have an opportunity here to preprocess messages before they are
> > +    // sent, eg. split long messages. If a message is split here, the split 
> > +    // will be visible in the UI.
> > +    // prpls can return null if they don't need to make any change
> > +    let messages = this.target.prepareForSending(om) || [om.message];
> 
> Why the || here? In case this is not implemented by a prpl?

The || here is something I added to make no-op implementations trivial: they can just return null instead of bothering with returning an array. I expect this to be particularly handy for the C++ implementation in purplexpcom; but this isn't really required.

Comment 42

[arlolra]

Thanks for taking a look. Comments are forthcoming.

I updated the OTR extension to make use of this new API. Here's what sending looks like,
https://github.com/arlolra/ctypes-otr/blob/master/chrome/content/otr.js#L410-L457

A few things came up:

 * OTR does this thing where if you set a policy, REQUIRE_ENCRYPTION, it'll buffer messages internally until the authenticated key exchange is complete, and then send them off. In this case, no message is returned from libotr but you still want to call writeMessage to display them to the sender (and assume they'll be delivered eventually).

 * We described in comment #14 that there're a few instances where OTR wants to send its own messages (call target.sendMsg) in response to having received a msg. In that case, any other listener for "sending-message" will never see them.

 * We don't have priorities for observers so OTR kind of wants to be alone listening on "sending-message" anyways to ensure it's last in the chain. Anything else altering the msgs there (when OTR is enabled) is playing with undefined behaviour.

 * With all that in mind, I think we should eliminate imISendableMessage and just keep imIOutgoingMessage, but change the conversation attribute to target. We can still provide the conversation there for "preparing-message" since imIOutgoingMessage inherits from prplIConversation (I hope that's right). Any fragmentation OTR does can inject messages (with target.sendMsg and OTRL_FRAGMENT_SEND_ALL_BUT_LAST as I had it before, since it already needs to do that above). This avoids having to loop over getSendableMessages(), which I find a little unpleasant in practice.

Comment 43

[Florian Quèze [:florian]]

(In reply to arlolra from comment #42)

>  * OTR does this thing where if you set a policy, REQUIRE_ENCRYPTION, it'll
> buffer messages internally until the authenticated key exchange is complete,
> and then send them off. In this case, no message is returned from libotr but
> you still want to call writeMessage to display them to the sender (and
> assume they'll be delivered eventually).

Wouldn't you want to display them once they are sent?

>  * We described in comment #14 that there're a few instances where OTR wants
> to send its own messages (call target.sendMsg) in response to having
> received a msg. In that case, any other listener for "sending-message" will
> never see them.
> 
>  * We don't have priorities for observers so OTR kind of wants to be alone
> listening on "sending-message" anyways to ensure it's last in the chain.
> Anything else altering the msgs there (when OTR is enabled) is playing with
> undefined behaviour.

These 2 points don't worry me too much.


>  * With all that in mind, I think we should eliminate imISendableMessage and
> just keep imIOutgoingMessage, but change the conversation attribute to
> target.

I don't see how that relates to the above.

> since imIOutgoingMessage inherits from prplIConversation (I hope that's
> right).

I hope you meant imIConversation inherits from prplIConversation, otherwise, no, that's not right.

Comment 44

[arlolra]

(In reply to Florian Quèze [:florian] [:flo] from comment #43)
> (In reply to arlolra from comment #42)
> 
> >  * OTR does this thing where if you set a policy, REQUIRE_ENCRYPTION, it'll
> > buffer messages internally until the authenticated key exchange is complete,
> > and then send them off. In this case, no message is returned from libotr but
> > you still want to call writeMessage to display them to the sender (and
> > assume they'll be delivered eventually).
> 
> Wouldn't you want to display them once they are sent?

Ideally, but I don't think there's any context provided with the ciphertext to say to which message it corresponds. I guess you can listen for when the AKE is complete and assume each subsequent messages pair up, but that's not always true. Depending on who initiates the AKE, you may be sending another response after going secure. Also, with fragmentation, you'd need to parse the head (?OTRv3,1,3) to determine where each message ends. 

Pidgin-otr seems to just assumes delivery.

> 
> >  * We described in comment #14 that there're a few instances where OTR wants
> > to send its own messages (call target.sendMsg) in response to having
> > received a msg. In that case, any other listener for "sending-message" will
> > never see them.
> > 
> >  * We don't have priorities for observers so OTR kind of wants to be alone
> > listening on "sending-message" anyways to ensure it's last in the chain.
> > Anything else altering the msgs there (when OTR is enabled) is playing with
> > undefined behaviour.
> 
> These 2 points don't worry me too much.
> 
> 
> >  * With all that in mind, I think we should eliminate imISendableMessage and
> > just keep imIOutgoingMessage, but change the conversation attribute to
> > target.
> 
> I don't see how that relates to the above.

Well, the above said,

"Any fragmentation OTR does can inject messages (with target.sendMsg and OTRL_FRAGMENT_SEND_ALL_BUT_LAST as I had it before, since it already needs to do that above)"

which means we don't really need the getSendableMessages() array, which is only significant difference between those two interfaces.

> 
> > since imIOutgoingMessage inherits from prplIConversation (I hope that's
> > right).
> 
> I hope you meant imIConversation inherits from prplIConversation, otherwise,
> no, that's not right.

Whoops, yes, that's what I meant. Sorry.

Comment 45

[arlolra]

Attachment: transform.patch from comment 44

Sorry for the extended delay. I added some comments to the interfaces, as requested, and removed the imISendableMessage interface as described in comment 44.

Updated the OTR extension to use this API and it seems to be working reasonably well.
https://github.com/arlolra/ctypes-otr/commit/b7f2305b75576b6b44ae43bd9278d99ac1a649e8

As always, your feedback is appreciated.

Comment 46

[Patrick Cloke [:clokep]]

Comment on attachment 8480381 [details] [diff] [review]
transform.patch from comment 44

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

This looks good! I made a few comments...half of which are nits. The other half are pretty much asking for more comments. My only major concern is  the ordering in the sendMsg function, see below.

::: chat/components/public/imIConversationsService.idl
@@ +85,5 @@
> +// opportunity to swap or cancel the message.
> +[scriptable, uuid(bd2f77d4-1fad-432d-a914-6bb5ed5c13d0)]
> +interface imIMessage: prplIMessage {
> +           attribute boolean cancelled;
> +           attribute AUTF8String decodedMessage;

Decoded message? That's pretty specific...what's that really mean anyway?

::: chat/components/public/prplIConversation.idl
@@ +45,5 @@
>  
>    /* Send a message in the conversation */
>    void sendMsg(in AUTF8String aMsg);
>  
> +  void prepareForSending(in imIOutgoingMessage message,

This needs a comment above it.

::: chat/components/src/imConversations.js
@@ +14,5 @@
>  XPCOMUtils.defineLazyGetter(this, "bundle", function()
>    Services.strings.createBundle("chrome://chat/locale/conversations.properties")
>  );
>  
> +function OutgoingMessage(message, conversation) {

Nit: Start parameters with "a", so:
function OutgoingMessage(aMessage, aConversation)

This is true for all functions.

@@ +39,5 @@
> +
> +  get message() this.prplMessage.message,
> +  set message(msg) { this.prplMessage.message = msg; },
> +
> +  // from prplIMessage

If we have the message...do we really need all these fields? (Does this just make the change less invasive in other places?)

@@ +325,5 @@
>           (aTopic == "update-typing" &&
>            this._prplConv[aTargetId].typingState == Ci.prplIConvIM.TYPING)))
>        this.target = this._prplConv[aTargetId];
> +
> +    if (aTopic == "new-text") {

I'm not entirely sure I understand this change.

@@ +359,5 @@
> +  sendMsg: function(aMsg) {
> +    // Add-ons (eg. pastebin) have an opportunity to cancel the message at this
> +    // point, or change the text content of the message.
> +    // If an add-on wants to split a message, it should truncate the first
> +    // message, and insert new messages using the conversation's sendMsg method.

Are we concerned about ordering here? What if I'm using the pastebin add-on, send a huge message (A) immediately followed by a very short message (B). If the pastebin add-on cancels the first message (A) and adds a new short message (A*), I'd end up sending B, A*; but the user wanted to send A / A*, B.

@@ +365,5 @@
> +    this.notifyObservers(om, "preparing-message");
> +    if (om.cancelled)
> +      return;
> +
> +    // Prpls have an opportunity here to preprocess messages before they are

I think we normally write this out in comments "Protocols".

::: chat/modules/imThemes.jsm
@@ +370,1 @@
>          msgClass.push("action");

I still wish this was a flag...but I'm certainly not going to ask you to change it. :)

::: chat/protocols/xmpp/xmpp.jsm
@@ +38,5 @@
>  );
>  
> +XPCOMUtils.defineLazyGetter(this, "TXTToHTML", function() {
> +  let cs = Cc["@mozilla.org/txttohtmlconv;1"].getService(Ci.mozITXTToHTMLConv);
> +  return function(aTXT) cs.scanTXT(aTXT, cs.kEntities);

Nit: aTxt.

@@ +251,5 @@
>  
> +  prepareForSending: function(aOm, aCount) {
> +    if (aCount)
> +      aCount.value = 1;
> +    return [TXTToHTML(aOm.message)];

While we're touching this, can you add a comment about why this is necessary please.

Comment 47

[arlolra]

Comment on attachment 8480381 [details] [diff] [review]
transform.patch from comment 44

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

Thanks for taking a look. I'll get this cleaned up shortly.

::: chat/components/public/imIConversationsService.idl
@@ +85,5 @@
> +// opportunity to swap or cancel the message.
> +[scriptable, uuid(bd2f77d4-1fad-432d-a914-6bb5ed5c13d0)]
> +interface imIMessage: prplIMessage {
> +           attribute boolean cancelled;
> +           attribute AUTF8String decodedMessage;

The content to display to the user.

::: chat/components/src/imConversations.js
@@ +39,5 @@
> +
> +  get message() this.prplMessage.message,
> +  set message(msg) { this.prplMessage.message = msg; },
> +
> +  // from prplIMessage

The latter (less invasive elsewhere).

@@ +325,5 @@
>           (aTopic == "update-typing" &&
>            this._prplConv[aTargetId].typingState == Ci.prplIConvIM.TYPING)))
>        this.target = this._prplConv[aTargetId];
> +
> +    if (aTopic == "new-text") {

The comment where the imIMessage interface is defined sums it up.

When a new message is to be displayed, we notify listening observers so they can make changes. In the OTR case, that's decrypting the message. The imMessage wraps a prplMessage to provide a field for cancelling the message (if it's not intended to be displayed to the user) or a place to put the decoded content (without making originalMessage editable).

@@ +359,5 @@
> +  sendMsg: function(aMsg) {
> +    // Add-ons (eg. pastebin) have an opportunity to cancel the message at this
> +    // point, or change the text content of the message.
> +    // If an add-on wants to split a message, it should truncate the first
> +    // message, and insert new messages using the conversation's sendMsg method.

There were past proposals (and patches) that did more to handle the async case.

We settled on pushing that business into the add-on. It has the responsibility of buffering message and assuring proper order.

Similarly, the OTR extension maintains a map of encrypted/decrypted messages so that it knows which plaintext message to display to the sender when the prpl waits for an async confirmation before emitting it as sent.

::: chat/protocols/xmpp/xmpp.jsm
@@ +251,5 @@
>  
> +  prepareForSending: function(aOm, aCount) {
> +    if (aCount)
> +      aCount.value = 1;
> +    return [TXTToHTML(aOm.message)];

Hmmm, on the left hand side this is being applied for the senders benefit, but here it's affecting both.

It's been some time but I seem to recall this being superfluous and should probably be removed entirely. Please correct me if I'm wrong.

Comment 48

[arlolra]

Attachment: transform.patch from comment 46

Updated based on clokep's review.

Comment 49

[Patrick Cloke [:clokep]]

Comment on attachment 8484485 [details] [diff] [review]
transform.patch from comment 46

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

Just a minor nit, overall it looks good! Thanks for updating this so fast. :)

::: chat/components/public/imIConversationsService.idl
@@ +86,5 @@
> +// eventually gets shown to the user.
> +[scriptable, uuid(bd2f77d4-1fad-432d-a914-6bb5ed5c13d0)]
> +interface imIMessage: prplIMessage {
> +           attribute boolean cancelled;
> +           attribute AUTF8String displayMessage;

Nit: Put the comment about what this field is right above the field.

Comment 50

[aleth [:aleth]]

Comment on attachment 8484485 [details] [diff] [review]
transform.patch from comment 46

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

This looks good to me overall!  Thanks for all your work on this.

::: chat/components/public/prplIConversation.idl
@@ +49,5 @@
> +  /* Preprocess messages before they are sent (eg. split long messages).
> +     Can return null if no changes are to be made. */
> +  void prepareForSending(in imIOutgoingMessage message,
> +                         [optional] out unsigned long messageCount,
> +                         [retval, array, size_is(messageCount)] out string messages);

Unless I am missing something, you probably need wstring here so Unicode messages work. Please test this!

::: chat/modules/jsProtoHelper.jsm
@@ +471,5 @@
>        }
>      }
>    },
>  
> +  prepareForSending: function(aOm, aCount) null,

Can we use something more descriptive than aOm? Is it short for aOriginalMessage?

::: chat/protocols/irc/irc.js
@@ +133,5 @@
>                        " :\r\n";
>      return this._account.maxMessageLength -
>             this._account.countBytes(baseMessage);
>    },
> +  prepareForSending: function(aOm, aCount) {

Same here, for legibility.

Comment 51

[Florian Quèze [:florian]]

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

> ::: chat/components/public/prplIConversation.idl
> @@ +49,5 @@
> > +  /* Preprocess messages before they are sent (eg. split long messages).
> > +     Can return null if no changes are to be made. */
> > +  void prepareForSending(in imIOutgoingMessage message,
> > +                         [optional] out unsigned long messageCount,
> > +                         [retval, array, size_is(messageCount)] out string messages);
> 
> Unless I am missing something, you probably need wstring here so Unicode
> messages work. Please test this!

AUTF8String for unicode strings.

string and wstring are deprecated, they cause the whole string to be memcpy'ed each time the xpconnect boundary is crossed.

Comment 52

[arlolra]

Attachment: transform.patch from 49-50

Fixed the nits and switched to wstring.

There was also a problem that system messages weren't being displayed. I moved the imMessage definition to the jsProtoHelper, and now emit them directly when the conversation is attached to a Message, rather than swapping in observeConv, which isn't watching for system messages from the conversation service itself. Hopefully that makes sense.

Comment 53

[Florian Quèze [:florian]]

Comment on attachment 8485224 [details] [diff] [review]
transform.patch from 49-50

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

I haven't re-read all the previous discussion, only looked at the code, and overall this looks pretty good. I just found one issue.

(In reply to arlolra from comment #52)

> I moved the imMessage definition to the jsProtoHelper, and now emit them
> directly when the conversation is attached to a Message, rather than
> swapping in observeConv,

This change isn't correct, sorry.

> which isn't watching for system messages from the
> conversation service itself.

Is it possible to wrap the system messages from the conversation service itself?

::: chat/components/src/imConversations.js
@@ +295,3 @@
>      this.notifyObservers(aSubject, aTopic, aData);
>      if (aTopic == "new-text") {
>        Services.obs.notifyObservers(aSubject, aTopic, aData);

Isn't this the place to do |new imMessage(aSubject)|?

::: chat/modules/jsProtoHelper.jsm
@@ +429,5 @@
>  }
>  Message.prototype = GenericMessagePrototype;
>  
>  
> +function imMessage(aPrplMessage) {

At first glance, this new object should be implemented in imConversations.js (otherwise the messages from libpurple protocol plugins won't benefit from this wrapping).

Comment 54

[arlolra]

Comment on attachment 8485224 [details] [diff] [review]
transform.patch from 49-50

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

> Is it possible to wrap the system messages from the conversation service itself?

Yeah, but it's ugly,

  systemMessage: function(aText, aIsError) {
    let flags = {system: true, noLog: true, error: !!aIsError};
    (new Message("system", aText, flags)).conversation = this;
  },

gets changed to,

  systemMessage: function(aText, aIsError) {
    let flags = {system: true, noLog: true, error: !!aIsError};
    let message = new Message("system", aText, flags)
    message._conversation = this;
    this.notifyObservers(new imMessage(message), "new-text", null);
  },

The reason for that is because setting imMessage.conversation invokes the setter on the prplMessage, which ends up calling notifyObservers with the prplMessage itself.

Maybe you can think of something cleaner.

::: chat/components/src/imConversations.js
@@ +295,3 @@
>      this.notifyObservers(aSubject, aTopic, aData);
>      if (aTopic == "new-text") {
>        Services.obs.notifyObservers(aSubject, aTopic, aData);

It has to be before notifying "received-message" above. But observeConv isn't observing the conversationService itself, so this doesn't get fired for system messages.

::: chat/modules/jsProtoHelper.jsm
@@ +429,5 @@
>  }
>  Message.prototype = GenericMessagePrototype;
>  
>  
> +function imMessage(aPrplMessage) {

Very true. I guess I should put it back.

Comment 55

[arlolra]

Attachment: transform.patch from comments 53-54

Reverted the changes in the last patch to jsProtoHelper and solved it a different way, by moving a few things from observeConv to notifyObservers. Hopefully that's acceptable.

Comment 56

[Patrick Cloke [:clokep]]

Comment on attachment 8485329 [details] [diff] [review]
transform.patch from comments 53-54

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

::: chat/components/src/imConversations.js
@@ +359,5 @@
> +    // Protocols can return null if they don't need to make any changes.
> +    // (nb. passing null with retval array results in an empty array)
> +    if (messages.length == 0) {
> +      messages = [om.message];
> +    }

Nit: No { } around single line statements.

Comment 57

[Patrick Cloke [:clokep]]

I just went to try this...and realized that this requires changes to purplexpcom too, I don't want to redo work so: have you looked at these changes at all, arlolra?

Comment 58

[arlolra]

Attachment: purple part

The related purple patch. Not quite sure this is right but seems to work.

Comment 59

[Florian Quèze [:florian]]

Comment on attachment 8488122 [details] [diff] [review]
purple part

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

::: purplexpcom/src/purpleConvChat.h
@@ +59,5 @@
>    }
>    NS_IMETHOD RemoveObserver(nsIObserver *aObserver) {
>      return purpleConversation::RemoveObserver(aObserver);
>    }
> +  NS_IMETHOD PrepareForSending(imIOutgoingMessage *message, uint32_t *messageCount, char16_t * **messages) {

Codying style:
- keep the "a" prefix for arguments.
- insert line breaks after "," to avoid having more than 80 chars per line.

::: purplexpcom/src/purpleConversation.cpp
@@ +151,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
> +/* void prepareForSending (in imIOutgoingMessage message, [optional] out unsigned long messageCount, [array, size_is (messageCount), retval] out wstring messages); */
> +NS_IMETHODIMP purpleConversation::PrepareForSending(imIOutgoingMessage *message, uint32_t *messageCount, char16_t * **messages)

Same coding style comments apply here.

@@ +153,5 @@
>  
> +/* void prepareForSending (in imIOutgoingMessage message, [optional] out unsigned long messageCount, [array, size_is (messageCount), retval] out wstring messages); */
> +NS_IMETHODIMP purpleConversation::PrepareForSending(imIOutgoingMessage *message, uint32_t *messageCount, char16_t * **messages)
> +{
> +  messages = NULL;

Real reason for the r-: this line has no effect.

You want:
  *aMessageCount = 0;
  *aMessages = nullptr;

Comment 60

[Florian Quèze [:florian]]

Comment on attachment 8485329 [details] [diff] [review]
transform.patch from comments 53-54

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

Looks good, thanks!

Given that aleth already said "This looks good to me overall!" in comment 50, I don't think we should wait for his review here.

Note: I haven't tried this, but comment 57 looks like Patrick intends to do it.

::: chat/components/src/imConversations.js
@@ +359,5 @@
> +    // Protocols can return null if they don't need to make any changes.
> +    // (nb. passing null with retval array results in an empty array)
> +    if (messages.length == 0) {
> +      messages = [om.message];
> +    }

I also saw this during my previous review but skipped it to not be annoying, as it doesn't matter all that much... but while we are here, I would probably write !messages.length rather than messages.length == 0.

Comment 61

[arlolra]

Attachment: transform-purple.patch

Thanks for the review.

Comment 62

[arlolra]

Attachment: transform.patch

Fixed up all the nits, coding conventions and the return value in purplexpcom PrepareForSending.

Thanks again for the review.

Comment 63

[Florian Quèze [:florian]]

Comment on attachment 8488366 [details] [diff] [review]
transform-purple.patch

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

::: purplexpcom/src/purpleConversation.cpp
@@ +152,5 @@
>  }
>  
> +/* void prepareForSending (in imIOutgoingMessage aMsg,
> +                    [optional] out unsigned long aMsgCount,
> +                    [array, size_is (aMsgCount), retval] out wstring aMsgs); */

The indent seems wrong here.

Comment 64

[arlolra]

Comment on attachment 8488366 [details] [diff] [review]
transform-purple.patch

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

::: purplexpcom/src/purpleConversation.cpp
@@ +152,5 @@
>  }
>  
> +/* void prepareForSending (in imIOutgoingMessage aMsg,
> +                    [optional] out unsigned long aMsgCount,
> +                    [array, size_is (aMsgCount), retval] out wstring aMsgs); */

Yeah ... but this seemed the most legible. I'd need to split up the last line to fit 80 chars:

 [array, size_is (aMsgCount), retval]
  out wstring aMsgs); */

Comment 65

[Florian Quèze [:florian]]

(In reply to arlolra from comment #64)

> Yeah ... but this seemed the most legible. I'd need to split up the last
> line to fit 80 chars:
> 
>  [array, size_is (aMsgCount), retval]
>   out wstring aMsgs); */

The 80 chars thing is more a guideline than a strict rule. It's not a problem to have a few more chars if that significantly improves legibility. In that case, the last parameter shouldn't be split.

Comment 66

[arlolra]

Attachment: transform-purple.patch

Comment 67

[arlolra]

I see. Ok, cleaned that up.

Comment 68

[Patrick Cloke [:clokep]]

I compiled and tested this and didn't see any issues. I'll commit this once I get approval.

Comment 69

[Patrick Cloke [:clokep]]

Thanks!

https://hg.mozilla.org/comm-central/rev/d2a0c6d324fa
http://hg.mozilla.org/users/florian_queze.net/purple/rev/d971cac153ee

Florian and I did have a conversation about how hard it would be to test this, any thoughts?

Comment 70

[arlolra]

clokep: It looks like you committed the wrong patches :(

It should have been the ones from comment 62 and comment 66, which are the non-obsolete ones from the top.

Comment 71

[Patrick Cloke [:clokep]]

Backouts:
https://hg.mozilla.org/comm-central/rev/a27468ae7846
http://hg.mozilla.org/users/florian_queze.net/purple/rev/ecb887e33c33

Updated versions:
https://hg.mozilla.org/comm-central/rev/e2c85d70fda2
http://hg.mozilla.org/users/florian_queze.net/purple/rev/6550fcf407f0

Thanks for catching that. I should have checked what I had locally before pushing.

Comment 72

[Patrick Cloke [:clokep]]

This busted a couple of tests, I pushed a fix because I'd rather see this stick than get backed out:
https://hg.mozilla.org/comm-central/rev/0c3c6ce0353f