Closed Bug 939783 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: keul, Assigned: msucan)

References

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

Attached file 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()
Keywords: testcase
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
Summary: console.trace() group traces even if part of trace is diffrent → console.trace() group traces even if part of trace is different
Attached patch bug939783-1.diff (obsolete) — Splinter Review
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)
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+
Attached patch bug939783-3.diffSplinter Review
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
Landed: https://hg.mozilla.org/integration/fx-team/rev/2552d554372d
Whiteboard: [fixed-in-fx-team]
(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
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.