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

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Keul]

Attachment: 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()

Comment 1

[Mihai Sucan [:msucan]]

Good find! Thanks for the bug report and for the test case.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: 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!

Comment 3

[Rob Campbell [:rc] (:robcee)]

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.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: 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!

Comment 5

[Mihai Sucan [:msucan]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/2552d554372d

Comment 6

[Mihai Sucan [:msucan]]

(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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2552d554372d