993014 - Tree and table widgets for devtools

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

The tree and table widget required in Storage Inspector will be landed in this bug.

Initially I would like to land the code first and keep this bug open while I write the test cases along with the test cases of Storage Inspector.

(Right now, there are no standalone tests for any widget, but we should start having them :) )

Comment 1

[Paul Rouget [:paul]]

Can't we use a xul:tree?

Comment 2

[Girish Sharma [:Optimizer]]

When I initially asked on IRC, everyone was against using XUL, specially Dave, as it would be a lot of work in the future to convert that to non-XUL when we have to. Also, at this point, I am almost done with these two widgets. and tbh, I think the effort was more or less the same. (The effort to wrap the xul:tree into the simple api of the widgets i created, vs creating them from scratch with that api).

The code is almost done, just splitting it out from the main storage inspector frontend bug.

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: light theme icons

I have found some okay icons from different sites. Only light theme for now. Have to create a good filter to change them for dark theme.

Darrin, what do you think of these icons ? Do you have better ones in mind ?

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: dark theme icons

Icons for dark theme.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Hi Brian, this patch adds the tree and table widgets. As per the above comments, I will be adding tests alongside my tests for storage inspector frontend and I intend to land this patch and leave-open.

I also implemented the attachment option in the tree view where you can associate any arbitrary object to any node in the tree.

I am not sure about the various icons for different types of nodes. So leaving the call on you and Darrin.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

fixed some typos/other issues while using the widgets with the storage inspector.

I have pushed a try build with the storage inspector front end patch so that you can try out the widgets.

try : https://tbpl.mozilla.org/?tree=Try&rev=5318bf03dfea

Comment 7

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #4)
> Created attachment 8402991 [details]
> dark theme icons
> 
> Icons for dark theme.

I really hate the look of the folder icons, they look too big and too rounded. Can you try using my folder icons from bug 970517 ? Thanks !

The other icons look alright though, I'd just use a different font (Segoe UI or Open Sans) for the file icons

Also, it seems unclear what's the (...) icon. If it's the generic file icon, i'd rather put nothing in it.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Created attachment 8404308 [details] [diff] [review]
> patch v0.2
> 
> fixed some typos/other issues while using the widgets with the storage
> inspector.
> 
> I have pushed a try build with the storage inspector front end patch so that
> you can try out the widgets.
> 
> try : https://tbpl.mozilla.org/?tree=Try&rev=5318bf03dfea

This try push has failed to build: "RuntimeError: File "../shared/devtools/images/[e-mail]" not found in /builds/....".  Are you missing a file in the patch?

Comment 9

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Girish Sharma [:Optimizer] from comment #6)
> > Created attachment 8404308 [details] [diff] [review]
> > patch v0.2
> > 
> > fixed some typos/other issues while using the widgets with the storage
> > inspector.
> > 
> > I have pushed a try build with the storage inspector front end patch so that
> > you can try out the widgets.
> > 
> > try : https://tbpl.mozilla.org/?tree=Try&rev=5318bf03dfea
> 
> This try push has failed to build: "RuntimeError: File
> "../shared/devtools/images/[e-mail]" not found in
> /builds/....".  Are you missing a file in the patch?

Nope, he forgot to update jar.mn for linux.

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: light-icons v2

Okay, much better icons now.

Attaching dark version and new patch with try soon.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: dark-icons v2

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Changes:
- all 3 jar.mn updated now.
- New icons.

try : https://tbpl.mozilla.org/?tree=Try&rev=285b9f700649
builds at : https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-285b9f700649

Comment 13

[Paul Rouget [:paul]]

Do we want itchpad to use that?

Comment 14

[Girish Sharma [:Optimizer]]

I have discussed that with Brain too. He said, that for initial v1, he will be going to use whatever is there in itchpad, but the plan is to finally switch to these widgets only.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: tree-issues.png

I'm still working through the patch, but wanted to point out some UI issues I've bumped into with the tree widget (at least on OSX). I'm checking this by loading a page with a bunch of storage, like http://www.bbc.com/news/.

1) There seems to be too much left indentation between the different levels.
2) I can't scroll down
3) Word wrapping is weird (see the javascript:"" tree item).  You can also reproduce this by shrinking the tree sidebar to a smaller size.  I haven't looked closely at the CSS yet, but you should be able to solve this with white-space: nowrap, and you may need vertical-align: middle also.
4) When clicking on an item that doesn't have children (like the www.bbc.com entry), then the folder icon toggles even though nothing happens.  You can click over and over on it and it will keep toggling.  If there is no action related to the click, the UI should not toggle.

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Created attachment 8407514 [details]
> tree-issues.png
> 
> I'm still working through the patch, but wanted to point out some UI issues
> I've bumped into with the tree widget (at least on OSX). I'm checking this
> by loading a page with a bunch of storage, like http://www.bbc.com/news/.
> 
> 1) There seems to be too much left indentation between the different levels.
> 2) I can't scroll down

Does both of these cases appear only when the below case (3) is observed ? (i.e. text overflow) ?

I have seen (1), but never have seen a case when I am not able to scroll (on windows and osx)

> 3) Word wrapping is weird (see the javascript:"" tree item).  You can also
> reproduce this by shrinking the tree sidebar to a smaller size.  I haven't
> looked closely at the CSS yet, but you should be able to solve this with
> white-space: nowrap, and you may need vertical-align: middle also.

Yes, fixed locally.

> 4) When clicking on an item that doesn't have children (like the www.bbc.com
> entry), then the folder icon toggles even though nothing happens.  You can
> click over and over on it and it will keep toggling.  If there is no action
> related to the click, the UI should not toggle.

Yeah, fixed in local.

I am almost done writing tests for these two widgets, and will upload a new patch in couple of hours.

The existing patch will not change much. Only minor bug fixes while working through front end of storage inspector.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8405807 [details] [diff] [review]
patch v0.3

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

Here are comments on everything except for the TableWidget.

::: browser/devtools/shared/widgets/TreeWidget.jsm
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

Could use const {Cu, Ci} = require("chrome");

@@ +6,5 @@
> +"use strict";
> +
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const HTML = "http://www.w3.org/1999/xhtml";

Use HTML_NS as convention

@@ +11,5 @@
> +
> +Cu.import("resource://gre/modules/devtools/event-emitter.js");
> +let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
> +
> +this.EXPORTED_SYMBOLS = ["TreeWidget"];

On some of the newer widgets I've seen them saved as js files with module.exports.TreeWidget and loaded with require() instead of jsm with this.EXPORTED_SYMBOLS = ["TreeWidget"]; and loaded with Cu.import.  I'm not sure if there is an advantage to this pattern, so you surely don't need to switch it, but if you want it to match some of the other new widgets you could do this.

@@ +78,5 @@
> +    }
> +    this._selectedLabel = this.root.setSelectedItem(aId);
> +    if (!this._selectedLabel) {
> +      this._selectedItem = null;
> +    }

Nit: is there a reason for not using the same line `} else {` formatting that is used throughout most of the other devtools code?

@@ +99,5 @@
> +    return this._selectedItem;
> +  },
> +
> +  destroy: function() {
> +    this.menupopup && this.menupopup.remove();

Why would this.menupopup be set here?  I don't see it in the rest of the file.

@@ +195,5 @@
> +   * Adds an item in the tree. The item can be added as a child to any node in
> +   * the tree. The method will also create any subnode not present in the process.
> +   *
> +   * @param {array|string} aItem
> +   *        url of the item if `this.containsURL` is true.

I'm not sure if parts is a good name for this.  The add function receives this as aValue, so maybe this should be called values.  Also, please update this comment to indicate that parts (values) can be undefined.

@@ +196,5 @@
> +   * the tree. The method will also create any subnode not present in the process.
> +   *
> +   * @param {array|string} aItem
> +   *        url of the item if `this.containsURL` is true.
> +   *        Otherwise, [ids, parts] where ids is the array of ids to be inserted

It seems the type for this comment would be [[ids], [parts]]

@@ +221,5 @@
> +        parts = ids;
> +      }
> +    }
> +    else {
> +      let uri = Services.io.newURI(aItem, null, null);

I would suggest making a urlToItem function that contains this functionality.  It would give some more space to explain exactly the expected input/output for a URL, and also would allow us to test this logic directly.  At the least, add a comment here explaining what a sample URL would turn into

@@ +268,5 @@
> +    }
> +    if (!target) {
> +      return;
> +    }
> +    if (target.hasAttribute("expanded")) {

I'm thinking we only want to add the expanded attribute if there are children a level below (see point 4 on Comment 15)

@@ +320,5 @@
> +          this.selectPreviousItem();
> +        }
> +        break;
> +
> +      default: return;

default: return; seems like a line that could cause confusion in the future.  Do we really need to return in the case that it wasn't an arrow key pressed?  More specifically, do we need to preventDefault in the case of arrow keys?  The selectedLabel != currentSelected seems pretty safe to run on any keypress

@@ +429,5 @@
> +   *        inserted is direct child of this node.
> +   */
> +  add: function(aItem, aValue ,  aOptions = {}) {
> +    if (aItem.length == this.level) {
> +      return;

What is this actually checking? It seems like this case should not be happening - should we throw an error in this case or is this some expected input that we want to silently ignore?

@@ +439,5 @@
> +    else if(!!id) {
> +      let labelString = label;
> +      if (typeof label != "string") {
> +        labelString = label.textContent;
> +      }

The next 10 or so lines have a lot going on, it's going to take me some more time to digest exactly what is happening.  It would help if there were some comments explaining the steps here.

@@ +459,5 @@
> +      }
> +      this.items.set(id, treeItem);
> +    }
> +  },
> +

Please add a function header comment for remove and setSelectedItem

::: browser/themes/shared/devtools/widgets.inc.css
@@ +1013,5 @@
>  }
>  
> +/* Table Widget */
> +
> +%filter substitution

the substitution filter is already included in this file for another widget.  Maybe we should just move that to the top of the file and remove this one

@@ +1030,5 @@
> +  overflow: auto;
> +}
> +
> +.theme-dark .table-widget-body,
> +.theme-dark .table-widget-empty-text {

The dark theme toolbar styling is different now: https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css#129.  Could you maybe just apply the .theme-toolbar class to these elements so you don't need to specify them in CSS

@@ +1173,5 @@
> +  font-size: large;
> +  margin-top: -20px !important;
> +}
> +
> +.theme-light .table-widget-empty-text {

If you end up adding theme-toolbar on the empty text then you shouldn't need this.  If not, try to use the corresponding color that you are using with dark theme on this element, and leave a comment with the color from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.

@@ +1184,5 @@
> +}
> +
> +/* Tree Widget */
> +
> +.tree-widget-container {

Something is going on where I can't scroll on OSX.  If this is not an issue on Windows then I can try to help and figure it out.  But I'm wondering if we need to have a container block element for scrolling the list.

@@ +1215,5 @@
> +.tree-widget-item[level="1"] {
> +  font-size: 400;
> +}
> +
> +.theme-dark .tree-widget-item.selected {

Could add .theme-selected instead the .selected class and remove these dark and light theme rules

@@ +1274,5 @@
> +
> +/* Indentation of child items in the tree */
> +
> +/* First level */
> +.tree-widget-item[level="1"] + ul > li > .tree-widget-item {

I wish we could use calc with attr to make this happen without listing all the levels.

::: toolkit/devtools/server/actors/storage.js
@@ +1234,5 @@
>        return null;
>      }
>  
>      let rows = yield connection.execute("SELECT name FROM database");
> +    if (rows.length != 1) {t

extra character here

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: table-headers.gif

The header sort on the table seems to get reset after clicking a couple of times on the active header (see animated gif)

Comment 19

[Girish Sharma [:Optimizer]]

Yeah, that is expected. Just like netmonitor's headers. Its a three state sorting. ascending, descending or do not sort on me (i.e. sort on the unique column)

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #19)
> Yeah, that is expected. Just like netmonitor's headers. Its a three state
> sorting. ascending, descending or do not sort on me (i.e. sort on the unique
> column)

That's not how the netmonitor seems to work for me.  It just toggles ascending/descending on the single column, which is what I would expect.

Comment 21

[Girish Sharma [:Optimizer]]

Ah damn. I can almost bet that it was a three state sorting (defaulting to timeline) at some point of time.

But anyways, this is also how all the XUL tree widgets work in Firefox, like Library view, etc.

Do you want me to make it a 2 way sorting instead of 3 ?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #21)
> Ah damn. I can almost bet that it was a three state sorting (defaulting to
> timeline) at some point of time.
> 
> But anyways, this is also how all the XUL tree widgets work in Firefox, like
> Library view, etc.
> 
> Do you want me to make it a 2 way sorting instead of 3 ?

My suggestion would be 2 way sorting to match what the netmonitor is doing.  Also, at least for me, the 3 state is not something I am used to and I thought it was a bug.

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: table-headers-disable.png

When there are two selected columns, I can't remove the second (note that "Name" is disabled all the time here).  I see in the code that this is intentional, but just checking to see why.  We would only want to disable it if there is one left, right?

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Created attachment 8407704 [details]
> table-headers-disable.png
> 
> When there are two selected columns, I can't remove the second (note that
> "Name" is disabled all the time here).  I see in the code that this is
> intentional, but just checking to see why.  We would only want to disable it
> if there is one left, right?

Yeah, this is intentional. There is actually no point of having a table with one column, in my opinion. Its not a strong opinion though.

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8405806 [details]
dark-icons v2

I've seen on the UX team dashboard that darrin had icons ready for the tree widget.

Comment 26

[Girish Sharma [:Optimizer]]

(In reply to Tim Nguyen [:ntim] from comment #25)
> Comment on attachment 8405806 [details]
> dark-icons v2
> 
> I've seen on the UX team dashboard that darrin had icons ready for the tree
> widget.

Can you post the links or something ?

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8405807 [details] [diff] [review]
patch v0.3

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

::: browser/devtools/shared/widgets/TableWidget.jsm
@@ +55,5 @@
> +  this.uniqueId = uniqueId || "name";
> +  this.highlightUpdated = highlightUpdated || false;
> +  this.removableColumns = removableColumns || false;
> +
> +  this.tbody = this.document.createElement("hbox");

Can (should) we use HTML_NS for any of these elements?  I see the column is using it, and it seems like it would be required for loading one of these in an HTML doc.  I could see this being more work than it is worth right now, but just thinking out loud about it.

@@ +90,5 @@
> +};
> +
> +TableWidget.prototype = {
> +
> +  count: 0,

this.count is used in many places in this object, but I wonder why we can't just use this.items.size instead.  Seems like it would save a bunch of work

@@ +127,5 @@
> +  },
> +
> +  destroy: function() {
> +    this.off(EVENTS.ROW_SELECTED, this.bindSelectedRow);
> +    this.menupopup && this.menupopup.remove();

Not a big fan of the true && doSomething() syntax here (I'd rather just see an if statement), but it's up to you if you want to keep it

@@ +150,5 @@
> +      this.document.documentElement.appendChild(popupset);
> +    }
> +
> +    this.menupopup = this.document.createElement("menupopup");
> +    this.menupopup.id = "table-widget-column-select";

What if two tables were added to the same document?  I guess we could cross that bridge when we get there, but it should be possible to do so and right now the multiple popups would be tricky to deal with.

@@ +177,5 @@
> +      }
> +      this.menupopup.appendChild(menuitem);
> +    }
> +    let checked = this.menupopup.querySelectorAll("menuitem[checked]");
> +    if (checked.length == 2) {

I uploaded a screenshot about this, but why does it disable when two are left instead of one?

@@ +245,5 @@
> +    this.populateMenuPopup();
> +  },
> +
> +  /**
> +   * Selected the row corresponding to the `aId` json.

s/Selected/Selects

@@ +307,5 @@
> +    }
> +    this.items.set(aItem[this.uniqueId], aItem);
> +    this.count++;
> +    this.tbody.removeAttribute("empty");
> +    !aSuppressFlash && this.emit(EVENTS.ROW_UPDATED, aItem[this.uniqueId]);

ditto about true && doSomething() syntax

@@ +406,5 @@
> + *        The table object to which the column belongs.
> + * @param {string} aId
> + *        Id of the column.
> + * @param {String} aHeader
> + *        The dislpayed string on the column's header.

s/dislpayed/displayed

@@ +409,5 @@
> + * @param {String} aHeader
> + *        The dislpayed string on the column's header.
> + * @param {object} aOptions
> + *        Options containing the following properties
> + *        - highlightUpdated {boolean} true to highlight the changed/added row.

Instead of passing in highlightUpdated as an option, could we just read it off of the table object?  I can't think of any case when we would want it to be different.

@@ +410,5 @@
> + *        The dislpayed string on the column's header.
> + * @param {object} aOptions
> + *        Options containing the following properties
> + *        - highlightUpdated {boolean} true to highlight the changed/added row.
> + *        - headerContextMenu {XULPopup} reference to the table's header context

First, the type on this comment is wrong.  It is really a string referring to the context menu id.  However, instead of passing this headerContextMenu bas an option, could we have a getter on the table object that returns menupopup.id if available (or null / undefined otherwise)?  This would remove the need for the custom options on this constructor and would make this interaction a bit clearer.

@@ +457,5 @@
> +  this.table.on(EVENTS.COLUMN_SORTED, this.onColumnSorted);
> +
> +  if (this.highlightUpdated) {
> +    this.onRowUpdated = this.onRowUpdated.bind(this);
> +    this.table.on(EVENTS.ROW_UPDATED, this.onRowUpdated);

I'm a bit confused about this.  There is extra functionality beyond flashing the row that happens in this.onRowUpdated.  I believe that we could always bind to onRowUpdated and just check highlightUpdated inside of that function before flashing.

@@ +586,5 @@
> +   * Selects the next row. Cycles to first if last row is selected.
> +   */
> +  selectNextRow: function() {
> +    this._updateItems();
> +    let index = this.items[this.selectedRow]|0 + 1;

What is this bitwise operator doing?

@@ +714,5 @@
> +    if (!this._itemsDirty) {
> +      return;
> +    }
> +    for (let i = 0; i < this.cells.length; i++) {
> +      this.items[this.cells[i].id] = i;

There is quite a bit of work keeping this.items and this.columns in sync.  I would actually prefer if we had items be a getter that returned the object version of this.cells.  This would be trading off a bit of speed for more clear code (how many columns would we really ever have?).  I'm fairly sure that we could remove all of the lines modifying items, any itemsDirty tracking, and any calls to updateItems without changing any behavior by doing this.  I may be misunderstanding but this looks to me like it would work.

Comment 28

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8405807 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 8405807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are comments on everything except for the TableWidget.
> 
> ::: browser/devtools/shared/widgets/TreeWidget.jsm
> @@ +5,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +"use strict";
> > +
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> 
> Could use const {Cu, Ci} = require("chrome");
> 
> @@ +6,5 @@
> > +"use strict";
> > +
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> > +const HTML = "http://www.w3.org/1999/xhtml";
> 
> Use HTML_NS as convention
> 
> @@ +11,5 @@
> > +
> > +Cu.import("resource://gre/modules/devtools/event-emitter.js");
> > +let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
> > +
> > +this.EXPORTED_SYMBOLS = ["TreeWidget"];
> 
> On some of the newer widgets I've seen them saved as js files with
> module.exports.TreeWidget and loaded with require() instead of jsm with
> this.EXPORTED_SYMBOLS = ["TreeWidget"]; and loaded with Cu.import.  I'm not
> sure if there is an advantage to this pattern, so you surely don't need to
> switch it, but if you want it to match some of the other new widgets you
> could do this.
> 
> @@ +78,5 @@
> > +    }
> > +    this._selectedLabel = this.root.setSelectedItem(aId);
> > +    if (!this._selectedLabel) {
> > +      this._selectedItem = null;
> > +    }
> 
> Nit: is there a reason for not using the same line `} else {` formatting
> that is used throughout most of the other devtools code?
> 
> @@ +99,5 @@
> > +    return this._selectedItem;
> > +  },
> > +
> > +  destroy: function() {
> > +    this.menupopup && this.menupopup.remove();
> 
> Why would this.menupopup be set here?  I don't see it in the rest of the
> file.
> 
> @@ +195,5 @@
> > +   * Adds an item in the tree. The item can be added as a child to any node in
> > +   * the tree. The method will also create any subnode not present in the process.
> > +   *
> > +   * @param {array|string} aItem
> > +   *        url of the item if `this.containsURL` is true.
> 
> I'm not sure if parts is a good name for this.  The add function receives
> this as aValue, so maybe this should be called values.  Also, please update
> this comment to indicate that parts (values) can be undefined.
> 
> @@ +196,5 @@
> > +   * the tree. The method will also create any subnode not present in the process.
> > +   *
> > +   * @param {array|string} aItem
> > +   *        url of the item if `this.containsURL` is true.
> > +   *        Otherwise, [ids, parts] where ids is the array of ids to be inserted
> 
> It seems the type for this comment would be [[ids], [parts]]
> 
> @@ +221,5 @@
> > +        parts = ids;
> > +      }
> > +    }
> > +    else {
> > +      let uri = Services.io.newURI(aItem, null, null);
> 
> I would suggest making a urlToItem function that contains this
> functionality.  It would give some more space to explain exactly the
> expected input/output for a URL, and also would allow us to test this logic
> directly.  At the least, add a comment here explaining what a sample URL
> would turn into


Done for all of the above.

> @@ +268,5 @@
> > +    }
> > +    if (!target) {
> > +      return;
> > +    }
> > +    if (target.hasAttribute("expanded")) {
> 
> I'm thinking we only want to add the expanded attribute if there are
> children a level below (see point 4 on Comment 15)

I fixed this at css level. I actually want to be consistent as per code design so letting the expand this there.
 
> @@ +320,5 @@
> > +          this.selectPreviousItem();
> > +        }
> > +        break;
> > +
> > +      default: return;
> 
> default: return; seems like a line that could cause confusion in the future.
> Do we really need to return in the case that it wasn't an arrow key pressed?
> More specifically, do we need to preventDefault in the case of arrow keys? 
> The selectedLabel != currentSelected seems pretty safe to run on any keypress
> 

Yeah, I do not want to prevent default when I am not doing anything for keypress. Custom nodes might have custom listeners attached to them.

> @@ +429,5 @@
> > +   *        inserted is direct child of this node.
> > +   */
> > +  add: function(aItem, aValue ,  aOptions = {}) {
> > +    if (aItem.length == this.level) {
> > +      return;
> 
> What is this actually checking? It seems like this case should not be
> happening - should we throw an error in this case or is this some expected
> input that we want to silently ignore?

Added more comments in this whole method explaining each line. This is the exit condition for a recursive .add call.
 
> @@ +439,5 @@
> > +    else if(!!id) {
> > +      let labelString = label;
> > +      if (typeof label != "string") {
> > +        labelString = label.textContent;
> > +      }
> 
> The next 10 or so lines have a lot going on, it's going to take me some more
> time to digest exactly what is happening.  It would help if there were some
> comments explaining the steps here.
> 
> @@ +459,5 @@
> > +      }
> > +      this.items.set(id, treeItem);
> > +    }
> > +  },
> > +
> 
> Please add a function header comment for remove and setSelectedItem
> 
> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +1013,5 @@
> >  }
> >  
> > +/* Table Widget */
> > +
> > +%filter substitution
> 
> the substitution filter is already included in this file for another widget.
> Maybe we should just move that to the top of the file and remove this one

Done

> @@ +1030,5 @@
> > +  overflow: auto;
> > +}
> > +
> > +.theme-dark .table-widget-body,
> > +.theme-dark .table-widget-empty-text {
> 
> The dark theme toolbar styling is different now:
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> devtools/dark-theme.css#129.  Could you maybe just apply the .theme-toolbar
> class to these elements so you don't need to specify them in CSS
> 

I actually just use the toolbar color here for dark theme, as the sidebar background is too dark and it pops out in eyes.

For light theme, I shifted to sidebar background color from the mdn page.

I dont want to use the toolbar class as it has border color and other stuff too. And no noise.

> @@ +1173,5 @@
> > +  font-size: large;
> > +  margin-top: -20px !important;
> > +}
> > +
> > +.theme-light .table-widget-empty-text {
> 
> If you end up adding theme-toolbar on the empty text then you shouldn't need
> this.  If not, try to use the corresponding color that you are using with
> dark theme on this element, and leave a comment with the color from
> https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.
> 
> @@ +1184,5 @@
> > +}
> > +
> > +/* Tree Widget */
> > +
> > +.tree-widget-container {
> 
> Something is going on where I can't scroll on OSX.  If this is not an issue
> on Windows then I can try to help and figure it out.  But I'm wondering if
> we need to have a container block element for scrolling the list.

Hmm, this is weird. This is a normal li, it should be scrollable. Will investigate.
 
> @@ +1215,5 @@
> > +.tree-widget-item[level="1"] {
> > +  font-size: 400;
> > +}
> > +
> > +.theme-dark .tree-widget-item.selected {
> 
> Could add .theme-selected instead the .selected class and remove these dark
> and light theme rules

Done.
 
> @@ +1274,5 @@
> > +
> > +/* Indentation of child items in the tree */
> > +
> > +/* First level */
> > +.tree-widget-item[level="1"] + ul > li > .tree-widget-item {
> 
> I wish we could use calc with attr to make this happen without listing all
> the levels.

Me too :(

> ::: toolkit/devtools/server/actors/storage.js
> @@ +1234,5 @@
> >        return null;
> >      }
> >  
> >      let rows = yield connection.execute("SELECT name FROM database");
> > +    if (rows.length != 1) {t
> 
> extra character here

Whoops.


(In reply to Brian Grinstead [:bgrins] from comment #27)
> Comment on attachment 8405807 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 8405807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/TableWidget.jsm
> @@ +55,5 @@
> > +  this.uniqueId = uniqueId || "name";
> > +  this.highlightUpdated = highlightUpdated || false;
> > +  this.removableColumns = removableColumns || false;
> > +
> > +  this.tbody = this.document.createElement("hbox");
> 
> Can (should) we use HTML_NS for any of these elements?  I see the column is
> using it, and it seems like it would be required for loading one of these in
> an HTML doc.  I could see this being more work than it is worth right now,
> but just thinking out loud about it.

so, tbody and column, should be in an xul element, so as to make the splitters work. This lets us get resizable columns, without a bit of extra code.

> @@ +90,5 @@
> > +};
> > +
> > +TableWidget.prototype = {
> > +
> > +  count: 0,
> 
> this.count is used in many places in this object, but I wonder why we can't
> just use this.items.size instead.  Seems like it would save a bunch of work
>

You're right .
 
> @@ +127,5 @@
> > +  },
> > +
> > +  destroy: function() {
> > +    this.off(EVENTS.ROW_SELECTED, this.bindSelectedRow);
> > +    this.menupopup && this.menupopup.remove();
> 
> Not a big fan of the true && doSomething() syntax here (I'd rather just see
> an if statement), but it's up to you if you want to keep it

I'll remove :)

> @@ +150,5 @@
> > +      this.document.documentElement.appendChild(popupset);
> > +    }
> > +
> > +    this.menupopup = this.document.createElement("menupopup");
> > +    this.menupopup.id = "table-widget-column-select";
> 
> What if two tables were added to the same document?  I guess we could cross
> that bridge when we get there, but it should be possible to do so and right
> now the multiple popups would be tricky to deal with.

Lets keep it simple now. But it should not be too hard for future.

> @@ +177,5 @@
> > +      }
> > +      this.menupopup.appendChild(menuitem);
> > +    }
> > +    let checked = this.menupopup.querySelectorAll("menuitem[checked]");
> > +    if (checked.length == 2) {
> 
> I uploaded a screenshot about this, but why does it disable when two are
> left instead of one?

comment 24

> @@ +245,5 @@
> > +    this.populateMenuPopup();
> > +  },
> > +
> > +  /**
> > +   * Selected the row corresponding to the `aId` json.
> 
> s/Selected/Selects
> 
> @@ +307,5 @@
> > +    }
> > +    this.items.set(aItem[this.uniqueId], aItem);
> > +    this.count++;
> > +    this.tbody.removeAttribute("empty");
> > +    !aSuppressFlash && this.emit(EVENTS.ROW_UPDATED, aItem[this.uniqueId]);
> 
> ditto about true && doSomething() syntax
> 
> @@ +406,5 @@
> > + *        The table object to which the column belongs.
> > + * @param {string} aId
> > + *        Id of the column.
> > + * @param {String} aHeader
> > + *        The dislpayed string on the column's header.
> 
> s/dislpayed/displayed
> 
> @@ +409,5 @@
> > + * @param {String} aHeader
> > + *        The dislpayed string on the column's header.
> > + * @param {object} aOptions
> > + *        Options containing the following properties
> > + *        - highlightUpdated {boolean} true to highlight the changed/added row.
> 
> Instead of passing in highlightUpdated as an option, could we just read it
> off of the table object?  I can't think of any case when we would want it to
> be different.
> 

Network monitor is one .

> @@ +410,5 @@
> > + *        The dislpayed string on the column's header.
> > + * @param {object} aOptions
> > + *        Options containing the following properties
> > + *        - highlightUpdated {boolean} true to highlight the changed/added row.
> > + *        - headerContextMenu {XULPopup} reference to the table's header context
> 
> First, the type on this comment is wrong.  It is really a string referring
> to the context menu id.  However, instead of passing this headerContextMenu
> bas an option, could we have a getter on the table object that returns
> menupopup.id if available (or null / undefined otherwise)?  This would
> remove the need for the custom options on this constructor and would make
> this interaction a bit clearer.

Hmm, nice.

> @@ +457,5 @@
> > +  this.table.on(EVENTS.COLUMN_SORTED, this.onColumnSorted);
> > +
> > +  if (this.highlightUpdated) {
> > +    this.onRowUpdated = this.onRowUpdated.bind(this);
> > +    this.table.on(EVENTS.ROW_UPDATED, this.onRowUpdated);
> 
> I'm a bit confused about this.  There is extra functionality beyond flashing
> the row that happens in this.onRowUpdated.  I believe that we could always
> bind to onRowUpdated and just check highlightUpdated inside of that function
> before flashing.

You are right. I actually call this._updateItems(); in the method which is important.

> @@ +586,5 @@
> > +   * Selects the next row. Cycles to first if last row is selected.
> > +   */
> > +  selectNextRow: function() {
> > +    this._updateItems();
> > +    let index = this.items[this.selectedRow]|0 + 1;
> 
> What is this bitwise operator doing?

Converting a string to number, if it is a string.

> @@ +714,5 @@
> > +    if (!this._itemsDirty) {
> > +      return;
> > +    }
> > +    for (let i = 0; i < this.cells.length; i++) {
> > +      this.items[this.cells[i].id] = i;
> 
> There is quite a bit of work keeping this.items and this.columns in sync.  I
> would actually prefer if we had items be a getter that returned the object
> version of this.cells.  This would be trading off a bit of speed for more
> clear code (how many columns would we really ever have?).  I'm fairly sure
> that we could remove all of the lines modifying items, any itemsDirty
> tracking, and any calls to updateItems without changing any behavior by
> doing this.  I may be misunderstanding but this looks to me like it would
> work.

So, this syncing does not happens always, and it is there to keep the reverse array this.items up to date.

this.items contains a map of id of the cell verses the index of that particular cell. Keeping this map makes most of the operations of the table O(1) where n is the number of rows. if we have an id to index map, and index to id map, things like, getting item at row X, or getting the index of row with this json, or removing an existing, or updating an existing become O(1) with the only limitation of pushing a new row, which needs figuring out the correct location of the new row, thus O(n)

Comment 29

[Brian Grinstead [:bgrins]]

Sounds great, I've just left a couple more comments below:

> > @@ +320,5 @@
> > > +          this.selectPreviousItem();
> > > +        }
> > > +        break;
> > > +
> > > +      default: return;
> > 
> > default: return; seems like a line that could cause confusion in the future.
> > Do we really need to return in the case that it wasn't an arrow key pressed?
> > More specifically, do we need to preventDefault in the case of arrow keys? 
> > The selectedLabel != currentSelected seems pretty safe to run on any keypress
> > 
> 
> Yeah, I do not want to prevent default when I am not doing anything for
> keypress. Custom nodes might have custom listeners attached to them.
> 

I see that we wouldn't want to preventDefault for others, but is there a reason *to* preventDefault when the arrows are presssed?

> > @@ +409,5 @@
> > > + * @param {String} aHeader
> > > + *        The dislpayed string on the column's header.
> > > + * @param {object} aOptions
> > > + *        Options containing the following properties
> > > + *        - highlightUpdated {boolean} true to highlight the changed/added row.
> > 
> > Instead of passing in highlightUpdated as an option, could we just read it
> > off of the table object?  I can't think of any case when we would want it to
> > be different.
> > 
> 
> Network monitor is one .

I should be more clear about what I meant by this.  When the column is built, the highlightUpdated is always set to table.highlightUpdated.  By wouldn't want it to be different, I meant be different from the table object.  Of course the table object may have it set to false, but I'm saying we could read it from table.highlightUpdated rather than passing it in as an option.

> 
> > @@ +586,5 @@
> > > +   * Selects the next row. Cycles to first if last row is selected.
> > > +   */
> > > +  selectNextRow: function() {
> > > +    this._updateItems();
> > > +    let index = this.items[this.selectedRow]|0 + 1;
> > 
> > What is this bitwise operator doing?
> 
> Converting a string to number, if it is a string.

Would parseInt or Math.floor do the same thing?  If so, I'd use that since it is more obvious.

Comment 30

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> > > @@ +320,5 @@
> > > > +          this.selectPreviousItem();
> > > > +        }
> > > > +        break;
> > > > +
> > > > +      default: return;
> > > 
> > > default: return; seems like a line that could cause confusion in the future.
> > > Do we really need to return in the case that it wasn't an arrow key pressed?
> > > More specifically, do we need to preventDefault in the case of arrow keys? 
> > > The selectedLabel != currentSelected seems pretty safe to run on any keypress
> > > 
> > 
> > Yeah, I do not want to prevent default when I am not doing anything for
> > keypress. Custom nodes might have custom listeners attached to them.
> > 
> 
> I see that we wouldn't want to preventDefault for others, but is there a
> reason *to* preventDefault when the arrows are presssed?

Oh yes! to prevent auto scrolling.

> > > @@ +409,5 @@
> > > > + * @param {String} aHeader
> > > > + *        The dislpayed string on the column's header.
> > > > + * @param {object} aOptions
> > > > + *        Options containing the following properties
> > > > + *        - highlightUpdated {boolean} true to highlight the changed/added row.
> > > 
> > > Instead of passing in highlightUpdated as an option, could we just read it
> > > off of the table object?  I can't think of any case when we would want it to
> > > be different.
> > > 
> > 
> > Network monitor is one .
> 
> I should be more clear about what I meant by this.  When the column is
> built, the highlightUpdated is always set to table.highlightUpdated.  By
> wouldn't want it to be different, I meant be different from the table
> object.  Of course the table object may have it set to false, but I'm saying
> we could read it from table.highlightUpdated rather than passing it in as an
> option.

Ah.. Already did that in local. I thought your question was split up in two parts, one to use the property from table, second that do we want that configurable at all on a table level :D

> > 
> > > @@ +586,5 @@
> > > > +   * Selects the next row. Cycles to first if last row is selected.
> > > > +   */
> > > > +  selectNextRow: function() {
> > > > +    this._updateItems();
> > > > +    let index = this.items[this.selectedRow]|0 + 1;
> > > 
> > > What is this bitwise operator doing?
> > 
> > Converting a string to number, if it is a string.
> 
> Would parseInt or Math.floor do the same thing?  If so, I'd use that since
> it is more obvious.

It is .. but it is more code and a bit slower too (not that performance matter at this level, but still)

Also, we use these unary casting operators all over the place, like +"3" is 3 too.

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Addressed review comments.
Added a lot of tests. All sorts of it.

try : https://tbpl.mozilla.org/?tree=Try&rev=9666c02228df

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: tree-scrolling.gif

Just a couple of quick notes after applying the patch:

1) Now I'm always getting a horizontal scrollbar, but never a vertical (see gif).
2) I'm also not seeing any 'active' styling on the selected element when I click on a site in the tree.
3) The table sorting seems to still be doing the 3 state thing.  Were you wanting to discuss this change further?  The test seems to cover sorting twice to make sure ascending/descending.  Maybe there should be a third case to cover one more trigger to make sure it goes back to ascending instead of removing the sort.

Comment 33

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5

- Fixed tree scrolling issues along with indentation issues.
- Added missing files TreeWidget.js and TableWidget.js
- Added another sorting test to check that third click does ascending sort only.

Please use the updated patch in the frontend bug to test this one.

ongoing try : https://tbpl.mozilla.org/?tree=Try&rev=4718aa547041

green try : https://tbpl.mozilla.org/?tree=Try&rev=7cb764e66731

Comment 34

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

Comment on attachment 8408901 [details] [diff] [review]
patch v0.5

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

::: browser/devtools/shared/widgets/TreeWidget.js
@@ +473,5 @@
> +      return;
> +    }
> +    // Get the id and label corresponding to this level inside the tree.
> +    let [id, label] = [item[this.level], value[this.level]];
> +    if (this.items.has(id)) {

If I clear() the TreeWidget, this.items for the root will be null following destroy().

@@ +516,5 @@
> +   * @param {array} item
> +   *        Ids of items leading up to the item to be removed.
> +   */
> +  remove: function(item) {
> +    let id = item.shift();

Need check if item is not undefined since this.node.remove() in destroy() does not pass in any item.

Comment 35

[Girish Sharma [:Optimizer]]

(In reply to Gabriel L (:gluong) from comment #34)
> Comment on attachment 8408901 [details] [diff] [review]
> patch v0.5
> 
> Review of attachment 8408901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/TreeWidget.js
> @@ +473,5 @@
> > +      return;
> > +    }
> > +    // Get the id and label corresponding to this level inside the tree.
> > +    let [id, label] = [item[this.level], value[this.level]];
> > +    if (this.items.has(id)) {
> 
> If I clear() the TreeWidget, this.items for the root will be null following
> destroy().

Yes, you are right. I'll have an updated patch in couple of minutes

> @@ +516,5 @@
> > +   * @param {array} item
> > +   *        Ids of items leading up to the item to be removed.
> > +   */
> > +  remove: function(item) {
> > +    let id = item.shift();
> 
> Need check if item is not undefined since this.node.remove() in destroy()
> does not pass in any item.

this.node is actually a DOMNode. But there are other places where remove is called with no arguments. Fixed locally.

Comment 36

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

- Fixed the TreeWidget.clear method. Added a test to check if it clears properly.
- Fixed TreeWidget in RTL mode.

Comment 37

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6.1

breaK!

Comment 38

[Darrin Henein [:darrin]]

Comment on attachment 8405805 [details]
light-icons v2

I'm not opposed to the actual icons, but they look very disproportionate to the text (the icons seem too big or the text is too small). Also a nit, the icons and text do not seem vertically aligned (text seems too high). The large icons and misalignment make the tree feel a bit too tight, I would suggest icons which fit the text size a bit better (fwiw the globe icon seems closest to that right now).

Comment 39

[Darrin Henein [:darrin]]

Comment on attachment 8405806 [details]
dark-icons v2

See comment on light icons.

Comment 40

[Girish Sharma [:Optimizer]]

(In reply to Darrin Henein [:darrin] from comment #38)
> Comment on attachment 8405805 [details]
> light-icons v2
> 
> I'm not opposed to the actual icons, but they look very disproportionate to
> the text (the icons seem too big or the text is too small). Also a nit, the
> icons and text do not seem vertically aligned (text seems too high). The

That is because the file icons are not square but rectangle. Thus if their bottom vertically match up with text, there will be only 12px of height left for icons, making only 9 px of width. And nothing will be visible in 9 px width.

> large icons and misalignment make the tree feel a bit too tight, I would
> suggest icons which fit the text size a bit better (fwiw the globe icon
> seems closest to that right now).

Because globe is a square icon (same width and height).

Comment 41

[Girish Sharma [:Optimizer]]

Darrin and I talked on IRC.

He suggested to make the folder icons smaller to 16px from their current 18px (and they look much better).

Globe icon is already okay.

Regarding other icons (file icons) - They will not really be used by anything right now, while globe and folder icons are used by Storage Inspector.

Thus I have opened bug 999504 to improve the file type icons in the TreeWidget, and will go ahead with the current icons for landing purposes.

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8409633 [details] [diff] [review]
patch v0.6.1

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

Just a couple of notes, still reviewing changes

::: browser/devtools/shared/test/browser_tableWidget_basic.js
@@ +35,5 @@
> +          col4: "Column 4"
> +        },
> +        uniqueId: "col1",
> +        emptyText: "This is dummy empty text",
> +        highlightUpdated: true,

is there a reasonable way to test the highlightUpdated option here?

@@ +342,5 @@
> +
> +/**
> + * Tests if clicking the table items does the expected behavior
> + */
> +let testClickSelection = Task.async(function*() {

This test should probably be split into a couple of smaller ones.  There seem to be easy points of separation here: test_basic could be the testAPI function, test_selection could be testClickSelection and testKeyboardNavigation

::: browser/devtools/shared/test/browser_treeWidget_basic.js
@@ +220,5 @@
> +
> +/**
> + * Tests if clicking the tree items does the expected behavior
> + */
> +let testClickSelection = Task.async(function*() {

Same comment as on the table test - this should probably be split into a couple of smaller ones.  There seem to be easy points of separation here: test_basic could be the testAPI function, test_selection could be testClickSelection and testKeyboardNavigation

::: browser/devtools/shared/widgets/TableWidget.js
@@ +852,5 @@
> + */
> +function Cell(column, item, nextCell) {
> +  let document = column.document;
> +
> +  this.label = document.createElementNS(XUL_NS, "label");

Seeing the table API usage in the test, it became clear to me that this interface would not work for the network monitor.  This allows only string values, while the network monitor needs to have specific DOM nodes to be passed into the row.  For instance, an image response would have a small thumbnail, there is a different shape based on the status code, and the timeline needs to be passed in as a custom node.  What do you think about this?  Do you see a path forward to overload the contents of a row with more advanced data types, or is this widget meant to be suited only for text based content?

Comment 43

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #42)
> Comment on attachment 8409633 [details] [diff] [review]
> patch v0.6.1
> 
> Review of attachment 8409633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a couple of notes, still reviewing changes
> 
> ::: browser/devtools/shared/test/browser_tableWidget_basic.js
> @@ +35,5 @@
> > +          col4: "Column 4"
> > +        },
> > +        uniqueId: "col1",
> > +        emptyText: "This is dummy empty text",
> > +        highlightUpdated: true,
> 
> is there a reasonable way to test the highlightUpdated option here?

Not very easily. The flashing here happens differently than any where else in the devtools code. I am not using timers to remove the class. Instead, triggering an animation due to an attribute change.

Even that attribute gets reverted back in sync manner in the next line itself.

Do we test the flashing feature of markup view ? If yes, I can probably test in the same manner somehow.


> @@ +342,5 @@
> > +
> > +/**
> > + * Tests if clicking the table items does the expected behavior
> > + */
> > +let testClickSelection = Task.async(function*() {
> 
> This test should probably be split into a couple of smaller ones.  There
> seem to be easy points of separation here: test_basic could be the testAPI
> function, test_selection could be testClickSelection and
> testKeyboardNavigation
> 
> ::: browser/devtools/shared/test/browser_treeWidget_basic.js
> @@ +220,5 @@
> > +
> > +/**
> > + * Tests if clicking the tree items does the expected behavior
> > + */
> > +let testClickSelection = Task.async(function*() {
> 
> Same comment as on the table test - this should probably be split into a
> couple of smaller ones.  There seem to be easy points of separation here:
> test_basic could be the testAPI function, test_selection could be
> testClickSelection and testKeyboardNavigation

Yeah, they can be split up into three tests.

> ::: browser/devtools/shared/widgets/TableWidget.js
> @@ +852,5 @@
> > + */
> > +function Cell(column, item, nextCell) {
> > +  let document = column.document;
> > +
> > +  this.label = document.createElementNS(XUL_NS, "label");
> 
> Seeing the table API usage in the test, it became clear to me that this
> interface would not work for the network monitor.  This allows only string
> values, while the network monitor needs to have specific DOM nodes to be
> passed into the row.  For instance, an image response would have a small
> thumbnail, there is a different shape based on the status code, and the
> timeline needs to be passed in as a custom node.  What do you think about
> this?  Do you see a path forward to overload the contents of a row with more
> advanced data types, or is this widget meant to be suited only for text
> based content?

So , I talked to Victor too about it. Right now, no, it can't be used directly as is. But changing the Cell constructor to accept a DOMNode instead of string is an easy peasy. (Just like its done in TreeWidget)

When the time comes, this change can be included in the patch converting net monitor to table widget ..

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8409633 [details] [diff] [review]
patch v0.6.1

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

Some more comments.

Also, Let's remove css.svg, html.svg, img.svg, js.svg, manifest.svg and txt.svg (and the corresponding entries in jar.mns / comments in TreeWidget) from this patch since they are not needed right now - we can add these back in when we need them (in bug 999504).

::: browser/devtools/shared/widgets/TreeWidget.js
@@ +252,5 @@
> +   *            └ baz
> +   *        If [values] is undefined, [ids] will be used as values instead.
> +   * @param {object} options
> +   *        An options object containing the following options:
> +   *         - defaultType {string} The default type of the items in the ids

This interface is confusing to me for a few reasons.

1) This optional nested array overloading is hard to follow / remember.  I can say tree.add([[ids]]), tree.add([[ids],[labels]), or tree.add(string, {containsURL: true}). I notice in the storage patch that there is only one place you pass in a custom label and no places that you pass in containsURL.  So the 'normal' case is just an array of ids, and I always forget and want to omit the extra nested array and just say tree.add(['id1']).

    // Current interface:
    tree.add([['id1', 'id2']]); // id -> id2
    tree.add([['id1', 'id2'], ['label1', 'label2']]); // label1 -> label2

There are a couple of alternative ways we could pass in the optional data without overloading:

    tree.add(['id1', 'id2'], {type:'dir', labels: ['label1', 'label2']}); // label1 -> label2

Or they would be passed in the array as an object if needed.

    tree.add([{id:'id1',label:'label1'}, {id:'id2',label:'label2'}]); // label1 -> label2
    tree.add(['id1', {id:'id2',label:'label2'}]); // id1 -> label2

This could maybe be extended to address point 3 also if it took the options for each item instead of a second param that is only applying to some of the elements:

    tree.add([{id:'id1',label:'label1', type: 'dir'}, {id:'id2',label:'label2', type: 'js'}]); // label1 -> label2

2) This containsURL option doesn't appear to be in use in the storage inspector patch.  If we don't need it now, let's remove it to simplify the interface and remove some code.

3) If I'm reading the code right, then it is only applying the options to any items in the tree that have not yet been created.

So the result of the following commands:

    tree.add([['item1'], ['Item 1']]);
    tree.add([['item1', 'item2'], ['Item 1', 'Item 2']], {type: 'js'});

Is different than running this single command:

    tree.add([['item1', 'item2'], ['Item 1', 'Item 2']], {type: 'js'});

4) If I change a label for an earlier item it looks like it will not be updated.  I know we don't need to support this functionality right now, so I'm not asking you to implement this, but it is another place where the interface doesn't act as I would expect.

    tree.add([['item1'], ['Item 1']]);
    tree.add([['item1', 'item2'], ['Item Label Updated', 'Item 2']]);

I don't really know what the best way to handle all of these situations - I guess these are the types of issues come along with the implicit creation of elements.

@@ +491,5 @@
> +    if (this.items.has(id)) {
> +      // An item with same id already exists, thus calling the add method of that
> +      // child to add the passed node at correct position.
> +      this.items.get(id).add(item, value, options);
> +    } else if(!!id) {

Why !!id?  This is in a conditional, it will be converted without any help.  Further, what is the case when id is falsy here?  You already have an exit conition about for when item.length == this.level

Comment 45

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #44)
> Comment on attachment 8409633 [details] [diff] [review]
> patch v0.6.1
> 
> Review of attachment 8409633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some more comments.
> 
> Also, Let's remove css.svg, html.svg, img.svg, js.svg, manifest.svg and
> txt.svg (and the corresponding entries in jar.mns / comments in TreeWidget)
> from this patch since they are not needed right now - we can add these back
> in when we need them (in bug 999504).


We can .. I just wanted them (the icons) to get some attention :)

> ::: browser/devtools/shared/widgets/TreeWidget.js
> @@ +252,5 @@
> > +   *            └ baz
> > +   *        If [values] is undefined, [ids] will be used as values instead.
> > +   * @param {object} options
> > +   *        An options object containing the following options:
> > +   *         - defaultType {string} The default type of the items in the ids
> 
> This interface is confusing to me for a few reasons.
> 
> 1) This optional nested array overloading is hard to follow / remember.  I
> can say tree.add([[ids]]), tree.add([[ids],[labels]), or tree.add(string,
> {containsURL: true}). I notice in the storage patch that there is only one

Remember, containsURL is a creation time option, and not an option for tree.add method.

So ideally, debugger / other url based trees, would just call

tree.add(url, {attachment: <if any>});

and the type and everything will be taken care of ..
(type taken care part is not done yet)

> place you pass in a custom label and no places that you pass in containsURL.
> So the 'normal' case is just an array of ids, and I always forget and want
> to omit the extra nested array and just say tree.add(['id1']).
> 
>     // Current interface:
>     tree.add([['id1', 'id2']]); // id -> id2
>     tree.add([['id1', 'id2'], ['label1', 'label2']]); // label1 -> label2
> 
> There are a couple of alternative ways we could pass in the optional data
> without overloading:
> 
>     tree.add(['id1', 'id2'], {type:'dir', labels: ['label1', 'label2']}); //
> label1 -> label2
> 

I considered this. The issue is that id array and label array is very closely linked, but in this representation, it appears as loosely linked. Also, too many options ..

> Or they would be passed in the array as an object if needed.
> 
>     tree.add([{id:'id1',label:'label1'}, {id:'id2',label:'label2'}]); //
> label1 -> label2
>     tree.add(['id1', {id:'id2',label:'label2'}]); // id1 -> label2
> 
> This could maybe be extended to address point 3 also if it took the options
> for each item instead of a second param that is only applying to some of the
> elements:
> 
>     tree.add([{id:'id1',label:'label1', type: 'dir'},
> {id:'id2',label:'label2', type: 'js'}]); // label1 -> label2
> 

This way appears to be very good, but it has a hidden catch. If we go by this way, the API for inserting urls and nodes in the tree (if not now, in future) will be completely different ..

normal ids and labels:

tree.add(['id1', {id:'id2',label:'label2', type: 'js';}, {id: 'id3', attachment: foobar}]);

url:

tree.add("http://google.com/javascript.js", {attachment: foobar});

DOM Nodes:

tree.add(['id1', node1, {id: 'id3', label: 'label3'}]);

This makes detecting the second index is actually a DOMNode requires instanceof operator :/

(as node1.id can be something, which makes us believe that it is an object, like in the third index of the passed array)

> 2) This containsURL option doesn't appear to be in use in the storage
> inspector patch.  If we don't need it now, let's remove it to simplify the
> interface and remove some code.

I am not against removing it. Just that my effort was wasted and will be duplicated ..

> 3) If I'm reading the code right, then it is only applying the options to
> any items in the tree that have not yet been created.
> 
> So the result of the following commands:
> 
>     tree.add([['item1'], ['Item 1']]);
>     tree.add([['item1', 'item2'], ['Item 1', 'Item 2']], {type: 'js'});
> 
> Is different than running this single command:
> 
>     tree.add([['item1', 'item2'], ['Item 1', 'Item 2']], {type: 'js'});
> 

Yeah, I know :|

> 4) If I change a label for an earlier item it looks like it will not be
> updated.  I know we don't need to support this functionality right now, so
> I'm not asking you to implement this, but it is another place where the
> interface doesn't act as I would expect.
> 
>     tree.add([['item1'], ['Item 1']]);
>     tree.add([['item1', 'item2'], ['Item Label Updated', 'Item 2']]);
> 
> I don't really know what the best way to handle all of these situations - I
> guess these are the types of issues come along with the implicit creation of
> elements.
> 

Any renaming of items should be done via tree.update which does not exist now, but can be created once need is arisen.

(Just like table.update)
> @@ +491,5 @@
> > +    if (this.items.has(id)) {
> > +      // An item with same id already exists, thus calling the add method of that
> > +      // child to add the passed node at correct position.
> > +      this.items.get(id).add(item, value, options);
> > +    } else if(!!id) {
> 
> Why !!id?  This is in a conditional, it will be converted without any help. 
> Further, what is the case when id is falsy here?  You already have an exit
> conition about for when item.length == this.level

Ah, yes. (id) should work too

Comment 46

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #45)
> (In reply to Brian Grinstead [:bgrins] from comment #44)
> > Comment on attachment 8409633 [details] [diff] [review]
> > patch v0.6.1
> > 
> > Review of attachment 8409633 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Some more comments.
> > 
> > Also, Let's remove css.svg, html.svg, img.svg, js.svg, manifest.svg and
> > txt.svg (and the corresponding entries in jar.mns / comments in TreeWidget)
> > from this patch since they are not needed right now - we can add these back
> > in when we need them (in bug 999504).
> 
> 
> We can .. I just wanted them (the icons) to get some attention :)
> 
> > ::: browser/devtools/shared/widgets/TreeWidget.js
> > @@ +252,5 @@
> > > +   *            └ baz
> > > +   *        If [values] is undefined, [ids] will be used as values instead.
> > > +   * @param {object} options
> > > +   *        An options object containing the following options:
> > > +   *         - defaultType {string} The default type of the items in the ids
> > 
> > This interface is confusing to me for a few reasons.
> > 
> > 1) This optional nested array overloading is hard to follow / remember.  I
> > can say tree.add([[ids]]), tree.add([[ids],[labels]), or tree.add(string,
> > {containsURL: true}). I notice in the storage patch that there is only one
> 
> Remember, containsURL is a creation time option, and not an option for
> tree.add method.
> 
> So ideally, debugger / other url based trees, would just call
> 
> tree.add(url, {attachment: <if any>});
> 
> and the type and everything will be taken care of ..
> (type taken care part is not done yet)
> 
> > place you pass in a custom label and no places that you pass in containsURL.
> > So the 'normal' case is just an array of ids, and I always forget and want
> > to omit the extra nested array and just say tree.add(['id1']).
> > 
> >     // Current interface:
> >     tree.add([['id1', 'id2']]); // id -> id2
> >     tree.add([['id1', 'id2'], ['label1', 'label2']]); // label1 -> label2
> > 
> > There are a couple of alternative ways we could pass in the optional data
> > without overloading:
> > 
> >     tree.add(['id1', 'id2'], {type:'dir', labels: ['label1', 'label2']}); //
> > label1 -> label2
> > 
> 
> I considered this. The issue is that id array and label array is very
> closely linked, but in this representation, it appears as loosely linked.
> Also, too many options ..
> 
> > Or they would be passed in the array as an object if needed.
> > 
> >     tree.add([{id:'id1',label:'label1'}, {id:'id2',label:'label2'}]); //
> > label1 -> label2
> >     tree.add(['id1', {id:'id2',label:'label2'}]); // id1 -> label2
> > 
> > This could maybe be extended to address point 3 also if it took the options
> > for each item instead of a second param that is only applying to some of the
> > elements:
> > 
> >     tree.add([{id:'id1',label:'label1', type: 'dir'},
> > {id:'id2',label:'label2', type: 'js'}]); // label1 -> label2
> > 
> 
> This way appears to be very good, but it has a hidden catch. If we go by
> this way, the API for inserting urls and nodes in the tree (if not now, in
> future) will be completely different ..
> 
> normal ids and labels:
> 
> tree.add(['id1', {id:'id2',label:'label2', type: 'js';}, {id: 'id3',
> attachment: foobar}]);
> 
> url:
> 
> tree.add("http://google.com/javascript.js", {attachment: foobar});
> 
> DOM Nodes:
> 
> tree.add(['id1', node1, {id: 'id3', label: 'label3'}]);
> 
> This makes detecting the second index is actually a DOMNode requires
> instanceof operator :/
> 
> (as node1.id can be something, which makes us believe that it is an object,
> like in the third index of the passed array)
> 
> > 2) This containsURL option doesn't appear to be in use in the storage
> > inspector patch.  If we don't need it now, let's remove it to simplify the
> > interface and remove some code.
> 
> I am not against removing it. Just that my effort was wasted and will be
> duplicated ..
> 

What is the use case for containsURL / URL based trees?  You mention the debugger, but I'm not sure exactly where in the debugger a URL based tree would be used.

Regarding wasting the effort, we could always add a comment with a link to a version of the attachment that has the URL functionality so that we could grab it if/when needed.  The thing is that the API that we may be wanting to use for that may be different than what we imagine it will be right now.

Comment 47

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #46)

> What is the use case for containsURL / URL based trees?  You mention the
> debugger, but I'm not sure exactly where in the debugger a URL based tree
> would be used.
>

Umm, the left sidemenuwidget containing the sources list ??

So many people have requested to convert that flat view into a nice tree like chrome has.

Also, the containsURL can be used in itchpad too.


> Regarding wasting the effort, we could always add a comment with a link to a
> version of the attachment that has the URL functionality so that we could
> grab it if/when needed.  The thing is that the API that we may be wanting to
> use for that may be different than what we imagine it will be right now.

Ok..

Comment 48

[Brian Grinstead [:bgrins]]

> This way appears to be very good, but it has a hidden catch. If we go by
> this way, the API for inserting urls and nodes in the tree (if not now, in
> future) will be completely different ..
> 
> normal ids and labels:
> 
> tree.add(['id1', {id:'id2',label:'label2', type: 'js';}, {id: 'id3',
> attachment: foobar}]);
> 
> url:
> 
> tree.add("http://google.com/javascript.js", {attachment: foobar});
> 
> DOM Nodes:
> 
> tree.add(['id1', node1, {id: 'id3', label: 'label3'}]);
> 
> This makes detecting the second index is actually a DOMNode requires
> instanceof operator :/
> 
> (as node1.id can be something, which makes us believe that it is an object,
> like in the third index of the passed array)


Maybe the object could only ever be a string or an object, and we could add props onto that object for these special cases.  So DOM nodes could use:

    tree.add(['id1', {id:'id2', node:node1}, {id: 'id3', label: 'label3'}]);

We could figure out the ideal interface for URLs later, but it could probably be serviced by a utility function like:

    tree.add(...TreeWidget.urlToIds("http://google.com/javascript.js"), {attachment: foobar});

Comment 49

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #47)
> (In reply to Brian Grinstead [:bgrins] from comment #46)
> 
> > What is the use case for containsURL / URL based trees?  You mention the
> > debugger, but I'm not sure exactly where in the debugger a URL based tree
> > would be used.
> >
> 
> Umm, the left sidemenuwidget containing the sources list ??
> 
> So many people have requested to convert that flat view into a nice tree
> like chrome has.

Gotcha, I was thinking of the current debugger UI and hadn't thought of modifying that into a tree.  We would have to work out what that UI would looks like, but I'm assuming that we would want it to only be split on host instead of split into all of the different parts.  Instead of:

https://developer.mozilla.org
  └ en-US
     └ docs
        └ Tools
          └ file1.js
        └ file2.js

It would be:

https://developer.mozilla.org
  └ file1.js
  └ file2.js

But rather than getting into that discussion here, I'm suggesting that we work that out in a new bug and make any needed modifications to the tree from there.

Comment 50

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #48)
> > This way appears to be very good, but it has a hidden catch. If we go by
> > this way, the API for inserting urls and nodes in the tree (if not now, in
> > future) will be completely different ..
> > 
> > normal ids and labels:
> > 
> > tree.add(['id1', {id:'id2',label:'label2', type: 'js';}, {id: 'id3',
> > attachment: foobar}]);
> > 
> > url:
> > 
> > tree.add("http://google.com/javascript.js", {attachment: foobar});
> > 
> > DOM Nodes:
> > 
> > tree.add(['id1', node1, {id: 'id3', label: 'label3'}]);
> > 
> > This makes detecting the second index is actually a DOMNode requires
> > instanceof operator :/
> > 
> > (as node1.id can be something, which makes us believe that it is an object,
> > like in the third index of the passed array)
> 
> 
> Maybe the object could only ever be a string or an object, and we could add
> props onto that object for these special cases.  So DOM nodes could use:
> 
>     tree.add(['id1', {id:'id2', node:node1}, {id: 'id3', label: 'label3'}]);
> 

Hmm, although a bit difficult to understand (like when to deliberately leave out the whole object and just put a string 'id1' ) but this could work.


> We could figure out the ideal interface for URLs later, but it could
> probably be serviced by a utility function like:
> 
>     tree.add(...TreeWidget.urlToIds("http://google.com/javascript.js"),
> {attachment: foobar});

As far as I can see, API for url is going to be different what so ever. It will have 2 arguments - (string, object) while others will have single argument - [object] .The utility thing is not required though, simply:

tree.add("url", {attachment: foobar});

Comment 51

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #50)
> (In reply to Brian Grinstead [:bgrins] from comment #48)
> > > This way appears to be very good, but it has a hidden catch. If we go by
> > > this way, the API for inserting urls and nodes in the tree (if not now, in
> > > future) will be completely different ..
> > > 
> > > normal ids and labels:
> > > 
> > > tree.add(['id1', {id:'id2',label:'label2', type: 'js';}, {id: 'id3',
> > > attachment: foobar}]);
> > > 
> > > url:
> > > 
> > > tree.add("http://google.com/javascript.js", {attachment: foobar});
> > > 
> > > DOM Nodes:
> > > 
> > > tree.add(['id1', node1, {id: 'id3', label: 'label3'}]);
> > > 
> > > This makes detecting the second index is actually a DOMNode requires
> > > instanceof operator :/
> > > 
> > > (as node1.id can be something, which makes us believe that it is an object,
> > > like in the third index of the passed array)
> > 
> > 
> > Maybe the object could only ever be a string or an object, and we could add
> > props onto that object for these special cases.  So DOM nodes could use:
> > 
> >     tree.add(['id1', {id:'id2', node:node1}, {id: 'id3', label: 'label3'}]);
> > 
> 
> Hmm, although a bit difficult to understand (like when to deliberately leave
> out the whole object and just put a string 'id1' ) but this could work.
> 

It's the same decision that you would have to make currently: Do I want a special label for this element?  If yes, then pass in an object (currently pass in [[ids],[labels]]).  If no, then just pass in an array of ids (currently pass in [[ids]]).

> > We could figure out the ideal interface for URLs later, but it could
> > probably be serviced by a utility function like:
> > 
> >     tree.add(...TreeWidget.urlToIds("http://google.com/javascript.js"),
> > {attachment: foobar});
> 
> As far as I can see, API for url is going to be different what so ever. It
> will have 2 arguments - (string, object) while others will have single
> argument - [object] .The utility thing is not required though, simply:
> 
> tree.add("url", {attachment: foobar});

We won't really know what the API for URLs should look like until we have a tool that wants to implement something URL-based.  If we decide that we want tree-like collapsing on the debugger sources list, for instance, then we can work it out then.  For now, if we remove this altogether it removes overloading and complexity that isn't needed.

Comment 52

[Girish Sharma [:Optimizer]]

Attachment: patch v0.7

- Divided the tree and table widget tests into three test files each
- Changed the TreeWidget.add API as per discussion
- Removed icons for css, html, txt, manifest and corresponding CSS and jar.mn entries
- Change the size of the folder icons to 16px as per discussion with Darrin

Comment 53

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #52)
> Created attachment 8413381 [details] [diff] [review]
> patch v0.7
> 
> - Divided the tree and table widget tests into three test files each
> - Changed the TreeWidget.add API as per discussion
> - Removed icons for css, html, txt, manifest and corresponding CSS and
> jar.mn entries
> - Change the size of the folder icons to 16px as per discussion with Darrin

Not sure if TreeWidget and TableWidget are the best names. Why not simply Table or Tree ?
Makes more sense to me like this :
var tree = new Tree();
var table = new Table();

Comment 54

[Victor Porof [:vporof][:vp]]

"widget" = something visual. "tree" by itself implies a data source or algorithm.

Comment 55

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #52)
> Created attachment 8413381 [details] [diff] [review]
> patch v0.7
> 
> - Divided the tree and table widget tests into three test files each
> - Changed the TreeWidget.add API as per discussion
> - Removed icons for css, html, txt, manifest and corresponding CSS and
> jar.mn entries
> - Change the size of the folder icons to 16px as per discussion with Darrin

Great, thanks.  Is there a new patch for the storage inspector frontend in bug 970517 that I can apply for testing?

Comment 56

[Girish Sharma [:Optimizer]]

I'll upload one soon. Although , the functionality is exactly the same as per the storage inspector, so for just testing it out, previous version of patch should also suffice.

Comment 57

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #56)
> I'll upload one soon. Although , the functionality is exactly the same as
> per the storage inspector, so for just testing it out, previous version of
> patch should also suffice.

Applying the patches on top of each other fails on the jar.mn files.  Even after resolving this, the storage panel is still blank - I'm guessing because the API for the TreeWidget has changed?

Comment 58

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #57)
> (In reply to Girish Sharma [:Optimizer] from comment #56)
> > I'll upload one soon. Although , the functionality is exactly the same as
> > per the storage inspector, so for just testing it out, previous version of
> > patch should also suffice.
> 
> Applying the patches on top of each other fails on the jar.mn files.  Even
> after resolving this, the storage panel is still blank - I'm guessing
> because the API for the TreeWidget has changed?

I meant previous version of widgets patch :)

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8413381 [details] [diff] [review]
patch v0.7

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

This is looking good, just some minor notes here.  Once you have a new version of the storage patch I'd like to play with them applied together.  Also, can you push to try?

::: browser/devtools/shared/widgets/TableWidget.js
@@ +396,5 @@
> +  sortBy: function(column) {
> +    this.emit(EVENTS.COLUMN_SORTED, column);
> +    this.sortedOn = column;
> +
> +    if (!this.items || !this.items.size) {

Don't think this.items would be falsy here (it is initialized in the constructor).  All the other functions access it without checking

@@ +714,5 @@
> +    this.cells[index].value = item[this.id];
> +  },
> +
> +  /**
> +   * Updates the `this.items` id vs index map.

Please add more detail to this comment.  We've discussed the reasoning here, but could you add some info about the purpose of this.items and when _itemsDirty should be true.  Also, I'm not sure if `this.items` is the best name for what this is doing - would it be better if it indicated that it was meant for lookups in the cell array?

::: browser/devtools/shared/widgets/TreeWidget.js
@@ +215,5 @@
> +  clearSelection: function() {
> +    this.selectedItem = -1;
> +  },
> +
> +  /**

We can remove the urlToIds function for now - it is not used

@@ +519,5 @@
> +   * subtree. Otherwise, call the remove method of appropriate child. This
> +   * recursive method goes on till we have reached the end of the branch or the
> +   * current item is to be removed.
> +   *
> +   * @param {array} item

Nit: use plural "items" for this param for consistency

@@ +539,5 @@
> +  /**
> +   * If this item is to be selected, then selected and expands the item.
> +   * Otherwise, if a child item is to be selected, just expands this item.
> +   *
> +   * @param {array} item

Nit: use plural "items" for this param for consistency

Comment 60

[Girish Sharma [:Optimizer]]

Attachment: patch v0.8

Addressed latest review comments.

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

(Front end patch is also updated now)

Comment 61

[Brian Grinstead [:bgrins]]

Comment on attachment 8416181 [details] [diff] [review]
patch v0.8

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

Can you please rebase this patch?  I'm getting conflicts on the jar.mn files.  Also, update the commit message

Comment 62

[Girish Sharma [:Optimizer]]

Attachment: rebased 0.8

(I've chosen the worst possible location for the jar.mn entries)

Comment 63

[Brian Grinstead [:bgrins]]

Comment on attachment 8417461 [details] [diff] [review]
rebased 0.8

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

Please fix the merge error on windows/jar.mn on tip.

I'll also note that I'm not a fan of the UX when clicking on a tree item - it always toggles the children under the tree when selecting a item.  This is frustrating when trying to navigate a nested structure because things keep collapsing below you.  My suggestion would be that it auto-opens children when selecting an item, but does not auto-close.  This could be handled as a follow up if you'd like - I'm sure it will come up in the review of the storage inspector.

::: browser/themes/shared/devtools/widgets.inc.css
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* Various substitutions used by various widgets */

This comment can be removed

@@ +1029,5 @@
> +  overflow: auto;
> +}
> +
> +.theme-light .table-widget-body {
> +  background: #F7F7F7 !important; /* Background-Sidebar, !important because

You shouldn't need !important here.  .theme-body background color is set in the light theme but it isn't the selector prefixed with .theme-light so this rule will have priority.

@@ +1095,5 @@
> +  background: rgba(0,0,0,0.15);
> +}
> +
> +.table-widget-column-header:not(:active)[sorted=ascending] {
> +  background-image: radial-gradient(farthest-side at center top, hsla(200,100%,70%,.7), hsla(200,100%,70%,0.3));

Please leave comment that this gradient is taken from netmonitor.inc.css.  The headers (and sorting visuals) are changing for the netmonitor in Bug 951714, so we will want to keep this CSS in mind when it happens

@@ +1111,5 @@
> +/* Cells */
> +
> +.table-widget-cell {
> +  width: 100%;
> +  margin: -1px 0 !important;

Why is it -1px for top and bottom margin?  And why does it need to be important?

@@ +1261,5 @@
> +}
> +
> +.tree-widget-item[expanded] + ul {
> +  transition: max-height 0.3s ease-in;
> +  max-height: 1000px;

What happens if the contents of this ul is > 1000px?  Are we allowing scrolling inside of this element?

Comment 64

[Paul Rouget [:paul]]

For keybindings (arrow keys) and unfold/fold behavior (what to unfold when clicking twisty), it would be great if we could reproduce the native experience of a xul:tree (see the bookmarks and history sidebars).

Not sure if this widget supports behaviors like this for example:

+ fold1
+ fold2

click fold1 twisty:

- fold1
  - leaf
  - leaf
  + fold1.1
  + fold1.2
+ fold2

click fold1.1 twisty:

- fold1
  - leaf
  - leaf
  - fold1.1
    - leaf
    - leaf
  + fold1.2
+ fold2

click fold 1 twisty:

+ fold1
+ fold2

click fold 1 twisty again:

- fold1
  - leaf
  - leaf
  - fold1.1
    - leaf
    - leaf
  + fold1.2
+ fold2


See how fold1.1 stays open?

Comment 65

[Girish Sharma [:Optimizer]]

It does :)

Comment 66

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

Comment on attachment 8417461 [details] [diff] [review]
rebased 0.8

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

Just a couple of things I noticed while working with the TreeWidget.

::: browser/devtools/shared/widgets/TreeWidget.js
@@ +251,5 @@
> +        this.attachments.set(JSON.stringify(
> +          items.slice(0, i + 1).map(item => item.id || item)
> +        ), items[i].attachment);
> +      }
> +    }

We should remove the placeholder text (empty text node) if it exists when we add an item. In addition, add back the placeholder text when we clear the tree.

@@ +488,5 @@
> +    if (nextSibling) {
> +      this.children.insertBefore(treeItem.node, nextSibling.node);
> +    } else {
> +      this.children.appendChild(treeItem.node);
> +    }

We should have an option to specify whether or not the tree items should be sorted or not.

Comment 67

[Girish Sharma [:Optimizer]]

(In reply to Gabriel Luong (:gluong) from comment #66)
> Comment on attachment 8417461 [details] [diff] [review]
> rebased 0.8
> 
> Review of attachment 8417461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a couple of things I noticed while working with the TreeWidget.
> 
> ::: browser/devtools/shared/widgets/TreeWidget.js
> @@ +251,5 @@
> > +        this.attachments.set(JSON.stringify(
> > +          items.slice(0, i + 1).map(item => item.id || item)
> > +        ), items[i].attachment);
> > +      }
> > +    }
> 
> We should remove the placeholder text (empty text node) if it exists when we
> add an item. In addition, add back the placeholder text when we clear the
> tree.

I am handling that through CSS, so no need to remove and add it back

> @@ +488,5 @@
> > +    if (nextSibling) {
> > +      this.children.insertBefore(treeItem.node, nextSibling.node);
> > +    } else {
> > +      this.children.appendChild(treeItem.node);
> > +    }
> 
> We should have an option to specify whether or not the tree items should be
> sorted or not.

Umm, That would be a bit clashing to the way the widget is written. Maybe in future a push mechanism can be added. But a tree is sorted by its nature.

Comment 68

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

(In reply to Girish Sharma [:Optimizer] from comment #67)
> (In reply to Gabriel Luong (:gluong) from comment #66)
> > Comment on attachment 8417461 [details] [diff] [review]
> > rebased 0.8
> > 
> > Review of attachment 8417461 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Just a couple of things I noticed while working with the TreeWidget.
> > 
> > ::: browser/devtools/shared/widgets/TreeWidget.js
> > @@ +251,5 @@
> > > +        this.attachments.set(JSON.stringify(
> > > +          items.slice(0, i + 1).map(item => item.id || item)
> > > +        ), items[i].attachment);
> > > +      }
> > > +    }
> > 
> > We should remove the placeholder text (empty text node) if it exists when we
> > add an item. In addition, add back the placeholder text when we clear the
> > tree.
> 
> I am handling that through CSS, so no need to remove and add it back

Ah, that was what I saw looking through the css, but only the table version of "tree-widget-empty-text" exists, which is why I believe the placeholder is not hiding for the TreeWidget. 

> > @@ +488,5 @@
> > > +    if (nextSibling) {
> > > +      this.children.insertBefore(treeItem.node, nextSibling.node);
> > > +    } else {
> > > +      this.children.appendChild(treeItem.node);
> > > +    }
> > 
> > We should have an option to specify whether or not the tree items should be
> > sorted or not.
> 
> Umm, That would be a bit clashing to the way the widget is written. Maybe in
> future a push mechanism can be added. But a tree is sorted by its nature.

I agree, however, it made more sense for me to see it in the order presented in the file for the source outline view. I also have toggle-able sorted trees in mind for later iterations.

Comment 69

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #63)
> Comment on attachment 8417461 [details] [diff] [review]
> rebased 0.8
> 
> Review of attachment 8417461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please fix the merge error on windows/jar.mn on tip.
> 
> I'll also note that I'm not a fan of the UX when clicking on a tree item -
> it always toggles the children under the tree when selecting a item.  This
> is frustrating when trying to navigate a nested structure because things
> keep collapsing below you.  My suggestion would be that it auto-opens
> children when selecting an item, but does not auto-close.  This could be
> handled as a follow up if you'd like - I'm sure it will come up in the
> review of the storage inspector.

I thought about it a bit, and then saw that all other tree in the browser behaves like this only, so I am leaving this as is. Maybe in future, we will separate out the twisty in its own element so that single clicking the element will just select and clicking the twisty/ double clicking the element will collapse/expand

Comment 70

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #63)
> Comment on attachment 8417461 [details] [diff] [review]
> rebased 0.8
> 
> Review of attachment 8417461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please fix the merge error on windows/jar.mn on tip.
> 
> I'll also note that I'm not a fan of the UX when clicking on a tree item -
> it always toggles the children under the tree when selecting a item.  This
> is frustrating when trying to navigate a nested structure because things
> keep collapsing below you.  My suggestion would be that it auto-opens
> children when selecting an item, but does not auto-close.  This could be
> handled as a follow up if you'd like - I'm sure it will come up in the
> review of the storage inspector.
> 
> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +2,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +/* Various substitutions used by various widgets */
> 
> This comment can be removed

Done.

> @@ +1029,5 @@
> > +  overflow: auto;
> > +}
> > +
> > +.theme-light .table-widget-body {
> > +  background: #F7F7F7 !important; /* Background-Sidebar, !important because
> 
> You shouldn't need !important here.  .theme-body background color is set in
> the light theme but it isn't the selector prefixed with .theme-light so this
> rule will have priority.

Done 

> @@ +1095,5 @@
> > +  background: rgba(0,0,0,0.15);
> > +}
> > +
> > +.table-widget-column-header:not(:active)[sorted=ascending] {
> > +  background-image: radial-gradient(farthest-side at center top, hsla(200,100%,70%,.7), hsla(200,100%,70%,0.3));
> 
> Please leave comment that this gradient is taken from netmonitor.inc.css. 
> The headers (and sorting visuals) are changing for the netmonitor in Bug
> 951714, so we will want to keep this CSS in mind when it happens

Done
 
> @@ +1111,5 @@
> > +/* Cells */
> > +
> > +.table-widget-cell {
> > +  width: 100%;
> > +  margin: -1px 0 !important;
> 
> Why is it -1px for top and bottom margin?  And why does it need to be
> important?

The -1 is taken from netmonitor. !important because otherwise global.css rule of margin: 0 overrides the -1

> @@ +1261,5 @@
> > +}
> > +
> > +.tree-widget-item[expanded] + ul {
> > +  transition: max-height 0.3s ease-in;
> > +  max-height: 1000px;
> 
> What happens if the contents of this ul is > 1000px?  Are we allowing
> scrolling inside of this element?

Converted it into an animation with the final result of max-height: unset so that there is no limit.

Comment 71

[Girish Sharma [:Optimizer]]

Attachment: widgets v0.9

Should address all review comments.
I also took care of empty text in tree widget.

try : https://tbpl.mozilla.org/?tree=Try&rev=3d8046573c0a

Comment 72

[Brian Grinstead [:bgrins]]

Comment on attachment 8425110 [details] [diff] [review]
widgets v0.9

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

Looks like there are some linux dt failures in your latest push, but the code changes look fine

Comment 73

[Girish Sharma [:Optimizer]]

yeah, those are from the storage patch. here is an isolated widgets only push : https://tbpl.mozilla.org/?tree=Try&rev=8c3b77456933

Comment 74

[Girish Sharma [:Optimizer]]

Attachment: patch v1

did a very minor css change of adding word-break: keep-all to prevent some urls with breaking characters to flow to net lines.

landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/70eebd17c567

Comment 75

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70eebd17c567

Comment 76

[Victor Porof [:vporof][:vp]]

So any items after level 6 won't be indented?
http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/widgets.inc.css?from=widgets.inc.css#1503

...

Comment 77

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #76)
> So any items after level 6 won't be indented?
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> widgets.inc.css?from=widgets.inc.css#1503
> 
> ...

Yup, until attr is supported on non-pseudo-element and non-content property that is.

Comment 78

[Victor Porof [:vporof][:vp]]

Any reason why the indentation couldn't just be computed in js, like in every other table? I had to write my own table widget for the new profiler frontend because of this implementation. Between that and adding the surrounding UI for times and percentages, just creating a new widget was faster.

Comment 79

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #78)
> Any reason why the indentation couldn't just be computed in js, like in
> every other table? I had to write my own table widget for the new profiler
> frontend because of this implementation. Between that and adding the
> surrounding UI for times and percentages, just creating a new widget was
> faster.

It can very well be computed in JS. More than 6 level was just not part of the requirement at the time of creating this widget (for storage inspector).

But.. what do you mean by a table, times and percentages ?

I am pretty sure a small patch would have enabled the dynamic computation of padding/indentation and you could have easily used the tree widget.

Comment 80

[Victor Porof [:vporof][:vp]]

The call tree in the profiler has both a tree and additional information surrounding it and every tree item. Since this was supposed to be a *generic* TreeWidget, I would've expected indentation to work above level 6.

Comment 81

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #80)
> The call tree in the profiler has both a tree and additional information
> surrounding it and every tree item. Since this was supposed to be a

Ah.. well that use case is definitely different, but happy to accept patches to make it work for you (if possible).


> surrounding it and every tree item. Since this was supposed to be a
> *generic* TreeWidget, I would've expected indentation to work above level 6.

Supposed to be - yes. Due to lack of time I just made a bare minimum, as required by Storage Inspector. This does not mean we can keep on working on this widget to make it more awesome and closer to a generic all purpose Tree Widget :)

Please file bugs for any feature request that you see fit.

Comment 82

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #78)
> Any reason why the indentation couldn't just be computed in js, like in
> every other table? I had to write my own table widget for the new profiler
> frontend because of this implementation. Between that and adding the
> surrounding UI for times and percentages, just creating a new widget was
> faster.

If it is just a few changes needed to use this implementation it seems like it would be good to standardize on one tree widget so we have less code / maintenance, esp for things like keyboard / RTL support and UX conventions.

Though I'm looking at the screencap for the profiler (https://bugzilla.mozilla.org/attachment.cgi?id=8447297) and it looks like there is some integration between a table and a tree, where collapsing the tree item most likely removes the parent table row.  I'm not sure how easy that would be to do using the TableWidget/TreeWidget (maybe this is what you mean by it being faster to make a new widget?).

Comment 83

[Victor Porof [:vporof][:vp]]

Yes, it seemed that the current TreeWidget's capabilities weren't enough for the Profiler's use case, and the way it's implemented made it hard to both extend and adapt quickly and in an elegant way.

Comment 84

[Victor Porof [:vporof][:vp]]

Unrelated, but now that I'm taking a closer look at this I'm seeing a few very odd things going on in the code, like:

>    if (!typeof id == "array") {
>      return;
>    }

AFAIK, in JavaScript, there is no typeof operation that returns "array". `typeof []` == "object".

Comment 85

[Girish Sharma [:Optimizer]]

Looking at the gif from comment #82 , I would say that the tree widget could have never done that. The items in the tree are vertically linked, they cannot be linked horizontally with external items (items that reside in other columns of the surrounding table).

It was probably a good decision to build something new for such a unique use case.

I also want to have a single implementation of a tree/table widget, but only where it fits well. For instance the tree in WebIDE can be replace with this tree etc..

Comment 86

[Victor Porof [:vporof][:vp]]

If you take a look at the Profiler's implementation, there is no a table, nor two widgets playing nicely together, but just a single and simple tree (with a single type of node). The only "unique" property of all nodes in the tree is that the expando arrows aren't necessarily on the leftmost side, and the horizontal indentation isn't necessarily applied to the leftmost node. That's it. But I digress :)

Comment 87

[Girish Sharma [:Optimizer]]

If you feel like your tree implementation is the way to go, I'd be more than happy to scrap this one and adopt to yours. I have always hated duplication of code and effort.

Is your tree in fx-team or still in patches ? I want to look at the API it exposes to check my use case.

Comment 88

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

(In reply to Girish Sharma [:Optimizer] from comment #87)
> If you feel like your tree implementation is the way to go, I'd be more than
> happy to scrap this one and adopt to yours. I have always hated duplication
> of code and effort.
> 
> Is your tree in fx-team or still in patches ? I want to look at the API it
> exposes to check my use case.

Ouch, that would set my source outline back a little bit, but I would like to look at the API and also see how it would fit for my use case.

Comment 89

[Victor Porof [:vporof][:vp]]

(In reply to Gabriel Luong (:gl) from comment #88)
> 
> Ouch, that would set my source outline back a little bit, but I would like
> to look at the API and also see how it would fit for my use case.

Yeah, let's not do this for now.

Comment 90

[Girish Sharma [:Optimizer]]

From the looks of https://bugzilla.mozilla.org/attachment.cgi?id=8447371 the utils/tree.js is currently tightly coupled to the notion of Frame and Thread and assumes certain properties on those objects. If the patch is already in development and we want that tree to be the one tree implementation in devtoosl codebase, then it would be better to make it a bit more generic.

But if it is too much of a change, don't sweat it. We want that profiler to land ASAP :)

Comment 91

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #90)
> From the looks of https://bugzilla.mozilla.org/attachment.cgi?id=8447371 the
> utils/tree.js is currently tightly coupled to the notion of Frame and Thread
> and assumes certain properties on those objects. If the patch is already in
> development and we want that tree to be the one tree implementation in
> devtoosl codebase, then it would be better to make it a bit more generic.
> 
> But if it is too much of a change, don't sweat it. We want that profiler to
> land ASAP :)

Close. The only UI implementation in utils/thread.js is the CallView (everything else is just profiler data manipulation). A CallView is a tree node view which is abstract enough to not care about anything. Making it more generic would be trivial, simply taking out the actual UI creation outside of `attachTo`, and making that function take an already created node instead.

However, I'm not sure if there's any immediate benefit to doing that, but filing a new bug and taking this discussion there might be a good idea.

Comment 92

[Victor Porof [:vporof][:vp]]

(In reply to Gabriel Luong (:gl) from comment #88)
> (In reply to Girish Sharma [:Optimizer] from comment #87)
> 
> Ouch, that would set my source outline back a little bit, but I would like
> to look at the API and also see how it would fit for my use case.

Ok, since there have been strong requests for it, I made the tree mechanism from bug 879008 generic. Whoever's interested, take a look at comment 60 in there for additional details,

Comment 93

[Girish Sharma [:Optimizer]]

After looking at the usage of CallView and this new AbstractTreeItem, I am wondering a bit. Since you handle the creation of the tree item yourself and the indentation is also handled by you yourself, you could have easily used the TreeWidget from this bug. You just needed to override the original indentation classes and do something like:

tree.add({parentId, child1Id, {id: child2Id, node: customNode});

where customNode is of the following structure:

<div display:inline><div .timeDiv /><div .costDiv /><div .callDiv/><div .callViewDiv/></div>

Something like what your attachTo method was doing, but you would have needed to do it outside of the treeWidget, before calling the .add method of the tree.

And then a simple method addition in the tree to handle the indentation dynamically through JS would have done the job for you as I am also maintaining level and other information about parent , etc in the TreeItem.

Right now, except for the implementation part of it, AbstractTreeView is almost similar to TreeWidget.

Comment 94

[Victor Porof [:vporof][:vp]]

The AbstractTreeView's implementation is much, much lighter than the TreeWidget. Compare the `focus` method vs. the `selectedItem` setter, or `_getSiblingAtDelta` vs. `getNextVisibleItem` for example in the two implementations. I don't really know why the TreeWidget's internal structure ended up being so complicated, but since performance is very important in the profiler, where the tree can end up handling massive amounts of data, I'm going to use the AbstractTreeView instead.

Please fix comment 84 in a followup, or make it a good first bug :)

Comment 95

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #94)
> The AbstractTreeView's implementation is much, much lighter than the
> TreeWidget.

Its only your point of view of comparisons. After looking at the API exposed by AbstractTreeView, its clear that some operations of ATV are faster that TW and vice versa

> Compare the `focus` method vs. the `selectedItem` setter, or

These two methods are not even doing the same thing.

selectedItem setter has to make sure that the item is visible, thus expanding all the parents. While focus is simply focusing the node. In fact, your focus method cannot even be called for a currently unexpanded node. The best comparison of selectedItem setter is your expand method in which you add all the dom nodes for a parent to expand it.

Its just a matter of implementation, your expand method is super heavy as compared to my expand method which has to just toggle an attribute. And for you to show an hidden/unexpanded element out of the blue, you will have to construct and add the corresponding DOM elements for that element and all the hidden parents up the hierarchy .

> `_getSiblingAtDelta` vs. `getNextVisibleItem` for example in the two

Again, its just a matter of choosing what to make fast. TreeWidget is supposed to emit events and data related to any selected item which is used by other modules (of storage inspector) so that part is super efficient in TreeWidget, but AbstractTreeView does not care about that, its structure is different, it does not have all the nodes in the DOM tree all the time and it has to construct and add/delete them on the fly. Thus you are able to make the getSiblingAtDelta that simple, but if all nodes are in DOM all the time, there is no easier way.

One might say that adding all nodes at once is a bit heavier, and if that is the case for Profiler (huge amount of branching etc) then it can be optimized in the TreeWidget.

> implementations. I don't really know why the TreeWidget's internal structure
> ended up being so complicated, but since performance is very important in

I have no idea why you say that but what I can see, the final DOM structure of TreeWidget is pretty light

<li>
  <div> parent display label goes here</div>
  <ul>
    <li> <repeat> </li>
    <li> <repeat> </li>
  </ul>
</li>

and thus things like insertion/deletion expanding etc become super light.

Pretty sure if you have brought this up before writing the ATV, we would have worked things out and made TreeWidget even more better. But now I won't stop you from using whatever you feel like is good.

> Please fix comment 84 in a followup, or make it a good first bug :)

Sure, will do as part of front end for storage inspector

Comment 96

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #95)
> (In reply to Victor Porof [:vporof][:vp] from comment #94)
> 
> I have no idea why you say that but what I can see, the final DOM structure
> of TreeWidget is pretty light
> 
> <li>
>   <div> parent display label goes here</div>
>   <ul>
>     <li> <repeat> </li>
>     <li> <repeat> </li>
>   </ul>
> </li>
> 

I'm not talking about rendering performance at all, but about tree creation and interaction methods (selection, focus, navigation etc.), which you admitted yourself are quite heavy in the TreeWidget. I would assume the rendering performance is similar if not exactly the same between the two implementations.

Also, for example, with your implementation, having different odd|even items backgrounds in the tree would be very costly, as the structure isn't flat so you can't just use :nth-child selectors. Computing different backgrounds every time an item is inserted would be fantastically slow in the profiler's particular scenario.

The only thing I've been trying to say the past few comments is that it would've been nice if the TreeWidget implementation would've been abstract enough to handle the use-cases I've hinted at above. As it stands, too many assumptions are made in the TreeWidget, and modifying the existing code to remove them would've been dirty and inefficient from my perspective.

One (more productive) solution for unifying all our tree implementations would be for a TreeWidget to be built on top of the AbstractTreeItem class, so that consumers who don't care too much about customization (and just want to insert labels in a tree) would use that. But that's just a friendly suggestion that I personally think is the best approach in the long run.

I'm done here.

Comment 97

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #96)
> (In reply to Girish Sharma [:Optimizer] from comment #95)
> > (In reply to Victor Porof [:vporof][:vp] from comment #94)
> > 
> > I have no idea why you say that but what I can see, the final DOM structure
> > of TreeWidget is pretty light
> > 
> > <li>
> >   <div> parent display label goes here</div>
> >   <ul>
> >     <li> <repeat> </li>
> >     <li> <repeat> </li>
> >   </ul>
> > </li>
> > 
> 
> I'm not talking about rendering performance at all, but about tree creation
> and interaction methods (selection, focus, navigation etc.), which you
> admitted yourself are quite heavy in the TreeWidget. I would assume the
> rendering performance is similar if not exactly the same between the two
> implementations.
> 

Please read comment 95 again. Somethings are heavier some are lighter, for instance, interaction method like expanding a parent is super light in TreeWidget while heavy in ATV. Navigation is conditionally heavy for edge cases, otherwise light. Insertion, even though not being linear, is pretty fast too.

And whatever heaviness is being talked here might not even matter in all its practicality. 

> Also, for example, with your implementation, having different odd|even items
> backgrounds in the tree would be very costly, as the structure isn't flat so
> you can't just use :nth-child selectors. Computing different backgrounds
> every time an item is inserted would be fantastically slow in the profiler's
> particular scenario.

I agree, but given that each node is of fixed height, that can be very easily achieved by a striped background. Pure CSS.

> The only thing I've been trying to say the past few comments is that it
> would've been nice if the TreeWidget implementation would've been abstract
> enough to handle the use-cases I've hinted at above. As it stands, too many
> assumptions are made in the TreeWidget, and modifying the existing code to
> remove them would've been dirty and inefficient from my perspective.

These use cases were not existing at the point when I was writing the implementation and all the assumptions were made wrt using TreeWidget as a source tree and not a flat tablish tree.
Again, we could have come to a better solution if this discussion happened before you created a new tree implementation of your own.

> One (more productive) solution for unifying all our tree implementations
> would be for a TreeWidget to be built on top of the AbstractTreeItem class,
> so that consumers who don't care too much about customization (and just want
> to insert labels in a tree) would use that. But that's just a friendly
> suggestion that I personally think is the best approach in the long run.

Feel free to do that. I just care about the tree's functionality.

Comment 98

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #97)

> Please read comment 95 again. Somethings are heavier some are lighter, for
> instance, interaction method like expanding a parent is super light in
> TreeWidget while heavy in ATV.

You keep saying "some things", but it's apparently only this particular case, which is equally heavy the first time a node is expanded in both implementations :) Everything else is heavier with the TreeWidget.

> I agree, but given that each node is of fixed height, that can be very
> easily achieved by a striped background. Pure CSS. 

Assumptions, assumptions, assumptions...

> > One (more productive) solution for unifying all our tree implementations
> > would be for a TreeWidget to be built on top of the AbstractTreeItem class,
> > so that consumers who don't care too much about customization (and just want
> > to insert labels in a tree) would use that. But that's just a friendly
> > suggestion that I personally think is the best approach in the long run.
> 
> Feel free to do that. I just care about the tree's functionality.

Comment 87 and comment 82 seem to have a different premise, which was the whole point of this discussion.

Comment 99

[Girish Sharma [:Optimizer]]

At this point , it just looks like that you are trying to prove a point that you write better code.

I have already said a multiple comments that I am happy to adopt to whatever final tree widget is chosen and that is why i started to look into the API of your tree implementation. At that point I found that feature wise, you could have easily used TreeWidget instead of creating a new widget. At that point, you started to compare totally different methods wrt performance and pin point areas where TreeWidget is slower and what not.

Again, if ATV provides all API for my requirement and also for a requirement of a generic tree implementation, I am happy to switch to it. Right now, I cannot find APIs to

 - Select a particular node given its unique id / reference
 - Know if a particular node is selected
 - Get notified when a node is selected

Also, to do things like expanding a node, focusing a node etc. you need to first get hold of that node (CallView object in your case), there is no method to simply say

tree.select(<path to the node / unique id of the node>)

(These missing API are probably because profiler does not need these methods .. The same set of assumptions that I took while writing TreeWidget for Storage Inspector's use.)

If these APIs are provided in the ATV then lets all use that everywhere.

Lets refrain from commenting more on this bug. Please open a new bug for unification of Tree Widget implementations and lets discuss there.

Comment 100

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #99)
> At this point , it just looks like that you are trying to prove a point that
> you write better code.
> 

:) That is very far from the truth, I'm just trying to be pragmatic and find a solution for the long run. Filed bug 1031692 for potentially continuing discussion, and I'm hoping that you won't take this personally.