[webkit-reviews] review granted: [Bug 122927] Web Inspector: CSS Regions: When a flow is clicked the content of flow needs to be displayed : [Attachment 216090] Patch V1

Tue Nov 5 16:16:46 PST 2013

Joseph Pecoraro <joepeck at webkit.org> has granted Alexandru Chiculita
<achicu at adobe.com>'s request for review:
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

Attachment 216090: Patch V1
https://bugs.webkit.org/attachment.cgi?id=216090&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.ContentNodeWasRemov
ed, 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.Conte
ntNodeWasAdded, this._onContentNodeWasAdded, this);
> +	  
this.representedObject.removeEventListener(WebInspector.ContentFlow.Event.Conte
ntNodeWasRemoved, 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];