console.trace should accept arguments

ASSIGNED
Assigned to

Status

P3
normal
ASSIGNED
3 years ago
2 months ago

People

(Reporter: fayolle-florent, Assigned: sy.fen0)

Tracking

41 Branch

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [btpp-backlog], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Component: Untriaged → Developer Tools: Console
(Reporter)

Comment 1

3 years ago
Created attachment 8616445 [details]
console-trace-current-display.png

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

Florent

Comment 2

3 years ago
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.
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.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Assignee: nobody → sy.fen0

Comment 5

8 months ago
mozreview-review
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 ?
Attachment #8935330 - Flags: review-
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

8 months ago
mozreview-review-reply
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

8 months ago
mozreview-review-reply
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 ?
(Assignee)

Comment 8

8 months ago
mozreview-review-reply
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

8 months ago
mozreview-review-reply
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

8 months ago
mozreview-review
Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

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

Deferring to Nicolas' review
Attachment #8935330 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

8 months ago
mozreview-review
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
Attachment #8935330 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

8 months ago
mozreview-review-reply
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

8 months ago
mozreview-review
Comment on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

https://reviewboard.mozilla.org/r/206230/#review215566
Attachment #8935330 - Flags: review?(bgrinstead) → review+
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.
Flags: needinfo?(fayolle-florent)
See comment 18
Flags: needinfo?(fayolle-florent) → needinfo?(sy.fen0)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

7 months ago
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
Flags: needinfo?(sy.fen0)
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.
(Assignee)

Comment 23

7 months ago
Got it. Everything looks good there. Thank you.

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.