[webkit-reviews] review denied: [Bug 216897] Web Inspector: Collapse blackboxed call frames in Sources : [Attachment 412160] 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 412160: Patch

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




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

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

There are some smaller coding fixes to address, which I commented about. There
is the larger issue of the UI design, which does not address some revisions
that we discussed yesterday.

We had consensus that the eyeball should belong on the trailing (right) side of
the TreeElement, and upon the eyeball being clicked, it shows the frames that
have been hidden. Patrick also had a good suggestion for formatting the label.
Lastly, I still would prefer to not use a disclosure triangle at all, since it
breaks the visual flow of the call frames and implies a hierarchical
relationship between call frames, which is misleading. A clickable placeholder
that disappears (or turns into an eyeball on the top grouped frame) would be my
preference, as it requires one click and can use a button appearance to look
more clickable.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:43
>> +localizedStrings["%d call frames blackboxed"] = "%d call frames
blackboxed";
> 
> It looks odd to me to have the number of call frames come before the reason
they are collapsed. Looking at it in the context of the rest of the stack
trace, I think I would expect something more akin to `"Blackboxed — %d call
frames"`.

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548
> +	   function flush(blackboxFrameEndIndex) {

This is named as an index, but it can take the value callFrames.length, which
is not a valid index. Please adjust the caller and the code here (and add
assertions) so only valid indices can be passed.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:552
> +	       let adjacentCount = blackboxFrameEndIndex -
blackboxFrameStartIndex;

(:548) ...you'll need to add back 1 here to endIndex.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:558
> +	   }

This function would be more naturally expressed using Array.prototype.splice()
once the start/end index of the blackboxed call frame range has been found.
Then we can get rid of groupedCallFrames altogether. It would be good to copy
the `callFrames` parameter using callFrames.slice() to avoid mutating the
original.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:567
> +		      
sourceCodeToBlackboxData.set(callFrame.sourceCodeLocation.sourceCode,
blackboxData);

It seems like this Map is never read, can we remove it?

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:401
> +	   experimentalSettingsView.addSetting(WI.UIString("Debugging:"),
WI.settings.experimentalCollapseBlackboxedCallFrames, WI.UIString("Collapse
blackboxed call frames"));

Both of these labels need informative UIString keys/descriptions.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1958
> +	   if (treeElement instanceof WI.GeneralTreeElement)

Why is this needed?

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:63
> +		       let blackboxedTreeElement = new
WI.GeneralTreeElement("blackboxed-group", title);

It's going to be a lot easier to iterate on this if we have a TreeElement
subclass for the blackboxed call frame group.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:69
> +		   } else {

Please put this branch first and early continue at the end of it. This will
help reduce the amount of unneeded indentation.

if (!Array.isArray(callFrame)) {
    // ...
    continue;
}

// Handle grouping case

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:79
> +	      
renderCallFrames(WI.debuggerManager.groupBlackboxedCallFrames(callFrames),
true);

It makes no sense to be for DebuggerManager to be involved here, all it's doing
in the function is restructuring the array.

It would be better to define this helper on Array.prototype in WI.Utilities
(Utilities.js) so that we can make this work for any array & predicate, and
write unit tests for it.

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.js:111
> +	       if (WI.settings.experimentalCollapseBlackboxedCallFrames.value)

To avoid duplication and unnecessary detail in this already large function,
please put the settings check inside renderCallFrames()