Closed Bug 1172314 Opened 9 years ago Closed 6 years ago

console.trace should accept arguments

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Version:
41 Branch
Type:
defect

Tracking

(firefox64 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

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

References

(
URL
)

Details

(Whiteboard: [btpp-backlog])

Attachments

(3 files)

console-trace-current-display.png
7.03 KB, image/png
Details
Bug 1172314 - Print arguments passed into console.trace.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1172314 - Print arguments passed into console.trace. r=bgrins.
46 bytes, text/x-phabricator-request
bgrins
: review+
Details | Review 
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
Component: Untriaged → Developer Tools: Console
Also maybe we should get rid of the print of "console.trace():" in the console output.

Florent
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]
Assignee: nobody → sy.fen0
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
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 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 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 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 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 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 on attachment 8935330 [details]
Bug 1172314 - Print arguments passed into console.trace.

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

Updated and rebased.
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)
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.
Got it. Everything looks good there. Thank you.
Product: Firefox → DevTools
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.
Added trace extra parameters to be printed alongside console.trace() output.
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 on attachment 9012514 [details]
Bug 1172314 - Print arguments passed into console.trace. r=bgrins.

Brian Grinstead [:bgrins] has approved the revision.
Attachment #9012514 - Flags: review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc7e23a0dfa
Print arguments passed into console.trace. r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/dfc7e23a0dfa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: