122927 – Web Inspector: CSS Regions: When a flow is clicked the content of flow needs to be displayed

RESOLVED FIXED Bug 122927

Web Inspector: CSS Regions: When a flow is clicked the content of flow needs to be displayed

https://bugs.webkit.org/show_bug.cgi?id=122927

Summary Web Inspector: CSS Regions: When a flow is clicked the content of flow needs ...

Alexandru Chiculita

Reported 2013-10-16 16:48:49 PDT

Implement the content view for the flows.

Attachments
Patch V1 (15.28 KB, patch) 2013-11-05 15:38 PST, Alexandru Chiculita	joepeck: review+ joepeck: commit-queue-	Details Formatted Diff Diff
Screenshot 1 (deleted) 2013-11-06 10:50 PST, Alexandru Chiculita	no flags	Details
Screenshot 2 (deleted) 2013-11-06 10:51 PST, Alexandru Chiculita	no flags	Details
Patch for landing (16.76 KB, patch) 2013-11-06 12:05 PST, Alexandru Chiculita	joepeck: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Patch for landing (16.72 KB, patch) 2013-11-06 13:38 PST, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2013-10-16 16:49:40 PDT

<rdar://problem/15247057>

Alexandru Chiculita

Comment 2 2013-11-05 15:38:56 PST

Created attachment 216090 [details] Patch V1

Joseph Pecoraro

Comment 3 2013-11-05 16:15:34 PST

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];

Timothy Hatcher

Comment 4 2013-11-05 16:32:10 PST

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

Timothy Hatcher

Comment 5 2013-11-05 16:42:54 PST

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.

Joseph Pecoraro

Comment 6 2013-11-05 16:47:10 PST

(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.

Alexandru Chiculita

Comment 7 2013-11-06 10:50:46 PST

Created attachment 216191 [details] Screenshot 1

Alexandru Chiculita

Comment 8 2013-11-06 10:51:24 PST

Created attachment 216192 [details] Screenshot 2

Timothy Hatcher

Comment 9 2013-11-06 11:08:12 PST

Nice!

Alexandru Chiculita

Comment 10 2013-11-06 11:54:00 PST

(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.

Alexandru Chiculita

Comment 11 2013-11-06 12:05:54 PST

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.

Timothy Hatcher

Comment 12 2013-11-06 12:19:43 PST

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?

Alexandru Chiculita

Comment 13 2013-11-06 13:08:05 PST

(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.

Joseph Pecoraro

Comment 14 2013-11-06 13:09:59 PST

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)

WebKit Commit Bot

Comment 15 2013-11-06 13:10:59 PST

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

Alexandru Chiculita

Comment 16 2013-11-06 13:38:18 PST

Created attachment 216217 [details] Patch for landing

WebKit Commit Bot

Comment 17 2013-11-06 14:06:49 PST

Comment on attachment 216217 [details] Patch for landing Clearing flags on attachment: 216217 Committed r158788: <http://trac.webkit.org/changeset/158788>

WebKit Commit Bot

Comment 18 2013-11-06 14:06:53 PST

All reviewed patches have been landed. Closing bug.

Joseph Pecoraro

Comment 19 2013-11-06 17:13:43 PST

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;

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Alexandru Chiculita

Reported

2013-10-16 16:48 PDT

Modified

2013-11-06 17:13 PST History

CC List

6 users Show

URL

Keywords InRadar

Depends on

123424

Blocks

57312 122760

Dependancies

tree graph