Bug 19156 - Inspector should support console.dirXML
Summary: Inspector should support console.dirXML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 14354 19871
  Show dependency treegraph
 
Reported: 2008-05-20 16:15 PDT by Adam Roben (:aroben)
Modified: 2008-09-07 09:49 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (10.77 KB, patch)
2008-08-16 08:49 PDT, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
screenshot (169.02 KB, image/png)
2008-08-16 08:52 PDT, Keishi Hattori
no flags Details
proposed patch (58.51 KB, patch)
2008-08-18 07:51 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
alt key fixed (59.02 KB, patch)
2008-08-18 08:10 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
patch (59.92 KB, patch)
2008-08-18 08:34 PDT, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
new patch (61.50 KB, patch)
2008-09-06 20:33 PDT, Keishi Hattori
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2008-05-20 16:15:28 PDT
The Inspector should support console.dirXML for Firebug parity.
Comment 1 Mark Rowe (bdash) 2008-05-20 16:23:04 PDT
<rdar://problem/5950860>
Comment 2 Keishi Hattori 2008-08-16 08:49:46 PDT
Created attachment 22832 [details]
proposed patch

Doesn't highlight so not very interactive, but working.
Comment 3 Keishi Hattori 2008-08-16 08:52:07 PDT
Created attachment 22833 [details]
screenshot
Comment 4 Timothy Hatcher 2008-08-16 21:22:41 PDT
Comment on attachment 22832 [details]
proposed patch

Too much coped code. We need to share this with the Element's DOM more. But the approach seems good!

I propose to factor out the TreeOutline code from ElementsPanel.js into a new file and make it a TreeOutline subclass, like ElementsTreeOutline or NodeTreeOutline. Then you might not need the ConsoleElementsInspector, since that class is very small I don't see the need,

Also Firebug shows the element that was passed , not just the children. So you need to show the body in your example.

For the record, I hate the "dir" names...
Comment 5 Keishi Hattori 2008-08-18 07:51:12 PDT
Created attachment 22850 [details]
proposed patch

I moved a lot of things around so I'm not sure if it's working properly. As far as I can tell the Alt key is broken but other than that seems to be working. 

The ElementsTreeOutline in the console is selectable but the z-index of highlight is -1 so it's hidden. Is there a good way to disable select in a TreeOutline?
Comment 6 Keishi Hattori 2008-08-18 07:54:57 PDT
Oh, and how do I add a js file to the build system?
Comment 7 Keishi Hattori 2008-08-18 08:10:09 PDT
Created attachment 22851 [details]
alt key fixed

Alt key works now.
Comment 8 Keishi Hattori 2008-08-18 08:34:50 PDT
Created attachment 22852 [details]
patch

Fixed inspect element from context menu and search.
Comment 9 Timothy Hatcher 2008-08-18 10:56:55 PDT
Comment on attachment 22852 [details]
patch

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
Comment 10 Timothy Hatcher 2008-08-18 11:26:15 PDT
Comment on attachment 22852 [details]
patch

Also one really important change to make!

+    updateTreeOutline: function()
+    {
+        this.removeChildrenRecursive();

This needs to be removeChildren(). removeChildrenRecursive() is doing more work than is needed here, and just removing the top level children is enough. This should be a good performance win!

I know it was removeChildrenRecursive in the original code, not sure why I did that!!
Comment 11 Keishi Hattori 2008-09-06 20:33:49 PDT
Created attachment 23223 [details]
new patch

I think moving all hover related code to WebInspector turned out great. Made the code simpler.
Comment 12 Timothy Hatcher 2008-09-07 08:53:58 PDT
Comment on attachment 23223 [details]
new patch

This is ready to land! Nice job!
Comment 13 Timothy Hatcher 2008-09-07 09:49:00 PDT
Landed in r36255.