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

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied  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 362079: Patch

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




--- Comment #39 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 362079
  --> https://bugs.webkit.org/attachment.cgi?id=362079
Patch

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

Setting r-. While addressing Devin's latest feedback I noticed an issue with
shift-selecting using the up/down arrow keys. I think it was introduced in the
switch from the index- to object-based SelectionController.

>> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:80
>> +	    return this._cookies.indexOf(object);
> 
> While you're at it, we could also assert that
`this._cookies.includes(object)` so that we check in both "directions
(converting object-to-index and index-to-object).

I'd rather not add a second O(N) function call for every index lookup. We could
have Table assert that the value returned by the data source is non-false.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:480
>> +	    if (representedObject.__treeElement)
> 
> This should probably have a comment, not necessarily about the DOM tree, but
about how this can be used to have multiple `TreeElement` that all effectively
have the same `representedObject`.

Actually that is the problem this solves. We currently have a one-to-many
relationship between represented objects and DOMTreeElements, and we need a
one-to-one for SelectionController to function.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:904
>> +	    console.assert(treeElement, "Missing TreeElement for
representedObject.", item);
> 
> I feel like this assertion may get triggered if we try to get the previous
item after selecting the first item.  Perhaps we should just remove it (or
modify it to also pass if `item ===
this.selectionControllerLastSelectableItem(controller)`).

This only asserts if the starting TreeElement (in the case you mention that
would be the first item) can't be found, which should not happen.