[webkit-reviews] review denied: [Bug 216897] Web Inspector: Collapse blackboxed call frames in Sources : [Attachment 415783] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Nikita Vasilyev <nvasilyev at apple.com>'s
request for review:
Bug 216897: Web Inspector: Collapse blackboxed call frames in Sources
https://bugs.webkit.org/show_bug.cgi?id=216897

Attachment 415783: Patch

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




--- Comment #34 from Brian Burg <bburg at apple.com> ---
Comment on attachment 415783
  --> https://bugs.webkit.org/attachment.cgi?id=415783
Patch

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

>>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1559
>>>> +/* Blackboxed - N call frames */
>>> 
>>> this needs a better comment
>> 
>> Note that the most important thing for localizers is verbatim surrounding
text. Here, it's important to know that there's "Blackboxed —" before "%d call
frames". What are you suggesting?
> 
> We almost always include information about where this can be found in the UI
and what "kind" of display it has (e.g. title, label, etc.).  I don't think
this comment is clear that "Blackboxed - %d call frames" is the literal text
being shown or whether "Blackboxed - " is some sort of "category" for a bunch
of localized strings.  I think a better comment would be something like
(wording could be bikeshedded):
> ```
>     WI.UIString("%d call frames", "%d call frames @ Blackboxed Group in
Debugger Call Stack", "Part of the 'Blackboxed - %d call frames' label shown in
the debugger call stack when paused instead of subsequent call frames that have
been blackboxed.");
> ```

Devin's suggestion looks good to me.

>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:30
>>> +.blackboxed-group:not(.selected) {
>> 
>> IMO it's odd that using ↑/↓ actually does select it.  I would expect ↑/↓ to
basically skip over it (just like how we skip over async boundaries).  I
suppose we want to be able to show blackboxed frames via keyboard navigation,
but it still feels kinda weird because the main content area doesn't change
when its selected ��
> 
> Yes, I do think we should provide a way to show blackboxed frames via
keyboard. I'd prefer selecting the 1st blackboxed call frame on expansion
rather getting rid of keyboard navigation altogether.

Since we don't have agreement on expected behavior, let's address this in a
followup. It's not enabled by default, so this can be fixed whenever.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.css:39
>> +	content: "";
> 
> This looks really weird.  I feel like we either need some sort of icon (maybe
a transparent version of the eye?) or need to style the text differently (maybe
italics?) so that this looks different from the rest of the items.

Work on a new blackboxing icon is still underway. Let's address this in a
followup. Nikita, please file a bug for this if you haven't already.

>>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:62
>>>> +	      iconElement.className =
WI.GeneralTreeElement.IconElementStyleClassName;
>>> 
>>> Can we subclass `WI.GeneralTreeElement` instead of using its CSS classes
and mimicking its DOM structure?
>> 
>> Subclassing GeneralTreeElement was my 1st stub at it, actually, and it was
more complex. GeneralTreeElement has `_disclosureButton`, and this one doesn't.
> 
> The `_disclosureButton` is `display: none;` when there are no children.  I
don't understand how it would've been more complex when you're effectively
copying the DOM structure and CSS classes here (which could make modifying that
same structure in `WI.GeneralTreeElement` more difficult in the future as the
engineer may not know that this exists).
> 
> Also, we've been trying to move away from having constant variables for CSS
classes.  This makes that more difficult.

I agree with Devin. Rolling a separate tree element will cause accessibility
issues (no ARIA roles) and code maintenance issues (when the CSS changes).

>>>> Source/WebInspectorUI/UserInterface/Views/BlackboxedGroupTreeElement.js:91
>>>> +		 
this.childrenListNode.style.setProperty("--tree-outline-item-padding",
`${treeOutlineItemPaddingNewValue}px`);
>>> 
>>> This seems exceptionally hacky.  Instead of adding the
`WI.CallFrameTreeElement` to this as children, we should lazily create them in
`onexpand` and then do something like `this.parent.insertChild(new
WI.CallFrameTreeElement(blackboxedCallFrame),
this.parent.children.indexOf(this))` and then `this.parent.removeChild(this)`. 
This will let us avoid having to muck with CSS and keep the state of the
`WI.TreeOutline` clean since the `WI.BlackboxedGroupTreeElement` will no longer
be in the DOM (it will also let you avoid the
`.tree-outline:not(.hide-disclosure-buttons) .item.blackboxed-group > .icon`
CSS rule because `WI.BlackboxedGroupTreeElement` will no longer be a
`.parent`).
>> 
>> `this.parent.insertChild(new WI.CallFrameTreeElement(blackboxedCallFrame),
this.parent.children.indexOf(this))` — this seems to me just a different kind
of hacky.
> 
> How is that hacky?  We're using existing API to effectively replace one tree
element with multiple.	We do this kind of thing in a few different places
already in Web Inspector (both with `WI.TreeOutline` and with regular DOM
nodes).

I agree with Devin's suggestion for how to implement this.

As written, this is an undesirable way to set the padding, because it forces a
sync style recalc on expand.