[webkit-reviews] review denied: [Bug 176766] Web Inspector: Implement `queryObjects` Command Line API : [Attachment 321372] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 176766: Web Inspector: Implement `queryObjects` Command Line API
https://bugs.webkit.org/show_bug.cgi?id=176766

Attachment 321372: Patch

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




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

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

r- just because I want to see a few more tests

> Source/JavaScriptCore/ChangeLog:12
> +	   Iterates over all live JSCell in the heap to find these objects.

I would say "Does a GC and then iterates over..."

This is introducing new API. I think we should have a paragraph in the
ChangeLog, above the files, that describes what we are adding. And overview of
`queryObjects()` and explains our current Object/Array/Function exception and
reasoning. ChangeLogs are a great opportunity to include this information, and
normally it is just copying it from the Bugzilla bug description and putting it
in here with polish!

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:660
> +    HeapIterationScope iterationScope(vm.heap);

I'm tempted to say you should put this in an anonymous block:

    {
	HeapIterationScope iterationScope(vm.heap);
	...
    }

I think we want to keep the iteration scope as small as possible. In this case
it wouldn't change things, but in the future if someone were to add code around
this it would be less error prone to holding the scope longer than necessary.

> LayoutTests/inspector/console/queryObjects.html:30
> +	   {prototypeOrConstructor: "HTMLImageElement", resultCount: 0},

Lets have a test for HTMLBodyElement / HTMLHeadElement where we expect there to
be 1. Or create a few <meta> tags above and access them. We could also test
that `queryObjects(Node).length > 20` or something like that.

Node is an interesting case. We do not create Node JavaScript value wrappers
for all nodes unless they are needed. So queryObjects(Node) only gets the ones
created, not all nodes. This actually matches Chrome's behavior:

    js> queryObjects(Node)
    Array (153)

    js> document.querySelectorAll("*")
    Array (348)
    // Expand to see all instances

    js> queryObjects(Node)
    Array (378)

So I think we are fine there. Its an interesting, but not otherwise harmful
exposure of implementation behavior. A developer might get confused that
queryObjects(Node).filter((x) => ...) doesn't find their target Node, but I
think thats a good thing. This lets developers see the actual live instances of
a type. Also, I don't think Developers will want to use this (in its intended
use case) for Node. They would more likely use this for their own object /
class types.

This does raise above that HTMLMetaElement might not work without first doing a
`document.querySelectorAll("meta")` or something to create wrappers.

> LayoutTests/inspector/console/queryObjects.html:39
> +    ];

We should have tests for calling it with non-Object values.

Things like: 1, true, null, NaN, Symbol(), "test", String("test"), Number(1),
to make sure it returns no results or throws.

Behavior in Chrome is to throw if it is not an object:

    > queryObjects(true)
    Prototype should be an Object.

    > queryObjects("test")
    Prototype should be an Object.

    > queryObjects(Symbol())
    Prototype should be instance of Object

> LayoutTests/inspector/console/queryObjects.html:76
> +	       queryObjects(prototypeOrConstructor, (remoteObject, wasThrown,
savedResultIndex) => {

In Chrome, no parameter produces no result without an exception.

    js> queryObjects()
    undefined

This almost makes me think we should have queryObjects take a `...list` of
prototypes / constructors to enable `queryObjects(Foo, Bar)`, however right now
that is not possible without users doing their own concat of each list.

I think we start by striving to match behavior and maybe reach out to the
Chrome folks to more strongly specify behavior.