| Summary: | Web Inspector: CSS Regions: When a flow is clicked the content of flow needs to be displayed | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||
| Component: | Web Inspector | Assignee: | Alexandru Chiculita <achicu> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer, WebkitBugTracker | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | 123424 | ||||||||||||||
| Bug Blocks: | 57312, 122760 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Alexandru Chiculita
2013-10-16 16:48:49 PDT
Created attachment 216090 [details]
Patch V1
Comment on attachment 216090 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=216090&action=review r=me, though I'd like to see a screenshot attached to the bug. Are there any style concerns about the spacing / gap between multiple DOM Tree Outlines? > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:15 > + * Nit: Trailing whitespace here (I only know cause it showed up in other files you added). > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:44 > + contentFlow.addEventListener(WebInspector.ContentFlow.Event.ContentNodeWasAdded, this._onContentNodeWasAdded, this); > + contentFlow.addEventListener(WebInspector.ContentFlow.Event.ContentNodeWasRemoved, this._onContentNodeWasRemoved, this); Style: The "_onFoo" methods can just be "_foo". We tend to avoid the "on" where possible. Currently only really old code (DataGrid, TreeElement) have that naming. > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:50 > + constructor: WebInspector.ContentFlowTreeView, Style: Recent change! We are going to start putting __proto__ here, after constructor. For example: constructor: WebInspector.ContentFlowTreeView, __proto__: WebInspector.ContentView.prototype, > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:91 > + this._nodesMap[id].setVisible(true, WebInspector.isConsoleFocused()); Nit: Stash "isConsoleFocused" into a local variable outside the for loop to prevent potentially calling it multiple times. This is also a good opportunity to name it after the action, "var omitFocus = …". > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:104 > + this.representedObject.removeEventListener(WebInspector.ContentFlow.Event.ContentNodeWasAdded, this._onContentNodeWasAdded, this); > + this.representedObject.removeEventListener(WebInspector.ContentFlow.Event.ContentNodeWasRemoved, this._onContentNodeWasRemoved, this); If you update the function names, be sure to update them here too! > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:123 > + this._selectedTreeElement = selectedTreeElement; > + if (selectedTreeElement) > + ConsoleAgent.addInspectedNode(selectedTreeElement.representedObject.id); Hmm, we may need to be smarter about this. I'm imagining this scenario: 1. User opens the inspector by inspecting <body> -> $0 = <body> 2. User opens a ContentFlowTreeView with <div id="foo"> selected -> $0 = <div id="foo">, $1 = <body> 3. User switches back to the DOM Tree view -> no change 4. User sees <body> selected in the DOM Tree view and evaluates "$0" in the JS Console => unexpectedly gets <div id="foo">, expected <body> Does that sound like a problem? > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:146 > + this._nodesMap[node.id] = contentNodeOutline; Style: Please add a newline before and after this line. It helps visually separate the different tasks being performed. > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:156 > + for (var i = 0; i < contentNodes.length; ++i) { > + var contentNodeOutline = this._createContentNodeTree(contentNodes[i]); > + this.element.appendChild(contentNodeOutline.element); > + } You could be the first user of for..of loops in the inspector! Totally optional of course (but certain a cool factor) for (var contentNode of contentNodes) { ... } > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:174 > + var contentNodeOutline = this._nodesMap[event.data.node.id]; > + contentNodeOutline.close(); > + contentNodeOutline.element.remove(); You should delete the node map entry to avoid leaking it and looping over it in the loops above: delete this._nodesMap[event.data.node.id]; Comment on attachment 216090 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=216090&action=review > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:30 > +WebInspector.ContentFlowTreeView = function(contentFlow) We like to keep the base class name as the suffix. So this would be WebInspector.ContentFlowTreeContentView Comment on attachment 216090 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=216090&action=review > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:39 > + // Map of contentNode ids to DOMTreeOutline objects. > + this._nodesMap = {}; You should consider using Map. this._nodesMap = new Map; It will requite using .get() and .set() instead of [] though. Going forward we would like to use Map when we know it is a map. (In reply to comment #5) > (From update of attachment 216090 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216090&action=review > > > Source/WebInspectorUI/UserInterface/ContentFlowTreeView.js:39 > > + // Map of contentNode ids to DOMTreeOutline objects. > > + this._nodesMap = {}; > > You should consider using Map. > > this._nodesMap = new Map; > > It will requite using .get() and .set() instead of [] though. > > Going forward we would like to use Map when we know it is a map. Note to iterate over a Map type, there is Map.prototype.forEach. You cannot use for..in. Created attachment 216191 [details]
Screenshot 1
Created attachment 216192 [details]
Screenshot 2
Nice! (In reply to comment #9) > Nice! I've created a test page to play with the code http://jsfiddle.net/achicu/D356p/. I need to fix https://bugs.webkit.org/show_bug.cgi?id=123577. Created attachment 216199 [details]
Patch for landing
I've incorporated the feedback and I've added a new fixme for the selected node ConsoleAgent $n issue.
I didn't see any issues with the margins/padding of the tree outline.
Joe, let me know if you need to review this again, otherwise please update the cq+ flag.
Comment on attachment 216199 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=216199&action=review Looking good. One question: > Source/WebInspectorUI/UserInterface/DOMTreeElement.js:486 > + this.treeOutline.selectDOMNode(null); Does this cause excess work? Will the UI flash any? (In reply to comment #12) > (From update of attachment 216199 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216199&action=review > > Looking good. One question: > > > Source/WebInspectorUI/UserInterface/DOMTreeElement.js:486 > > + this.treeOutline.selectDOMNode(null); > > Does this cause excess work? Will the UI flash any? It will basically just set the this._selectedDOMNode to null and invoke the SelectedNodeChanged handlers. The handlers for SelectedNodeChanged will trigger SelectionPathComponentsDidChange twice. That's once for null on the old tree and again with non-null for the new tree. I've tracked that one down, the update of the NavigationBar happens on a timer. It means that the layout is only updated once using the last value. Comment on attachment 216199 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=216199&action=review > Source/WebInspectorUI/UserInterface/ContentFlowTreeContentView.js:50 > + Nit, you can remove this blank line. (I updated the wiki page) Comment on attachment 216199 [details] Patch for landing Rejecting attachment 216199 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 216199, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ntentView.js patching file Source/WebInspectorUI/UserInterface/DOMTreeElement.js patching file Source/WebInspectorUI/UserInterface/DOMTreeOutline.js patching file Source/WebInspectorUI/UserInterface/Main.html patching file Source/WebInspectorUI/UserInterface/Main.js patching file Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Joseph Pecoraro']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/22498018 Created attachment 216217 [details]
Patch for landing
Comment on attachment 216217 [details] Patch for landing Clearing flags on attachment: 216217 Committed r158788: <http://trac.webkit.org/changeset/158788> All reviewed patches have been landed. Closing bug. Comment on attachment 216217 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=216217&action=review > Source/WebInspectorUI/UserInterface/ContentFlowTreeContentView.js:39 > + this._nodesMap = new Map(); Style: Here is another one for the Style guide. No () in constructors if not necessary. This could be: this._nodesMap = new Map; |