955642 - Handle Twitter direct messages (DMs)

Status: RESOLVED FIXED

(Chat Core :: Twitter, enhancement)

Description

[Instantbot]

*** Original post on bio 2197 by deOmega <jahkae AT gmail.com> at 2013-10-03 18:11:00 UTC ***

I think it was discussed that this feature will be  implemented eventually, so hopefully this is a reminder.  I do not  see any way currently to initiate, view or respond to direct messages on twitter.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 2197 at 2013-10-03 18:16:13 UTC ***

Yes, this isn't currently supported, but definitely is wanted.

Comment 2

[Instantbot]

*** Original post on bio 2197 by deOmega <jahkae AT gmail.com> at 2013-10-04 12:20:57 UTC ***

Still the cleanest twitter client imo, but indeed, functionality is far too limited.

Comment 3

[arlolra]

Attachment: dm.patch

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8719092 [details] [diff] [review]
dm.patch

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

Thanks for the patch! I haven't had a chance to try this yet, but it looks pretty good. I left some comments, mostly nits and a few places that I think code can be further shared.

In the future when attaching large patches like this, it helps to give additional context, e.g.:
"In order to support Twitter Direct Messages, I added a new conversation type based on GenericConvIM. Since a lot of it shared implementation with the current Conversation class, I used the same technique the IRC code uses with Object.assign to share a bunch of methods between them. Besides adding the new conversation type, I had to:
* Plug into onDataAvailable to display the private messages.
* Set-up the mechanism to allow creating of new private messages."

::: chat/protocols/twitter/twitter.js
@@ -195,5 @@
> -      this.systemMessage(_("error.general", error, aMsg), true);
> -    }, this);
> -    this.sendTyping("");
> -  },
> -  sendTyping: function(aString) {

Doesn't this make sense to have for direct messages too?

@@ +273,5 @@
>      return text;
>    },
> +};
> +
> +function Conversation(aAccount) {

Nit: Seems like this file uses the style of putting the { on the next line!

It might be clearer to rename this to TimelineConversation.

@@ +420,5 @@
>  };
>  
> +Object.assign(Conversation.prototype, GenericTwitterConversation);
> +
> +function DM(aAccount, aName) {

Please call this DirectMessageConversation

@@ +424,5 @@
> +function DM(aAccount, aName) {
> +  this._init(aAccount, aName);
> +}
> +
> +DM.prototype = {

Nit: Don't put a blank line between declaring a classes' construction and it's prototype. Same goes for between the prototype and the Object.assign call.

@@ +426,5 @@
> +}
> +
> +DM.prototype = {
> +  __proto__: GenericConvIMPrototype,
> +  sendMsg: function (aMsg) {

Why does this not do the length checking we do for the timeline? I'd expect this method be identical, except call a different method on the account to actually send the tweet.

@@ +433,5 @@
> +      let error = this._parseError(aData);
> +      this.systemMessage(_("error.general", error, aMsg), true);
> +    }, this);
> +  },
> +  onSentCallback: function(aData) {

This is identical, right? Share it between the two implementations.

@@ +439,5 @@
> +    if (tweet.sender_screen_name != this.nick)
> +      throw "Wrong screen_name... Uh?";
> +    this.displayMessages([tweet]);
> +  },
> +  displayMessages: function(aMessages) {

Is displayMessages ever called here?

@@ +450,5 @@
> +      // account.setUserInfo(tweet.recipient);
> +      this.displayTweet(tweet);
> +    }
> +  },
> +  displayTweet: function(aTweet) {

I suspect this is shared between the two?

@@ +467,5 @@
> +
> +    (new Tweet(aTweet, name, text, flags)).conversation = this;
> +  },
> +  get nick() { return this._account.name; },
> +  set nick(aNick) {},

Nit: No trailing comma.

@@ +478,5 @@
>    this._init(aProtocol, aImAccount);
>    this._knownMessageIds = new Set();
>    this._userInfo = new Map();
>    this._friends = new Set();
> +  this._conversations = new Map();

Is this for ALL conversations or just direct messages? (Add a comment.)

@@ +649,5 @@
>    destroy: function(aTweet, aOnSent, aOnError, aThis) {
>      let url = "1.1/statuses/destroy/" + aTweet.id_str + ".json";
>      this.signAndSend(url, null, [], aOnSent, aOnError, aThis);
>    },
> +  dmTweet: function(aMsg, aName, aOnSent, aOnError, aThis) {

Just call this method |directMessage|, I think.

@@ +1187,5 @@
> +
> +  getConversation: function(aName) {
> +    if (!this._conversations.has(aName)) {
> +      this._conversations.set(aName, new DM(this, aName));
> +    }

Nit: No { } around single line statements.

@@ +1192,5 @@
> +    return this._conversations.get(aName);
> +  },
> +  createConversation: function(aName) {
> +    return this.getConversation(aName);
> +  },

Nit: No comma on trailing item.

Comment 5

[arlolra]

Comment on attachment 8719092 [details] [diff] [review]
dm.patch

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

Thanks for the review! And yeah, sorry for not leaving much context.

::: chat/protocols/twitter/twitter.js
@@ -195,5 @@
> -      this.systemMessage(_("error.general", error, aMsg), true);
> -    }, this);
> -    this.sendTyping("");
> -  },
> -  sendTyping: function(aString) {

Nope, there's no limit for DMs
https://blog.twitter.com/2015/removing-the-140-character-limit-from-direct-messages

@@ +426,5 @@
> +}
> +
> +DM.prototype = {
> +  __proto__: GenericConvIMPrototype,
> +  sendMsg: function (aMsg) {

Again, https://blog.twitter.com/2015/removing-the-140-character-limit-from-direct-messages

@@ +433,5 @@
> +      let error = this._parseError(aData);
> +      this.systemMessage(_("error.general", error, aMsg), true);
> +    }, this);
> +  },
> +  onSentCallback: function(aData) {

No, this one is using `tweet.sender_screen_name`, the other is `tweet.user.screen_name`.

@@ +439,5 @@
> +    if (tweet.sender_screen_name != this.nick)
> +      throw "Wrong screen_name... Uh?";
> +    this.displayMessages([tweet]);
> +  },
> +  displayMessages: function(aMessages) {

It's called on the line above and in the stream, `onDataAvailable`.

@@ +450,5 @@
> +      // account.setUserInfo(tweet.recipient);
> +      this.displayTweet(tweet);
> +    }
> +  },
> +  displayTweet: function(aTweet) {

It's similar but for the difference in defining `name` and `ensureParticipantExists` is omitted here.

We got push those checks into `displayMessages` though, and then share it.

@@ +478,5 @@
>    this._init(aProtocol, aImAccount);
>    this._knownMessageIds = new Set();
>    this._userInfo = new Map();
>    this._friends = new Set();
> +  this._conversations = new Map();

Just direct messages. Will add a comment.

Comment 7

[arlolra]

Attachment: dm.patch from comment 5

In this one,

* Fixed up the nits;
* Moved `displayTweet` to the `GenericTwitterConversation`;
* Added a `removeConversation` to the account for cleanup;
* Added an `unInit` to the `DirectMessageConversation` to call `removeConversation`.

Comment 8

[arlolra]

We probably need to move some of the stuff in displayTweet into a prepareForDisplaying method. Even still, Twitter substitutes links, which is going to violate the contract the otr extension expects :(

Comment 9

[arlolra]

Attachment: dm.patch from comment 8

Ok, moved that logic from displayTweet.

There's a small fix to the message transformation pipeline. We shouldn't be passing system message to be preparedForDisplaying.

Comment 10

[arlolra]

Actually, wait, we can do better for the otr extension. Another patch coming.

Comment 11

[arlolra]

Attachment: dm.patch from comment 10

Alrighty, this one gets us to a point where it works well with the OTR extension.

 * Updates the twitter-text.js
 * Displays what was passed to sendMsg, instead of what's returned from Twitter
 * Uses Twitter's autoLinker (rather than the custom parser) to add mentions and links

Going to put this down until you get a chance to look at it now.

Comment 12

[Florian Quèze [:florian]]

Needing to expose wrappedJSObject on an object is almost always the sign that something isn't right with the interfaces exposed by the object.

let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; doesn't look right at all.

From a very brief look (I haven't reviewed the whole patch), it seems you want to use the message id to lookup the internal message object.

Comment 13

[arlolra]

Thanks for taking a look.

> it seems you want to use the message id to lookup the internal message object.

Is there an API to do that?

Comment 14

[Florian Quèze [:florian]]

(In reply to arlolra from comment #13)
> Thanks for taking a look.
> 
> > it seems you want to use the message id to lookup the internal message object.
> 
> Is there an API to do that?

No, but each message automatically receives a unique ID: http://mxr.mozilla.org/comm-central/source/chat/modules/jsProtoHelper.jsm#387

It should be possible for a conversation to keep all the message ids in a map pointing to the internal message object.

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8721135 [details] [diff] [review]
dm.patch from comment 10

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

r-, but mostly some questions. At least the commented out code needs to go.

::: chat/components/src/imConversations.js
@@ +418,5 @@
>        aSubject = new imMessage(aSubject);
>        this.notifyObservers(aSubject, "received-message");
>        if (aSubject.cancelled)
>          return;
> +      if (!aSubject.system)

This change would need to be documented. Does this match what other messengers (e.g. Pidgin) do?

::: chat/protocols/twitter/twitter.js
@@ +53,5 @@
>    getActions: function(aCount) {
>      let account = this.conversation._account;
>      let actions = [];
>  
> +    // TODO(arlolra): DirectMessageConversation actions.

What actions do we expect here?

@@ +156,5 @@
> +    this.displayMessages([tweet]);
> +  },
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet;

This seems like a pretty terrible hack that will at least need a comment. Generally if you're reaching into wrappedJSObject something is wrong with the interface. Why do we need to do this?

@@ +181,5 @@
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {
> +      urlEntities: tweet.entities.urls.map(function(u) {
> +        var o = Object.assign(u);
> +        // But remove the indices so they apply in the face of modifications.
> +        delete o.indices;

Why this deletion of the indices here? Why was this code changed from previous version instead of just moved?

@@ +269,5 @@
>      this.notifyObservers(null, "status-text-changed",
>                           _("replyingToStatusText", aTweet.text));
>    },
>    reTweet: function(aTweet) {
> +    this._account.reTweet(aTweet, null, function(aException, aData) {

Why did we get rid of the callback here?

@@ +280,5 @@
>        this.systemMessage(_("error.tooLong"), true);
>        throw Cr.NS_ERROR_INVALID_ARG;
>      }
> +    this._account.tweet(aMsg, this.inReplyToStatusId,
> +                        this.onSentCallback.bind(this, aMsg),

This change seems very odd. Why are we binding to this?

@@ +363,5 @@
> +      if (!("sender" in tweet) || !("recipient" in tweet) ||
> +          !("text" in tweet) || !("id_str" in tweet))
> +        continue;
> +      // account.setUserInfo(tweet.sender);
> +      // account.setUserInfo(tweet.recipient);

What's with the commented out code?

@@ +732,5 @@
>          continue;
>        }
> +      if ("direct_message" in msg) {
> +        let dm = msg["direct_message"];
> +        if (dm.sender_screen_name !== this.name)  // These are displayed on send.

Do we display these based on a response we get from the server or do we just blindly display them?

Comment 16

[arlolra]

Comment on attachment 8721135 [details] [diff] [review]
dm.patch from comment 10

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

Thanks for taking a look.

::: chat/components/src/imConversations.js
@@ +418,5 @@
>        aSubject = new imMessage(aSubject);
>        this.notifyObservers(aSubject, "received-message");
>        if (aSubject.cancelled)
>          return;
> +      if (!aSubject.system)

I'm not sure I follow. This is just omitting system messages from being transformed. Alternatively, we could return early in prepareForDisplaying. But either way, we shouldn't be applying transformations to those messages. This is a bug fix, I think.

::: chat/protocols/twitter/twitter.js
@@ +53,5 @@
>    getActions: function(aCount) {
>      let account = this.conversation._account;
>      let actions = [];
>  
> +    // TODO(arlolra): DirectMessageConversation actions.

Good question. Doesn't seem like there are any.

@@ +156,5 @@
> +    this.displayMessages([tweet]);
> +  },
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet;

Yeah, flo was chastising me for this as well, https://bugzilla.mozilla.org/show_bug.cgi?id=955642#c12

We need the actual tweet here for all the uses in this function, see below.

I did the hacky thing just to get us this far. I'm happy to change if you can suggest what the canonical way of doing it would be.

@@ +181,5 @@
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {
> +      urlEntities: tweet.entities.urls.map(function(u) {
> +        var o = Object.assign(u);
> +        // But remove the indices so they apply in the face of modifications.
> +        delete o.indices;

There's a comment there, "// But remove the indices so they apply in the face of modifications."

Because of the transformations that may have happened before getting here, the indices may no longer align, and thus the entities won't be applied. By removing it, they're just generally applied wherever the pattern matches.

The previous version depended heavily on the indices, which may have been invalidated. Also, there's the added bonus that direct messages which were encrypted will now be linkified.

@@ +269,5 @@
>      this.notifyObservers(null, "status-text-changed",
>                           _("replyingToStatusText", aTweet.text));
>    },
>    reTweet: function(aTweet) {
> +    this._account.reTweet(aTweet, null, function(aException, aData) {

'cause it's a no-op. The tweet that's returned has already been displayed (that's how you were able to retweet it to begin with).

@@ +280,5 @@
>        this.systemMessage(_("error.tooLong"), true);
>        throw Cr.NS_ERROR_INVALID_ARG;
>      }
> +    this._account.tweet(aMsg, this.inReplyToStatusId,
> +                        this.onSentCallback.bind(this, aMsg),

It's documented in the onSentCallback above.

I discussed this on the bug, https://bugzilla.mozilla.org/show_bug.cgi?id=955642#c8

But also see, https://github.com/mozilla/releases-comm-central/blob/master/chat/components/public/imIConversationsService.idl#L72-L75

Mainly, we need the original text that's sent to pass the message transform pipeline in order not to violate the contract there. Twitter does some link substitution and whatnot, which we emulate in prepareForSending. All this only happens on the displaying side, of course.

@@ +363,5 @@
> +      if (!("sender" in tweet) || !("recipient" in tweet) ||
> +          !("text" in tweet) || !("id_str" in tweet))
> +        continue;
> +      // account.setUserInfo(tweet.sender);
> +      // account.setUserInfo(tweet.recipient);

Remnants from porting this method from the TimelineConversation. I probably wasn't sure if it was necessary when I was getting a handle on the code. Will fix.

@@ +732,5 @@
>          continue;
>        }
> +      if ("direct_message" in msg) {
> +        let dm = msg["direct_message"];
> +        if (dm.sender_screen_name !== this.name)  // These are displayed on send.

We display them if Twitter returns a success (ie. the onSentCallback is called).

Comment 17

[arlolra]

Attachment: dm.patch from comment 16

This splits out the twitter-text.jsm changes to its own patch, and removes the todo and uncomments the commented out code.

Comment 18

[Patrick Cloke [:clokep]]

Attachment: twitter-text.js update

Comment 19

[Patrick Cloke [:clokep]]

Attachment: dm patch

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8731014 [details] [diff] [review]
twitter-text.js update

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

I tested this briefly and it seemed to work fine!

Comment 21

[Patrick Cloke [:clokep]]

Comment on attachment 8731015 [details] [diff] [review]
dm patch

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

::: chat/protocols/twitter/twitter.js
@@ +53,5 @@
>    getActions: function(aCount) {
>      let account = this.conversation._account;
>      let actions = [];
>  
> +    if (!this.conversation.isChat) {

Be explicit in this block. set the value to 0. This should also be the first thing at the top of getActions. It does not depend on "account" or "actions" being set.

@@ +58,5 @@
> +      if (aCount)
> +        aCount.value = actions.length;
> +      return actions;
> +    }
> +    else if (account.connected) {

Nit: No else after a return.

@@ +145,5 @@
> +    if (aDate)
> +      flags.time = aDate;
> +    this.writeMessage("twitter.com", aMessage, flags);
> +  },
> +  onSentCallback: function(aMsg, aData) {

I'd like Florian to look at this method and prepareForDisplaying.

@@ +181,5 @@
> +      text = text.slice(0, offset) + retweet.text;
> +    }
> +
> +    // Pass in the url entities so the t.co links are replaced.
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {

I still don't understand the changes made to this method. Are they just an improvement to use Twitter's API or are they required for some reason?

@@ +195,5 @@
> +  },
> +  displayTweet: function(aTweet, aUser) {
> +    let name = aUser.screen_name;
> +
> +    let flags = name == this.nick ? {outgoing: true} : {incoming: true};

Why this change? (Checking against this.nick instead of this._account.name.) Doesn't this.nick only exist for MUCs?

@@ +205,5 @@
> +        Array.isArray(aTweet.entities.user_mentions) &&
> +        aTweet.entities.user_mentions.some(mention => mention.screen_name == this.nick))
> +      flags.containsNick = true;
> +
> +    (new Tweet(aTweet, name, aTweet.text, flags)).conversation = this;

Where did parseTweet go? What about the call to _ensureParticipantExists?

@@ +387,5 @@
>    this._knownMessageIds = new Set();
>    this._userInfo = new Map();
>    this._friends = new Set();
> +  // Contains just `DirectMessageConversation`s
> +  this._conversations = new Map();

I'm wondering if this should be a NormalizedMap.

Comment 22

[arlolra]

Comment on attachment 8731015 [details] [diff] [review]
dm patch

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

Thanks for taking a look.

::: chat/protocols/twitter/twitter.js
@@ +53,5 @@
>    getActions: function(aCount) {
>      let account = this.conversation._account;
>      let actions = [];
>  
> +    if (!this.conversation.isChat) {

Ok

@@ +181,5 @@
> +      text = text.slice(0, offset) + retweet.text;
> +    }
> +
> +    // Pass in the url entities so the t.co links are replaced.
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {

Did you read all the comments I left on the last patch? I answered that there pretty thoroughly.

@@ +195,5 @@
> +  },
> +  displayTweet: function(aTweet, aUser) {
> +    let name = aUser.screen_name;
> +
> +    let flags = name == this.nick ? {outgoing: true} : {incoming: true};

On both conversation types, nick is defined as,

`get nick() { return this._account.name; }`

@@ +205,5 @@
> +        Array.isArray(aTweet.entities.user_mentions) &&
> +        aTweet.entities.user_mentions.some(mention => mention.screen_name == this.nick))
> +      flags.containsNick = true;
> +
> +    (new Tweet(aTweet, name, aTweet.text, flags)).conversation = this;

The function of parseTweet is now served in prepareForDisplaying.

_ensureParticipantExists was moved to displayMessages, just before the call to displayTweet (L320), since it only applies in the TimelineConversation and displayTweet is shared.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8731015 [details] [diff] [review]
dm patch

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

::: chat/protocols/twitter/twitter.js
@@ +146,5 @@
> +      flags.time = aDate;
> +    this.writeMessage("twitter.com", aMessage, flags);
> +  },
> +  onSentCallback: function(aMsg, aData) {
> +    // The conversation may have been unitialized in the time it takes for

Is this comment saying that if the user sends a message and then immediately closes the conversation tab, it's fine to not record the message in the log?

@@ +148,5 @@
> +  },
> +  onSentCallback: function(aMsg, aData) {
> +    // The conversation may have been unitialized in the time it takes for
> +    // the async callback to fire.  Use `_observers` as a proxy for uninit'd.
> +    if (!Array.isArray(this._observers))

Isn't if (!this._observers) doing the same thing?

@@ +155,5 @@
> +    let tweet = JSON.parse(aData);
> +    // The OTR extension requires that the protocol not modify the message
> +    // (see the notes at `imIOutgoingMessage`).  That's the contract we made.
> +    // Unfortunately, Twitter trims tweets and substitutes links.
> +    tweet.text = aMsg;

I'm confused by this. If the OTR-encrypted message we sent is modified by the twitter server, isn't it going to be garbage on the receiving end?

@@ +160,5 @@
> +    this.displayMessages([tweet]);
> +  },
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet;

No.

Comment 24

[arlolra]

Comment on attachment 8731015 [details] [diff] [review]
dm patch

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

Thanks for taking a look, flo.

::: chat/protocols/twitter/twitter.js
@@ +146,5 @@
> +      flags.time = aDate;
> +    this.writeMessage("twitter.com", aMessage, flags);
> +  },
> +  onSentCallback: function(aMsg, aData) {
> +    // The conversation may have been unitialized in the time it takes for

The situation where this arose was when you close an OTR conversation, the add-on automatically sends a disconnect message so the other side can end its session. You probably haven't seen this in practice because a) the default for closing a chat is to put it on hold, b) I don't think any of the other protocols do async sends.

However, this problem exists in the code *before* this patch. The reason why I added this comment / workaround was to avoid an error in the console for the above case. However, you can probably reproduce this in a 1-1 conversation with high enough roundtrip latencies or using keyboard shortcuts to close.

I'm not saying it's ok, but fixing it might not be straightforward.

@@ +148,5 @@
> +  },
> +  onSentCallback: function(aMsg, aData) {
> +    // The conversation may have been unitialized in the time it takes for
> +    // the async callback to fire.  Use `_observers` as a proxy for uninit'd.
> +    if (!Array.isArray(this._observers))

Yup

@@ +155,5 @@
> +    let tweet = JSON.parse(aData);
> +    // The OTR extension requires that the protocol not modify the message
> +    // (see the notes at `imIOutgoingMessage`).  That's the contract we made.
> +    // Unfortunately, Twitter trims tweets and substitutes links.
> +    tweet.text = aMsg;

The encrypted messages get through just fine because they're base64 encoded. It's the unencrypted messages that Twitter modifies, that would end up being dropped by the OTR add-on.


The situation where you're having a 1-1 conversation with the add-on enabled but not currently in an OTR session.

@@ +160,5 @@
> +    this.displayMessages([tweet]);
> +  },
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet;

I had previously asked what's an acceptable way of doing this. Describe what you'd like to see and I'll code it up.

Comment 25

[Florian Quèze [:florian]]

(In reply to arlolra from comment #24)

> @@ +155,5 @@
> > +    let tweet = JSON.parse(aData);
> > +    // The OTR extension requires that the protocol not modify the message
> > +    // (see the notes at `imIOutgoingMessage`).  That's the contract we made.
> > +    // Unfortunately, Twitter trims tweets and substitutes links.
> > +    tweet.text = aMsg;
> 
> The encrypted messages get through just fine because they're base64 encoded.
> It's the unencrypted messages that Twitter modifies, that would end up being
> dropped by the OTR add-on.
> 
> 
> The situation where you're having a 1-1 conversation with the add-on enabled
> but not currently in an OTR session.

I hope having the add-on enabled doesn't cause all messages to be modified when there's nothing OTR related going on. Is this for the case when the magic whitespace tags are being added?

> @@ +160,5 @@
> > +    this.displayMessages([tweet]);
> > +  },
> > +  prepareForDisplaying: function(aMsg) {
> > +    let text = aMsg.displayMessage;
> > +    let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet;
> 
> I had previously asked what's an acceptable way of doing this. Describe what
> you'd like to see and I'll code it up.

I did in comment 14.

Comment 26

[arlolra]

> I hope having the add-on enabled doesn't cause all messages to be modified when there's nothing OTR related going on. Is this for the case when the magic whitespace tags are being added?

The whitespace tags are only added for one roundtrip (message to contact with tags, unencrypted response), after that they aren't applied. And, for Twitter I've disabled them altogether, because Twitter will strip them.

The messages aren't normally modified, but between sending and displaying are stored (indexed by their content (the libpurple limitation)) to determine whether they should be displayed or are internal messages the user shouldn't see. Remember we don't have any stored context on the messages after calling sendMsg on the prpl.

> I did in comment 14.

Ok, I'm not sure why I wasn't clear that was what you wanted. Sorry, and thanks!

Comment 27

[arlolra]

Attachment: dm patch from comment 26

This one keeps a map of message ids to internal messages on the conversation to access while preparing to display.

Comment 28

[arlolra]

****, I forgot the nits from last one.

Comment 29

[arlolra]

Attachment: dm patch from comment 28

Comment 30

[aleth [:aleth]]

Comment on attachment 8731481 [details] [diff] [review]
dm patch from comment 28

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

It would probably be a lot easier to understand this patch if it was split in two: 1) Twitter DMs and 2) Make Twitter work well with OTR.

::: chat/protocols/twitter/twitter.js
@@ +161,5 @@
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    if (!this._tweets.has(aMsg.id))
> +      return;
> +    let tweet = this._tweets.get(aMsg.id)._tweet;

Can you remove the entry now you have retrieved it, or will it be needed again?

This needs an explanatory comment saying why exactly this stuff is done in prepareForDisplaying.

Comment 31

[aleth [:aleth]]

Comment on attachment 8731481 [details] [diff] [review]
dm patch from comment 28

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

::: chat/protocols/twitter/twitter.js
@@ -230,5 @@
> -        error = "(" + error + ")";
> -    } catch(e) {}
> -    return error;
> -  },
> -  parseTweet: function(aTweet) {

(In reply to arlolra from comment #11)
>  * Uses Twitter's autoLinker (rather than the custom parser) to add mentions
> and links

So basically instead of using the server-parsed data twitter sends us with each tweet, you want to regenerate that info using the JS library? For potential regression testing alone, that change should probably be separate, especially as there are no tests...

Comment 32

[arlolra]

Comment on attachment 8731481 [details] [diff] [review]
dm patch from comment 28

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

::: chat/protocols/twitter/twitter.js
@@ +161,5 @@
> +  prepareForDisplaying: function(aMsg) {
> +    let text = aMsg.displayMessage;
> +    if (!this._tweets.has(aMsg.id))
> +      return;
> +    let tweet = this._tweets.get(aMsg.id)._tweet;

It can be removed; good point.

I'm not sure about the comment though. Isn't that the point of prepareForDisplaying, to postprocess messages before they are displayed?

@@ -230,5 @@
> -        error = "(" + error + ")";
> -    } catch(e) {}
> -    return error;
> -  },
> -  parseTweet: function(aTweet) {

Yes, if any modification happens to the tweet, the offsets in the server parsed data won't line up and will be dropped.

The JS library is spec'd and developed by Twitter, so I'm not expecting the results to differ.

I could pull that out but, as you said, there're no tests anyways.

Comment 33

[arlolra]

In an effort to revive this review, I've broken the patch down to bite sized chunks in,
https://github.com/arlolra/releases-comm-central/commits/dm

Hopefully we can get everything landed soon, save for the last one, which is probably the most controversial (albeit necessary for my use case).

Thanks for taking a look.

Comment 34

[arlolra]

Attachment: 0001-Update-twitter-text.jsm.patch [checked-in comment 45]

Comment 35

[arlolra]

Attachment: 0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55]

Comment 36

[arlolra]

Attachment: 0003-Pull-out-shared-methods-from-TimelineConversation.patch [checked-in comment 45]

Comment 37

[arlolra]

Attachment: 0004-Handle-uninitialized-conversations-on-async-callback.patch [checked-in comment 45]

Comment 38

[arlolra]

Attachment: 0005-Make-displayMessages-a-method-of-the-TimelineConvers.patch [checked-in comment 45]

Comment 39

[arlolra]

Attachment: 0006-Retweets-are-already-displayed.patch [checked-in comment 45]

Comment 40

[arlolra]

Attachment: 0007-Implement-DirectMessageConversation.patch

Comment 41

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch

Comment 42

[Patrick Cloke [:clokep]]

Comment on attachment 8768515 [details] [diff] [review]
0007-Implement-DirectMessageConversation.patch

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

This works and it looks pretty good! There's a couple of minor requests, but I don't think any of them are too complicated.

::: chat/protocols/twitter/twitter.js
@@ +51,4 @@
>    __proto__: GenericMessagePrototype,
>    _deleted: false,
>    getActions: function(aCount) {
> +    if (!this.conversation.isChat) {

Please add a comment above this saying that direct messages have no actions.

@@ -145,5 @@
>        return;
>  
>      let tweet = JSON.parse(aData);
> -    if (tweet.user.screen_name != this._account.name)
> -      throw "Wrong screen_name... Uh?";

Why are we removing this check?

@@ +440,5 @@
> +    for (let tweet of aMessages) {
> +      if (!("sender" in tweet) || !("recipient" in tweet) ||
> +          !("text" in tweet) || !("id_str" in tweet))
> +        continue;
> +      account.setUserInfo(tweet.sender);

I assume this is to store the user info, but tooltips on direct message conversations don't seem to work. Would it be hard to implement that?

@@ +811,5 @@
>          continue;
>        }
> +      if ("direct_message" in msg) {
> +        let dm = msg["direct_message"];
> +        if (dm.sender_screen_name !== this.name)  // These are displayed on send.

I think this was blocking testing when I was trying to DM myself from the website. But that's kind of a stupid use case.

Comment 43

[arlolra]

Comment on attachment 8768515 [details] [diff] [review]
0007-Implement-DirectMessageConversation.patch

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

::: chat/protocols/twitter/twitter.js
@@ +51,4 @@
>    __proto__: GenericMessagePrototype,
>    _deleted: false,
>    getActions: function(aCount) {
> +    if (!this.conversation.isChat) {

Ok

@@ -145,5 @@
>        return;
>  
>      let tweet = JSON.parse(aData);
> -    if (tweet.user.screen_name != this._account.name)
> -      throw "Wrong screen_name... Uh?";

onSentCallback is called for Tweets and DMs now, which don't have the same structure.  So, this would fail for every DM.

Also, in 0006-Retweets-are-already-displayed.patch, we removed ReTweets from calling onSentCallback, since they're already displayed.

But otherwise, I'm not sure how that case can arise.  In other words, I don't think it's actually preventing anything useful.

@@ +440,5 @@
> +    for (let tweet of aMessages) {
> +      if (!("sender" in tweet) || !("recipient" in tweet) ||
> +          !("text" in tweet) || !("id_str" in tweet))
> +        continue;
> +      account.setUserInfo(tweet.sender);

Can you explain what you mean by tooltips?  Is that the same as actions?  Because above we said that it doesn't have any actions.

@@ +811,5 @@
>          continue;
>        }
> +      if ("direct_message" in msg) {
> +        let dm = msg["direct_message"];
> +        if (dm.sender_screen_name !== this.name)  // These are displayed on send.

Yeah, I'm inclined to not support that.

Comment 44

[arlolra]

Attachment: 0007-Implement-DirectMessageConversation.patch from comment 43 [checked-in comment 45]

Comment 45

[Patrick Cloke [:clokep]]

Landed parts 0001, 0003, 0004, 0005, 0006, and, 0007.

https://hg.mozilla.org/comm-central/rev/803b13b2bf60
https://hg.mozilla.org/comm-central/rev/de9e1f18b4d9
https://hg.mozilla.org/comm-central/rev/050354367d67
https://hg.mozilla.org/comm-central/rev/cacce2042fc9
https://hg.mozilla.org/comm-central/rev/81f331660687
https://hg.mozilla.org/comm-central/rev/f885abf8aac7

Comment 46

[aleth [:aleth]]

Comment on attachment 8768513 [details] [diff] [review]
0006-Retweets-are-already-displayed.patch [checked-in comment 45]

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

::: chat/protocols/twitter/twitter.js
@@ +346,4 @@
>                           _("replyingToStatusText", aTweet.text));
>    },
>    reTweet: function(aTweet) {
> +    this._account.reTweet(aTweet, null, function(aException, aData) {

So now there will be no user feedback at all when retweeting? Won't current users (given their expectations) experience this as broken?

Comment 47

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #46)
> Comment on attachment 8768513 [details] [diff] [review]
> 0006-Retweets-are-already-displayed.patch [checked-in comment 45]
> 
> Review of attachment 8768513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/twitter/twitter.js
> @@ +346,4 @@
> >                           _("replyingToStatusText", aTweet.text));
> >    },
> >    reTweet: function(aTweet) {
> > +    this._account.reTweet(aTweet, null, function(aException, aData) {
> 
> So now there will be no user feedback at all when retweeting? Won't current
> users (given their expectations) experience this as broken?

Please see comment 16. The retweet is returned as part of the timeline, so no reason to run a callback on success.

Comment 48

[Patrick Cloke [:clokep]]

Comment on attachment 8768509 [details] [diff] [review]
0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55]

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

Florian, can you just make sure this isn't crazy?

Comment 49

[arlolra]

Any chance this'll get reviewed in time for esr52?

Comment 50

[aleth [:aleth]]

arlolra: If you need a review from florian your best bet is to ping him on IRC ;)

Comment 51

[arlolra]

aleth: can I bother you to ping him on my behalf?  I won't be able to be on IRC for at least the next week.

Comment 52

[Florian Quèze [:florian]]

Comment on attachment 8768509 [details] [diff] [review]
0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55]

(In reply to Patrick Cloke [:clokep] from comment #48)
> Comment on attachment 8768509 [details] [diff] [review]
> 0002-System-messages-shouldn-t-need-preparation-for-displ.patch
> 
> Review of attachment 8768509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Florian, can you just make sure this isn't crazy?

Doesn't look crazy to me.

Comment 53

[Florian Quèze [:florian]]

Comment on attachment 8768516 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch

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

Removing the review flag for now; I'm really not convinced this isn't going to regress the tooltips of @mentions and #hashtags.

::: chat/protocols/twitter/twitter.js
@@ -241,5 @@
> -        entArray = entArray.concat(entities.user_mentions.map(um => ({
> -          start: um.indices[0],
> -          end: um.indices[1],
> -          str: "@" + um.screen_name,
> -          text: '@<span class="ib-person">' + um.screen_name + "</span>",

I don't see how this could be handled by twitter-text.jsm.

The only function there that uses the screen_name field is autoLinkWithJSON, and we are not calling it.

Comment 54

[arlolra]

> 11:11:23 AM - flo-retina: arlolra: the main review comment is: before reviewing line by line, I would
> like to be sure the behavior stays the same or similar (ie. doesn't regress) when hovering and clicking
> links, @mentions and #hashtags

Thanks, I'll rebase and address that.

clokep: can we check-in 0002-System-messages-shouldn-t-need-preparation-for-displ.patch now?

Comment 55

[Patrick Cloke [:clokep]]

(In reply to arlolra from comment #54)
> > 11:11:23 AM - flo-retina: arlolra: the main review comment is: before reviewing line by line, I would
> > like to be sure the behavior stays the same or similar (ie. doesn't regress) when hovering and clicking
> > links, @mentions and #hashtags
> 
> Thanks, I'll rebase and address that.
> 
> clokep: can we check-in
> 0002-System-messages-shouldn-t-need-preparation-for-displ.patch now?

attachment 8768509 [details] [diff] [review] checked in https://hg.mozilla.org/comm-central/rev/a5f17a18f5229d61317e409d7daf7f67b1c4638d

Comment 56

[arlolra]

Thanks.

Comment 57

[arlolra]

Comment on attachment 8768516 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch

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

::: chat/protocols/twitter/twitter.js
@@ -241,5 @@
> -        entArray = entArray.concat(entities.user_mentions.map(um => ({
> -          start: um.indices[0],
> -          end: um.indices[1],
> -          str: "@" + um.screen_name,
> -          text: '@<span class="ib-person">' + um.screen_name + "</span>",

Adding `usernameClass: 'ib-person',` to the `twttr.txt.autoLink` options fixes the tooltips.

Comment 58

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 57

Rebased and verified that "hovering and clicking links, @mentions and #hashtags" all work.

Comment 59

[Florian Quèze [:florian]]

Comment on attachment 8807700 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 57

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

Looks reasonable, thanks. In the future, please attach patches with 8 lines of context.

I wonder if there are cases where we should clear this._tweets. I guess it's empty most of the time, and it would be a bug if it starts accumulating data. Am I understanding correctly? Should we warn somewhere if it's not empty? When the account gets disconnected maybe? (just random thoughts, maybe it's fine as is)

::: chat/protocols/twitter/twitter.js
@@ +244,5 @@
>  
>      this._account.LOG("Tweet: " + text);
>  
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {
> +      usernameClass: 'ib-person',

nit: double quotes

@@ +247,5 @@
> +    aMsg.displayMessage = twttr.txt.autoLink(text, {
> +      usernameClass: 'ib-person',
> +      // Pass in the url entities so the t.co links are replaced.
> +      urlEntities: tweet.entities.urls.map(function(u) {
> +        var o = Object.assign(u);

nit: let

Comment 60

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 59

Fixed nits.

> it would be a bug if it starts accumulating data. Am I understanding correctly?

Yes, and we have tests that prepareForDisplaying is called after setting the conversation on the message.

> Should we warn somewhere if it's not empty? When the account gets disconnected maybe? (just random thoughts, maybe it's fine as is)

We could, but probably unnecessary.

Comment 61

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a8855a65a9c7acc41a30a5b323cad62ac6a44438
Bug 955642 - Make DMs work with the OTR extension. r=florian

Comment 62

[aleth [:aleth]]

Apparently the last checkin led to some JS errors:

16:38:13.078 [Exception... "[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]'[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]' when calling method: [prplIConversation::prepareForDisplaying]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://gre/components/imConversations.js :: UIConversation.prototype.notifyObservers :: line 422"  data: yes] 1 jsProtoHelper.jsm:484
16:38:33.182 ReferenceError: aTweet is not defined 1 twitter.js:213:1
16:38:33.182 NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]'[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]' when calling method: [prplIConversation::prepareForDisplaying] 1 imConversations.js:422

Can you fix this? Or we can back out the patch for now.

Comment 63

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/f04314c15550d64b97669c37b408e988bb7ed8b5
Bug 955642 - Followup to fix missed variable name changes in a8855a65a9c7. rs=bustage-fix

Comment 64

[aleth [:aleth]]

Just a sloppy search/replace by the looks of it.

Comment 65

[arlolra]

****, sorry aleth.  Thanks for fixing :(

Comment 66

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/3bbc1b01ea13a5b280e635d0c3109e35bbc7aa46
Backed out changeset f04314c15550 (Bug 955642) for tooltip regression. rs=bustage-fix

https://hg.mozilla.org/comm-central/rev/b10948d0a008d21ccf941acc2bca0f44973294fd
Backed out changeset a8855a65a9c7 (Bug 955642) for tooltip regression. rs=bustage-fix DONTBUILD

Comment 67

[aleth [:aleth]]

Tooltips on @mentions in tweets are broken.

Comment 68

[arlolra]

> Tooltips on @mentions in tweets are broken.

Broken in what way?  They worked fine in all my testing.

Comment 69

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #67)
> Tooltips on @mentions in tweets are broken.

If you hover over @somename in a tweet the tooltip for somename does not appear. I know you added code for this, but it doesn't seem to be working in the latest TB daily.

Comment 70

[arlolra]

Ahh, so I think the problem is probably from,
https://github.com/mozilla/releases-comm-central/commit/c92d89dc7102f9d502f0e048904aba4a0e60ef5c

The reason is that the ib-person class is now on the a instead of wrapped span. Doing this fixes it,

diff --git a/chat/modules/imContentSink.jsm b/chat/modules/imContentSink.jsm
index e03c5bb..d7cc9af 100644
--- a/chat/modules/imContentSink.jsm
+++ b/chat/modules/imContentSink.jsm
@@ -80,7 +80,8 @@ var kStandardMode = {
     'div': true,
     'a': {
       'title': true,
-      'href': kAllowedURLs
+      'href': kAllowedURLs,
+      'class': kAllowedMozClasses
     },
     'em': true,
     'strong': true,
@@ -117,7 +118,8 @@ var kPermissiveMode = {
     'div': true,
     'a': {
       'title': true,
-      'href': kAllowedURLs
+      'href': kAllowedURLs,
+      'class': kAllowedMozClasses
     },
     'font': {
       'face': true,

Comment 71

[aleth [:aleth]]

(In reply to arlolra from comment #70)
> The reason is that the ib-person class is now on the a instead of wrapped
> span. Doing this fixes it,

I see. You'll need r? from flo for a content policy change.

I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing?

Comment 72

[arlolra]

> Ahh, so I think the problem is probably from,
> https://github.com/mozilla/releases-comm-central/commit/c92d89dc7102f9d502f0e048904aba4a0e60ef5c

Hmm, no, reverting that doesn't change anything.  I'm not sure why it was working for me at the time I made the patch.

> I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing?

Maybe instead of the content policy change I should patch the twitter-text library to add the span and wrap the @ as well.

Comment 73

[aleth [:aleth]]

(In reply to arlolra from comment #72)
> Maybe instead of the content policy change I should patch the twitter-text
> library to add the span and wrap the @ as well.

If possible it would be best to avoid this, as it makes updating the library painful.

Comment 74

[arlolra]

> I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing?

Need to set the option, { usernameIncludeSymbol: true }

Comment 75

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 74

Comment 76

[aleth [:aleth]]

Comment on attachment 8812495 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 74

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

This works, but I'll leave the imContentSink review to flo.

Comment 77

[Florian Quèze [:florian]]

It seems ib-person is the only class we want to allow on <a> tags, and the moz-txt-* classes are irrelevant there, should we filter for just ib-person then?

Comment 78

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 77

Comment 79

[Florian Quèze [:florian]]

Comment on attachment 8812808 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 77

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

::: chat/content/imtooltip.xml
@@ +477,5 @@
> +           // which includes the at symbol.  Also, we want to show tooltips
> +           // in DMs as well.
> +           if (conv.account.protocol.normalizedName === "twitter") {
> +             let textContent = elt.textContent.replace(/^@/, "");
> +             return updateTooltipFromParticipant(textContent, conv);

Are we sure updateTooltipFromParticipant works fine for a non-chat conversation?

I see it calls getParticipant and getNormalizedChatBuddyName on the conversation object.

::: chat/modules/imContentSink.jsm
@@ +47,4 @@
>    aClassName => aClassName == "moz-txt-underscore" ||
>                  aClassName == "moz-txt-tag" ||
>                  aClassName == "ib-person";
> +var kAllowedAClasses = aClassName => aClassName == "ib-person";

Whenever I read this variable name I miss the 'A', maybe we could name this 'kAllowedAnchorClasses' instead?

Comment 80

[aleth [:aleth]]

Comment on attachment 8812808 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 77

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

::: chat/content/imtooltip.xml
@@ +477,5 @@
> +           // which includes the at symbol.  Also, we want to show tooltips
> +           // in DMs as well.
> +           if (conv.account.protocol.normalizedName === "twitter") {
> +             let textContent = elt.textContent.replace(/^@/, "");
> +             return updateTooltipFromParticipant(textContent, conv);

Indeed, this doesn't work, as those functions are not defined. You get a tooltip, but for the wrong nick.

If you would like to support these tooltips in DMs, you will have to add a separate and modified version of updateTooltipFromParticipant.

Comment 81

[arlolra]

> If you would like to support these tooltips in DMs, you will have to add a separate and modified version of updateTooltipFromParticipant.

Is that something you want?  Personally, I don't, because it kind of leaks a limited amount of data from OTR conversations.

Comment 82

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 81

Comment 83

[aleth [:aleth]]

Comment on attachment 8812889 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 81

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

::: chat/content/imtooltip.xml
@@ +476,5 @@
> +           let textContent = elt.textContent;
> +           // The `ib-person` class is placed on the link for Twitter,
> +           // which includes the at symbol.
> +           if (conv.account.protocol.normalizedName === "twitter")
> +             textContent = textContent.replace(/^@/, "");

Instead adding protocol specific code here in the frontend, just strip any leading @ in twitter's getNormalizedChatBuddyName(). (I don't think @@nicks are allowed, right?) That should have the same effect.

Comment 84

[arlolra]

Attachment: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86]

Comment 85

[aleth [:aleth]]

Comment on attachment 8813013 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86]

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

Does this address all your concerns flo?

Comment 86

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ea7f01bba3959be9ed9052ffa5eb6d96bfa8a484
Bug 955642 - Make DMs work w/ the OTR extension and use twitter-text to parse entities. r=aleth,florian

Comment 87

[aleth [:aleth]]

OK via IRC.

Comment 88

[aleth [:aleth]]

Comment on attachment 8813013 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86]

[Approval Request Comment]
User impact if declined: none, but all the other parts of this are in 52, so let's avoid confusion in the future.
Testing completed (on c-c, etc.): yes

Comment 89

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #88)
> [Approval Request Comment]
> User impact if declined: none, but all the other parts of this are in 52, so
> let's avoid confusion in the future.
> Testing completed (on c-c, etc.): yes

Well, "none" is only strictly true if the OTR addon doesn't exist for Thunderbird. Does it?

Comment 90

[arlolra]

It's still an open issue https://github.com/arlolra/ctypes-otr/issues/70, so no.

But I have local branch that's getting close.  I was hoping to finish it up over the holidays.

Comment 91

[Jorg K (CEST = GMT+2)]

Comment on attachment 8813013 [details] [diff] [review]
0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86]

Please don't land bugs crossing cycles and then request uplift for the second part. Since the target milestone was 52 here, I don't see this bug in my Aurora query.

At least NI me or something.

Comment 92

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/7a2d9b9abd056a134e182d64ebdede2289233990

I landed this now separately before I forget since no "needs landing" query is picking this up. Strangely Mark Banner's/Kent's queries were picking up the "approval-comm-aurora?" before, but once approved, the bug disappeared and I had to dig it out from my FF history.