Bug 200971 - Web Inspector: create additional command line api functions for other console methods
Summary: Web Inspector: create additional command line api functions for other console...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 201193
  Show dependency treegraph
 
Reported: 2019-08-21 02:33 PDT by Devin Rousso
Modified: 2019-08-30 09:45 PDT (History)
13 users (show)

See Also:


Attachments
Patch (41.69 KB, patch)
2019-08-21 02:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (44.14 KB, patch)
2019-08-21 23:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (41.89 KB, patch)
2019-08-23 16:01 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (44.59 KB, patch)
2019-08-23 16:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-08-21 02:33:18 PDT
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.
Comment 1 Devin Rousso 2019-08-21 02:46:51 PDT
Created attachment 376856 [details]
Patch
Comment 2 Devin Rousso 2019-08-21 23:20:09 PDT
Created attachment 376991 [details]
Patch
Comment 3 Joseph Pecoraro 2019-08-23 11:40:59 PDT
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
Comment 4 Devin Rousso 2019-08-23 15:45:16 PDT
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`.
Comment 5 Devin Rousso 2019-08-23 16:01:44 PDT
Created attachment 377170 [details]
Patch
Comment 6 Devin Rousso 2019-08-23 16:27:43 PDT
Created attachment 377176 [details]
Patch
Comment 7 WebKit Commit Bot 2019-08-23 17:47:03 PDT
Comment on attachment 377176 [details]
Patch

Clearing flags on attachment: 377176

Committed r249078: <https://trac.webkit.org/changeset/249078>
Comment 8 WebKit Commit Bot 2019-08-23 17:47:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-08-23 17:48:28 PDT
<rdar://problem/54661719>
Comment 11 Joseph Pecoraro 2019-08-26 12:04:59 PDT
> 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.
Comment 12 Devin Rousso 2019-08-26 16:02:59 PDT
(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.
Comment 13 Ryan Haddad 2019-08-26 17:33:40 PDT
(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
Comment 14 Devin Rousso 2019-08-26 17:50:18 PDT
(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.
Comment 15 Ryan Haddad 2019-08-27 09:45:54 PDT
(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
Comment 16 Devin Rousso 2019-08-27 12:37:15 PDT
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
Comment 17 Ryan Haddad 2019-08-30 09:45:38 PDT
(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.