[webkit-reviews] review granted: [Bug 174968] Web Inspector: add TreeElement virtualization for the Recording tab : [Attachment 316936] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 174968: Web Inspector: add TreeElement virtualization for the Recording tab
https://bugs.webkit.org/show_bug.cgi?id=174968

Attachment 316936: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 316936
  --> https://bugs.webkit.org/attachment.cgi?id=316936
Patch

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

r=me, we should apply this to the CallStackTreeOutline in the debugger tab,
which will need narrowing down of the scroll container.

> Source/WebInspectorUI/ChangeLog:18
> +	   the TreeOutline node still takes up the same amount of space after
some of the TreeElement's

Grammar: "TreeElement's" => "TreeElements"

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:466
> +	   if (this.treeOutline)
> +	       this.treeOutline.updateVirtualizedElements(this);

Move this after the onreveal.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:648
> +	   if (isNaN(treeItemHeight))

Just console.assert !NaN

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:666
> +	   function walk(parent, callback) {

When walk recurses, the shouldReturn callback might return `true`, and then we
will continue to walk when we un-nest the recursion.

You can solve this with 2 return values:

    {shouldReturn, count}

Or avoid recursion and do some cool iterative stack based solution.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:688
> +	   let extraRows = Math.max(numberToShow * 5, 50);

This variable name is vague since you end up doing (extraRows * 2) all over the
place. This is the number of pad rows you are applying to each side.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:697
> +	       let index = walk(this, ({treeElement, count}) => {
> +		   if (treeElement === focusedTreeElement)
> +		       return true;
> +		   return false;
> +	       });

Style: Simplify this

    let index = walk(this, ({treeElement}) => treeElement ===
focusedTreeElement);

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:703
> +	       }
> +	       else if (index > lastItem) {

Style: else on the same line

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:720
> +	   this._virtualizedTopSpacer.style.setProperty("height",
(Math.max(firstItem, 0) * this._virtualizedTreeItemHeight) + "px");

Why not just: style.height = ...;