[webkit-reviews] review granted: [Bug 19120] Add a DataGrid object for Databases and Profiles to share : [Attachment 21245] Revised patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 19120: Add a DataGrid object for Databases and Profiles to share
http://bugs.webkit.org/show_bug.cgi?id=19120

Attachment 21245: Revised patch
http://bugs.webkit.org/attachment.cgi?id=21245&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+    this._headerTable = document.createElement("table");
+    this._headerTable.className = "header";
+
+    this._dataTable = document.createElement("table");
+    this._dataTable.className = "data";

Perhaps "data-grid-header" and "data-grid-data" would be better class names? We
could also use "datagrid" instead of "data-grid" in all cases.

+    removeChildren: function()
+    {
+	 for (var i = 0; i < this.children.length; ++i) {

Can just be:

for (var i in this.children) {

+    removeChildrenRecursive: function()
+    {
+	 var childrenToRemove = this.children;
+
+	 var child = this.children[0];
+	 while (child) {
+	     if (child.children.length)
+		 childrenToRemove = childrenToRemove.concat(child.children);
+	     child = child.traverseNextNode(false, this, true);
+	 }

Might be clearer as a for loop.

+	 for (var i = 0; i < childrenToRemove.length; ++i) {
+	     var child = childrenToRemove[i];
+	     child.deselect();
+	     child._detach();
+
+	     child.children = [];
+	     child.dataGrid = null;
+	     child.parent = null;
+	     child.nextSibling = null;
+	     child.previousSibling = null;
+	 }

Can use for-in here.
Can we share this code with removeChildren somehow? A helper function should do
the trick.

+	 var currentAncestor = this.parent;
+	 while (currentAncestor && !currentAncestor.root) {
+	     if (!currentAncestor.expanded) {
+		 this._revealed = false;
+		 return false;
+	     }
+
+	     currentAncestor = currentAncestor.parent;
+	 }

Might be clearer as a for loop.

+	 for (var i = 0; i < this.children.length; ++i)
+	     this.children[i].revealed = x && this.expanded;

Can use for-in here.

+    get shouldRefreshChildren() {
+	 return this._shouldRefreshChildren;
+    },
+
+    set shouldRefreshChildren(x) {
+	 this._shouldRefreshChildren = x;
+	 if (x && this.expanded)
+	     this.expand();
+    },

Opening braces should be on the next line.
You should check that the value is changing in shouldRefreshChildren.

+	 for (var columnIdentifier in this.dataGrid.columns) {
+	     var cell = this.createCell(columnIdentifier);
+	     this._element.appendChild(cell);
+	 }

These four lines are repeated a few times. Should we put them into a helper
function?

+    createCell: function(columnIdentifier) {

Opening brace should be on the next line.

+	 for (var i = 0; i < this.children.length; ++i)
+	     this.children[i].revealed = false;

Can use for-in. (I'm going to stop saying this for every instance of array
iteration, but you could do it everywhere.)

+	 this.dispatchEventToListeners("collapsed");

Should we use more specific event names? Like "datagrid-collapsed"?

+    collapseRecursively: function()
+    {
+	 var item = this;
+	 while (item) {
+	     if (item.expanded)
+		 item.collapse();
+	     item = item.traverseNextNode(false, this, true);
+	 }
+    },

Could use a for loop. (I'm going to stop saying this for every while loop like
this, but you could switch to for loops in other places, too.)

In traverseNextNode, you have a number of lines that look like this:

skipHidden ? (revealed ? someNode : null) : someNode

These could all be simplified like this:

(!skipHidden || revealed) ? someNode : null

A similar comment applies to traversePreviousNode.

+	 if (node && (!skipHidden || (skipHidden && this.expanded))) {

This can just be:

if (node && (!skipHidden || this.expanded)) {

+    isEventWithinDisclosureTriangle: function(event)
+    {
+	 var cell = event.target.enclosingNodeOrSelfWithNodeName("td");
+	 if (!cell.hasStyleClass("disclosure"))
+	     return false;
+	 var computedLeftPadding =
window.getComputedStyle(cell).getPropertyCSSValue("padding-left").getFloatValue
(CSSPrimitiveValue.CSS_PX);
+	 var left = cell.totalOffsetLeft + computedLeftPadding;
+	 return event.pageX >= left && event.pageX <= left +
this.disclosureToggleWidth && this.hasChildren;
+    },

Maybe we should return false early if this.hasChildren is false.

+		 if (text.length > columns[columnIdentifier].width)
+		     columns[columnIdentifier].width = text.length;

This can be:

columns[columnIdentifier].width = Math.max(text.length,
columns[columnIdentifier].width);

r=me