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

Attachment 356241: Patch

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




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

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

I agree. r- because the test is missing.

I think with the Object/Array restriction this is still useful and unlikely to
be dangerous.

> Source/JavaScriptCore/ChangeLog:28
> +	   them are highlighy sensitive and undefined behaviour can occur if
they are modified.

Typo: "highlighy"

> Source/WebCore/ChangeLog:9
> +	   Test: inspector/console/queryObjects.html

r-, where is the test?

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:60
> +    for (let method of CommandLineAPI.methods) {

Hmm, we normally don't use `for..of` in Injected Scripts because it is
observable if the page overrides the Array prototype Symboling.iterator.
InjectedScriptSource is pretty keen to not use it.

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:242
> +    queryObjects(...args)
> +    {
> +	   return InjectedScriptHost.queryObjects(...args);
> +    },

Can we enumerate them to avoid potentially using an observable iterator here.
Seems like there is only 1 supported argument anyways right now.