1070953 - Message style preview is broken

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[aleth [:aleth]]

Timestamp: 22/09/2014 14:55:10
Error: TypeError: msg is undefined
Source File: chrome://chat/content/convbrowser.xml
Line: 484

Looks like a regression from bug 982580.

Comment 1

[aleth [:aleth]]

Bah, I meant Bug 983347 of course.

Comment 2

[arlolra]

Attachment: message-style.patch

Comment 3

[aleth [:aleth]]

Comment on attachment 8493391 [details] [diff] [review]
message-style.patch

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

::: im/content/preferences/messagestyle.js
@@ +6,5 @@
>  Components.utils.import("resource:///modules/jsProtoHelper.jsm", jsProtoHelper);
>  
> +let imConversations = {};
> +Services.scriptloader.loadSubScript("resource:///components/imConversations.js",
> +                                    imConversations);

Why did you use loadSubScript rather than Cu.import here?

Comment 4

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #3)
> Comment on attachment 8493391 [details] [diff] [review]
> message-style.patch
> 
> Review of attachment 8493391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: im/content/preferences/messagestyle.js
> @@ +6,5 @@
> >  Components.utils.import("resource:///modules/jsProtoHelper.jsm", jsProtoHelper);
> >  
> > +let imConversations = {};
> > +Services.scriptloader.loadSubScript("resource:///components/imConversations.js",
> > +                                    imConversations);
> 
> Why did you use loadSubScript rather than Cu.import here?

Because it isn't a module, so it doesn't export anything. I find this a little bit messy, however.

Comment 5

[Florian Quèze [:florian]]

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

> I find this a little bit messy, however.

Agreed. I'm afraid this would cause the script to be loaded again in a different scope. I would be OK with this if it was just for a test, but for something actually shipped, it doesn't seem satisfying :-/.

Comment 6

[arlolra]

Any better suggestions?

Could move the definition to jsProtoHelper (which was already shot down) ... or add a fakeImMessage shim?

Comment 7

[Florian Quèze [:florian]]

Would it be possible to somehow define something looking a bit like this?

FakeMessage.prototype = {
  __proto__: jsProtoHelper.Message.prototype,
  get displayMessage() this.message
};

Comment 8

[arlolra]

Attachment: message-style.patch from comment 7

Comment 9

[arlolra]

Seems to work. See the attached.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8493933 [details] [diff] [review]
message-style.patch from comment 7

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

::: im/content/preferences/messagestyle.js
@@ +25,5 @@
> +}
> +FakeMessage.prototype = {
> +  __proto__: jsProtoHelper.Message.prototype,
> +  get displayMessage() this.originalMessage
> +};

Given that Message in jsProtoHelper is just:
436 function Message(aWho, aMessage, aObject) {
437   this._init(aWho, aMessage, aObject);
438 }
439 Message.prototype = GenericMessagePrototype;

we can simplify:

function Message(aWho, aMessage, aObject) {
  this._init(aWho, aMessage, aObject);
}
Message.prototype = {
  __proto__: jsProtoHelper.GenericMessagePrototype,
  get displayMessage() this.originalMessage
};

@@ +48,5 @@
>       "message1", "message2", "message3"].forEach(function(aText) {
>        msg[aText] = bundle.getString(aText);
>      });
>      let conv = new Conversation(msg.nick2);
>      const Message = jsProtoHelper.Message;

Remove this line.

@@ +50,5 @@
>      });
>      let conv = new Conversation(msg.nick2);
>      const Message = jsProtoHelper.Message;
>      conv.messages = [
> +      new FakeMessage(msg.buddy1, msg.message1, {outgoing: true, _alias: msg.nick1, time: makeDate("10:42:22"), _conversation: conv}),

Let's keep the Message name. The fake conversation object is called just Conversation, not FakeConversation.

Comment 11

[arlolra]

Attachment: message-style.patch from comment 10

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8493968 [details] [diff] [review]
message-style.patch from comment 10

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

Thanks! (I'm assuming of course that you have tested this and that it works :)).

Comment 13

[arlolra]

> Thanks! (I'm assuming of course that you have tested this and that it works :)).

Yup, as far as I can tell.

Comment 14

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/2b6f0a685995