[webkit-reviews] review granted: [Bug 124614] Web Inspector: [CSS Regions] Show a list of containing regions when clicking a node that is part of a flow : [Attachment 217845] Patch V2

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Alexandru Chiculita
<achicu at adobe.com>'s request for review:
Bug 124614: Web Inspector: [CSS Regions] Show a list of containing regions when
clicking a node that is part of a flow
https://bugs.webkit.org/show_bug.cgi?id=124614

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

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217845&action=review


> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:34
> +    -webkit-user-select: none;

Why prevent selecting the flow name? The user might want to copy it.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:71
> +.details-section > .content > .group > .row.simple.content-flow-link >
.value > div > .go-to-arrow {
> +    display: none;
> +    width: 16px;
> +    height: 16px;
> +}
> +
> +.details-section > .content > .group > .row.simple.content-flow-link:hover >
.value > div > .go-to-arrow {
> +    display: block;
> +}

The go-to arrow should always be visible. There is already a style for
.go-to-arrow in a row in DetailsSection.css.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:58
> +    goToRegionFlowButton.addEventListener("click",
this._goToRegionFlowArrowWasClicked.bind(this), false);

We omit the false in addEventListener.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:69
> +    goToContentFlowButton.addEventListener("click",
this._goToContentFlowArrowWasClicked.bind(this), false);

Ditto.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:108
> +	   this._regionFlowNameRow.element.classList.toggle("hidden", !flow);
> +	   this._flowNamesSection.element.classList.toggle("hidden", !flow &&
!this._contentFlow);

The variable flow confused me here. I think this._regionFlow would be more
readable given the use of this._contentFlow.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:120
> +	   this._contentFlowNameRow.element.classList.toggle("hidden", !flow);

DetailsSectionSimpleRow auto hides if it has an empty value. The styles for the
sections and groups also adjust padding around empty rows, so it would be best
to use the auto hide behavior.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:121
> +	   this._flowNamesSection.element.classList.toggle("hidden", !flow &&
!this._regionFlow);

Ditto about flow.

> Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:194
> +	       }
> +	       this.regionFlow = flowData.regionFlow;

Newline.

> Source/WebInspectorUI/UserInterface/DOMTreeDataGrid.css:70
> +    content: url(Images/DOMElement.svg);

Not all DOM nodes are elements. The icon should be based on the node type. This
can be a FIXME.

> Source/WebInspectorUI/UserInterface/RuntimeManager.js:79
> +	       }
> +	       var properties = new Map;

Newline.

> Source/WebInspectorUI/UserInterface/RuntimeManager.js:82
> +		   properties.set(property.name, property);
> +	       callback(null, properties);

Newline.