[webkit-reviews] review denied: [Bug 19156] Inspector should support console.dirXML : [Attachment 22852] patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 19156: Inspector should support console.dirXML
https://bugs.webkit.org/show_bug.cgi?id=19156

Attachment 22852: patch
https://bugs.webkit.org/attachment.cgi?id=22852&action=edit

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
There are a few issues that are preventing this from landing.

The WebInspector.ElementsPanel constructor is indented 1 more space than it
needs to be, causing massive changed lines. Undo this so real changes can be
seen.

+     this.treeOutline._updateHoverHighlight = function() {
+	  if ("_updateHoverHighlightTimeout" in this) {
+	      clearTimeout(this._updateHoverHighlightTimeout);
+	      delete this._updateHoverHighlightTimeout;
+	  }
+
+	  if (this._hoveredDOMNode && this.panel.visible &&
(this.panel._altKeyDown || this.panel.forceHoverHighlight))
+	      InspectorController.highlightDOMNode(this._hoveredDOMNode);
+	  else
+	      InspectorController.hideDOMNodeHighlight();
+     },

Why do you need thsi when the same function is in ElementsTreeOutline? Also you
are ending the function with a coma, not a semicolon.

+     this.treeOutline.element.addEventListener("mousemove",
this._onmousemove.bind(this), false);
+     this.treeOutline.element.addEventListener("mouseout",
this._onmouseout.bind(this), false);

These event listeners can be removed and all of the _onmousemove and
_onmouseout functions should move to ElementsTreeOutline.

-	 this.updateTreeSelection();
+	 this.treeOutline.updateTreeSelection();

Rename this to updateSelection since the word tree is redundant now.

     reset: function()
     {
-	 this.rootDOMNode = null;
-	 this.focusedDOMNode = null;
-
	 this.altKeyDown = false;
-	 this.hoveredDOMNode = null;
	 this.forceHoverHighlight = false;

You still need to set rootDOMNode, focusedDOMNode and hoveredDOMNode to null
here.

     get forceHoverHighlight()
     set forceHoverHighlight()
     get altKeyDown()
     set altKeyDown()

These functions should move with set hoveredDOMNode. The more I think about the
hovered node code, it should live in inspector.js seperate from the tree
outline. That way code like the breadcrumbs and console messages can support
hover and use the shared functions to update the hovered node.

So move _updateHoverHighlight, _updateHoverHighlightSoon, get/set
hoveredDOMNode, get/set forceHoverHighlight, and get/set altKeyDown to
WebInspector.prototype in inspector.js.

Move _getDocumentForNode, _parentNodeOrFrameElement and
_isAncestorIncludingParentFrames to utilities.js, since they are move helper
functions and not good class methods. Drop the underscore prefix too.

     _onmousemove: function(event)
     {
-	 var element = this._treeElementFromEvent(event);
+	 var element = this.treeOutline._treeElementFromEvent(event);
	 if (!element)
	     return;
 
	 this.altKeyDown = event.altKey;
-	 this.hoveredDOMNode = element.representedObject;
	 this.forceHoverHighlight = element.selected;
     },
 
@@ -1091,332 +909,8 @@ WebInspector.ElementsPanel.prototype = {
	 if (event.target !== this.treeListElement)
	     return;
	 this.altKeyDown = false;
-	 this.hoveredDOMNode = null;
	 this.forceHoverHighlight = false;
     }
 }

These event listener methods should move entirely to ElementsTreeOutline.

To add the file to the Windows project and Qt project see:

http://trac.webkit.org/changeset/33408#file1
http://trac.webkit.org/changeset/33408#file7

Also you need to add the file to inspector.html, see:

http://trac.webkit.org/changeset/33408#file8