[webkit-reviews] review denied: [Bug 193605] Web Inspector: Frontend performance is very slow reloading theverge.com - 50% of time in TreeOutline _indexOfTreeElement : [Attachment 361980] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 193605: Web Inspector: Frontend performance is very slow reloading
theverge.com - 50% of time in TreeOutline _indexOfTreeElement
https://bugs.webkit.org/show_bug.cgi?id=193605

Attachment 361980: Patch

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




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

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

r-, as this breaks selection in the Network table.  Everything else looks
great, although I'd like to see a followup.  Awesome work!

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:32
> +	   console.assert(typeof comparator === "function");

NIT: this should be below the `delegate` assertion, since it's listed after in
the parameters.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:35
> +	   this._comparator = comparator;

Ditto (>32).

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:136
> +		   let previous = this._previousSelectableItem(item);
> +		   let next = this._nextSelectableItem(item);
> +
> +		   while (!this.hasSelectedItem(previous) &&
!this.hasSelectedItem(next)) {
> +		       previous = this._previousSelectableItem(previous);
> +		       next = this._nextSelectableItem(next);
>		   }
> +
> +		   if (this.hasSelectedItem(previous))
> +		       this._lastSelectedItem = previous;
> +		   else if (this.hasSelectedItem(next))
> +		       this._lastSelectedItem = next;

It could be potentially costly to try to "eagerly" find the closest item in
_both_ directions _at the same time_.  I think we should be a bit more
"incremental", meaning we should only try one thing at a time.

    let previous = item;
    let next = item;
    while (!this._lastSelectedItem && previous && next) {
	previous = this._previousSelectableItem(previous);
	if (this.hasSelectedItem(previous)) {
	    this._lastSelectedItem = previous;
	    break;
	}

	next = this._nextSelectableItem(next);
	if (this.hasSelectedItem(next)) {
	    this._lastSelectableItem = next;
	    break;
	}
    }

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:167
> -    removeSelectedItems()
> +    willRemoveSelectedItems()

I personally think that `removeSelectedItems` is a better name, as it is
"removing" the selected items from this controller.  Adding "will" makes me
think that there's some sort of timeout/rAF that will "delay"  the removal. 
None of the callers have any sort of "will" (or future tense) naming either, so
I think it makes sense to match (e.g. no "will").

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:172
> +	   let orderedSelection =
Array.from(this._selectedItems).sort(this._comparator);

Rather than sort the entire list of selected items, couldn't we just find the
first/last item (e.g. some sort of `min` and `max` for the array)?  I've
written utilities functions for something like this in the past.

    Object.defineProperty(Array.prototype, "min",
    {
	value(comparator)
	{
	    function defaultComparator(a, b)
	    {
		return a - b;
	    }
	    comparator = comparator || defaultComparator;

	    let min = 0;
	    for (let i = 1; i < this.length; ++i) {
		if (comparator(this[min], this[i]) > 0)
		    min = i;
	    }
	    return this[min];
	},
    });

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:175
> +	   let lastSelectedItem = orderedSelection.lastValue;

Ditto (>172).

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:369
>	   if (this._suppressSelectionDidChange ||
!this._delegate.selectionControllerSelectionDidChange)

NIT: we should remove the
`!this._delegate.selectionControllerSelectionDidChange` check if we assert that
it exists in thee constructor.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:383
> +	       if (current === lastItem)

We should assert that if `lastItem` is provided, it is included in `items`
(e.g. to cover the case where we `lastItem` isn't reachable).

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:394
> +	       if (current === lastItem)

Ditto (>383), except we'd want to assert that it's not included in `items`.

> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:86
> +	   if (index < 0 || index >= this._cookies.length)
> +	       return null;

This check seems unnecessary.  If anything, we should just be asserting, as
`undefined` is effectively equivalent to `null` for our typical falsy checks.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:327
> +	       if (!treeElement.__closeTagProxyObject) {

Style: unnecessary brackets.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:328
> +		   treeElement.__closeTagProxyObject = {treeElement};

Can we use the `__treeElementIdentifier` instead?  Regardless, I think we
should always have a `__` prefix, as we're effectively tacking on a view object
to a model object (I realize that this is a "fake" model object, but the
principle stands nonetheless).	If not, maybe we could use a different key,
like `__proxyObjectTreeElement`?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:332
> +	   return this._rowIndexForRepresentedObject(object);

This is not the right function to use.	`_rowIndexForRepresentedObject` is only
supposed to be used with actual model objects (e.g. `Resource` or `Node`), not
the "bag-of-stuff" objects used by the Network table.  You should just use
`this._filteredEntries.indexOf(object)`.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:338
> +	   if (index < 0 || index >= this._filteredEntries.length)
> +	       return null;

Ditto (>CookieStorageContentView.js:85).

> Source/WebInspectorUI/UserInterface/Views/Table.js:1421
> +

Style: extra newline.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1423
> +	       if (rowIndexes.binaryIndexOf(index) >= 0) {

Clever!

> Source/WebInspectorUI/UserInterface/Views/Table.js:-1414
> -	   this._selectionController.didRemoveItems(rowIndexes);

This slightly scares me, as it assumes that the caller deselects an item before
it is removed.	I think we should either find some way of asserting (inside
`SelectionController`) that removed items aren't selected or not remove
`didRemoveItems` and have `SelectionController` simply remove items from its
set.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:60
> +	   let comparator = (a, b) => {

Nicely written!

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:-349
> -	       treeOutline._selectionController.didRemoveItems(removedIndexes);

Ditto (<Table.js:1414).

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:-397
> -		  
treeOutline._selectionController.didRemoveItems(removedIndexes);

Ditto (<Table.js:1414).

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:848
> +		   treeElement.listItemElement.classList.remove("selected");

Style: missing newline after.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:858
> +		   treeElement.listItemElement.classList.add("selected");

Style: missing newline after.