Closed Bug 922212 Opened 6 years ago Closed 5 years ago

Add console.dirxml

Categories

(DevTools :: Console, defect, P3)

x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: fayolle-florent)

References

Details

Attachments

(1 file, 4 obsolete files)

https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consoledirxmlobject

> Prints an XML/HTML Element representation of the specified object if possible
> or the JavaScript Object view if it is not.
>
>     var list = document.querySelector("#myList");
>     console.dirxml(list);
>
>     // The above is equivalent to ...
>     // console.log("%o", list);
Priority: -- → P3
An initial stopgap implementation here could make dirxml() an alias of dir(). For DOM nodes we can use the default console.log(node) representation.
Depends on: 1139794
I would do like the Chrome DevTools do: print only the element itself, and offer an "expand" icon to display the child nodes. That can be the "target".

As a first step, as suggested Mihai, we can reuse the representation of console.log(node) for XML/HTML nodes, and console.dir() for the other objects.

I can take it. 
Also can you mark it as a Firebug gap please?

Florent
Attached patch 922212.patch (obsolete) — Splinter Review
I just transform the RDP message to fake either a console.log or a console.dir call. Is that fine for you?

TODO: tests.

Florent
Attachment #8593572 - Flags: feedback?(nfitzgerald)
> TODO: tests.

And I also need to support WebWorkers.

Florent
Comment on attachment 8593572 [details] [diff] [review]
922212.patch

Review of attachment 8593572 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

FWIW, I wouldn't mind pushing worker support to a follow up if it makes it easier to land this initial patch.
Attachment #8593572 - Flags: feedback?(nfitzgerald) → feedback+
Thanks Nick!

I'll work on the tests this evening or this week-end.

Florent
Attached patch 922212.patch (obsolete) — Splinter Review
Tests done. Could you please review?

About the web workers, this example works: |new Worker("data:text/javascript, console.dirxml({a: 'b'})");|.

I wonder if there is any way to test the rendering for a DOM Element through workers (we can't access to any DOM APIs through workers, right?).

Florent
Attachment #8593572 - Attachment is obsolete: true
Attachment #8594450 - Flags: review?(nfitzgerald)
Attachment #8594450 - Flags: review?(bgrinstead)
Comment on attachment 8594450 [details] [diff] [review]
922212.patch

Review of attachment 8594450 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see if we could do a more direct fallback to console.log("%o") (see the comment inline).  Also, I don't think Nick needs to review this patch, unless if he feels like taking a look.

::: browser/devtools/webconsole/test/browser.ini
@@ +381,5 @@
>  [browser_webconsole_console_custom_styles.js]
>  [browser_webconsole_console_api_stackframe.js]
>  [browser_webconsole_column_numbers.js]
>  [browser_console_open_or_focus.js]
> +[browser_webconsole_bug_922212_console_dirxml.js]

This file doesn't seem to be added to the patch

::: browser/devtools/webconsole/test/head.js
@@ +1623,5 @@
>  
>    return deferred.promise;
>  }
>  
> +/**

Is there a reason this function moved location in the file?  It makes it hard to see what changed.

::: browser/devtools/webconsole/webconsole.js
@@ +1274,5 @@
>        }
> +      case "dirxml": {
> +        let firstArg = args[0];
> +        // Force console.dirxml() to only take one argument.
> +        args.splice(1);

So according to the spec, dirxml is equivalent to

console.dirxml(list) is equivalent to console.log("%o", list).  I wonder if we could take advantage of that in the implementation, and just rewrite the args / level to match so we don't need to handle the special cases below.

@@ +1275,5 @@
> +      case "dirxml": {
> +        let firstArg = args[0];
> +        // Force console.dirxml() to only take one argument.
> +        args.splice(1);
> +        if (firstArg && (firstArg.preview &&

Maybe you want null / undefined to be treated as a log here (which I would think would be captured by VariablesView.isPrimitive).  If so, don't want to guard with `firstArg &&`
Attachment #8594450 - Flags: review?(bgrinstead)
Assignee: nobody → fayolle-florent
Status: NEW → ASSIGNED
Comment on attachment 8594450 [details] [diff] [review]
922212.patch

Review of attachment 8594450 [details] [diff] [review]:
-----------------------------------------------------------------

Trust bgrins on the devtools review changes.

Passing console API additions at the platform level to :mrbkap.
Attachment #8594450 - Flags: review?(nfitzgerald) → review?(mrbkap)
Attached patch 922212.patch (obsolete) — Splinter Review
> Trust bgrins on the devtools review changes.

Sure I trust him :). I just saw you were interested in that feature, so I thought you could have wanted to review it as well.

Or is there an etiquette to ask for a review explained somewhere?

> Maybe you want null / undefined to be treated as a log here (which I would think would be captured by VariablesView.isPrimitive).  If so, don't want to guard with `firstArg &&`

I am not entirely sure of what you mean here. If we pass null or undefined, firstArg would equal to |{type: "null"}| or |{type: "undefined"}|.
But maybe you point that if we pass no argument, that should be treated by |console.log| instead of |console.dir|, and therefore we can first test with |VariablesView.isPrimitive| so we can remove the |firstArg &&| guard. If so, that's a good point, and I updated the patch accordingly.

> Is there a reason this function moved location in the file?  It makes it hard to see what changed.

Yes, I expose it to the tests. Before it was private to |checkOutputForInputs|. And I need it to use it to make sure of the existence of the inspector for console.dirxml. Sorry :/.

> console.dirxml(list) is equivalent to console.log("%o", list).  I wonder if we could take advantage of that in the implementation, and just rewrite the args / level to match so we don't need to handle the special cases below.

I asked myself this question. But the output is not the same between for example:
|console.log("%o", window);|
and:
|console.dir(window);|

And I tend to think that it should fallback to the rendering of |console.dir()|. I can't find arguments for this, that's just my feeling. Other points of view are welcome here. What is your feeling Bryan?

Florent
Attachment #8594450 - Attachment is obsolete: true
Attachment #8594450 - Flags: review?(mrbkap)
Attachment #8594939 - Flags: review?(bgrinstead)
> What is your feeling Bryan?

Oops, sorry for having misspelled your name, Brian.

Florent
I just meant that I'm fine with bgrins reviewing the devtools parts, and I don't need to look it over again. Generally you just need one person to review from each module you touch. Here the patch is touching devtools code and DOM code (the cpp/h/webidl changes). So bgrins is a great reviewer for the devtools code, and mrbkap would be a great reviewer for the DOM code.
Comment on attachment 8594939 [details] [diff] [review]
922212.patch

Review of attachment 8594939 [details] [diff] [review]:
-----------------------------------------------------------------

The webidl changes look good to me.
Attachment #8594939 - Flags: review+
(In reply to fayolle-florent from comment #10)
> I asked myself this question. But the output is not the same between for
> example:
> |console.log("%o", window);|
> and:
> |console.dir(window);|
> 
> And I tend to think that it should fallback to the rendering of
> |console.dir()|. I can't find arguments for this, that's just my feeling.
> Other points of view are welcome here. What is your feeling Bryan?

The expected output here is poorly specified and so the implementation is wildly different among tools:

Chrome seems to almost alias this function as `log` with the exception of using a slightly different array previewer.  It also includes all the parameters passed in, so `console.dirxml(window, "foo", [{a:1},2,3], document.body)` prints out all of the results.  Also, `console.dirxml(window)` prints the same things as `console.log(window)` - which is different from `console.dir(window)`.

Firebug 2 seems to only care about the first parameter, and does some weird stuff with strings and numbers - `console.dirxml("foo")` prints "<></>".  With `window` or `document` it prints out the <html> element as XML.  For nodes it prints out the corresponding node as XML.

I like that Chrome preserves all of the input (it seems weird to ignore subsequent params).  In fact, I believe we could just alias `dirxml` as `log` (group it into the case statement) to make the implementation dead simple.  Of course that's up for discussion, but I don't see a strong reason to do something different here.
Comment on attachment 8594939 [details] [diff] [review]
922212.patch

Review of attachment 8594939 [details] [diff] [review]:
-----------------------------------------------------------------

The changes all look straightforward.  I'd like to discuss about Comment 12 before proceeding with this implementation, though.  IMO there isn't a reason to worry about how this outputs on non-element objects since it's not specified and (I imagine) rarely used for those objects.  Nick, Florent, thoughts?
Attachment #8594939 - Flags: review?(bgrinstead)
ni? for comment 15
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(fayolle-florent)
Not worth holding up the change to debate about, IMO. Ship it!

Then, we can at least hear back from Real Users and if they file bugs or complain about that case, then we can attack it with higher priority.
Flags: needinfo?(nfitzgerald)
I still slightly prefer the way implemented in my patch. Also you made a good point, saying that's not so important.

Florent
Flags: needinfo?(fayolle-florent)
Brian, if I should remove the |console.dir()| view for non-(HTML|XML) elements, I will do. Just tell me.

Florent
Flags: needinfo?(bgrinstead)
As discussed on IRC, let's just use `log` output for all arguments to simplify the code added here, since  it's poorly specced and we may want to adjust output the future.
Flags: needinfo?(bgrinstead)
Attached patch 922212.patch (obsolete) — Splinter Review
Patch updated.

Florent
Attachment #8594939 - Attachment is obsolete: true
Attachment #8597399 - Flags: review?(bgrinstead)
Comment on attachment 8597399 [details] [diff] [review]
922212.patch

Review of attachment 8597399 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Pushed to try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b96b64d618
Attachment #8597399 - Flags: review?(bgrinstead) → review+
Try push for the updated patch with a test fix (that was accidentally attached to Bug 1159009): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e48f342cd5f.  Florent, can you attach that patch to this bug?
Flags: needinfo?(fayolle-florent)
Attached patch 922212.patchSplinter Review
Done
Flags: needinfo?(fayolle-florent)
Attachment #8599432 - Flags: review?(bgrinstead)
Attachment #8597399 - Attachment is obsolete: true
Attachment #8599432 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ea0e4207486
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.