Closed Bug 587615 Opened 10 years ago Closed 10 years ago

lastTimestamp reset problems in WebConsole


(DevTools :: General, defect, P2)



(Not tracked)



(Reporter: msucan, Assigned: msucan)



(Whiteboard: [patchclean:1119])


(1 file, 2 obsolete files)

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.
Attached patch proposed fix and test code (obsolete) — Splinter Review
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!
Assignee: nobody → mihai.sucan
Attachment #476648 - Flags: feedback?(pwalton)
Whiteboard: [patchclean:0919]
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!
Attachment #476648 - Flags: feedback?(pwalton) → feedback+
Attached patch updated patch (obsolete) — Splinter Review
Thanks Patrick for your feedback+! I updated the patch based on your suggestions.

Asking for review from Shawn now.
Attachment #476648 - Attachment is obsolete: true
Attachment #479513 - Flags: review?(sdwilsh)
Whiteboard: [patchclean:0919] → [patchclean:0929]
Blocks: devtools4b8
No longer blocks: devtools4b8
Comment on attachment 479513 [details] [diff] [review]
updated patch

r=sdwilsh assuming you'll update the test to the new-world awesomeness
Attachment #479513 - Flags: review?(sdwilsh) → review+
Attached patch rebased patchSplinter Review
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.
Attachment #479513 - Attachment is obsolete: true
Attachment #491802 - Flags: approval2.0?
Whiteboard: [patchclean:0929] → [patchclean:1119]
Blocks: devtools4
mass change: filter on PRIORITYSETTING
Priority: -- → P2
Comment on attachment 491802 [details] [diff] [review]
rebased patch

Attachment #491802 - Flags: approval2.0? → approval2.0+
Whiteboard: [patchclean:1119] → [patchclean:1119][checkin]
Thanks Mike for the a+!

Patch checked in:
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1119][checkin] → [patchclean:1119]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.