console.trace() group traces even if part of trace is different

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Keul, Assigned: msucan)

Tracking

({testcase})

unspecified
Firefox 29
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8333844 [details]
test.html

when console.trace() is called multiple times, it is printed only one time in the console even if the path of the trace is different. (it frequently happens with ajax calls)

In the attached file test.html, there's two console.trace() in the code that produces only a maximum of 2 traces in the console, even if they are called trough 3 different functions : test1(), test2() and test3()
(Reporter)

Updated

4 years ago
Keywords: testcase
(Assignee)

Comment 1

4 years ago
Good find! Thanks for the bug report and for the test case.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Priority: -- → P3
Hardware: x86 → All
(Assignee)

Updated

3 years ago
Summary: console.trace() group traces even if part of trace is diffrent → console.trace() group traces even if part of trace is different
(Assignee)

Comment 2

3 years ago
Created attachment 8363903 [details] [diff] [review]
bug939783-1.diff

This patch fixes the bug here, and it changes the console.trace() output to be prettier, fixing bug 790309 as well.

All webconsole tests pass.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=cb7c5c5931d1

While attaching this patch I remembered I still need to add a test for this bug, so we make sure that we wont coalesce repeats with different frames again. Will add it tomorrow.

Thanks!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #8363903 - Flags: review?(rcampbell)
(Assignee)

Updated

3 years ago
Blocks: 790309
Comment on attachment 8363903 [details] [diff] [review]
bug939783-1.diff

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

looks good!

::: browser/devtools/webconsole/console-output.js
@@ +858,5 @@
>      }
>  
>      let repeatNode = this.document.createElementNS(XHTML_NS, "span");
>      repeatNode.setAttribute("value", "1");
> +    repeatNode.className = "message-repeats";

eh.

@@ +1099,5 @@
> + */
> +Messages.ConsoleTrace = function(packet)
> +{
> +  let options = {
> +    className: "consoleTrace cm-s-mozilla",

that's a funny lookin' class name.

@@ +1553,5 @@
> +      span.className = "cm-variable";
> +      span.textContent = frame.functionName;
> +      fn.appendChild(span);
> +      fn.appendChild(this.document.createTextNode("()"));
> +    } else {

should we try to use displayName here?

::: browser/devtools/webconsole/test/browser_console_addonsdk_loader_exception.js
@@ +69,5 @@
>    function onMessageFound(results)
>    {
>      let msg = [...results[0].matched][0];
>      ok(msg, "message element found");
> +    let locationNode = msg.querySelector(".message-location");

.message everywhere!

::: browser/devtools/webconsole/test/browser_webconsole_scratchpad_panel_link.js
@@ +59,5 @@
>        aToolbox.off("scratchpad-selected", selected);
>  
>        is(aToolbox.getCurrentPanel(), scratchpadPanel,
>          "Clicking link switches to Scratchpad panel");
>        

some whitespace here... oh well.
Attachment #8363903 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8364636 [details] [diff] [review]
bug939783-3.diff

Added a test that checks we dont mark console.trace() calls as duplicates if they have different frames. Also fixed a scratchpad test that failed in yesterday's try push.

Today I had a green try push: https://tbpl.mozilla.org/?tree=Try&rev=b9f166a80a11
.... but the infrastructure failed to run the win7 builds.

Now that Benvie landed bug 943681 I did a minor rebase. New try push: https://tbpl.mozilla.org/?tree=Try&rev=cb60b7e4de25

Will land this patch tomorrow if the try results look green enough.

Thank you Rob for the quick review!
Attachment #8363903 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Landed: https://hg.mozilla.org/integration/fx-team/rev/2552d554372d
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 6

3 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Comment on attachment 8363903 [details] [diff] [review]
> bug939783-1.diff
> 
> Review of attachment 8363903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good!
> 
> ::: browser/devtools/webconsole/console-output.js
> @@ +858,5 @@
> >      }
> >  
> >      let repeatNode = this.document.createElementNS(XHTML_NS, "span");
> >      repeatNode.setAttribute("value", "1");
> > +    repeatNode.className = "message-repeats";
> 
> eh.
> 
> @@ +1099,5 @@
> > + */
> > +Messages.ConsoleTrace = function(packet)
> > +{
> > +  let options = {
> > +    className: "consoleTrace cm-s-mozilla",
> 
> that's a funny lookin' class name.
> 
> @@ +1553,5 @@
> > +      span.className = "cm-variable";
> > +      span.textContent = frame.functionName;
> > +      fn.appendChild(span);
> > +      fn.appendChild(this.document.createTextNode("()"));
> > +    } else {
> 
> should we try to use displayName here?

You mean "displayName" as a class name or as a property of function objects? If the latter, that is not available. This is a frame object (very) similar to what we get from Components.stack -- IIANM the function name is determined by spidermonkey.
https://hg.mozilla.org/mozilla-central/rev/2552d554372d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.