Bug 200971 – Web Inspector: create additional command line api functions for other console methods

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Right now, we only show:
 - dir
 - clear
 - table
 - profile
 - profileEnd

But we could show other things:
 - log
 - record
 - recordEnd
 - trace()

Given that all of the `console.*` methods are already always available, having these "shortcuts" (even in cases where the underlying `console.*` has no effect) would be useful/consistent.

[Devin Rousso]

Created attachment 376856 [details]
Patch

[Devin Rousso]

Created attachment 376991 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 376991 [details]
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/JavaScriptRuntimeCompletionProvider.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/JavaScriptRuntimeCompletionProvider.js:277
> +                    }
> +                    else if (targetData.pauseReason === WI.DebuggerManager.PauseReason.Exception) {

Style: `else` on the line above.

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

Nice

[Devin Rousso]

Comment on attachment 376991 [details]
Patch

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

>> 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.

The reason I didn't do this (I actually would've used a `Set`) is that `Set.prototype.add`/`Set.prototype.delete` are observable, whereas `object[key] = value` isn't.  If/When we move to a builtin, I'd switch to a `@Set`.

[Devin Rousso]

Created attachment 377170 [details]
Patch

[Devin Rousso]

Created attachment 377176 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377176 [details]
Patch

Clearing flags on attachment: 377176

Committed r249078: <https://trac.webkit.org/changeset/249078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54661719>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/249078/webkit

has caused two test to fail:
inspector/debugger/tail-deleted-frames-this-value.html 
inspector/timeline/line-column.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdebugger%2Ftail-deleted-frames-this-value.html%20inspector%2Ftimeline%2Fline-column.html

Diffs:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249096%20(6157)/inspector/debugger/tail-deleted-frames-this-value-diff.txt

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249096%20(6157)/inspector/timeline/line-column-diff.txt

I reproduced inspector/timeline/line-column.html easily by just running it in iterations.

[Joseph Pecoraro]

> Diffs:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/inspector/debugger/tail-deleted-frames-this-value-diff.txt
> 
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/inspector/timeline/line-column-diff.txt
> 
> I reproduced inspector/timeline/line-column.html easily by just running it
> in iterations.

Timeline test just needs a rebaseline.

Not sure about the debugger one.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #11)
> > Diffs:
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/inspector/debugger/tail-deleted-frames-this-value-diff.txt
> > 
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/inspector/timeline/line-column-diff.txt
> > 
> > I reproduced inspector/timeline/line-column.html easily by just running it in iterations.
> 
> Timeline test just needs a rebaseline.
> 
> Not sure about the debugger one.
Landed followup in r249122 <https://trac.webkit.org/r249122>.

Not sure why inspector/debugger/tail-deleted-frames-this-value.html is failing (didn't reproduce locally), so I added messages to the various `InspectorTest.assert` to see if I could figure out which one is being triggered.

[Ryan Haddad]

(In reply to Devin Rousso from comment #12)
> (In reply to Joseph Pecoraro from comment #11)
> Not sure why inspector/debugger/tail-deleted-frames-this-value.html is
> failing (didn't reproduce locally), so I added messages to the various
> `InspectorTest.assert` to see if I could figure out which one is being
> triggered.

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/debugger/tail-deleted-frames-this-value-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/debugger/tail-deleted-frames-this-value-actual.txt
@@ -13,6 +13,7 @@
 PASS: 'this' value should have expected property: bThis
 PASS: Call Frame 1 'this' value is correct.
 Expected frame: {"functionName":"c","thisValue":["cThis",0],"isTailDeleted":true}
+ASSERT: Should have functionName of 'c'.
 PASS: 'this' value should have expected property: cThis
 PASS: Call Frame 2 'this' value is correct.
 Tests done

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249124%20(6176)/results.html

[Devin Rousso]

(In reply to Ryan Haddad from comment #13)
> (In reply to Devin Rousso from comment #12)
> > (In reply to Joseph Pecoraro from comment #11)
> > Not sure why inspector/debugger/tail-deleted-frames-this-value.html is failing (didn't reproduce locally), so I added messages to the various `InspectorTest.assert` to see if I could figure out which one is being triggered.
> 
> --- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/ inspector/debugger/tail-deleted-frames-this-value-expected.txt
> +++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/ inspector/debugger/tail-deleted-frames-this-value-actual.txt
> @@ -13,6 +13,7 @@
>  PASS: 'this' value should have expected property: bThis
>  PASS: Call Frame 1 'this' value is correct.
>  Expected frame:
> {"functionName":"c","thisValue":["cThis",0],"isTailDeleted":true}
> +ASSERT: Should have functionName of 'c'.
>  PASS: 'this' value should have expected property: cThis
>  PASS: Call Frame 2 'this' value is correct.
>  Tests done
> 
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249124%20(6176)/results.html
🤦‍♂️ I feel like an idiot.  I didn't include the actual value in the assertion message.

Landed r249127 <https://trac.webkit.org/r249127>

Hopefully this will (finally) contain enough info to see what's going on, and why it's only having issues on the bots.

[Ryan Haddad]

(In reply to Devin Rousso from comment #14)
> 🤦‍♂️ I feel like an idiot.  I didn't include the actual value in the
> assertion message.
> 
> Landed r249127 <https://trac.webkit.org/r249127>
> 
> Hopefully this will (finally) contain enough info to see what's going on,
> and why it's only having issues on the bots.

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/debugger/tail-deleted-frames-this-value-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/debugger/tail-deleted-frames-this-value-actual.txt
@@ -13,6 +13,7 @@
 PASS: 'this' value should have expected property: bThis
 PASS: Call Frame 1 'this' value is correct.
 Expected frame: {"functionName":"c","thisValue":["cThis",0],"isTailDeleted":true}
+ASSERT: Should have functionName of 'c', but have 'b' instead.
 PASS: 'this' value should have expected property: cThis
 PASS: Call Frame 2 'this' value is correct.
 Tests done

[Devin Rousso]

Frankly, I'm not entirely sure why this was happening, but I've figured out a fix.

<https://webkit.org/b/201193> Web Inspector: don't attach properties to `injectedScript` for the CommandLineAPI

[Ryan Haddad]

(In reply to Devin Rousso from comment #16)
> Frankly, I'm not entirely sure why this was happening, but I've figured out
> a fix.
> 
> <https://webkit.org/b/201193> Web Inspector: don't attach properties to
> `injectedScript` for the CommandLineAPI

The test is still a flaky failure after this change, and it also introduced a crash documented in https://bugs.webkit.org/show_bug.cgi?id=201201

I plan on rolling these changes out shortly.