Closed
Bug 922212
Opened 11 years ago
Closed 9 years ago
Add console.dirxml
Categories
(DevTools :: Console, defect, P3)
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)
13.32 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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);
Reporter | ||
Updated•11 years ago
|
Priority: -- → P3
Comment 1•10 years ago
|
||
An initial stopgap implementation here could make dirxml() an alias of dir(). For DOM nodes we can use the default console.log(node) representation.
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
> TODO: tests.
And I also need to support WebWorkers.
Florent
Reporter | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
Thanks Nick! I'll work on the tests this evening or this week-end. Florent
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → fayolle-florent
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
> 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)
Assignee | ||
Comment 11•9 years ago
|
||
> What is your feeling Bryan?
Oops, sorry for having misspelled your name, Brian.
Florent
Reporter | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
(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 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
ni? for comment 15
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(fayolle-florent)
Reporter | ||
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
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)
Assignee | ||
Comment 19•9 years ago
|
||
Brian, if I should remove the |console.dir()| view for non-(HTML|XML) elements, I will do. Just tell me. Florent
Flags: needinfo?(bgrinstead)
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
Patch updated. Florent
Attachment #8594939 -
Attachment is obsolete: true
Attachment #8597399 -
Flags: review?(bgrinstead)
Comment 22•9 years ago
|
||
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+
Comment 23•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8599432 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•9 years ago
|
Attachment #8597399 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8599432 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7ea0e4207486
Keywords: checkin-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ea0e4207486
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•