Message style preview is broken

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: arlolra)

Tracking

({regression})

Details

(Whiteboard: [1.6-blocking])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Bah, I meant Bug 983347 of course.
Blocks: 983347
No longer blocks: 982580
(Assignee)

Comment 2

5 years ago
Posted patch message-style.patch (obsolete) — Splinter Review
Attachment #8493391 - Flags: review?(florian)
Attachment #8493391 - Flags: review?(clokep)
Attachment #8493391 - Flags: review?(aleth)
(Reporter)

Comment 3

5 years ago
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?
(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.
(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 :-/.
(Assignee)

Comment 6

5 years ago
Any better suggestions?

Could move the definition to jsProtoHelper (which was already shot down) ... or add a fakeImMessage shim?
Would it be possible to somehow define something looking a bit like this?

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

Comment 8

5 years ago
Attachment #8493391 - Attachment is obsolete: true
Attachment #8493391 - Flags: review?(florian)
Attachment #8493391 - Flags: review?(clokep)
Attachment #8493391 - Flags: review?(aleth)
(Assignee)

Comment 9

5 years ago
Seems to work. See the attached.
(Assignee)

Updated

5 years ago
Attachment #8493933 - Flags: review?(florian)
Attachment #8493933 - Flags: review?(clokep)
Attachment #8493933 - Flags: review?(aleth)
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.
Attachment #8493933 - Flags: review?(florian)
Attachment #8493933 - Flags: review-
Attachment #8493933 - Flags: feedback+
(Assignee)

Comment 11

5 years ago
Attachment #8493968 - Flags: review?(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 :)).
Attachment #8493968 - Flags: review?(florian) → review+
Attachment #8493933 - Attachment is obsolete: true
Attachment #8493933 - Flags: review?(clokep)
Attachment #8493933 - Flags: review?(aleth)
(Assignee)

Comment 13

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

Yup, as far as I can tell.
https://hg.mozilla.org/comm-central/rev/2b6f0a685995
Assignee: nobody → arlolra
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.