[webkit-reviews] review granted: [Bug 200458] Web Inspector: Implement `queryHolders` Command Line API : [Attachment 376464] 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 200458: Web Inspector: Implement `queryHolders` Command Line API
https://bugs.webkit.org/show_bug.cgi?id=200458

Attachment 376464: Patch

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




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

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

r=me

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:808
> +	   if (reason == SlotVisitor::RootMarkReason::Debugger)

Would be great to apply this to HeapSnapshotBuilder and make it so the Web
Inspector UI can hide debugger objects!

>
Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProv
ider.js:335
>      "queryInstances",
>      "queryObjects",
> +    "queryHolders",

Look like this was in alphabetical order. Might as well keep it that way!

> LayoutTests/inspector/console/queryHolders-expected.txt:66
> +-- Running test case: CommandLineAPI.queryHolders.OnlyHeldByDebugger
> +PASS: The result should have 0 items.

I wonder if this will confuse anyone. I think an empty array is the right thing
to do though.

> LayoutTests/inspector/console/queryHolders.html:97
> +    addTest(`PromiseFinally`);

Great tests!

Possible additional tests (not required):

  • An object only referenced via an Object Symbol property (should just be
like an object string property)
  • An object with a lot of references, like `window` or something like that
(ensure nothing crashes)
  • An object referenced in an array a lot of times only shows up once `arr =
[foo, foo]`, foo has a single `arr` holder, since it is de-duplicated.

What happens with:

  • Elements. With `$0` do we see the parent element? I'd suspect it might just
be the DOMWrapperWorld, which has no good output.

> LayoutTests/inspector/console/queryHolders.html:151
> +    suite.addTestCase({
> +	   name: "CommandLineAPI.queryHolders.NoParameter",

Yeah, I kinda think `queryHolders()` and `queryObjects()` no parameters should
throw an exception. Could be done as a follow-up. I realize that doesn't match
what Chrome does.