Last Comment Bug 787149 - Change log tree to list the days that logs exist, and then show all logs for that day when selected
: Change log tree to list the days that logs exist, and then show all logs for ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 17 Branch
: x86 All
: -- enhancement (vote)
: Thunderbird 18.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
: 740858 (view as bug list)
Depends on: 795971
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-30 11:24 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2013-01-22 07:19 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
WIP Patch 1 (10.77 KB, patch)
2012-08-31 12:01 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: feedback+
clokep: feedback+
Details | Diff | Review
WIP Patch 2 (20.22 KB, patch)
2012-09-12 11:17 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Review
Patch v1 (21.27 KB, patch)
2012-09-13 07:38 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review-
bwinton: ui‑review+
Details | Diff | Review
Patch v2 (21.63 KB, patch)
2012-09-14 09:59 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review+
Details | Diff | Review
Patch v3 (22.20 KB, patch)
2012-09-17 07:20 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review+
standard8: approval‑comm‑aurora+
Details | Diff | Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-08-30 11:24:26 PDT
Currently, the IM log tree lists each individual log file for a particular conversation. This is stratified in the "Today", "Yesterday", "Last Week", "Two Weeks Ago", and "X - Y" twisty tree nodes.

In the product council today, Jb requested that instead of this, we don't make "Today" a twisty - rather, when we click on "Today", we simply get every log for this conversation for today, concatenated in the log viewer.

We'd have a similar behaviour when clicking "Yesterday".

"Last Week" would be a twisty with the individual days with logs listed underneath it. Same with "Two Weeks Ago" and "X - Y".
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-08-30 12:06:24 PDT
As a side note, this was always the plan, but it was… uh… let's say "non-trivial", and so we didn't implement it for TB 15.  Now that we have a little more time, it would be great to see something along these lines!
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-08-31 07:01:49 PDT
Just a couple of notes:

What about conversations that go over the day boundary? Suppose I start a conversation at 11:30PM, and it carries on until 12:30AM, the next day. What day is that conversation log filed under? Should it be filed on just the first day? The first day and the second day? Or should we break up the conversation at the day boundary, so that the conversation before 12AM is on the first day, and the other portion is on the second?

My tentative plan is to modify logger.js to include a function "getDailyLogsForConversation" that uses an enumerator that reads the log files such that the LogMessages are grouped if they belong within the same day (or belong within the same log file...we still need to answer my above question).

I think that, along with some modifications in chat-messenger-overlay to handle the new log representations, will be sufficient to get what we want for the log viewer.

What about Gloda search results? Should we be listing the entire day's log when displaying a search result?
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-08-31 07:35:27 PDT
I just talked to Blake (who should be on vacation!), and I asked him about the 11:30PM -> 12:30AM conversation log situation.

He said that what he'd want is for the log to appear in both days.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-08-31 09:16:38 PDT
More notes - bwinton had asked why we don't simply have a single log file per day per conversation.

Florian's response is that, at least on Windows, this has the risk of destroying the *entire* log for that conversation on an unclean shutdown.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-08-31 12:01:37 PDT
Created attachment 657382 [details] [diff] [review]
WIP Patch 1

Here's my first crack at this.

I've added a new type of LogEnumerator, and a "LogCluster" implementation of imILog. Currently, previous logs are displayed out of order.

I'm not sure how performant this patch is. There are likely inefficiencies.

I also haven't yet addressed a lot of the edge cases we've been discussing both in this both and in IRC.

I'd still like to know if I'm heading in the right direction with this though. All feedback welcome.
Comment 6 Patrick Cloke [:clokep] 2012-09-03 17:45:53 PDT
Comment on attachment 657382 [details] [diff] [review]
WIP Patch 1

>diff --git a/chat/components/public/imILogger.idl b/chat/components/public/imILogger.idl
[...]
>+  nsISimpleEnumerator getDailyLogsForConversation(in prplIConversation aConversation);
I think this could use a comment above it, the title isn't totally self explanatory to me.

>diff --git a/chat/components/src/logger.js b/chat/components/src/logger.js
>+function LogConversation(aLineInputStreams)
[...]
>-  if (!line.value)
>-    throw "bad log file";

I wouldn't be opposed to checking if an array is here and then making it an array if not one, i.e.:
aLineInputStreams = Array.isArray(aLineInputStreams) ? aLineInputStreams: [aLineInputStreams];
But maybe I'm just lazy. :)

>+  for each (let [, inputStream] in Iterator(aLineInputStreams)) {
Is this just an array? Why not just:
for each (let inputStream in aLineInputStreams)

But if it's only called in a couple of places, it doesn't really matter.


>@@ -399,25 +403,122 @@ LogEnumerator.prototype = {
>+function DailyLogEnumerator(aEntries) {
>+  this._entries = {};
>+  const regexp = /([0-9]{4})-([0-9]{2})-([0-9]{2}).([0-9]{2})([0-9]{2})([0-9]{2})([+-])([0-9]{2})([0-9]{2}).*\.([a-z]+)$/;
This is matching...YYYY-MM-DD.HHmmSS+ZZzz? I think this needs a comment. :) Also, I'd prefer using \d instead of [0-9], it's shorter and clearer. Is the extension expected to match only lowercase [a-z] or should that be [A-Za-z]? This also prohibits having a double extension (.tar.gz type thing), which should be fine. Why is the extension capturing? I don't see it used anywhere.

Also, why don't we use ISO 8601 format for dates? :(

>+  while (aEntries.length > 0) {
>+    let entry = aEntries.shift();
I feel like shift() might not be very conducive to good performance since it modifies the array, maybe .reverse() and then .pop() would be better, or why are we shifting in the first place? Can't this be a for loop?

>+      let logDate = new Date(r[1], r[2] - 1, r[3], r[4], r[5], r[6]);
>+      let dayID = logDate.getFullYear() + logDate.getMonth() + logDate.getDate();
I feel like you could just use toDateString() or:

>+DailyLogEnumerator.prototype = {
[...]
>+  getNext: function() {
>+    let day = this._days.shift();
(Again I'm assuming shift is expensive.) Why not sort in the opposite direction and pop() instead?

>+    let dayEntries = this._entries[day];
Inline this._days.shift() here, unless you're going to use it again.

>+    dayEntries.sort(function(aLeft, aRight) {
>+      return aRight.time - aLeft.time;
>+    });
This could probably use a comment, leave out the { } and return:
dayEntries.sort(function(aLeft, aRight) aRight.time - aLeft.time);

What is a LogCluster? Can we get a comment here?
>+function LogCluster(aEntries) {

>+  let earliestLogDate = aEntries[0].time;
>+  earliestLogDate.setHours(0);
>+  earliestLogDate.setMinutes(0);
>+  earliestLogDate.setSeconds(0);
>+
>+  this.time = earliestLogDate.valueOf() / 1000;
Maybe, Math.floor(aEntries[0].time / (86400 * 1000))? (That's days since the UNIX epoch.)

>+  // Ugh.
>+  this.path = aEntries[0].file.path;
Ugh? Can I get a more expressive comment? :)

>+LogCluster.prototype = {
>+  __proto__: ClassInfo("imILog", "LogCluster object"),
>+  getConversation: function() {
>+    if (this.format != "json")
>+      return null;
So Instantbird has some .txt logs, I'm not sure how this would interact with this...since you hardcode the format above to be "json"?

>+    const PR_RDONLY = 0x01;
>+    let streams = [];
>+    for each (let [, entry] in Iterator(this._entries)) {
Again, why not just "for each (let entry in this._entries)"?

>+      let fis = new FileInputStream(entry.file, PR_RDONLY, 0444,
>+                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
>+      let lis = new ConverterInputStream(fis, "UTF-8", 1024, 0x0);
I think 0x0 could use an explanation (that you want to throw exceptions on unknown bytes).

> function Logger() { }
[...]
>+  getDailyLogsForConversation: function logger_getDailyLogsForConversation(aConversation) {
>+    let name = aConversation.normalizedName;
>+    if (aConversation.isChat &&
>+        aConversation.account.protocol.id != "prpl-twitter")
>+      name += ".chat";
I think we should abstract this name garbage out, it seems fragile having it in two separate methods.

Overall I think the changes look reasonable and dealing with JavaScript dates is a pain, but I think you're doing it in a fairly straightforward way.
Comment 7 Patrick Cloke [:clokep] 2012-09-04 05:02:39 PDT
Comment on attachment 657382 [details] [diff] [review]
WIP Patch 1

Mic complained on IRC that he doesn't like this patch. Now he gets to offer his feedback.
Comment 8 Benedikt Pfeifer [:Mic] 2012-09-04 07:02:06 PDT
I complained about having something so specific as this method (|getDailyLogsForConversation|) on the logger (we might not it need for Instantbird and it wouldn't go away easily again once it's there).

Wouldn't it be better in general if the logger was able to tell if there were messages in any given period of time (not just on a per day basis)? A method like |hasMessages(aConv, aStartTime, aEndTime)| could return true/false based on a table with the timestamps of messages for the conversation so that it (or you) wouldn't have to open the log files every time to look for logs with day changes?
Another method would allow to fetch messages (either directly or as "Log") in a similar way (|getMessages(aConv, aStartTime, aEndTime)|).

This way you can both build your list (and solve the "day change" problem that the current approach still has), load exactly the messages that were said at selected day without having overlap to the next day + it would be useful for other things too (getting the latest history, e.g. the last 2h, 12h, last day, ..)

What do you think?

PS: I can't edit flags.
Comment 9 Benedikt Pfeifer [:Mic] 2012-09-04 07:05:58 PDT
(In reply to Benedikt P. [:Mic] from comment #8)
> true/false based on a table with the timestamps of messages for the
> conversation so that it (or you) wouldn't have to open the log files every
> time to look for logs with day changes?

Oops, scratch the "to look for logs with day changes"-part, please.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-09-04 07:32:48 PDT
(In reply to Benedikt P. [:Mic] from comment #8)
> I complained about having something so specific as this method
> (|getDailyLogsForConversation|) on the logger (we might not it need for
> Instantbird and it wouldn't go away easily again once it's there).
> 
> Wouldn't it be better in general if the logger was able to tell if there
> were messages in any given period of time (not just on a per day basis)? A
> method like |hasMessages(aConv, aStartTime, aEndTime)| could return
> true/false based on a table with the timestamps of messages for the
> conversation so that it (or you) wouldn't have to open the log files every
> time to look for logs with day changes?
> Another method would allow to fetch messages (either directly or as "Log")
> in a similar way (|getMessages(aConv, aStartTime, aEndTime)|).
> 
> This way you can both build your list (and solve the "day change" problem
> that the current approach still has), load exactly the messages that were
> said at selected day without having overlap to the next day + it would be
> useful for other things too (getting the latest history, e.g. the last 2h,
> 12h, last day, ..)
> 
> What do you think?
> 
> PS: I can't edit flags.

It sounds alright in theory.

Using that approach, I could easily query for most of the days I'm interested in...

But how would I express that I want daily logs of conversations going back to the beginning of time?
Comment 11 Florian Quèze [:florian] [:flo] 2012-09-07 02:57:34 PDT
Comment on attachment 657382 [details] [diff] [review]
WIP Patch 1

I discussed this with mconley yesterday.

I'm going to try to summarize what I told him:
- My understanding of Mic's concern with this patch is that the new getDailyLogsForConversation method this is adding may no longer be useful after a while (typically after we finally implement inifite scrollback) but will be difficult to remove because it may be used by add-ons. This seems a valid concern to me, but I could argue that it's also applicable to most of the existing methods of imILogger, so I don't think it justifies blocking Mike's patch.
- I agree with most of clokep's review comments.
- Mike asked me if this is the "right approach". I couldn't really answer as I dislike the existing code around this enough that I don't really think there's a "right approach" (except maybe redesigning the whole logger code), but his approach is probably not worse than others :).
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-09-12 11:17:01 PDT
Created attachment 660525 [details] [diff] [review]
WIP Patch 2

Thanks for the feedback, folks! Checkpointing work.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-09-13 07:38:15 PDT
Created attachment 660837 [details] [diff] [review]
Patch v1

Ok, this patch appears to do the job. The one caveat is that I didn't find a solution for the case where messages leak over to the next day...those messages are still contained within the daily log that the log started on.

Are there any cases I missed?
Comment 14 Florian Quèze [:florian] [:flo] 2012-09-13 10:39:46 PDT
Comment on attachment 660837 [details] [diff] [review]
Patch v1

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

Overall this seems pretty acceptable (note that I haven't actually tried it, I trust you have tested it before attaching :-)), but I had enough review comments that I feel I'll have to look at this quickly again.

::: chat/components/public/imILogger.idl
@@ -41,5 @@
>    // Will return null if the log format isn't json.
>    imILogConversation getConversation();
>  };
> -
> -[scriptable, uuid(ab38c01c-2245-4279-9437-1d6bcc69d556)]

Why have you removed the empty line before this interface?

@@ +56,3 @@
>    nsISimpleEnumerator getSystemLogsForAccount(in imIAccount aAccount);
>    nsISimpleEnumerator getSimilarLogs(in imILog aLog);
> +  nsISimpleEnumerator getSimilarDailyLogs(in imILog aLog);

When looking at these interface changes, I wonder if what you wanted wasn't to add an optional aGroupByDay parameter to getLogsForConversation, getLogFromFile and getSimilarLogs.

::: chat/components/src/logger.js
@@ +315,4 @@
>      if (!line.value)
> +      throw "bad log file";
> +
> +    let data = JSON.parse(line.value);

Isn't this something that should be done only for the first log file? Or do we not care because we are sure all log files that are going to be clustered together will have the same values for these fields? (I wonder if in the future we will want to have a field with a timestamp of when the conversation started. If we do, when we will want to be careful.)

@@ +339,1 @@
>  LogConversation.prototype = {

Nit: Not having an empty line between a constructor and its prototype is usually intended (I see it's done at least for Log too). You could also do it for the 2 new constructor+prototype you are adding to keep the style of the whole file consistent.

@@ +380,5 @@
>                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
>      let lis = new ConverterInputStream(fis, "UTF-8", 1024, 0x0);
>      lis.QueryInterface(Ci.nsIUnicharLineInputStream);
>      try {
> +      return new LogConversation([lis]);

You don't need this change any more.

@@ +419,5 @@
> +/**
> + * Returns true if a Conversation is both a chat conversation, and not
> + * a Twitter conversation.
> + */
> +function convNeedsChatExtension(aConversation) {

Maybe use a name that conveys the meaning of the result, rather than how the result is going to be used?
Suggestions: convIsRealMUC convIsChat (note: MUC is the standard XMPP accronym for Multi-User Chat).

@@ +461,5 @@
> +        this._entries[dayID] = [];
> +
> +      this._entries[dayID].push({
> +        file: file,
> +        time: logDate,

Warning: Trailing coma.

@@ +466,5 @@
> +      });
> +    }
> +  }
> +
> +  this._days = Object.keys(this._entries).sort();

(new Date()).toDateString() gives me "Thu Sep 13 2012", are you really confident that calling .sort() here will do what you want?

@@ +477,5 @@
> +  _index: 0,
> +
> +  hasMoreElements: function() {
> +    return this._index < this._days.length;
> +  },

Nit: this looks like it can fit on a single line (without the { return }).

@@ +485,5 @@
> +    this._index++;
> +
> +    let dayEntries = this._entries[day];
> +    return new LogCluster(dayEntries);
> +  },

This too: new LogCluster(this._entries[this._days[this._index++]])

@@ +505,5 @@
> +  // Sort our list of entries for this day in increasing order.
> +  aEntries.sort(function(aLeft, aRight) aLeft.time - aRight.time);
> +
> +  this._entries = aEntries;
> +  this.format = "json";

If it's constant, move this to the prototype please.

@@ +508,5 @@
> +  this._entries = aEntries;
> +  this.format = "json";
> +  // Calculate the timestamp for the first entry down to the day.
> +  let timestamp = aEntries[0].time;
> +  timestamp.setHours(0);

If timestamp is a Date object, isn't setHours actually modifying it? Shouldn't you make a copy first?

@@ +526,5 @@
> +    let streams = [];
> +    for each (let entry in this._entries) {
> +      let fis = new FileInputStream(entry.file, PR_RDONLY, 0444,
> +                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
> +      // Pass in 0x0 so that we throw exceptions on unknown bytes.

Should we catch these exceptions and continue or return null?

@@ +549,4 @@
>  function Logger() { }
>  Logger.prototype = {
> +  _enumerateLogs: function logger__enumerateLogs(aAccount, aNormalizedName,
> +                                                 aEnumeratorClass) {

What about replacing aEnumatorClass with a boolean, so that the parameter could be omitted in existing callers?

@@ +587,1 @@
>    getLogsForConversation: function logger_getLogsForConversation(aConversation) {

Adding a boolean parameter here is trivial, and avoids some duplication.

@@ +595,5 @@
> +    if (convNeedsChatExtension(aConversation))
> +      name += ".chat";
> +    return this._enumerateLogs(aConversation.account, name, DailyLogEnumerator);
> +  },
> +  getDailyLogForFile: function logger_getDailyLogsForFile(aFile) {

getLogForFile seems the only case where the implementation for logs grouped by day is really different. But the existing implementation is so simple that you could keep it at the beginning and have it return early :).

@@ +619,5 @@
> +      let day = Math.floor(logTime / (86400 * 1000));
> +      if (targetDay == day) {
> +        relevantEntries.push({
> +          file: file,
> +          time: logTime,

nit: trailing coma.

@@ +621,5 @@
> +        relevantEntries.push({
> +          file: file,
> +          time: logTime,
> +        });
> +      } 

nit: trailing white space

::: mail/components/im/content/chat-messenger-overlay.js
@@ +590,4 @@
>        let contextPane = document.getElementById("contextPane");
>        if (item.conv.isChat) {
>          contextPane.setAttribute("chat", "true");
>          item.convView.showParticipants();        

You haven't added it, but as you are modifying code that's very close, maybe remove the trailing whitespace here? :)
Comment 15 Patrick Cloke [:clokep] 2012-09-13 10:54:24 PDT
Comment on attachment 660837 [details] [diff] [review]
Patch v1

+ * @returns an Array, where the first element is a Date object for the date
+ *          that the log file represents, and the file type as a string.

This sentence doesn't seem to parse properly in my brain. I think you mean: "An array, where the first element is the Date object that the log file represents, and where the second element is the file extension as a string."?
Comment 16 Blake Winton (:bwinton) (:☕️) 2012-09-13 14:01:27 PDT
Comment on attachment 660837 [details] [diff] [review]
Patch v1

It's sort of too bad we can't do "08-12" instead of "28-08-12 - 02-08-12"…
But it's better than it was previously, so I'm going to say ui-r=me.

You should definitely also send a try-build to JB, for his comments.

Thanks,
Blake.
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-09-14 09:59:05 PDT
Created attachment 661269 [details] [diff] [review]
Patch v2

Thanks for the review, Florian!
Comment 18 Mike Conley (:mconley) - (Needinfo me!) 2012-09-14 10:02:33 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> Comment on attachment 660837 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 660837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this seems pretty acceptable (note that I haven't actually tried it,
> I trust you have tested it before attaching :-)), but I had enough review
> comments that I feel I'll have to look at this quickly again.
> 
> ::: chat/components/public/imILogger.idl
> @@ -41,5 @@
> >    // Will return null if the log format isn't json.
> >    imILogConversation getConversation();
> >  };
> > -
> > -[scriptable, uuid(ab38c01c-2245-4279-9437-1d6bcc69d556)]
> 
> Why have you removed the empty line before this interface?

My mistake. I put it back.

> 
> @@ +56,3 @@
> >    nsISimpleEnumerator getSystemLogsForAccount(in imIAccount aAccount);
> >    nsISimpleEnumerator getSimilarLogs(in imILog aLog);
> > +  nsISimpleEnumerator getSimilarDailyLogs(in imILog aLog);
> 
> When looking at these interface changes, I wonder if what you wanted wasn't
> to add an optional aGroupByDay parameter to getLogsForConversation,
> getLogFromFile and getSimilarLogs.

Ok, I've removed the new functions, and added an optional boolean instead.

> 
> ::: chat/components/src/logger.js
> @@ +315,4 @@
> >      if (!line.value)
> > +      throw "bad log file";
> > +
> > +    let data = JSON.parse(line.value);
> 
> Isn't this something that should be done only for the first log file? Or do
> we not care because we are sure all log files that are going to be clustered
> together will have the same values for these fields? (I wonder if in the
> future we will want to have a field with a timestamp of when the
> conversation started. If we do, when we will want to be careful.)
> 

Ok, we just check on the first file now.

> @@ +339,1 @@
> >  LogConversation.prototype = {
> 
> Nit: Not having an empty line between a constructor and its prototype is
> usually intended (I see it's done at least for Log too). You could also do
> it for the 2 new constructor+prototype you are adding to keep the style of
> the whole file consistent.
> 

Done.

> @@ +380,5 @@
> >                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
> >      let lis = new ConverterInputStream(fis, "UTF-8", 1024, 0x0);
> >      lis.QueryInterface(Ci.nsIUnicharLineInputStream);
> >      try {
> > +      return new LogConversation([lis]);
> 
> You don't need this change any more.
> 

Fixed.

> @@ +419,5 @@
> > +/**
> > + * Returns true if a Conversation is both a chat conversation, and not
> > + * a Twitter conversation.
> > + */
> > +function convNeedsChatExtension(aConversation) {
> 
> Maybe use a name that conveys the meaning of the result, rather than how the
> result is going to be used?
> Suggestions: convIsRealMUC convIsChat (note: MUC is the standard XMPP
> accronym for Multi-User Chat).
> 

Done.

> @@ +461,5 @@
> > +        this._entries[dayID] = [];
> > +
> > +      this._entries[dayID].push({
> > +        file: file,
> > +        time: logDate,
> 
> Warning: Trailing coma.
> 

Fixed.

> @@ +466,5 @@
> > +      });
> > +    }
> > +  }
> > +
> > +  this._days = Object.keys(this._entries).sort();
> 
> (new Date()).toDateString() gives me "Thu Sep 13 2012", are you really
> confident that calling .sort() here will do what you want?
> 

Good call. I'm using toISOString now instead - that seems much more reliable.

> @@ +477,5 @@
> > +  _index: 0,
> > +
> > +  hasMoreElements: function() {
> > +    return this._index < this._days.length;
> > +  },
> 
> Nit: this looks like it can fit on a single line (without the { return }).
> 

You and your one-liners. :) Done.

> @@ +485,5 @@
> > +    this._index++;
> > +
> > +    let dayEntries = this._entries[day];
> > +    return new LogCluster(dayEntries);
> > +  },
> 
> This too: new LogCluster(this._entries[this._days[this._index++]])
> 

Done.

> @@ +505,5 @@
> > +  // Sort our list of entries for this day in increasing order.
> > +  aEntries.sort(function(aLeft, aRight) aLeft.time - aRight.time);
> > +
> > +  this._entries = aEntries;
> > +  this.format = "json";
> 
> If it's constant, move this to the prototype please.
> 

Done.

> @@ +508,5 @@
> > +  this._entries = aEntries;
> > +  this.format = "json";
> > +  // Calculate the timestamp for the first entry down to the day.
> > +  let timestamp = aEntries[0].time;
> > +  timestamp.setHours(0);
> 
> If timestamp is a Date object, isn't setHours actually modifying it?
> Shouldn't you make a copy first?
> 

Good call. We clone the Date now.

> @@ +526,5 @@
> > +    let streams = [];
> > +    for each (let entry in this._entries) {
> > +      let fis = new FileInputStream(entry.file, PR_RDONLY, 0444,
> > +                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
> > +      // Pass in 0x0 so that we throw exceptions on unknown bytes.
> 
> Should we catch these exceptions and continue or return null?
>

Currently, we catch and return null. I didn't modify that behaviour.
 
> @@ +549,4 @@
> >  function Logger() { }
> >  Logger.prototype = {
> > +  _enumerateLogs: function logger__enumerateLogs(aAccount, aNormalizedName,
> > +                                                 aEnumeratorClass) {
> 
> What about replacing aEnumatorClass with a boolean, so that the parameter
> could be omitted in existing callers?
> 

Done.

> @@ +587,1 @@
> >    getLogsForConversation: function logger_getLogsForConversation(aConversation) {
> 
> Adding a boolean parameter here is trivial, and avoids some duplication.
> 

Done.

> @@ +595,5 @@
> > +    if (convNeedsChatExtension(aConversation))
> > +      name += ".chat";
> > +    return this._enumerateLogs(aConversation.account, name, DailyLogEnumerator);
> > +  },
> > +  getDailyLogForFile: function logger_getDailyLogsForFile(aFile) {
> 
> getLogForFile seems the only case where the implementation for logs grouped
> by day is really different. But the existing implementation is so simple
> that you could keep it at the beginning and have it return early :).
> 

Done.

> @@ +619,5 @@
> > +      let day = Math.floor(logTime / (86400 * 1000));
> > +      if (targetDay == day) {
> > +        relevantEntries.push({
> > +          file: file,
> > +          time: logTime,
> 
> nit: trailing coma.
> 

Fixed.

> @@ +621,5 @@
> > +        relevantEntries.push({
> > +          file: file,
> > +          time: logTime,
> > +        });
> > +      } 
> 
> nit: trailing white space
> 

Fixed.

> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +590,4 @@
> >        let contextPane = document.getElementById("contextPane");
> >        if (item.conv.isChat) {
> >          contextPane.setAttribute("chat", "true");
> >          item.convView.showParticipants();        
> 
> You haven't added it, but as you are modifying code that's very close, maybe
> remove the trailing whitespace here? :)

Fixed.
Comment 19 Florian Quèze [:florian] [:flo] 2012-09-17 06:17:16 PDT
Comment on attachment 661269 [details] [diff] [review]
Patch v2

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

Looks OK overall (I still haven't tested it, but I'm not worried :-)).

r=me with my comments addressed, but feel free to re-request review if you feel it's needed or if you think I should really test the final version of the patch before it lands.

::: chat/components/src/logger.js
@@ +415,5 @@
> +  const kRegExp = /([\d]{4})-([\d]{2})-([\d]{2}).([\d]{2})([\d]{2})([\d]{2})([+-])([\d]{2})([\d]{2}).*\.([A-Za-z]+)$/;
> +
> +  let r = aFilename.match(kRegExp);
> +  if (!r)
> +    return null;

Maybe you need to return [] here instead of null? (see my comment about DailyLogEnumerator)

@@ +425,5 @@
> +/**
> + * Returns true if a Conversation is both a chat conversation, and not
> + * a Twitter conversation.
> + */
> +function convIsRealMUC(aConversation) {

This currently has a single caller, but there's another place in the same file (http://hg.mozilla.org/comm-central/annotate/8bbb9f722e7b/chat/components/src/logger.js#l77) doing the exact same test. Do you feel like reducing the duplication? :-)

@@ +457,5 @@
> +        continue;
> +
> +      let [logDate] = getDateFromFilename(file.leafName);
> +      if (!logDate) {
> +        // We'll skip this one, since it's got a busted filename.

If I try in my Error Console:
var [a] = null; a;

I get:
TypeError: null has no properties

Is there something I missed, or is this code broken for busted filenames?

@@ +493,5 @@
> +  hasMoreElements: function() this._index < this._days.length,
> +
> +  getNext: function() new LogCluster(this._entries[this._days[this._index++]]),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleEnumerator])

Nit: I wouldn't put empty lines here between all these one liners, but I don't want to torture you too much with my condensed code style :-).

@@ +525,5 @@
> +}
> +LogCluster.prototype = {
> +  format: "json",
> +
> +  __proto__: ClassInfo("imILog", "LogCluster object"),

Nit: usually we make __proto__ the very first line of the prototype to keep the inheritance path obvious.

@@ +532,5 @@
> +    let streams = [];
> +    for each (let entry in this._entries) {
> +      let fis = new FileInputStream(entry.file, PR_RDONLY, 0444,
> +                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
> +      // Pass in 0x0 so that we throw exceptions on unknown bytes.

My review comment here about the previous attachment was bogus. I mis-read this code comment as meaning that the next line would throw. Sorry about that.

@@ +640,5 @@
>      this._enumerateLogs(aAccount, ".system"),
> +  getSimilarLogs: function(aLog, aGroupByDay) {
> +    if (aGroupByDay)
> +      return new DailyLogEnumerator([new LocalFile(aLog.path).parent
> +                                                             .directoryEntries]);

Nit: we usually use {} when the instruction after a test can't fit on a single line.

@@ +642,5 @@
> +    if (aGroupByDay)
> +      return new DailyLogEnumerator([new LocalFile(aLog.path).parent
> +                                                             .directoryEntries]);
> +
> +    return new LogEnumerator([new LocalFile(aLog.path).parent.directoryEntries]);

Anyway, this feels like duplicated code.

What about doing |let enumerator = aGroupByDay ? DailyLogEnumerator : LogEnumerator;| exactly like you did in _enumerateLog?
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-09-17 07:20:26 PDT
Created attachment 661787 [details] [diff] [review]
Patch v3

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> Comment on attachment 661269 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 661269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK overall (I still haven't tested it, but I'm not worried :-)).
> 
> ::: chat/components/src/logger.js
> @@ +415,5 @@
> > +  const kRegExp = /([\d]{4})-([\d]{2})-([\d]{2}).([\d]{2})([\d]{2})([\d]{2})([+-])([\d]{2})([\d]{2}).*\.([A-Za-z]+)$/;
> > +
> > +  let r = aFilename.match(kRegExp);
> > +  if (!r)
> > +    return null;
> 
> Maybe you need to return [] here instead of null? (see my comment about
> DailyLogEnumerator)

Good call - I didn't check that failure path.

> 
> @@ +425,5 @@
> > +/**
> > + * Returns true if a Conversation is both a chat conversation, and not
> > + * a Twitter conversation.
> > + */
> > +function convIsRealMUC(aConversation) {
> 
> This currently has a single caller, but there's another place in the same
> file
> (http://hg.mozilla.org/comm-central/annotate/8bbb9f722e7b/chat/components/
> src/logger.js#l77) doing the exact same test. Do you feel like reducing the
> duplication? :-)
> 

Done.

> @@ +457,5 @@
> > +        continue;
> > +
> > +      let [logDate] = getDateFromFilename(file.leafName);
> > +      if (!logDate) {
> > +        // We'll skip this one, since it's got a busted filename.
> 
> If I try in my Error Console:
> var [a] = null; a;
> 
> I get:
> TypeError: null has no properties
> 
> Is there something I missed, or is this code broken for busted filenames?
> 

It was busted, yes. var [a] = []; a is now undefined, which we check for.

> @@ +493,5 @@
> > +  hasMoreElements: function() this._index < this._days.length,
> > +
> > +  getNext: function() new LogCluster(this._entries[this._days[this._index++]]),
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleEnumerator])
> 
> Nit: I wouldn't put empty lines here between all these one liners, but I
> don't want to torture you too much with my condensed code style :-).
> 

If we're in /chat, I think it's your way or the highway. I'll go your way. :)

> @@ +525,5 @@
> > +}
> > +LogCluster.prototype = {
> > +  format: "json",
> > +
> > +  __proto__: ClassInfo("imILog", "LogCluster object"),
> 
> Nit: usually we make __proto__ the very first line of the prototype to keep
> the inheritance path obvious.
> 

Fixed.

> @@ +532,5 @@
> > +    let streams = [];
> > +    for each (let entry in this._entries) {
> > +      let fis = new FileInputStream(entry.file, PR_RDONLY, 0444,
> > +                                    Ci.nsIFileInputStream.CLOSE_ON_EOF);
> > +      // Pass in 0x0 so that we throw exceptions on unknown bytes.
> 
> My review comment here about the previous attachment was bogus. I mis-read
> this code comment as meaning that the next line would throw. Sorry about
> that.
> 

No worries.

> @@ +640,5 @@
> >      this._enumerateLogs(aAccount, ".system"),
> > +  getSimilarLogs: function(aLog, aGroupByDay) {
> > +    if (aGroupByDay)
> > +      return new DailyLogEnumerator([new LocalFile(aLog.path).parent
> > +                                                             .directoryEntries]);
> 
> Nit: we usually use {} when the instruction after a test can't fit on a
> single line.
> 
> @@ +642,5 @@
> > +    if (aGroupByDay)
> > +      return new DailyLogEnumerator([new LocalFile(aLog.path).parent
> > +                                                             .directoryEntries]);
> > +
> > +    return new LogEnumerator([new LocalFile(aLog.path).parent.directoryEntries]);
> 
> Anyway, this feels like duplicated code.
> 
> What about doing |let enumerator = aGroupByDay ? DailyLogEnumerator :
> LogEnumerator;| exactly like you did in _enumerateLog?

Fixed.

> r=me with my comments addressed, but feel free to re-request review if you
> feel it's needed or if you think I should really test the final version of
> the patch before it lands.
> 

Since this is something we're (almost certainly) going to want to land on Aurora, I think the more testing the better. Would you mind giving it a run?
Comment 21 Florian Quèze [:florian] [:flo] 2012-09-18 02:44:05 PDT
Comment on attachment 661787 [details] [diff] [review]
Patch v3

I tested this. The only issue I noticed was caused by some local changes I had and disappeared after reverting them (they were just for debugging some performance issue, not something that I intend to put in a patch).

Great job Mike! :-)
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-09-18 07:02:13 PDT
Comment on attachment 661787 [details] [diff] [review]
Patch v3

Jb wanted this for TB 17.
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-09-18 07:11:10 PDT
comm-central: https://hg.mozilla.org/comm-central/rev/8f1eedc904bb
Comment 24 Florian Quèze [:florian] [:flo] 2012-09-20 09:10:38 PDT
The chat/ changes landed for Instantbird as http://hg.instantbird.org/instantbird/rev/dc7cb78c3122
Comment 25 Mark Banner (:standard8) 2012-10-05 08:54:26 PDT
Comment on attachment 661787 [details] [diff] [review]
Patch v3

Although this has interface changes, they are all optional additions, and I doubt whether any binary add-ons would be using that interface, and they've got the beta cycle to rebuild if necessary.
Comment 26 Florian Quèze [:florian] [:flo] 2012-10-05 11:25:45 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/d9b05686e2af
Comment 27 Florian Quèze [:florian] [:flo] 2013-01-22 07:19:45 PST
*** Bug 740858 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.