122213 - Error Console should also include time and date the error occurred

Status: VERIFIED FIXED

(Toolkit Graveyard :: Error Console, enhancement)

Description

[Mark Slater]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.8+) Gecko/20020127
BuildID:    2002012708

When writing shoddy JS, the JS console will fill with all the error msgs. To
make the tracking of errors in JS easier (and not having to hit the "Clear"
button in the JS console all the time), it would be neat to have the time and
date added to each error msg.

Comment 1

[Matthias Versen [:Matti]]

a valid RFE and no dupes found -> NEW

Comment 2

[Serge Gautherie (:sgautherie)]

I'm very much interested in this timestamp feature;
even more as, currently, they is no mean to know when an error occurs:
one have to check the JS.C. on a regular basis :-(

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <consoleBindings.xml>, proof of concept

(I tested it on W98SE.)

Using this patch will exibit the very basic behaviour.

Current issues are that the timestamps:
*are reseted if the JS.C. is reinitialised (as with disable_xul_cache).
*are not part of the 'toString()' involved in the |Copy| feature.

Then, I believe that the work could be splitted in two parts:
*current bug: U.I. design
*Bug 228205: back-end support

Comments welcomed.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 139362 [details] [diff] [review]
(Av1) <consoleBindings.xml>, proof of concept


'r=?': (see comment 3)

This patch review request is not for checkin, but only to get some kind of
approval for this very first step.

Comment 5

[Serge Gautherie (:sgautherie)]

Updating:
+(K) polish: quite easy to add, and would help a lot in output triaging.

Comment 6

[[e-mail]]

Comment on attachment 139362 [details] [diff] [review]
(Av1) <consoleBindings.xml>, proof of concept

I don't think we need a title for the timestamp. Also, for your information,
attributes should be all lowercase letters.

Comment 7

[Serge Gautherie (:sgautherie)]

Prepared...

I'd like to try and see putting the timestamp on the middle or left of the line:
How can I do that ?
And what do you think of this idea ?

Comment 8

[[e-mail]]

I don't think putting the timestamp in the middle of the line would be any good.
I don't know what you mean about putting it on the left, it already is.

Comment 9

[Serge Gautherie (:sgautherie)]

Middle: Agreed (no need to try then ;->)

Left: I meant Right :-<
I wonder if it could help to distinguish it from the actual content of the output:
1) Leave the output as readable as it is now, on the left
2) Allow to better "visually quick sort" on timestamp, being alone on the right

Comment 10

[[e-mail]]

That's no good, sometimes the JavaScript console gets a horizontal scrollbar so
you would want as much important information on the left as possible.

Comment 11

[Serge Gautherie (:sgautherie)]

Good point :->

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <consoleBindings.xml>

Av1, with comment 6 suggestion(s),
plus:

Remove fake timestamp on "InitialItems":
1) To keep the patch simple, I ignore the case of a new item being added while
processing the "InitialItems".
2) This is temporary, as all the "Date computing" will go, and only the U.I.
will remain, once bug 228205 is fixed.

Do we want the timestamp to be included in the |toString()|, for use by
|copySelectedItem| ?
I would say no: I feel it would be more often useless than wanted...

Comments welcomed.

Comment 13

[[e-mail]]

Comment on attachment 139539 [details] [diff] [review]
(Av2) <consoleBindings.xml>

>+      <field name="mInitialItemsProcessing">false</field>
I know that this is only proof of concept and the correct fix for this bug
would have dates on each message but I'd just declare mProcessingInitialItems
as true in the <field> which means it doesn't have to be set here.
>+          this.mInitialItemsProcessing = true;
>           while (++i < messages.length)
>             this.appendItem(messages[i]);
>+          this.mInitialItemsProcessing = false;

>+          row.setAttribute("timestamp", this.mInitialItemsProcessing ? "(no timestamp)" : Date());
Again for your information we wouldn't use literal message strings.

>+          <xul:box>
>+            <xul:description xbl:inherits="xbl:text=timestamp"/>
>+          </xul:box>
I don't think you need the box around the description. In fact, I don't even
think you need a description, a label would do as shown below, unless you want
the date to wrap in a narrow window perhaps. Also, the label/description should
have a class of some sort (hopefully more useful than that label class of label
which looks silly!)
>           <xul:box class="console-row-msg" align="start">
>             <xul:label class="label" xbl:inherits="value=typetext"/>

Comment 14

[Alex Vincent [:WeirdAl]]

I have one minor problem with this concept.  What if the JS console is closed
and the user hits a JS exception?  When the user opens the console, he'll get a
bunch of not-quite-useful timestamps reflecting when the console opened.

There needs to be a lower-level fix in the back-end.  bz suggests tinkering with
ConsoleService directly.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> I have one minor problem with this concept.  What if the JS console is closed
> and the user hits a JS exception?  When the user opens the console, he'll get a
> bunch of not-quite-useful timestamps reflecting when the console opened.

I'm not sure what you mean :-(

> There needs to be a lower-level fix in the back-end.  bz suggests tinkering with
> ConsoleService directly.

This is why we have "Bug 122213 depends on:  	228205", isn't it ?

Comment 16

[Ryan Jones-Ward [:sciguyryan]]

*** Bug 351667 has been marked as a duplicate of this bug. ***

Comment 17

[Michael Ventnor]

Attachment: Patch to nsIScriptError

This should do it. What happens is that during nsIScriptError::Init(), the time is automatically generated and saved using PR_Now(). Callers can still work without modification. Since any sensible caller creates the error object when the error occurs, its safe to assume this.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> Created an attachment (id=260303) [details]
> Patch to nsIScriptError

Why not handle |binding id="message"| too ?

Comment 19

[Michael Ventnor]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=260303) [details] [details]
> > Patch to nsIScriptError
> 
> Why not handle |binding id="message"| too ?
> 

No need to. Besides if I did that I would be doing the same thing as the previous person, getting the time the message was added rather than when the error happened.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> > Why not handle |binding id="message"| too ?
> 
> No need to. Besides if I did that I would be doing the same thing as the
> previous person, getting the time the message was added rather than when the

This person is me ;-)

> error happened.

What you do for error is fine/better ! My patch was only a UI proof of concept.
Still, I'm wondering what (else) would be needed to get "a" time (displayed) for the non-error messages too ?

Comment 21

[Michael Ventnor]

(In reply to comment #20)
> (In reply to comment #19)
> > > Why not handle |binding id="message"| too ?
> > 
> > No need to. Besides if I did that I would be doing the same thing as the
> > previous person, getting the time the message was added rather than when the
> 
> This person is me ;-)

Didn't notice :P

> 
> > error happened.
> 
> What you do for error is fine/better ! My patch was only a UI proof of concept.
> Still, I'm wondering what (else) would be needed to get "a" time (displayed)
> for the non-error messages too ?
> 

Could you explain this further? Everything that wants to print to the error console must go through nsIScriptError since the error console only understands nsIScriptErrors. It doesn't have to be an error, it can be a warning too. And I think messages (except for the console's evaluate) are nsIScriptErrors too. All of them get the time for free (in a sense that they need no modification) with this patch.

I'm still wondering if Niel is the best reviewer. He's the XPFE guy and we've moved on to toolkit since the last patch, and this patch also dabbles with XPConnect. I'm also worried about his queue, he still has outstanding review requests from 2003. :(

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> (In reply to comment #20)
> > Still, I'm wondering what (else) would be needed to get "a" time (displayed)
> > for the non-error messages too ?
> 
> Could you explain this further? Everything that wants to print to the error
> console must go through nsIScriptError since the error console only understands
> nsIScriptErrors. It doesn't have to be an error, it can be a warning too. And I
> think messages (except for the console's evaluate) are nsIScriptErrors too. All
> of them get the time for free (in a sense that they need no modification) with
> this patch.

In that case, that would be fine.
What made me wonder is that I think I needed the "message" binding in my patch to get the display in that case.
Maybe I can wait and see..

> I'm still wondering if Niel is the best reviewer. He's the XPFE guy and we've
> moved on to toolkit since the last patch, and this patch also dabbles with
> XPConnect. I'm also worried about his queue, he still has outstanding review
> requests from 2003. :(

Right:
Neil is an active reviewer;
but I may suggest to split your patch in 3 parts, according to <http://www.mozilla.org/owners.html>:
one for <js/src/xpconnect>,
one for <toolkit>,
one for <xpfe>, which I could do myself after.

Comment 23

[Michael Ventnor]

(In reply to comment #22)
> In that case, that would be fine.
> What made me wonder is that I think I needed the "message" binding in my patch
> to get the display in that case.
> Maybe I can wait and see..
> 

Wait, I think I see what you mean now. Messages do appear to have a different binding, but shares the same interface type (nsIScriptError). So I just need to add a label to contain the time.

> Right:
> Neil is an active reviewer;
> but I may suggest to split your patch in 3 parts, according to
> <http://www.mozilla.org/owners.html>:
> one for <js/src/xpconnect>,
> one for <toolkit>,
> one for <xpfe>, which I could do myself after.

I was explicitly told in another bug that patch splitting is a little no-no, after I tried that little charade to avoid Beltzner's ui-review queue ;)

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> So I just need to
> add a label to contain the time.

Yes, that's what I meant.

> I was explicitly told in another bug that patch splitting is a little no-no,
> after I tried that little charade to avoid Beltzner's ui-review queue ;)

I split TK/XPFE patchs, but that may not apply here.
Then, I would suggest to keep your TK patch as a whole, but anyway request 2 reviews from the involved owners/peers; Neil would review my XPFE port later.

Comment 25

[[e-mail]]

(In reply to comment #21)
>Everything that wants to print to the error console must go through
>nsIScriptError since the error console only understands nsIScriptErrors.
That's not true, the error console understands nsIConsoleMessage
(from which nsIScriptError inherits of course).

Comment 26

[[e-mail]]

Comment on attachment 260303 [details] [diff] [review]
Patch to nsIScriptError

>+          var errorTime = new Date(Math.floor(aObject.time / 1000));
I don't think we need to bother with the floor() here.

>+          var dateService = Components.classes["@mozilla.org/intl/scriptabledateformat;1"].
>+                            getService(Components.interfaces.nsIScriptableDateFormat);
My preference is for
Components.classes["..."]
          .getService(...);

>+          var textTime = dateService.FormatDateTime("", dateService.dateFormatShort,
>+            dateService.timeFormatSeconds, errorTime.getFullYear(), errorTime.getMonth()+1,
>+            errorTime.getDate(), errorTime.getHours(), errorTime.getMinutes(), errorTime.getSeconds());
Spaces around + please. Also I'd prefer 4 spaces of continuation indent.

Comment 27

[[e-mail]]

Comment on attachment 260303 [details] [diff] [review]
Patch to nsIScriptError

I've tested it and found no additional issues.

Comment 28

[Michael Ventnor]

Attachment: Patch 2

Comment 29

[[e-mail]]

(In reply to comment #28)
>Created an attachment (id=260684) [details]
But what about nsIConsoleMessage?

Comment 30

[Michael Ventnor]

(In reply to comment #29)
> (In reply to comment #28)
> >Created an attachment (id=260684) [details] [details]
> But what about nsIConsoleMessage?
> 

I just cannot get that to work. I keep getting a compile error from ld "undefined reference to nsScriptError::GetTime" even though GetTime was in nsConsoleMessage, there was no reference anywhere to any nsScriptError::GetTime, and nsScriptError should inherit from nsConsoleMessage. I just couldn't figure out whats wrong.

In addition, the message-appending function in the error console only takes strings and the error type, not message objects like the error-appending function, but thats not as big a problem as this mysterious compile error.

Comment 31

[[e-mail]]

(In reply to comment #30)
>(In reply to comment #29)
>>But what about nsIConsoleMessage?
>I just cannot get that to work. I keep getting a compile error from ld
>"undefined reference to nsScriptError::GetTime" even though GetTime was in
>nsConsoleMessage, there was no reference anywhere to any
>nsScriptError::GetTime, and nsScriptError should inherit from nsConsoleMessage.
Without seeing your code, my guess is that you added readonly attribute PRUint64 time; to both nsIConsoleMessage and nsIScriptError. (You still need to change the nsIScriptError uuid though, because its vtable will have changed.)

>In addition, the message-appending function in the error console only takes
>strings and the error type, not message objects like the error-appending
>function, but thats not as big a problem as this mysterious compile error.
I don't see how that's a problem - the message-appending function creates the message object for you.

Comment 32

[Michael Ventnor]

(In reply to comment #31)
> (In reply to comment #30)
> >(In reply to comment #29)
> >>But what about nsIConsoleMessage?
> >I just cannot get that to work. I keep getting a compile error from ld
> >"undefined reference to nsScriptError::GetTime" even though GetTime was in
> >nsConsoleMessage, there was no reference anywhere to any
> >nsScriptError::GetTime, and nsScriptError should inherit from nsConsoleMessage.
> Without seeing your code, my guess is that you added readonly attribute
> PRUint64 time; to both nsIConsoleMessage and nsIScriptError. (You still need to
> change the nsIScriptError uuid though, because its vtable will have changed.)

No, everything related to nsScriptError was kept completely untouched. I'm baffled.

> >In addition, the message-appending function in the error console only takes
> >strings and the error type, not message objects like the error-appending
> >function, but thats not as big a problem as this mysterious compile error.
> I don't see how that's a problem - the message-appending function creates the
> message object for you.
> 

I parse the time within the function. But without access to the message object, I can't retrieve the timestamp to parse.

Comment 33

[[e-mail]]

(In reply to comment #32)
>No, everything related to nsScriptError was kept completely untouched.
nsIScriptError, not nsScriptError...

>I parse the time within the function. But without access to the message object,
>I can't retrieve the timestamp to parse.
Perhaps you could change the XBL so that observe passes the entire message object to appendMessage?

Comment 34

[Michael Ventnor]

(In reply to comment #33)
> (In reply to comment #32)
> >No, everything related to nsScriptError was kept completely untouched.
> nsIScriptError, not nsScriptError...

That too...

> >I parse the time within the function. But without access to the message object,
> >I can't retrieve the timestamp to parse.
> Perhaps you could change the XBL so that observe passes the entire message
> object to appendMessage?
> 

Thats what I was planning to do as soon as I figure out my compile error and who is trying to call GetTime.

Comment 35

[[e-mail]]

(In reply to comment #33)
>Perhaps you could change the XBL so that observe passes the entire message
>object to appendMessage?
Actually that doesn't quite work as there's an internal call to appendMessage
(although that's not to say that a solution can't be found).

Comment 36

[Wayne Mery (:wsmwk)]

great to see a patch in progress, despite no progress on bug 228205.
is additional expertise needed for this to make 1.9?

this is #1 console deficiency IMO, making it difficult (some times impossible) to diagnose problems, i.e. match symptoms (the console message) to other aspects of a problem (failed message, a junk message log entry, etc) - so it's really more than "polish"

Comment 37

[:Gijs (he/him)]

Hey Michael, cool to see you were working on this. Are you still? :-)

Comment 41

[David E. Ross]

If bug #474864 (another RFE) is implemented to allow selective deletions of
entries, users will likely want to delete old entries and keep recent entries. 
This will require identifying which entries are "old".  I'm not saying that this bug blocks 474864 but that implementing this RFE would enhance the usefulness of that RFE.  

Over the almost 9 years since this bug report was submitted, four other bug reports have been closed as duplicates of this.  I think that indicates this should have some priority for implementation.

Comment 43

[Wayne Mery (:wsmwk)]

(In reply to Gijs Kruitbosch from comment #37)
> Hey Michael, cool to see you were working on this. Are you still? :-)

I think not.

Comment 44

[Ruth Milner]

Thunderbird 7.0.1 still has no timestamps on Error Console entries, even though this was first requested in 2002!

I'm trying to describe errors that may be associated with a bug I've reported (700229), and the lack of timestamps makes it impossible to tell which errors are correlated with various actions I took, or whether some of them occurred together. Surely that kind of information would be helpful to developers trying to use the error logs to figure out problems?

Comment 45

[rsx11m]

Comment on attachment 260684 [details] [diff] [review]
Patch 2

Patch does no longer apply to mozilla-central; 3 hunks failed even after adjustments for src/xpconnect path changes, thus cancelling requests:

> patching file js/xpconnect/src/nsScriptError.cpp
> Hunk #1 FAILED at 38.
> Hunk #2 FAILED at 117.
> 2 out of 2 hunks FAILED -- saving rejects to file js/xpconnect/src/nsScriptError.cpp.rej
> patching file js/xpconnect/src/xpcprivate.h
> Hunk #1 FAILED at 3181.
> 1 out of 1 hunk FAILED -- saving rejects to file js/xpconnect/src/xpcprivate.h.rej
> patching file js/xpconnect/idl/nsIScriptError.idl
> patching file toolkit/components/console/content/consoleBindings.xml
> Hunk #1 succeeded at 172 (offset 6 lines).
> Hunk #2 succeeded at 213 (offset 6 lines).
> Hunk #3 succeeded at 348 with fuzz 1 (offset 7 lines).

Comment 46

[:aceman]

OK, I can try to update the patch. Let's see what it does so far.

Comment 47

[Philip Chee]

> patching file js/xpconnect/src/nsScriptError.cpp
> Hunk #1 FAILED at 38.
> Hunk #2 FAILED at 117.

This fails because Bug 670896 added a "long long timeStamp" to nsIScriptError2. Sigh. Don't know why they didn't use PRUint64.

Comment 48

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #47)
> This fails because Bug 670896 added a "long long timeStamp" to
> nsIScriptError2.

Interesting: should comm-central callers (there are not too many) be switched to nsIScriptError2 from nsIScriptError?

http://mxr.mozilla.org/comm-central/search?string=nsIScriptError%5B%5E2%5D&regexp=on&case=1

Comment 49

[:aceman]

Hi Serge, will you pick up the patch after all?

Comment 50

[Philip Chee]

> Interesting: should comm-central callers (there are not too many) be switched to
> nsIScriptError2 from nsIScriptError?
Not unless you need the inner window ID from which the error comes from. This was added so that the Firefox web console could filter based on which tab it was viewing.

> Hi Serge, will you pick up the patch after all?
It was just a drive by comment. Please proceed if you are still willing.

Comment 51

[:aceman]

(In reply to Philip Chee from comment #47)
> > patching file js/xpconnect/src/nsScriptError.cpp
> > Hunk #1 FAILED at 38.
> > Hunk #2 FAILED at 117.
> 
> This fails because Bug 670896 added a "long long timeStamp" to
> nsIScriptError2. Sigh. Don't know why they didn't use PRUint64.

It is PRInt64 mTimeStamp now. Should I update it to PRUint64? It seems to be initialized and retrieved in the same way as the proposed mTime in the patch. Can I use it for the purposes of this feature instead of mTime?

Comment 52

[:aceman]

Actually it is PRUint64 as defined in xpcprivate.h. Only the GetTimeStamp method takes and output argument of PRInt64. Is that OK?

Comment 53

[:aceman]

Attachment: patch v3

I updated the patch and it seems to work. The current state:
- I also got the build failure Michael was seeing. But I managed to fix that by properly aligning the function names and arguments. The calling of GetTime was in consoleBindings.xml .
- I had to move the timeStamp property from nsIScriptError2 to nsIScriptError and made it PRUint64.
- I changed UUID of nsIScriptError to the one in the patch. I changed  UUID of nsIScriptError2 randomly, I don't know how to generate one.

Now, the timestamp displays in the error console in the top left corner of the message, above the message text. So it adds a third line to it (as in Time\n Message\n Sourcefile).
Was this design already agreed upon?

The discussion in this bug is quite old. Who should now review this?

Comment 54

[:aceman]

Ah, and I hooked onto the existing mTimeStamp property as mentioned in comment 51 so the patch got smaller. Is is OK?

Comment 55

[:aceman]

Also, when the message is copied from the console, the timestamp is not copied into the clipboard. That could be good. Is there any usecase when it would be needed?

Comment 56

[B.J. Herbison]

My use case for including the time stamp would be recording observed errors for later analysis. If I see something strange I'll save the information to compare it with later occurrences. The timing information is often more important in this case because the memory of when the events occurred will no longer be fresh.

Comment 57

[:aceman]

OK, I was able to add that too in my working version of the patch. Depends on the reviewers what they think about that.

Comment 58

[Philip Chee]

Just some drive-by comments. Please feel free to ignore anything I say.

> - I had to move the timeStamp property from nsIScriptError2 to nsIScriptError
> and made it PRUint64.

Is it necessary to remove timeStamp from nsIScriptError2? I don't know, so I'm asking. There are some callers ofnsIScriptError2 especially in the Firefox WebConsole (HUD). Make sure you didn't break them!

> - I changed UUID of nsIScriptError to the one in the patch. I changed  UUID
> of nsIScriptError2 randomly, I don't know how to generate one.

Please use a proper UUID generator:
http://www.google.com/search?q=uuid+generator&start=0&start=0&ie=utf-8&oe=utf-8&client=mozilla
Like this one:
http://www.famkruithof.net/uuid/uuidgen

> +      <method name="properFormatTime">
> +        <parameter name="aTime"/>
> +        <body><![CDATA[
> +          const dateServ = Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
> +                                     .getService(Components.interfaces.nsIScriptableDateFormat);
> +          var errorTime = new Date(aTime);
> +          return dateServ.FormatDateTime("", dateServ.dateFormatShort, dateServ.timeFormatSeconds,
> +              errorTime.getFullYear(), errorTime.getMonth() + 1, errorTime.getDate(),
> +              errorTime.getHours(), errorTime.getMinutes(), errorTime.getSeconds());
> +        ]]></body>
> +      </method>
Just my opinion. We don't need the year, month, and date here. Currently in Console2 I am doing:

if (aMessage instanceof Components.interfaces.nsIScriptError2)
{
	var ts = new Date(aMessage.timeStamp).toTimeString();
		var tss = ts.substr(0, ts.indexOf(" "));
    	row.setAttribute("timestamp", tss);
}

> Now, the timestamp displays in the error console in the top left corner of the
> message, above the message text. So it adds a third line to it (as in Time\n
> Message\n Sourcefile).

> +          <xul:box class="console-row-time">
> +            <xul:label class="label" xbl:inherits="value=time"/>
> +          </xul:box>

Messages don't have timestamps. Not every row has a timestamp.
Just a suggestion put it on the right side of the "console-row-msg":

>            <xul:box class="console-row-msg" align="start">
>              <xul:label class="label" xbl:inherits="value=typetext"/>
>              <xul:description class="console-error-msg" xbl:inherits="xbl:text=msg" flex="1"/>

> +            <xul:label class="label" xbl:inherits="value=time"/>

>            </xul:box>

Comment 59

[:aceman]

(In reply to Philip Chee from comment #58)
> > - I had to move the timeStamp property from nsIScriptError2 to nsIScriptError
> > and made it PRUint64.
> 
> Is it necessary to remove timeStamp from nsIScriptError2? I don't know, so
> I'm asking. There are some callers ofnsIScriptError2 especially in the
> Firefox WebConsole (HUD). Make sure you didn't break them!
Yes, it was required otherwise it wouldn't build (compiler errored out due to redeclaration of the variable). But I imagine nsIScriptError2 is an extension of nsIScriptError, it inherits everything nsIScriptError has. I can't build Firefox, only test on Thunderbird. If anybody can test if it broke Firefox, I would appreciate it.

Thanks for the other hints, I'll 

> Please use a proper UUID generator:
Can I use the one that I found preinstalled on my linux distro?
It seems to produce the correct format.

> Just my opinion. We don't need the year, month, and date here.
I don't know myself, I'll leave it in for now.

> Messages don't have timestamps. Not every row has a timestamp.
> Just a suggestion put it on the right side of the "console-row-msg":
Thanks, I'll see if it is nicer. 

Thanks for helpful hints.

Comment 60

[Ruth Milner]

(In reply to B.J. Herbison from comment #56)
> My use case for including the time stamp would be recording observed errors
> for later analysis. If I see something strange I'll save the information to
> compare it with later occurrences. The timing information is often more
> important in this case because the memory of when the events occurred will
> no longer be fresh.

I 100% agree with this, although I'm approaching it from the opposite direction: as a user relaying observed errors who needs to include the data in the report. Having to manually type in the timestamps next to copied-and-pasted messages would not only be tedious (leading fewer people to provide it) but prone to error.

I also agree with the suggestion to make the timestamp part of the message itself; IMO it's easier to see what it belongs with and has less chance of error in copying/pasting than if it's on a line by itself.

Comment 61

[:aceman]

(In reply to Philip Chee from comment #58)
> Messages don't have timestamps. Not every row has a timestamp.
> Just a suggestion put it on the right side of the "console-row-msg":
> 
> >            <xul:box class="console-row-msg" align="start">
> >              <xul:label class="label" xbl:inherits="value=typetext"/>
> >              <xul:description class="console-error-msg" xbl:inherits="xbl:text=msg" flex="1"/>
> > +            <xul:label class="label" xbl:inherits="value=time"/>
> >            </xul:box>

This doesn't seem to work. The timestamp is nowhere to be seen.

Comment 62

[Philip Chee]

> This doesn't seem to work. The timestamp is nowhere to be seen.
Are your error messages long enough to cause a horizontal scroll bar to appear at the bottom of the console?

In Console2.xml I have:

<xul:box>
  <xul:description xbl:inherits="xbl:text=msg" flex="1"/>
  <xul:label class="console-row-dupes" xbl:inherits="value=dupes" tooltiptext="&dupes.tooltip;"/>
  <xul:label class="console-row-timestamp" xbl:inherits="value=timestamp"/>
</xul:box>

Comment 63

[:aceman]

No scrollbar visible. Maybe the label needs some alignment specified. Via class or CSS in theme.

Comment 64

[[e-mail]]

Comment on attachment 580650 [details] [diff] [review]
patch v3

>+    /* Elapsed time, in milliseconds, from a platform-specific zero time to the
>+       time the message was created. */
>+    readonly attribute PRUint64 timeStamp;
The original definition of the attribute was in fact correct, since it's the result of dividing a PRTime by 1000. (And then stored in a PRUint64, just to confuse you, because the coder forgot to fix it as requested...) However as it points out the value isn't necessarily equal to the date. It was created for the HUDService for some reason and at the time it was decided that the wall clock date was sufficient for its purpose but they wanted to be able to change it at some later point. Since this was several months ago, and they haven't seen fit to change it, maybe we can get this changed to an actual date.

>-[scriptable, uuid(4472646b-c928-4d76-9e7c-6b91da7f24cc)]
>+[scriptable, uuid(4472646b-c928-4d76-9e7c-6b91da7f24cf)]
> interface nsIScriptError2 : nsISupports {
[UUID change discussed previously] It's probably beyond the scope of this bug, but nsIScriptError2 may have been created for reasons which no longer apply and it should simply be merged with nsIScriptError.

>+          row.setAttribute("time", this.properFormatTime(aObject.timeStamp));
I agree with one of the commenters that the time on its own is probably sufficient, although I would use Date's toLocaleTimeString method.

>+          <xul:box class="console-row-time">
>+            <xul:label class="label" xbl:inherits="value=time"/>
>+          </xul:box>
>           <xul:box class="console-row-msg" align="start">
>             <xul:label class="label" xbl:inherits="value=typetext"/>
>             <xul:description class="console-error-msg" xbl:inherits="xbl:text=msg" flex="1"/>
>           </xul:box>
I quite like the idea of having the time at the start of the same line as the type and description. (You would want to clipboard copy to look the same.)

Comment 65

[Ruth Milner]

(In reply to [e-mail] from comment #64)
> I agree with one of the commenters that the time on its own is probably sufficient

Is there a particular reason *not* to include the date as well? I can think of cases where you would need to look at errors over a span of more than 24 hours, or after >24 hours of inactivity. Unless including the date causes more than cosmetic problems, I would say that leaving out potentially useful information defeats the purpose of making this change. I'd bet that pretty soon you'd have someone asking for the date to be included as well. As an admittedly trivial example in real life, my answering machine only gives weekday and time on each message, and pretty much every time I go away for more than 7 days, it ends up being impossible to know which week a given message was left. Why beg for more changes to be needed later when you're making changes now?

Comment 66

[Boris Zbarsky [:bzbarsky]]

The HUDservice wanted to have a way to sort messages by the order they were dispatched in.  That required stamping each one with the time it happened.  Wall-clock date is actually completely broken for this, and that code really needs to switch to a monotonic clock at some point; if we want a wall-clock date we should have a separate property for it.  Imo.

> nsIScriptError2 may have been created for reasons which no longer apply 

It was created because the changes that introduced it came after Firefox 4 interface freeze, iirc.

Comment 67

[Philip Chee]

> It was created because the changes that introduced it came after Firefox 4 interface
> freeze, iirc.
Didn't Firefox 4 explicitly unfreeze all interfaces?

Comment 68

[Philip Chee]

> Is there a particular reason *not* to include the date as well? I can think of cases
> where you would need to look at errors over a span of more than 24 hours, or after >24
The error console has a hard coded FIFO limit of 500 messages. Anything older is pushed off the console. Over the span of 24 hours I'd expect that in the common case anything older than a few hours would be aged off.

Comment 69

[Boris Zbarsky [:bzbarsky]]

> Didn't Firefox 4 explicitly unfreeze all interfaces?

We always freeze interfaces close enough to a release so as not to break extensions.  For the rapid releases this would be somewhere between the aurora and beta transitions, for example.

Comment 70

[Ruth Milner]

(In reply to Philip Chee from comment #68)
> Over the span of 24 hours I'd expect that
> in the common case anything older than a few hours would be aged off.

I'm not sure where this estimate comes from. My Thunderbird (which is what I got into this discussion for) has generated only 30 entries in its Error Console in the past 12 hours. My Firefox has been running for more than three days, with multiple tabs and many sites visited, and has about 250. Of course, I can't tell how old the oldest ones are (yet :-) ), but since it's nowhere near 500, it appears that several days' worth of errors can remain there before it hits the limit. So the full date could well be relevant in diagnosing problems.

Comment 71

[:aceman]

(In reply to [e-mail] from comment #64)
> >+    /* Elapsed time, in milliseconds, from a platform-specific zero time to the
> >+       time the message was created. */
> >+    readonly attribute PRUint64 timeStamp;
> The original definition of the attribute was in fact correct, since it's the
> result of dividing a PRTime by 1000. (And then stored in a PRUint64, just to
> confuse you, because the coder forgot to fix it as requested...)
Which one is the original definition? The one in patch 2? Should I use the comment from there?

> It's probably beyond the scope of this
> bug, but nsIScriptError2 may have been created for reasons which no longer
> apply and it should simply be merged with nsIScriptError.
I'd rather not touch this in this bug. I do not guarantee I would be able to fix all the users of it.

> >+          <xul:box class="console-row-time">
> >+            <xul:label class="label" xbl:inherits="value=time"/>
> >+          </xul:box>
> >           <xul:box class="console-row-msg" align="start">
> >             <xul:label class="label" xbl:inherits="value=typetext"/>
> >             <xul:description class="console-error-msg" xbl:inherits="xbl:text=msg" flex="1"/>
> >           </xul:box>
> I quite like the idea of having the time at the start of the same line as
> the type and description. (You would want to clipboard copy to look the
> same.)
My current implementation has the time on a separate line above the description. I can try to move it before it. But it may be ugly and unaligned, as messages with severity of "info" do not have a timestamp.
I can include the time in the clipboard copy.

(In reply to Ruth Milner from comment #70)
> (In reply to Philip Chee from comment #68)
> > Over the span of 24 hours I'd expect that
> > in the common case anything older than a few hours would be aged off.
> 
> I'm not sure where this estimate comes from. My Thunderbird (which is what I
> got into this discussion for) has generated only 30 entries in its Error
> Console in the past 12 hours.
Good point. TB has much less messages and could be running for longer time spans than Firefox.

Comment 72

[[e-mail]]

(In reply to Ruth Milner from comment #65)
> (In reply to comment #64)
> > I agree with one of the commenters that the time on its own is probably sufficient
> Is there a particular reason *not* to include the date as well?
The number of people who would find the date immediately useful is much less than the number of people who would find the time useful. Maybe we could put the date in a tooltip, or expose it via a preference.

(In reply to Boris Zbarsky from comment #66)
> The HUDservice wanted to have a way to sort messages by the order they were
> dispatched in.  That required stamping each one with the time it happened.
I'm not convinced that it did. The console service already maintains the order of console messages. If you need to compare messages that have fallen off the console service, then maybe you just need an index or count property. I can't even find any consumers of the nsIScriptError2 timeStamp property.

(In reply to aceman from comment #71)
> (In reply to comment #64)
> > >+    /* Elapsed time, in milliseconds, from a platform-specific zero time to the
> > >+       time the message was created. */
> > >+    readonly attribute PRUint64 timeStamp;
> > The original definition of the attribute was in fact correct, since it's the
> > result of dividing a PRTime by 1000. (And then stored in a PRUint64, just to
> > confuse you, because the coder forgot to fix it as requested...)
> Which one is the original definition?
The one that you deleted, i.e. long long.

> > It's probably beyond the scope of this
> > bug, but nsIScriptError2 may have been created for reasons which no longer
> > apply and it should simply be merged with nsIScriptError.
> I'd rather not touch this in this bug. I do not guarantee I would be able to
> fix all the users of it.
Fair enough. We have other contributors who seem to like this sort of thing.

> > I quite like the idea of having the time at the start of the same line as
> > the type and description. (You would want to clipboard copy to look the
> > same.)
> My current implementation has the time on a separate line above the
> description. I can try to move it before it. But it may be ugly and
> unaligned, as messages with severity of "info" do not have a timestamp.
Nor do they have the "Warning:" or "Error:" prefix, do they?

Comment 73

[:aceman]

> (In reply to aceman from comment #71)
> > (In reply to comment #64)
> > > >+    /* Elapsed time, in milliseconds, from a platform-specific zero time to the
> > > >+       time the message was created. */
> > > >+    readonly attribute PRUint64 timeStamp;
> > > The original definition of the attribute was in fact correct, since it's the
> > > result of dividing a PRTime by 1000. (And then stored in a PRUint64, just to
> > > confuse you, because the coder forgot to fix it as requested...)
> > Which one is the original definition?
> The one that you deleted, i.e. long long.
So should I convert it back to 'long long' in nsIScriptError.idl but leave it as PRUint64 in nsScriptError.cpp?

> > > I quite like the idea of having the time at the start of the same line as
> > > the type and description. (You would want to clipboard copy to look the
> > > same.)
> > My current implementation has the time on a separate line above the
> > description. I can try to move it before it. But it may be ugly and
> > unaligned, as messages with severity of "info" do not have a timestamp.
> Nor do they have the "Warning:" or "Error:" prefix, do they?
Those prefixes are not used anywhere. They are not before warnings and errors, so currently all severities of messages are aligned. Do you see something else?

Comment 74

[Karsten Düsterloh]

> > Is there a particular reason *not* to include the date as well?
> The number of people who would find the date immediately useful is much less
> than the number of people who would find the time useful.

Is it? For me, a timestamp always includes the date - timestamps without dates are completely useless (there are morons in the Linux log world who believe differently and it's just a real pita), especially when you need to compare logs or are running the application for a couple of days.

> Maybe we could put the date in a tooltip, or expose it via a preference.

Tooltip requires searching your mouse.
Preference would be much better, but just displaying the complete timestamp would be best.
IMO, etc. pp.

Comment 75

[[e-mail]]

(In reply to aceman from comment #73)
> > > Which one is the original definition?
> > The one that you deleted, i.e. long long.
> So should I convert it back to 'long long' in nsIScriptError.idl
Yes please
> but leave it as PRUint64 in nsScriptError.cpp?
No, that would need to be PRInt64.

> > Nor do they have the "Warning:" or "Error:" prefix, do they?
> Those prefixes are not used anywhere. They are not before warnings and
> errors, so currently all severities of messages are aligned. Do you see
> something else?
Those were what I was actually referring to! A standard message is just a block of (monospaced) text, while a warning or error consists of a heading (conveniently either Warning: or Error:) and then the text of the message, i.e.
M e s s a g e (yes, hard to emulate monospaced text here...)
Warning: assignment to undeclared variable
Error: null has no properties
The option I originally thought of was to add the time so:
20:12:01 Error: null has no properties
but I've since come up with the following additional alternatives:
  Error: this is one of those errors that's quite long
20:12:01 and needs more than one line to do it justice
Or perhaps
20:12:01 Error: this is one of those errors that's quite long
20/11/2011      and needs more than one line to do it justice
Or perhaps even
20:12:01 20/11/2011 Error:
null has no properties

Comment 76

[Boris Zbarsky [:bzbarsky]]

> The console service already maintains the order of console messages.

Not all the console stuff goes through the console service.   And there were some complications with e10s.  But yes, they don't seem to be using it now.  Mihai?

Comment 77

[:aceman]

Attachment: error console screenshot

> M e s s a g e (yes, hard to emulate monospaced text here...)
> Warning: assignment to undeclared variable
> Error: null has no properties

Sorry, but this is not what I see. The "Error: " and "Warning: " prefixes are not in the Error console. See the attachment.

Comment 78

[[e-mail]]

(In reply to aceman from comment #77)
> Sorry, but this is not what I see.
It turns out this depends on which theme you're using, sigh.
If you're on the Mac, then you do get the headings.
But in bug 291002 Mike Beltzner had them hidden on Windows and Linux
(originally using the same file; bug 427717 split it up.)
I was using SeaMonkey's Modern theme, which also shows the headings.

Comment 79

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #76)
> > The console service already maintains the order of console messages.
> 
> Not all the console stuff goes through the console service.   And there were
> some complications with e10s.  But yes, they don't seem to be using it now. 
> Mihai?

That is correct. Not all of the Web Console stuff goes through the nsIConsoleService.

We need the date and time of each script error to be able to sort all the messages that come from different places.

We are fine if nsIScriptError2 merges into nsIScriptError.

We are currently not using the new timeStamp property fron nsIScriptError2 because we are still waiting for the review of bug 611032. When that lands, we will use it.

Comment 80

[:aceman]

(In reply to Mihai Sucan [:msucan] from comment #79)
> (In reply to Boris Zbarsky (:bz) from comment #76)
> We are currently not using the new timeStamp property fron nsIScriptError2
> because we are still waiting for the review of bug 611032. When that lands,
> we will use it.
So if you start using it, what will you do with it? Can I keep on displaying it's value as the timestamp of the error in error console? Or will you set it to something else than it is now (pr_now/1000)?

(In reply to [e-mail] from comment #78)
> It turns out this depends on which theme you're using, sigh.
> If you're on the Mac, then you do get the headings.
> But in bug 291002 Mike Beltzner had them hidden on Windows and Linux
> (originally using the same file; bug 427717 split it up.)
> I was using SeaMonkey's Modern theme, which also shows the headings.
I am using the default theme on Win XP and Linux. So what comes out of this? Is seems we need some UI people to decide this.

Comment 81

[[e-mail]]

(In reply to Boris Zbarsky from comment #76)
> > The console service already maintains the order of console messages.
> And there were some complications with e10s.
If you need to compare events from different windows, it would seem that Date.now() would work better across e10s than mozilla::TimeStamp.

(In reply to Mihai Sucan from comment #79)
> We need the date and time of each script error to be able to sort all the
> messages that come from different places.
I only see one sort command in bug 611032; it compares the timestamps of the messages that the console service already provided in the correct order...

I only see one other timestamp-like variable, and it defaults to Date.now(), which would seem to mean that nsIScriptError2's timestamp should match.

Comment 82

[Boris Zbarsky [:bzbarsky]]

One note: PR_Now and Date.now use different time sources; there is no particular reason they would agree.

Comment 83

[Mihai Sucan [:msucan]]

(In reply to :aceman from comment #80)
> (In reply to Mihai Sucan [:msucan] from comment #79)
> > (In reply to Boris Zbarsky (:bz) from comment #76)
> > We are currently not using the new timeStamp property fron nsIScriptError2
> > because we are still waiting for the review of bug 611032. When that lands,
> > we will use it.
> So if you start using it, what will you do with it? Can I keep on displaying
> it's value as the timestamp of the error in error console? Or will you set
> it to something else than it is now (pr_now/1000)?

We will only read that property. We do not need to change the nsIScriptError2 timestamps from the Web Console.

Yes, I believe you can continue to display the value of the timestamp in the Error Console.

(In reply to [e-mail] from comment #81)
> (In reply to Mihai Sucan from comment #79)
> > We need the date and time of each script error to be able to sort all the
> > messages that come from different places.
> I only see one sort command in bug 611032; it compares the timestamps of the
> messages that the console service already provided in the correct order...
> 
> I only see one other timestamp-like variable, and it defaults to Date.now(),
> which would seem to mean that nsIScriptError2's timestamp should match.

In that patch we sort messages that come from the nsIConsoleService (the nsIScriptError2 ones) and the Console API messages (which are Date.now() timestamps). We do the sorting with an array that holds both types of messages at once.

The nsIConsoleService does provide messages in order, but putting the Console API messages between the nsIScriptError2 messages, in the correct order, requires we have time stamps for both types of messages.

Comment 84

[Boris Zbarsky [:bzbarsky]]

> Yes, I believe you can continue to display the value of the timestamp in the Error
> Console.

Only if we change the interface accordingly.

Again, the current implementation of the interface happens to be broken.  If the consumer is ok with that brokenness, then we could just change the interface to do something totally different, keep the implementation, and call it a day, I guess.

Comment 85

[[e-mail]]

(In reply to Mihai Sucan from comment #83)
> We will only read that property. We do not need to change the
> nsIScriptError2 timestamps from the Web Console.
He means are you likely to want to change the meaning of that property?

> In that patch we sort messages that come from the nsIConsoleService (the
> nsIScriptError2 ones) and the Console API messages (which are Date.now()
> timestamps). We do the sorting with an array that holds both types of
> messages at once.
Ah yes, I overlooked that before. Sorry about that.

(In reply to Boris Zbarsky from comment #84)
> Again, the current implementation of the interface happens to be broken.
Is there a C++ equivalent of Date.now() or a JS equivalent of PR_Now()?
(Is the difference between Date.now() and PR_Now() likely to be significant?)

Comment 86

[Mihai Sucan [:msucan]]

(In reply to [e-mail] from comment #85)
> (In reply to Mihai Sucan from comment #83)
> > We will only read that property. We do not need to change the
> > nsIScriptError2 timestamps from the Web Console.
> He means are you likely to want to change the meaning of that property?

Ah, sorry I misunderstood his question.

We are not likely to want to change the meaning of the timestamp property. The internal type of the property does not matter to us, as long as we get a timestamp we can compare to Date.now().

> > In that patch we sort messages that come from the nsIConsoleService (the
> > nsIScriptError2 ones) and the Console API messages (which are Date.now()
> > timestamps). We do the sorting with an array that holds both types of
> > messages at once.
> Ah yes, I overlooked that before. Sorry about that.

No worries!

Comment 87

[Boris Zbarsky [:bzbarsky]]

> Is there a C++ equivalent of Date.now() or a JS equivalent of PR_Now()?

JS_Now() for the former.  Nothing for the latter.  I would be ok, I think, with defining this interface to return whatever Date.now() is returning.  As long as people are OK with the timestamps on this interface being allowed to decrease as time passes in the real world, and such, of course.

> (Is the difference between Date.now() and PR_Now() likely to be significant?)

I've seen it be on the order of several milliseconds.  Whether that's significant depends on your usage, of course.

> as long as we get a timestamp we can compare to Date.now().

I still think that's a broken way to think about it.  What you want is to be able to compare to a useful monotonic clock source, not Date.now().  You just don't have such a clock source in JS yet.  You will soon, though; these things are getting specced.

Comment 88

[:aceman]

Attachment: patch v4

Various fixes. I couldn't make the timestamp show in any other place than a new box above the message text.

Comment 89

[[e-mail]]

At a quick glance this seems reasonable except that I would have expected the clipboard to contain the strings in the same order that they are displayed.

Comment 90

[:aceman]

Any other comments from anybody? Boris? Should I change it to JS_now instead of pr_now/1000?
Could you point an example when the timestamps from date.now() can decrease? Daylight savings changes?

Comment 91

[Boris Zbarsky [:bzbarsky]]

> Could you point an example when the timestamps from date.now() can decrease?

Most obviously, NTP time adjustments.  But also the user changing the clock, and various other things.  Daylight savings time is a timezone change; it doesn't affect the values used here, which are always UTC.

Comment 92

[[e-mail]]

Comment on attachment 581049 [details] [diff] [review]
patch v4

Now I can see why you didn't put the timestamp first, although it would be nice if the clipboard could match the display (in sane themes, at least).

Comment 93

[:aceman]

(In reply to Boris Zbarsky (:bz) from comment #91)
> > Could you point an example when the timestamps from date.now() can decrease?
> Most obviously, NTP time adjustments.  But also the user changing the clock,
> and various other things.  Daylight savings time is a timezone change; it
> doesn't affect the values used here, which are always UTC.
OK, I think this could be OK for now. Common people will probably expect the time match the system time (what they see on the taskbar).

I don't know if that is good enough for some programmatic analysis of the timestamps.

Comment 94

[Boris Zbarsky [:bzbarsky]]

Sure, for display times Date.now is fine.  What it's not fine for is sorting.

Comment 95

[:aceman]

Attachment: patch v5

Comment 96

[[e-mail]]

Comment on attachment 581347 [details] [diff] [review]
patch v5

>+    /* Elapsed time, in milliseconds, from a platform-specific zero time to the
>+       time the message was created. */
>+    readonly attribute long long timeStamp;
I got confirmation on IRC that we should describe this as what we use it as, and when someone wants a true timestamp then they will add their own property instead. r=me with that fixed.

>+    mTimeStamp = JS_Now() / 1000;
[Would be nice if we could change mTimeStamp to be signed type to match JS_Now and timeStamp]

>-          var strBundle = this._ConsoleBox.mStrBundle;
>+          let strBundle = this._ConsoleBox.mStrBundle;
[I wouldn't normally have bothered to change this but I suppose you're changing so much of the rest it doesn't matter]

Comment 97

[:aceman]

(In reply to [e-mail] from comment #96)
> >-          var strBundle = this._ConsoleBox.mStrBundle;
> >+          let strBundle = this._ConsoleBox.mStrBundle;
> [I wouldn't normally have bothered to change this but I suppose you're
> changing so much of the rest it doesn't matter]

Did I do good or bad? I had to reorder some of the lines (to put timestamp in the front) so I changed this one to 'let' while there. I also fixed some trailing spaces in that function.

I'll fix the rest of the comments, thanks.

Comment 98

[[e-mail]]

I'm not keen on gratuitous changes from var to let, especially when you're not in an inner block where let actually has a special effect, but I'm not so bothered this time since you're effectively making wholesale changes anyway.

Comment 99

[David E. Ross]

Regarding all the discussion here about how and what to implement:  

Step back.  Do what is necessary to provide meaningful data when an end-user is requested to communicate the relevant Error Console entries for an asserted problem.  

Examples:

Is the time sufficient; or is the date also required?  If the latter, putting the date in a tooltip is not good enough.  

Should the user communicate the entire content of the Error Console, a single entry, or a span of entries?  The ability to communicate entries should involve the most simple actions by the end-user if that user's cooperation is desired.  If a span of entries might be useful to a developer, then there needs to be the capability to mark and copy a span.   

Is it significant to a developer whether an entry is a message, warning, or error?  If so, that indicator must be contained in the copied entry.

Comment 100

[:aceman]

I think the first point was already discussed. For now, both time and date are shown, we'll see how it pans out.

The other points are not the topic of this bug, IMHO. The last one is even solved, the indicator is copied already.

Comment 101

[Philip Chee]

Yes, lets get this in the tree (101 Comments already!!). Apple polishing can be moved to follow-up bugs.

Comment 102

[:aceman]

Actually, my suggestion would be to get this into FF12 and then there would be time to polish it in follow-up bugs. The placement of the timestamp and whether date should be even displayed should be resolved there (by someone else, as my experiments failed so far).
Or can this polishing be done on Aurora 11 channel? Also, I assume this will need superreview. I'll attach a patch with comment 96 fixed soon.

Comment 103

[:aceman]

Attachment: patch v6

Comment 104

[Dave Townsend [:mossop]]

Comment on attachment 582021 [details] [diff] [review]
patch v6

I'm not really a good sr for xpconnect, bz maybe as he's been involved in this bug already?

Comment 105

[[e-mail]]

Comment on attachment 582021 [details] [diff] [review]
patch v6

There were no significant changes that necessitated a new review, my review for patch v5 sufficed.

>+       Elapsed time, in milliseconds, from a platform-specific zero time to the
>+       time the message was created.
[This bit is probably unnecessary.]

Comment 106

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 582021 [details] [diff] [review]
patch v6

Please fix the "platform-specific zero time" bit; this property is now guaranteeing that it will return epoch time, no?  And no need to list the bug in the comment; it'll be obvious in the blame.

sr=me with the comment fixed.

Comment 107

[:aceman]

Attachment: patch v7

Comment 108

[[e-mail]]

Comment on attachment 582301 [details] [diff] [review]
patch v7

(Didn't need to ask me for rereview)

Comment 109

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7fd1460aa284

Comment 110

[:aceman]

I have filed the outstanding issues as followup bugs, per comment 101 and 102.
Thanks for your help.

Comment 111

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7fd1460aa284

Comment 112

[rsx11m]

aceman: I think it's up to someone other than the assignee to mark a bug report as verified fixed, but I'm happy to confirm that the time stamps are functioning well on aurora-channel builds. Thanks!

Comment 113

[:aceman]

Yeah, theoretically the reporter should do it:) But I really doubt anybody would bother with it for most of my bugs.

Comment 114

[rsx11m]

Many bugs are declared fixed but remain unverified, thus it's probably not a big deal unless in high-profile cases. The main thing is that the bug *is* fixed!  ;-)

Comment 115

[Mark Slater]

My bug! Finally fixed! Haha ;-)

While I don't have an aurora-channel build installed, I'll give rsx11 the benefit of doubt ;-)

Comment 116

[:aceman]

Hi, sorry for thinking you must be "dead" after 10 years :)

Comment 117

[Mark Slater]

No worries

Comment 118

[:aceman]

Firefox 11 final is released now, you can check it out :)