[webkit-reviews] review granted: [Bug 221763] Web Inspector: Grid list in Layout panel missing empty message : [Attachment 420517] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 221763: Web Inspector: Grid list in Layout panel missing empty message
https://bugs.webkit.org/show_bug.cgi?id=221763

Attachment 420517: Patch

https://bugs.webkit.org/attachment.cgi?id=420517&action=review




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 420517
  --> https://bugs.webkit.org/attachment.cgi?id=420517
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420517&action=review

r=me, nice fix :)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:128
> +	   }
> +	   else {

Style: we put `else` on the same line as `}`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:131
> +		   this._gridSection = new WI.CSSGridSection;

IMO you could/should just create this once in `initialLayout` to avoid having
extra allocations/deallocations.  You could then always know that
`this._gridSection` is valid and not ever have to invalidate (`= null;`) it.

You are correct tho in calling `removeSubview` and `appendChild`/`addSubview`
because `WI.DetailsSectionRow` will `removeChildren()` when it shows/hides the
empty message, so make sure to keep those bits :)