[webkit-reviews] review granted: [Bug 175451] Web Inspector: change style of RecordingNavigationSidebarPanel : [Attachment 320324] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 175451: Web Inspector: change style of RecordingNavigationSidebarPanel
https://bugs.webkit.org/show_bug.cgi?id=175451

Attachment 320324: Patch

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




--- Comment #28 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 320324
  --> https://bugs.webkit.org/attachment.cgi?id=320324
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:30
> +.item.action:not(.initial-state, .parent, .invalid) > .icon {

I think we can simplify the property cascade if we change this

    .tree-outline:not(:focus, .force-focus) .item.action:not(.initial-state,
.parent, .invalid) > .icon

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:59
> +:matches(:focus, .force-focus) .item.action.selected:not(.initial-state,
.invalid) > .icon {

Should `:matches(:focus, .force-focus)` only be applied to a parent
`.tree-outline`?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:124
> +.item.action:not(.selected) > .titles .parameter .inline-swatch {
> +    vertical-align: -1px;
> +}

NIT: I realize that this will be added in a future patch, but this is not used
right now.  I would personally prefer to have it removed from this patch and
added in the other one.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:140
> +.item.action.hit-region > .icon {
> +    content: url(../Images/HitRegion.svg);
> +}

This doesn't appear to be used anywhere.  Do we need it for WebGL?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:159
> +	       if
(WI.RecordingActionTreeElement._lineStyleNames.includes(name))

NIT: it seems odd to have some names inside these static lists while others are
listed inline in the switch-case.  Is there a reason for this difference, or
could all of them be listed inside the switch?	This way, you only need to do a
single check for determining the class of an action.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:298
> +WI.RecordingActionTreeElement._imageNames = [

For some speed/efficiency, you should make these `new Set([...])`.  This also
follows the pattern in RecordingAction.js.  Or you could just inline them in
the switch-case above (see above comment).