899753 - Add console.table support

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[Brian Grinstead [:bgrins]]

console.table is a nice way to display tabular data in the console.  Would it be possible to add this inside of the console?  Some links with more information about the feature:

* Console API: http://getfirebug.com/wiki/index.php/Console_API#console.table.28data.5B.2C_columns.5D.29
* More details about usage in Chrome: https://plus.google.com/+AddyOsmani/posts/PmTC5wwJVEc
* More details about usage in Firebug: http://www.softwareishard.com/blog/firebug/tabular-logs-in-firebug/

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 2

[Christian Heilmann]

Praising post about this getting lots of love from the Chrome team right now http://www.mariusschulz.com/2013/11/13/advanced-javascript-debugging-with-consoletable

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Just got an impassioned plea for this from a developer I met at FOSDEM who works at Wikimedia, they have some great debug functions that use console.table for easy app state reference.

Comment 4

[Rob Campbell [:rc] (:robcee)]

We keep seeing this brought up. Recently been discussed in the context of using it for reporting from our own internal tools. This needs to happen.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> We keep seeing this brought up. Recently been discussed in the context of
> using it for reporting from our own internal tools. This needs to happen.

Maybe it should be re-prioritized then. P3 doesn't seem to reflect how we feel about it. cc'ing Joe for his input.

Comment 6

[Rob Campbell [:rc] (:robcee)]

It's important, but unless we have someone to work on this, no point in upping the priority.

Comment 7

[Joe Walker [:jwalker]]

I'd love to see this done we 'just' need to find someone to do it...

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

My understanding of what adding console.table support entails:

1) Modify the console's webidl to have a console.table method.

2) In the platform, generate a new kind of console message for console.table that holds a strong reference to the tabular data. Not sure if this is necessary or just falls out of step (1).

3) Create a new console actor for table messages. Again, not sure if this is really needed or we can piggy back on existing actors. Looks like perhaps http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webconsole.js#1312 handles this for us.

4) Modify the console frontend to render the table message's data as a table. Sorting by clicking on table headers and all that can and should be follow up bugs.

------------------------------------------

bz: Can you expand more on (1) and (2)?

past: Can you expand on (3) and (4)?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That is the most useless spec-by-example specimens I have ever seen...

Can someone please describe the actual processing we want for the arguments to this method (and then ideally get it into the spec)? It's pretty hard to say anything sane about (1) and (2) (or implement the functionality) otherwise.

In any case, if I understand the examples correctly both arguments to this are some sort of bizarre "try to guess what the author meant" things.  Which means we'd probably just need label them "any" in the IDL and pass them directly though.  That's what a bunch of console methods do already...  Then the JS implementation will have to be very very careful working with the results.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> That is the most useless spec-by-example specimens I have ever seen...
> 
> Can someone please describe the actual processing we want for the arguments
> to this method (and then ideally get it into the spec)? It's pretty hard to
> say anything sane about (1) and (2) (or implement the functionality)
> otherwise.

I think we want something like this:

  console.table(data, columns=undefined):
    if columns is undefined:
      columns = union(Object.keys(o) for o of data);

    rows = map(o => [o[c] for c of columns],
               data);

    render `rows` as a table in the console with `columns` as the table headers

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, you basically need to use "any" here.  :(

Comment 12

[Bobby Holley (:bholley)]

Can we run the structured clone algorithm on the data passed by content? Bz, maybe we should add some kind of extended attribute to WebIDL indicating that a given |any| or |object| should be cloned?

Comment 13

[Boris Zbarsky [:bzbarsky]]

We can structured clone, and I did consider that, but if any of the things in that table aren't basic objects for some reason (e.g. are nodes), that will lose, no?  How do people use this API in practice?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> We can structured clone, and I did consider that, but if any of the things
> in that table aren't basic objects for some reason (e.g. are nodes), that
> will lose, no?

It depends on the structured clone callbacks that are used. We have callbacks (CloneNonReflectors) that just pass through any WNs and WebIDL objects as XrayWrappers. So long as this API never passes stringified data back to content (which it shouldn't, I'd hope), this should be fine.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, nice.  Arguably, we should do that for everything in the console API, if it doesn't hurt performance too much...

Comment 16

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Ah, nice.  Arguably, we should do that for everything in the console API, if
> it doesn't hurt performance too much...

Do you have the cycles to add such an extended attribute to Codegen?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I don't see an obvious need to do it in codegen so far.  We can do it in Console.cpp (which has to mess around with structured cloning anyway, for workers) and then once we figure out how it should look we can think about moving it to codegen once we have another consumer.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> I don't see an obvious need to do it in codegen so far.  We can do it in
> Console.cpp (which has to mess around with structured cloning anyway, for
> workers) and then once we figure out how it should look we can think about
> moving it to codegen once we have another consumer.

Oh, I'd forgotten that the console API was in C++.

FWIW, I think this would be a generally useful Codegen feature, because we could then use it for JS-implemented WebIDL. So if somebody is going to do the work for this, we might as well make it reusable. But if it's a lot more work to do in Codegen (or requires *ahem* a certain busy person to do it) I can understand just shimming it into Console.cpp.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Basically, once someone gets it working in Console.cpp we can cut/paste that into codegen.  ;)

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

No need for a new actor, as you mentioned the WebConsoleActor will receive the notification from the console API and send a consoleAPICall packet to the client. The frontend will need to handle the new packet.message.level in WCF_logConsoleAPIMessage, probably cribbing from console.dir and console.trace. A new message class will likely be necessary (see https://developer.mozilla.org/docs/Tools/Web_Console/Custom_output).

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add the table method to Console.{h,cpp,webidl}

It seems like this is all that's needed from the platform unless I'm missing something.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459371 [details] [diff] [review]
Part 0: Add the table method to Console.{h,cpp,webidl}

I wonder how you managed to get the ANSI color codes in there...  I thought tools usually turned those off when not writing to a TTY?  They sure do make the patch hard to read in a web browser; just try loading https://bug899753.bugzilla.mozilla.org/attachment.cgi?id=8459371 and see all the noise.  :(

In any case this will, I believe, just pass through the values as-is to the chrome code.  I wonder why we didn't do that with format strings instead of writing that gunk in C++...

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add the table method to Console.{h,cpp,webidl}

Argh, forgot to --no-color. Sorry!

Here's the patch without the color escape codes.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP]

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP2]

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP3]

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP4]

Should be demo-able for Nick's talk now

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP5]

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP6]

Comment 30

[Rob Campbell [:rc] (:robcee)]

tweaking priorities based on reality. Continue!

Comment 31

[Andrea Marchesini [:baku]]

> In any case this will, I believe, just pass through the values as-is to the
> chrome code.  I wonder why we didn't do that with format strings instead of
> writing that gunk in C++...

We did the refactoring in C++ because we want to have 1 single implementation of the console in workers and on main-thread. In theory would be nice to unify the JSM implementation as well.

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP7]

Unit tests will come in part 2

Comment 33

[Mihai Sucan [:msucan]]

Comment on attachment 8467623 [details] [diff] [review]
Part 1: Add console.table front end [WIP7]

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

This is looking very good. Thanks for your work. General comments:

- The following calls do not show the correct/expected output - please compare with chrome/firebug:

console.table([[1, 2], [3, 4]])
console.table({a: [1, 2], b: [3, 4]})

- take the family example from https://getfirebug.com/wiki/index.php/Console.table

Try console.table(family, 'firstName'). No '(index)' column is shown. Why? Chrome shows it and it makes sense to me.

- continuing with the family example, try:

  family.foo = new Person(document, window, document.body);
  console.table(family);

I see [object Object]. You should be able to render the objects using:

  domElem = this.message._renderValueGrip(objectActorGrip, { concise: true });

- maybe the row alternate color should be darker, to have a bit stronger contrast.

- there's no indicator for sort direction on the column header.

- table font size seems slightly bigger than the console output font.

- table font is not monospace. Is this desired?

- what happens with 'pathological' cases? Like tens of thousands of rows/columns. Are we limiting output somewhere? If not, we should do so to avoid breakage.

- did you check that the object actors for all the objects received are correctly released when console.table messages are removed?

More comments below.

::: browser/devtools/webconsole/console-output.js
@@ +5,5 @@
>  
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

Is this needed?

@@ +6,5 @@
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

You can replace this with:

loader.lazyImporter(this, "promise", "resource://gre/modules/Promise.jsm", "Promise");

@@ +15,5 @@
>  loader.lazyImporter(this, "Task","resource://gre/modules/Task.jsm");
>  loader.lazyImporter(this, "PluralForm", "resource://gre/modules/PluralForm.jsm");
>  
> +XPCOMUtils.defineLazyGetter(this, "TableWidget",
> +    () => devtools.require("devtools/shared/widgets/TableWidget").TableWidget);

You can replace the use of XPCOMUtils with:

loader.lazyGetter(this, "TableWidget",
                  () => require("devtools/shared/widgets/TableWidget").TableWidget);

Also devtools.require() is not needed. You need that only inside a jsm.

@@ +1642,5 @@
> +  this._populateTableData = this._populateTableData.bind(this);
> +  this._renderTable = this._renderTable.bind(this);
> +  Messages.Simple.call(this, this._renderTable, options);
> +
> +  this._repeatID.consoleApiLevel = packet.level;

How does this work? Do you take the console data and columns into consideration when the hash/id of the message is calculated? Maybe we should set filterDuplicates to false.

@@ +1666,5 @@
> +   */
> +  _data: null,
> +
> +  /**
> +   * Key value pair of the id and display name for the columns in the table.

This seems to suggest that I can use the second argument of console.table() to give a a different name for the columns I want displayed. Eg.

console.table(family, {firstName: 'first name'})

But now I see this is needed for TableWidget. Slightly confusing to see _columns as an object, instead of an array.

@@ +1705,5 @@
> +   */
> +  _setColumns: function(columns)
> +  {
> +    if (columns.class === "Array") {
> +      let items = columns.preview.items;

What happens when columns.preview.length > items.length? When the preview doesnt include all of the array items.

@@ +1753,5 @@
> +    this.client.getPrototypeAndProperties(aResponse => {
> +      let {ownProperties} = aResponse;
> +
> +      if (!hasColumnsArg) {
> +        this._columns["(index)"] = "(index)";

Please make sure this string is localizable.

@@ +1759,5 @@
> +
> +      for (let key of Object.keys(ownProperties || {})) {
> +        // Avoid outputting the length property if the data argument provided
> +        // is an array
> +        if (data.class === "Array" && key === "length") {

Do you support Set and Map? Those could work as well...

@@ +1771,5 @@
> +          let {preview} = property;
> +
> +          for (let previewKey of Object.keys(preview.ownProperties)) {
> +            let value = preview.ownProperties[previewKey].value;
> +            item[previewKey] = value;

Here you should use renderValueGrip().

@@ +1780,5 @@
> +          }
> +        } else {
> +          // Display the value for any non-object data input.
> +          item["Values"] = property;
> +          this._columns["Values"] = "Values";

Please make sure 'Values' is also localizable.

@@ +1822,5 @@
> +    body.classList.remove("devtools-monospace", "message-body");
> +    return body;
> +  },
> +
> +  _renderLocation: function() { },

console.table() messages show no source link. You could show it below the table right aligned (I like what chrome does).

@@ +1823,5 @@
> +    return body;
> +  },
> +
> +  _renderLocation: function() { },
> +  _renderRepeatNode: function() { },

In the message constructor duplicate filtering is enabled - you should render the repeat node or disable duplicate filtering.

Comment 34

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #33)

> - maybe the row alternate color should be darker, to have a bit stronger
> contrast.
> 

The table theme should be as per netmonitor. A current re theming is going on in bug 951714 , not sure if it improves row colors too.

> - there's no indicator for sort direction on the column header.

There is a light glow (top and bottom) of the header, which should also be updated as part of bug 951714 which adds arrows instead.

Comment 35

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #34)
> (In reply to Mihai Sucan [:msucan] from comment #33)
> 
> > - maybe the row alternate color should be darker, to have a bit stronger
> > contrast.
> > 
> 
> The table theme should be as per netmonitor. A current re theming is going
> on in bug 951714 , not sure if it improves row colors too.
> 
> > - there's no indicator for sort direction on the column header.
> 
> There is a light glow (top and bottom) of the header, which should also be
> updated as part of bug 951714 which adds arrows instead.

Sounds good. Thanks for the info.

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 0: Add the table method to Console.{h,cpp,webidl}

Rebased Part 0

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8467623 [details] [diff] [review]
Part 1: Add console.table front end [WIP7]

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

(In reply to Mihai Sucan [:msucan] from comment #33)
> Comment on attachment 8467623 [details] [diff] [review]
> Part 1: Add console.table front end [WIP7]
> 
> Review of attachment 8467623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking very good. Thanks for your work. General comments:
> 
> - The following calls do not show the correct/expected output - please
> compare with chrome/firebug:
> 
> console.table([[1, 2], [3, 4]])
> console.table({a: [1, 2], b: [3, 4]})
> 
> - take the family example from
> https://getfirebug.com/wiki/index.php/Console.table
> 
> Try console.table(family, 'firstName'). No '(index)' column is shown. Why?
> Chrome shows it and it makes sense to me.
> 

Fixed.

> - continuing with the family example, try:
> 
>   family.foo = new Person(document, window, document.body);
>   console.table(family);
> 
> I see [object Object]. You should be able to render the objects using:
> 
>   domElem = this.message._renderValueGrip(objectActorGrip, { concise: true
> });
> 

Rendering the value grip will be dealt in a follow up.

> - maybe the row alternate color should be darker, to have a bit stronger
> contrast.
> 
> - there's no indicator for sort direction on the column header.
> 

Table style changes are ongoing in bug 951714

> - table font size seems slightly bigger than the console output font.
> 

Table font size should be the same as console output font size.

> - table font is not monospace. Is this desired?
> 

Fixed. Table font is monospace.

> - what happens with 'pathological' cases? Like tens of thousands of
> rows/columns. Are we limiting output somewhere? If not, we should do so to
> avoid breakage.
> 

Currently, we are limiting on 1000 rows and 10 columns + 1 index column.

> - did you check that the object actors for all the objects received are
> correctly released when console.table messages are removed?
> 

Fixed. Released objects after processing the data.

::: browser/devtools/webconsole/console-output.js
@@ +5,5 @@
>  
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

Fixed. Remove devtools import.

@@ +6,5 @@
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

Fixed. Replaced with lazyImporter

@@ +15,5 @@
>  loader.lazyImporter(this, "Task","resource://gre/modules/Task.jsm");
>  loader.lazyImporter(this, "PluralForm", "resource://gre/modules/PluralForm.jsm");
>  
> +XPCOMUtils.defineLazyGetter(this, "TableWidget",
> +    () => devtools.require("devtools/shared/widgets/TableWidget").TableWidget);

Fixed. Replaced with lazyGetter

@@ +1642,5 @@
> +  this._populateTableData = this._populateTableData.bind(this);
> +  this._renderTable = this._renderTable.bind(this);
> +  Messages.Simple.call(this, this._renderTable, options);
> +
> +  this._repeatID.consoleApiLevel = packet.level;

Fixed. Set filterDuplicates to false.

@@ +1666,5 @@
> +   */
> +  _data: null,
> +
> +  /**
> +   * Key value pair of the id and display name for the columns in the table.

Yes, this could appear confusing as described. This is mainly due to the requirements of the TableWidget API.

@@ +1705,5 @@
> +   */
> +  _setColumns: function(columns)
> +  {
> +    if (columns.class === "Array") {
> +      let items = columns.preview.items;

Currently, the maximum number of columns displayed is 10, which is conveniently equal to the maximum preview.length.

@@ +1753,5 @@
> +    this.client.getPrototypeAndProperties(aResponse => {
> +      let {ownProperties} = aResponse;
> +
> +      if (!hasColumnsArg) {
> +        this._columns["(index)"] = "(index)";

Fixed. "(index)" is localizable.

@@ +1759,5 @@
> +
> +      for (let key of Object.keys(ownProperties || {})) {
> +        // Avoid outputting the length property if the data argument provided
> +        // is an array
> +        if (data.class === "Array" && key === "length") {

Fixed. Added support for Set and Map.

@@ +1771,5 @@
> +          let {preview} = property;
> +
> +          for (let previewKey of Object.keys(preview.ownProperties)) {
> +            let value = preview.ownProperties[previewKey].value;
> +            item[previewKey] = value;

I think we can leave this as a follow up. This requires additional modifications to TableWidget and possibility renderValueGrip().

@@ +1780,5 @@
> +          }
> +        } else {
> +          // Display the value for any non-object data input.
> +          item["Values"] = property;
> +          this._columns["Values"] = "Values";

Fixed. "Values" is localizable.

@@ +1822,5 @@
> +    body.classList.remove("devtools-monospace", "message-body");
> +    return body;
> +  },
> +
> +  _renderLocation: function() { },

I chose to display the source link similar to console.trace() for consistency.

@@ +1823,5 @@
> +    return body;
> +  },
> +
> +  _renderLocation: function() { },
> +  _renderRepeatNode: function() { },

Fixed. Disabled duplicate filtering in the message constructor.

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP8]

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP9]

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for Table console output

Comment 41

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP10]

Comment 42

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for Table console output

Comment 43

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=2fd3ec82eaf3

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8477244 [details] [diff] [review]
Part 1: Add console.table front end [WIP10]

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

r=me with the following comments addressed, thanks!

One problem I found is that the index column appears last in console.table([[1, 2], [3, 4]]). That's a bug.

Another issue is that the table doesn't always present the data in the expected order. Consider the "family" example that was mentioned in comment 33. I would expect the display to follow the insertion order, like Chrome does. Indeed, if I loop over the properties in the console I get insertion order:

for (var i in family) console.log(i);
"mother"
"father"
"daughter"
"son"

console.table() however uses a different order:

console.table(family);
"daughter"
"father"
"mother"
"son"

It would also be useful to indicate when the output has been clipped to 10 columns or 1000 rows (perhaps by adding a warning to the console.table(): header). This could be done as a followup if you prefer.

Finally, please make the one-line change that Mihai suggested for giving arbitrary objects a more useful (but still concise) output, and file the followup for better handling arbitrary object values (like DOM nodes) in the table. This followup work could deal with more exotic cases like console.table([document, document.body]) that Chrome handles nicely.

::: browser/devtools/webconsole/console-output.js
@@ +14,5 @@
>  loader.lazyImporter(this, "PluralForm", "resource://gre/modules/PluralForm.jsm");
> +loader.lazyImporter(this, "promise", "resource://gre/modules/Promise.jsm", "Promise");
> +loader.lazyImporter(this, "ObjectClient", "resource://gre/modules/devtools/dbg-client.jsm");
> +
> +loader.lazyGetter(this, "TableWidget", () => require("devtools/shared/widgets/TableWidget").TableWidget);

You can use lazyRequireGetter for TableWidget for simplicity. Also, since the loader preloads the promise library its module import can be further simplified to:

loader.lazyRequireGetter(this, "promise");

@@ +119,5 @@
> +// Maximum number of rows to display in console.table().
> +const TABLE_ROW_MAX_ITEMS = 1000;
> +
> +// Maximum number of columns to display in console.table().
> +const TABLE_COLUMN_MAX_ITEMS = 10;

These seem like good candidates for being exposed as preferences. I can imagine a user that routinely has to deal with, say 12 columns, and it would be a shame to not allow her to tweak the tool to fit her workflow.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +263,5 @@
> +# console table widget.
> +table.key=Key
> +
> +# LOCALIZATION NOTE (table.value): the column header displayed in the
> +# console table widget.

Repeating the same localization note verbatim doesn't add value, so just use a single one for all of them like we do in the reflow.* strings above.

Comment 45

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #44)
> Another issue is that the table doesn't always present the data in the
> expected order. Consider the "family" example that was mentioned in comment
> 33. I would expect the display to follow the insertion order, like Chrome
> does. Indeed, if I loop over the properties in the console I get insertion
> order:
> 
> for (var i in family) console.log(i);
> "mother"
> "father"
> "daughter"
> "son"
> 
> console.table() however uses a different order:
> 
> console.table(family);
> "daughter"
> "father"
> "mother"
> "son"
> 

The table widget will auto sort the unique key column initially. I presume that the family object is an array, in which case, the index should be the unique key and then the rows will be initially sorted on the index (and just presented in the correct order)

Comment 46

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8477245 [details] [diff] [review]
Part 2: Unit test for Table console output

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

r=me with comments addressed.

As the tbpl failures show, you need to fix the browser_webconsole_console_extras.js test, by removing console.table from test-console-extras.html. While you are there, please also remove profile/profileEnd.

::: browser/devtools/webconsole/test/browser_webconsole_output_table.js
@@ +3,5 @@
> +  http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> + "use strict";
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console-table.html";

Add a short comment describing this test like we do in other tests.

@@ +47,5 @@
> +    data: [
> +      { index: "0", 0: 1, 1: 2 },
> +      { index: "1", 0: 3, 1: 4 }
> +    ],
> +    columns: { index: "(index)", 0: "0", 1: "1" }

See if you can test that the columns are displayed in the correct order in this case, as discussed in my previous comment.

::: browser/devtools/webconsole/test/test-console-table.html
@@ +40,5 @@
> +      var mySet = new Set();
> +
> +      mySet.add(1);
> +      mySet.add(5);
> +      mySet.add("some text");

Please add null and undefined values as well and test that they are displayed appropriately.

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Girish Sharma [:Optimizer] from comment #45)
> The table widget will auto sort the unique key column initially. I presume
> that the family object is an array, in which case, the index should be the
> unique key and then the rows will be initially sorted on the index (and just
> presented in the correct order)

family is an object and the index contains the property names, but your point still stands. Is there a way to tell the table widget not to sort its contents?

Comment 48

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #47)
> (In reply to Girish Sharma [:Optimizer] from comment #45)
> > The table widget will auto sort the unique key column initially. I presume
> > that the family object is an array, in which case, the index should be the
> > unique key and then the rows will be initially sorted on the index (and just
> > presented in the correct order)
> 
> family is an object and the index contains the property names, but your
> point still stands. Is there a way to tell the table widget not to sort its
> contents?

I see, there are two things that we can do here:

 A) add support for disabling sorting in TableWidget, either
   - over disabling sorting will lead to a static table in which user will not be able to sort on any column if he wants to, which I think will be a bad experience from user POV
   - initially keep the table unsorted, but then as soon as user sorts by any column him|herself, he won't be able to come back to the non sorted state.

 B) add back the (index) column and sort on that, but keep it hidden by default (maybe even show it)

I think that Option B is a better choice here.

Comment 49

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Girish Sharma [:Optimizer] from comment #48)
> (In reply to Panos Astithas [:past] from comment #47)
> > (In reply to Girish Sharma [:Optimizer] from comment #45)
> > > The table widget will auto sort the unique key column initially. I presume
> > > that the family object is an array, in which case, the index should be the
> > > unique key and then the rows will be initially sorted on the index (and just
> > > presented in the correct order)
> > 
> > family is an object and the index contains the property names, but your
> > point still stands. Is there a way to tell the table widget not to sort its
> > contents?
> 
> I see, there are two things that we can do here:
> 
>  A) add support for disabling sorting in TableWidget, either
>    - over disabling sorting will lead to a static table in which user will
> not be able to sort on any column if he wants to, which I think will be a
> bad experience from user POV
>    - initially keep the table unsorted, but then as soon as user sorts by
> any column him|herself, he won't be able to come back to the non sorted
> state.
> 
>  B) add back the (index) column and sort on that, but keep it hidden by
> default (maybe even show it)
> 
> I think that Option B is a better choice here.

My requirement here is that the behaviour is unsurprising to *existing* users of console.table - so however you decide to mimic chrome's initial behaviour here is fine with me.

Comment 50

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8477244 [details] [diff] [review]
Part 1: Add console.table front end [WIP10]

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

(In reply to Panos Astithas [:past] from comment #44)
> Comment on attachment 8477244 [details] [diff] [review]
> Part 1: Add console.table front end [WIP10]
> 
> Review of attachment 8477244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the following comments addressed, thanks!
> 
> One problem I found is that the index column appears last in
> console.table([[1, 2], [3, 4]]). That's a bug.
>

This should be fixed once 1059015 lands.

> Another issue is that the table doesn't always present the data in the
> expected order. Consider the "family" example that was mentioned in comment
> 33. I would expect the display to follow the insertion order, like Chrome
> does. Indeed, if I loop over the properties in the console I get insertion
> order:
> 
> for (var i in family) console.log(i);
> "mother"
> "father"
> "daughter"
> "son"
> 
> console.table() however uses a different order:
> 
> console.table(family);
> "daughter"
> "father"
> "mother"
> "son"
> 

Leaving this as a follow up. I don't think we are causing too much harm by sorting the index column. At least this shouldn't block it from landing. To make this conform with Chrome, this requires changes in the TableWidget to not sort the uniqueId. A note Chrome doesn't actually allow for sorting the index column.

> It would also be useful to indicate when the output has been clipped to 10
> columns or 1000 rows (perhaps by adding a warning to the console.table():
> header). This could be done as a followup if you prefer.
> 

Leaving this as a follow up.

> Finally, please make the one-line change that Mihai suggested for giving
> arbitrary objects a more useful (but still concise) output, and file the
> followup for better handling arbitrary object values (like DOM nodes) in the
> table. This followup work could deal with more exotic cases like
> console.table([document, document.body]) that Chrome handles nicely.
> 

Fixed. This also renders the grip in the Sets and Maps.

Also, note that I added '_' to the keys index, key, and value to prevent potential key crashes.

::: browser/devtools/webconsole/console-output.js
@@ +14,5 @@
>  loader.lazyImporter(this, "PluralForm", "resource://gre/modules/PluralForm.jsm");
> +loader.lazyImporter(this, "promise", "resource://gre/modules/Promise.jsm", "Promise");
> +loader.lazyImporter(this, "ObjectClient", "resource://gre/modules/devtools/dbg-client.jsm");
> +
> +loader.lazyGetter(this, "TableWidget", () => require("devtools/shared/widgets/TableWidget").TableWidget);

Fixed. Used loader.lazyRequireGetter(this, "promise");

@@ +119,5 @@
> +// Maximum number of rows to display in console.table().
> +const TABLE_ROW_MAX_ITEMS = 1000;
> +
> +// Maximum number of columns to display in console.table().
> +const TABLE_COLUMN_MAX_ITEMS = 10;

I think this should be a follow up. Currently, we cannot handle more than 10 columns in setColumns because it only provides a preview of 10 items in the array without calling the server to get more.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +263,5 @@
> +# console table widget.
> +table.key=Key
> +
> +# LOCALIZATION NOTE (table.value): the column header displayed in the
> +# console table widget.

Fixed. Removed repeating localization note.

Comment 51

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP11]

Apply bug 1058249 and bug 1059015 before testing. This will deal with the column bug and rendering the value grip.

Comment 52

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8477245 [details] [diff] [review]
Part 2: Unit test for Table console output

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

Fixed tests in browser_webconsole_console_extras.js

::: browser/devtools/webconsole/test/browser_webconsole_output_table.js
@@ +3,5 @@
> +  http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> + "use strict";
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console-table.html";

Fixed. Added a comment description

@@ +47,5 @@
> +    data: [
> +      { index: "0", 0: 1, 1: 2 },
> +      { index: "1", 0: 3, 1: 4 }
> +    ],
> +    columns: { index: "(index)", 0: "0", 1: "1" }

Leaving this as a follow up. I think there is already a sufficient test for column orders in browser_tableWidget_basic.js.

::: browser/devtools/webconsole/test/test-console-table.html
@@ +40,5 @@
> +      var mySet = new Set();
> +
> +      mySet.add(1);
> +      mySet.add(5);
> +      mySet.add("some text");

Fixed. Add null and undefined values and tested them.

Comment 53

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for Table console output [WIP2]

Comment 54

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=31d1871c8cdd

Comment 55

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8479519 [details] [diff] [review]
Part 1: Add console.table front end [WIP11]

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

I just skimmed through the changes, since I've already given this an r+. Please don't forget to file the 3 followup bugs!

::: browser/devtools/webconsole/console-output.js
@@ +15,5 @@
> +loader.lazyImporter(this, "ObjectClient", "resource://gre/modules/devtools/dbg-client.jsm");
> +
> +loader.lazyRequireGetter(this, "promise");
> +
> +loader.lazyGetter(this, "TableWidget", () => require("devtools/shared/widgets/TableWidget").TableWidget);

This can also use lazyRequireGetter since it's not a JSM.

Comment 56

[Girish Sharma [:Optimizer]]

We also need to file a followup bug to handle the iteration index from within the TableWidget itself.

Right now, the constructed object after fetching from server has an extra key called "index" which manages the index of the item in the object (array index or object key).

This can very easily clash and fail if the object itself has a key named index.

After the support in TableWidget, one can simply do

tableWidget.addAll({
 "foo1": {firstName: "foo1", lastName:"bar1"},
 "foo2": {firstName: "foo2", lastName:"bar2"},
 "foo3": {firstName: "foo3", lastName:"bar3"},
})

will render

+------------+-----------------+-------------------+
|  (index)   |    firstName    |    lastName       |
+--------------------------------------------------+
|   foo1     |     foo1        |      bar1         |
+--------------------------------------------------+
|   foo2     |     foo2        |      bar2         |
+--------------------------------------------------+
|   foo3     |     foo3        |      bar3         |
+------------+-----------------+-------------------+

and likewise

tableWidget.addAll([[1,3,5],[2,4,6]])

will render

+------------+-----------------+-------------------+-------------------+
|  (index)   |       0         |        1          |        2          |
+----------------------------------------------------------------------+
|     0      |       1         |        3          |        5          |
+----------------------------------------------------------------------+
|     1      |       2         |        4          |        6          |
+----------------------------------------------------------------------+

Comment 57

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=b1518792b3ab

Comment 58

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Panos Astithas [:past] from comment #55)
> Comment on attachment 8479519 [details] [diff] [review]
> Part 1: Add console.table front end [WIP11]
> 
> Review of attachment 8479519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just skimmed through the changes, since I've already given this an r+.
> Please don't forget to file the 3 followup bugs!
> 
> ::: browser/devtools/webconsole/console-output.js
> @@ +15,5 @@
> > +loader.lazyImporter(this, "ObjectClient", "resource://gre/modules/devtools/dbg-client.jsm");
> > +
> > +loader.lazyRequireGetter(this, "promise");
> > +
> > +loader.lazyGetter(this, "TableWidget", () => require("devtools/shared/widgets/TableWidget").TableWidget);
> 
> This can also use lazyRequireGetter since it's not a JSM.

This didn't seem to work. 

https://tbpl.mozilla.org/?tree=Try&rev=69632eaca629

Comment 59

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add console.table front end [WIP12]

(In reply to Gabriel Luong (:gl) from comment #58)
> (In reply to Panos Astithas [:past] from comment #55)
> > Comment on attachment 8479519 [details] [diff] [review]
> > Part 1: Add console.table front end [WIP11]
> > 
> > Review of attachment 8479519 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I just skimmed through the changes, since I've already given this an r+.
> > Please don't forget to file the 3 followup bugs!
> > 
> > ::: browser/devtools/webconsole/console-output.js
> > @@ +15,5 @@
> > > +loader.lazyImporter(this, "ObjectClient", "resource://gre/modules/devtools/dbg-client.jsm");
> > > +
> > > +loader.lazyRequireGetter(this, "promise");
> > > +
> > > +loader.lazyGetter(this, "TableWidget", () => require("devtools/shared/widgets/TableWidget").TableWidget);
> > 
> > This can also use lazyRequireGetter since it's not a JSM.
> 
> This didn't seem to work. 
> 
> https://tbpl.mozilla.org/?tree=Try&rev=69632eaca629

Fixed. Used lazyRequireGetter for TableWidget.

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 0: Add the table method to Console.{h,cpp,webidl}

Added r=bz in commit message

Comment 61

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/9ca794d04c52
https://hg.mozilla.org/integration/fx-team/rev/adfbc7b15572
https://hg.mozilla.org/integration/fx-team/rev/d65379a03696

Comment 62

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Add table prop to console API test

Comment 63

[Gabriel Luong [:gl] (ΦωΦ)]

http://hg.mozilla.org/integration/fx-team/rev/07d046eea634

Comment 64

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/9ca794d04c52
https://hg.mozilla.org/mozilla-central/rev/adfbc7b15572
https://hg.mozilla.org/mozilla-central/rev/d65379a03696
https://hg.mozilla.org/mozilla-central/rev/07d046eea634

Comment 66

[Liz Henry (:lizzard) (relman/hg->git project)]

Gabriel, might this be good to nominate for a release note?

Comment 67

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Liz Henry :lizzard from comment #66)
> Gabriel, might this be good to nominate for a release note?

Definitely

Comment 68

[Gabriel Luong [:gl] (ΦωΦ)]

+1(In reply to Nick Fitzgerald [:fitzgen] from comment #67)
> (In reply to Liz Henry :lizzard from comment #66)
> > Gabriel, might this be good to nominate for a release note?
> 
> Definitely

+1

Comment 69

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, nomming for relnote. Nick or Gabriel, can you comment to improve the wording and add links to any posts or documentation of the feature? Thanks! p.s. this looks very cool :)


Release Note Request (optional, but appreciated)
[Why is this notable]:  Cool feature added to Dev Tools, useful for Javascript debugging
[Suggested wording]: (Developer Tools) console.table function added to web console
[Links (documentation, blog post, etc)]:

Comment 70

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Liz Henry :lizzard from comment #69)
> OK, nomming for relnote. Nick or Gabriel, can you comment to improve the
> wording and add links to any posts or documentation of the feature? Thanks!
> p.s. this looks very cool :)

Firebug started it: https://getfirebug.com/wiki/index.php/Console_API#console.table.28data.5B.2C_columns.5D.29

Here is a blog post describing its use with Chrome: http://blog.mariusschulz.com/2013/11/13/advanced-javascript-debugging-with-consoletable

I think we will be talking about it in the next hacks blog post we put out about what went up to Aurora.

Comment 71

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Liz Henry :lizzard from comment #69)
> OK, nomming for relnote. Nick or Gabriel, can you comment to improve the
> wording and add links to any posts or documentation of the feature? Thanks!
> p.s. this looks very cool :)

Will Bamberg just wrote up some great MDN docs here: https://developer.mozilla.org/en-US/docs/Web/API/Console.table

Comment 72

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "console.table function added to web console" as wording with a link to the MDN page. Thanks

Comment 73

[Andrei Vaida [:avaida]]

Verified fixed on Firefox 34.0b10 (build1 / 20141117202603) using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5.

I did notice a potential inconsistency though, in the case of collapsible tables such as the one generated by this sample snippet [1], the table isn't collapsible in Firefox's Web Console. As a comparison, the same snippet generates a table that can be collapsed in Chrome's console.

Gabriel, what's your take on this?

[1] http://pastebin.mozilla.org/7385158