Comment 1 adu 2013-01-09 05:23:46 PST

Can i pick this Bug?

Comment 2 Patrick Cloke [:clokep] 2013-01-09 05:27:38 PST

(In reply to adu from comment #1)
> Can i pick this Bug?

Sure, I've assigned it to you. For reference, adu had contact Florian, Mike and I via email about this bug. I gave some background information and suggested he come on IRC to talk to us further.

Comment 3 adu 2013-01-10 07:07:37 PST

Created attachment 700404 [details] [diff] [review]
First Patch

I have attached the patch. Please check.

Comment 4 Mike Conley (:mconley) - (needinfo me!) 2013-01-10 07:09:27 PST

Comment on attachment 700404 [details] [diff] [review]
First Patch

Adding to my review queue.

Comment 5 adu 2013-01-11 05:51:17 PST

Created attachment 701040 [details] [diff] [review]
Second Patch

Please check my second patch.

Comment 6 Mike Conley (:mconley) - (needinfo me!) 2013-01-11 10:35:23 PST

Comment on attachment 701040 [details] [diff] [review]
Second Patch

Hey adu,

Thanks for the patch! Next time, make sure to set review? on me, so the patch gets added to my queue.

-Mike

Comment 7 Mike Conley (:mconley) - (needinfo me!) 2013-01-11 11:23:19 PST

Comment on attachment 701040 [details] [diff] [review]
Second Patch

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

Thanks adu - but what about log.lastWeek and log.twoWeeksAgo ?

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +89,5 @@
>  # %1$S is the date, %2$S is the date.
>  dateTime=%1$S %2$S
> +
> +# LOCALIZATION NOTE (log.today, log.yesterday, log.lastWeek, log.twoWeeksAgo):
> +#This is used for logging. 

I don't think this comment adds much - I think you can remove it.

Comment 8 adu 2013-01-12 03:34:59 PST

Created attachment 701448 [details] [diff] [review]
Third Patch

Please review my Third Patch.

Comment 9 Mike Conley (:mconley) - (needinfo me!) 2013-01-14 09:00:12 PST

Comment on attachment 701448 [details] [diff] [review]
Third Patch

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

Hey adu,

You wrote in your email:

> I removed the comment and  the strings "log.lastWeek" and "log.twoWeeksAgo"
> because those strings are not used in the chat-messenger. Hope i am on the
> right path.

"lastWeek" and "twoWeeksAgo" are indeed used in chat-messenger-overlay.js.

This line: http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1157

retrieves the appropriate string based on the group names defined here:

http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1088

So those strings are now missing. You should put them back, and make sure to retrieve the strings with "log." prefixing them.

Let me know in this bug if you have any more questions.

Thanks,

-Mike

Comment 10 adu 2013-01-15 03:49:59 PST

Created attachment 702217 [details] [diff] [review]
Patch File

hope this will fix the bug .Please review my patch.

Comment 11 Mike Conley (:mconley) - (needinfo me!) 2013-01-15 06:42:27 PST

Comment on attachment 702217 [details] [diff] [review]
Patch File

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

Almost there!

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1085,5 @@
>  
>      // The keys used in the 'groups' object should match string ids in
>      // messenger.properties, except 'other' that has a special handling.
>      let groups = {
> +      log.lastWeek: [],

This is not valid Javascript - you cannot have properties with periods in their names.

Among other reasons, it's because it's ambiguous. Suppose I had the following:

let groups = {
  log.lastWeek: [],
  log: {
    lastWeek: "Hello!"
  }
};

What does groups.log.lastWeek refer to then?

TL;DR:  These lines should stay the way they are.

@@ +1122,4 @@
>          continue;
>        }
>        else if (timeFromToday <= kWeekInMsecs)
> +        group = groups.log.lastWeek;

Please revert the changes to line 1125 and 1127.

@@ +1131,5 @@
>      }
>  
>      if (today)
>        this._rowMap.push(today);
>      if (yesterday)

The change you need to make is below, right here:

http://mxr.mozilla.org/comm-central/source/mail/components/im/content/chat-messenger-overlay.js#1157

You need to change:

groupName = msgBundle.getString(groupId);

to

groupName = msgBundle.getString("log." + groupId);

Comment 12 Florian Quèze [:florian] [:flo] 2013-01-15 07:02:51 PST

I think it would make sense to rename the msgBundle variable to chatBundle.

Comment 13 adu 2013-01-17 04:28:31 PST

Created attachment 703270 [details] [diff] [review]
Patch file

I replaced the variable msgBundle with chatBundle, as florian asked for.
changed line +1157 
>groupName = msgBundle.getString(groupId);
to 
 groupName = chatBundle.getString("log."+groupId);

Comment 14 Patrick Cloke [:clokep] 2013-01-17 04:56:57 PST

Comment on attachment 703270 [details] [diff] [review]
Patch file

It would be better for mconley to review this.

Comment 15 Mike Conley (:mconley) - (needinfo me!) 2013-01-17 12:11:43 PST

Comment on attachment 703270 [details] [diff] [review]
Patch file

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

From inspection, this looks good - just one nit. I'll test the patch tonight.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1153,5 @@
>          }
>        }
>        else {
>          // Otherwise, get the appropriate string for this group.
> +        groupName = chatBundle.getString("log."+groupId);

Nit: spaces on either side of the +

Comment 16 Mike Conley (:mconley) - (needinfo me!) 2013-01-17 16:00:30 PST

Comment on attachment 703270 [details] [diff] [review]
Patch file

I fixed the nit before landing this. Thanks adu!

Landed on comm-central as https://hg.mozilla.org/comm-central/rev/eceb4d4b7131