[webkit-reviews] review denied: [Bug 143589] Web Inspector: Better handling for large arrays and collections in Object Trees : [Attachment 378939] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 143589: Web Inspector: Better handling for large arrays and collections in
Object Trees
https://bugs.webkit.org/show_bug.cgi?id=143589

Attachment 378939: Patch

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




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

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

I can end up seeing multiple Object Prototype buttons.

    Issue:
    1. js> dir([])
    2. Expand Array Prototype button
      => I see two "Object Prototype" buttons

I'd expect the Show More buttons to be side by side. Currently this patch
shows:

    js> new Uint8Array(1000);
    0: 0
    1: 0
    ...
    99: 0
    ( Show 100 More )
    ( Show All (900 More) )

I'd have expected:

    js> new Uint8Array(1000);
    0: 0
    1: 0
    ...
    99: 0
    ( Show 100 More ) ( Show All (900 More) )

We can iterate on the UI later but it is always worth a screenshot attached to
the bug if you're adding / non trivially changing UI.

I have a few more things to review (tests), but r- for now given the issues.

>
Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProv
ider.js:258
>	       if (propertyNames && typeof propertyNames.length === "number") {
> -		   var max = Math.min(propertyNames.length, 1000);
> -		   for (var i = 0; i < max; ++i)
> +		   for (let i = 0; i < propertyNames.length; ++i)
>		       propertyNames[i] = true;
>	       }

Does this mean we create a completion list of 1000000 entries if the object is
large? We probably shouldn't do that. It would be better for the completion
controller generate numbers to handle a number range, instead of an object with
millions of keys.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:36
> +	   this._fetchEnd = WI.ObjectTreeView.showMoreFetchCount;

`WI.ObjectTreeView.showMoreFetchCount` is not a getter, so every time this
happens this._fetchEnd is a function.

>
Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:435
> +		   hadProto = true;
> +		   this.appendChild(new
WI.ObjectTreePropertyTreeElement(propertyDescriptor, propertyPath, mode,
prototypeName));

Either we should `continue` here or also check `hadProto` around line 442 to
avoid creating a second __proto__ button.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:106
> +    static showMoreFetchCount() { return 100; }

This should be a getter, otherwise everything below is totally broken.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:114
> +	   console.assert(typeof handleShowMoreClicked === "function",
handleShowMoreClicked);

Might as well assert for `handleShowAllClicked`. Perhaps also assert `typeof
resolvedValue.size === "number"` since that is used below; we're assuming
resolvedValue is a collection / array type with a size.

> LayoutTests/inspector/model/remote-object-get-properties-expected.txt:44
> +    length

I suspect these changes need to be reverted.