1172314 - console.trace should accept arguments

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150604162752

Steps to reproduce:

When taking a look at the Chrome DevTools implementation of console.trace, I saw that they accept arguments:
https://developer.chrome.com/devtools/docs/console-api#consoletraceobject

That's also described in this spec: https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consoletraceobject--object-

I think that's a nice addition and the Firefox DevTools probably should do the same.

Florent

Comment 1

[Florent Fayolle]

Attachment: console-trace-current-display.png

Also maybe we should get rid of the print of "console.trace():" in the console output.

Florent

Comment 2

[Bob]

Similar behaviour can currently be emulated with `console.assert`, e.g.:

> console.assert(false, message);

It would be nice if `console.trace` could accept arguments in a similar manner, and maybe display the trace collapsed like `console.assert` (and Chrome's `console.trace`) currently does - there currently does not appear to be any way to collapse the trace in Firefox.

Comment 3

[Lin Clark [:linclark]]

Thanks for the suggestion. I've added this to the backlog. We'll be rewriting the console.trace code soon, so this might happen as part of that.

Comment 4

[Stefan Yohansson]

Attachment: Bug 1172314 - Print arguments passed into console.trace.

Added trace extra parameters to be printed alongside console.trace() output.

Review commit: https://reviewboard.mozilla.org/r/206230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206230/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review211834

(doing a drive-by review since bgrins is busy those days)

Thanks for your patch, this looks almost right. Now we need to have some test to make sure we do thngs as expected.

::: devtools/client/webconsole/new-console-output/components/message-types/ConsoleApiCall.js:73
(Diff revision 1)
> +    messageBody = [
> +      dom.div({className: "cm-variable"}, parametersBody),
> +      dom.span({className: "cm-variable"}, "console.trace()")

it feels wierd to display the parameters before the "console.trace" label.
Maybe we could only return something like : 

messageBody = [
  dom.span({className: "cm-variable"}, "console.trace()"),
  ...parametersBody
];

This means we sould change how we initialize parametersBody : 

let parametersBody = Array.isArray(parameters) && parameters.length > 0
  ? formatReps(messageBodyConfig)
  : [];

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js:61
(Diff revision 1)
> +consoleApi.set("console.trace('bar')", {
> +  keys: ["console.trace('bar')"],
> +  code: `
> +function testStacktraceWithLog() {
> +  console.trace('bar')
> +}
> +function foo() {
> +  testStacktraceWithLog()
> +}
> +
> +foo()
> +`});

this is great to add a stub, now we should use it in a test :)

In the console, you can write mocha tests, which can be run with `cd devtools/client/webconsole && npm install && npm test`

The tests are located here : https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js#177-209

Here we could have a `it("render with arguments")` in which you'd use the stub you created.

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js:65
(Diff revision 1)
>  
> +consoleApi.set("console.trace('bar')", {
> +  keys: ["console.trace('bar')"],
> +  code: `
> +function testStacktraceWithLog() {
> +  console.trace('bar')

could you put a more complex object, like console.trace('foo', {bar:'baz'}, [1,2,3])

to see if it behaves like it should ?

Comment 6

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review211834

> it feels wierd to display the parameters before the "console.trace" label.
> Maybe we could only return something like : 
> 
> messageBody = [
>   dom.span({className: "cm-variable"}, "console.trace()"),
>   ...parametersBody
> ];
> 
> This means we sould change how we initialize parametersBody : 
> 
> let parametersBody = Array.isArray(parameters) && parameters.length > 0
>   ? formatReps(messageBodyConfig)
>   : [];

I am changing now, but I think we need to do it this way:


```
messageBody = [
  dom.span({className: "cm-variable"}, "console.trace()"),
  dom.div({className: "cm-variable"}, parametersBody),
];
```

otherwise, parametersBody will be printed alongside console.trace() without space, is it what you thinking?

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review211834

> I am changing now, but I think we need to do it this way:
> 
> 
> ```
> messageBody = [
>   dom.span({className: "cm-variable"}, "console.trace()"),
>   dom.div({className: "cm-variable"}, parametersBody),
> ];
> ```
> 
> otherwise, parametersBody will be printed alongside console.trace() without space, is it what you thinking?

i'd still not wrap the parametersBody in a div because of the way we handle layout for multiple successive parameters.
If we need some space after the console.trace label, we can add a padding-inline-end to the css . To do that, we need to specify a class for the span, in addition to cm-variable. Does that seems right for you ?

Comment 8

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review211834

> i'd still not wrap the parametersBody in a div because of the way we handle layout for multiple successive parameters.
> If we need some space after the console.trace label, we can add a padding-inline-end to the css . To do that, we need to specify a class for the span, in addition to cm-variable. Does that seems right for you ?

Yes indeed. Thanks for the clarification. We already have a css class specifically for that purpose or should I create one?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review211834

> Yes indeed. Thanks for the clarification. We already have a css class specifically for that purpose or should I create one?

i don't think we have one, so let's create it

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review212586

Deferring to Nicolas' review

Comment 11

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206230/diff/1-2/

Comment 12

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206230/diff/2-3/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review214552

Sorry for the delay - this looks good.  Two things to update before we can land it:

1) Can you update the commit message to be something like "Print arguments passed into console.trace"
2) webconsole.css needs a rebase

Comment 14

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206230/diff/3-4/

Comment 15

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review214552

Updated and rebased.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review215566

Comment 17

[Brian Grinstead [:bgrins]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cc2a1170e75a33359bc5aae3a183962a273e5ff

Comment 18

[Brian Grinstead [:bgrins]]

Sorry, looks like we need one more update before we can land it - there's an eslint failure due to a line being over 90 characters: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cc2a1170e75a33359bc5aae3a183962a273e5ff&selectedJob=153939298.

Comment 19

[Brian Grinstead [:bgrins]]

See comment 18

Comment 20

[Stefan Yohansson]

Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206230/diff/4-5/

Comment 21

[Stefan Yohansson]

Updated. I have a question: on reviewboard the 4-5 changeset is displaying changes in "devtools/client/themes/webconsole.css" file but I didn't update that file from update 4 to 5, should I rebase or is it normal?

https://reviewboard.mozilla.org/r/206230/diff/4-5

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

This is normal, you must have rebased your patch between #4 and #5, so the difference between those two versions include changes from other commits. You can simply look directly at the #5 (https://reviewboard.mozilla.org/r/206230/diff/5/) to make sure you don't have unwanted changes.

Comment 23

[Stefan Yohansson]

Got it. Everything looks good there. Thank you.

Comment 24

[Stefan Yohansson]

I think something is wrong with this issue.

I can see the changeset here: https://hg.mozilla.org/mozreview/gecko/rev/14711c4cc87a92daa9234ce88813c4b0d91874b8

but I can't find the commit when I clone the repository yesterday. (default)

trying to use the feature on Firefox 62.0 (Ubuntu Linux) and nothing changed trace can't receive parameters.

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1172314 - Print arguments passed into console.trace. r=bgrins.

Added trace extra parameters to be printed alongside console.trace() output.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Hello Stefan, looks like this patch slipped out of our radar and wasn't merged :( .
I genuinely apologize for that.

The link you point to is the patch that was pushed to mozreview, and not the repo used to build firefox (mozilla-central), which is why you can't find it in your local repo.

I took your patch and applied it on the latest mozilla-central, with some changes since the codebase changed a bit: https://phabricator.services.mozilla.com/D7051

It looks good locally, and I pushed to TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=80b82c31ef55881a0989dd4420333ea64d256076 to make sure we don't break any test.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 9012514 [details]
Bug 1172314 - Print arguments passed into console.trace. r=bgrins.

Brian Grinstead [:bgrins] has approved the revision.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfc7e23a0dfa
Print arguments passed into console.trace. r=bgrins.

Comment 29

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/dfc7e23a0dfa