[webkit-reviews] review granted: [Bug 200971] Web Inspector: create additional command line api functions for other console methods : [Attachment 376991] 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 200971: Web Inspector: create additional command line api functions for
other console methods
https://bugs.webkit.org/show_bug.cgi?id=200971

Attachment 376991: Patch

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




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

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

Very hard to read this patch with all of the changes happening but it looks
good.

Looks like some test failures may need to be addressed?

> Source/JavaScriptCore/inspector/InjectedScriptModule.cpp:65
> -    Deprecated::ScriptFunctionCall
function(injectedScript.injectedScriptObject(), "module"_s,
injectedScriptManager->inspectorEnvironment().functionCallHandler());
> +    Deprecated::ScriptFunctionCall
function(injectedScript.injectedScriptObject(), "hasInjectedModule"_s,
injectedScriptManager->inspectorEnvironment().functionCallHandler());

Nice

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:437
>	   return this._modules[name];

`this._modules` could be a Map. That would avoid complexities around
`this._modules["toString"]` returning a Function.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1541
> +injectedScript.addCommandLineAPIMethod("trace", function() { return
inspectedGlobalObject.console.trace(...arguments); });
> +injectedScript.addCommandLineAPIMethod("warn", function() { return
inspectedGlobalObject.console.warn(...arguments); });

I can't help but think this is actually a case where a loop would be better
(Console API methods), but I bet my opinion on this would change day to day.

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:97
> +    if (typeof types === "undefined") {

We can use real undefined checks:

    types === undefined

You could even use `let` instead of `var` if you wanted.

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:125
> +	       result = result.concat([

These lists are unlikely to change often. As such the 1 word per line actually
makes it harder to read. The original code was significantly easier to read for
example.

--- 

All of these would be better written as `push` instead of `concat`, to avoid
temporary wasted allocations.

    result = result.concat(["a", "b", "c"]);

is better written as:

    result.push("a", "b", "c");

>
Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProv
ider.js:51
> +    static get _commandLineAPIKeys()
> +    {
> +	   return [

This makes a new array over and over with these strings. Might be worth
avoiding the churn each time and just memoizing this. It seems to be used in a
`concat()` so creating it once would be fine as users don't modify it.

    if (!this.__cachedCommandLineAPIKeys)
	this.__cachedCommandLineAPIKeys = [...];
    return this.__cachedCommandLineAPIKeys;

>
Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProv
ider.js:277
> +		       }
> +		       else if (targetData.pauseReason ===
WI.DebuggerManager.PauseReason.Exception) {

Style: `else` on the line above.

>
Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProv
ider.js:284
> +		   switch (target.type) {

Nice