587615 - lastTimestamp reset problems in WebConsole

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Mihai Sucan [:msucan]]

In JSTerm.clearOutput() and HUDService.clearDisplay() we have:

  outputNode.lastTimestamp = 0;

While in other places of the code hudBox.lastTimestamp is used. The appendGroupIfNecessary method uses hudBox.lastTimestamp, not outputNode.lastTimestamp.

From the perspective of the user, I am not aware of a bug that is caused by this error in the code. However, I believe this is a bug that will catch us some day.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Asking Patrick for feedback - as far as I know he worked once with the code that does output grouping. Thanks!

Comment 2

[Patrick Walton (:pcwalton)]

Comment on attachment 476648 [details] [diff] [review]
proposed fix and test code

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -4075,17 +4075,21 @@ JSTerm.prototype = {
>   clearOutput: function JST_clearOutput()
>   {
>     let outputNode = this.outputNode;
> 
>     while (outputNode.firstChild) {
>       outputNode.removeChild(outputNode.firstChild);
>     }
> 
>-    outputNode.lastTimestamp = 0;
>+    let hudBox = outputNode;
>+    while (hudBox && hudBox.getAttribute("class") != "hud-box") {
>+      hudBox = hudBox.parentNode;
>+    }
>+    hudBox.lastTimestamp = 0;

I'd remove the "hudBox &&" from the conditional - if the output node isn't in a HUD box we're screwed, and the "hudBox.lastTimestamp = 0" statement following will fail anyway. Also I think 'hudBox.classList.contains("hud-box")' would be more future-proof.

Otherwise, looks good - my bad for screwing that up!

Comment 3

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks Patrick for your feedback+! I updated the patch based on your suggestions.

Asking for review from Shawn now.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 479513 [details] [diff] [review]
updated patch

r=sdwilsh assuming you'll update the test to the new-world awesomeness

Comment 5

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. Thanks Shawn for the review+!

Asking for approval2.0+: this patch fixes an error in the code which prevents it from properly determining the timestamp of the last message, for the purpose of appending a group when needed.

Comment 6

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 491802 [details] [diff] [review]
rebased patch

a=beltzner

Comment 8

[Mihai Sucan [:msucan]]

Thanks Mike for the a+!

Patch checked in:
http://hg.mozilla.org/mozilla-central/rev/ad189229497f