Bug 165116 – Web Inspector: Breakpoint Log action should support template literals

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Breakpoint Log action should support template literals. Logging a variable is overly cumbersome, requiring changing the action type from "Log" to "Evaluate JavaScript", and using console.log:

Action: Evaluate JavaScript
Text: console.log("x: " + x)

Allowing inline template literals in Log action text, similar to Xcode breakpoints use of "@exp@ = expression", would be nice:

Action: Log
Text: x: ${x}

Note:
No new backend infrastructure is needed, since we can convert Log actions containing template literals to the equivalent Evaluate JavaScript on the frontend.

[Matt Baker]

Created attachment 295547 [details]
[Image] New UI

[Matt Baker]

Created attachment 295621 [details]
Patch

[Build Bot]

Comment on attachment 295621 [details]
Patch

Attachment 295621 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2590428

New failing tests:
inspector/debugger/breakpoint-action-log.html

[Build Bot]

Created attachment 295627 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295621 [details]
Patch

Attachment 295621 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2590437

New failing tests:
inspector/debugger/breakpoint-action-log.html

[Build Bot]

Created attachment 295628 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Matt Baker]

Created attachment 295630 [details]
Patch

[Build Bot]

Comment on attachment 295630 [details]
Patch

Attachment 295630 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2590704

New failing tests:
inspector/debugger/breakpoint-action-log.html

[Build Bot]

Created attachment 295636 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295630 [details]
Patch

Attachment 295630 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2590717

New failing tests:
inspector/debugger/breakpoint-action-log.html

[Build Bot]

Created attachment 295639 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295630 [details]
Patch

Attachment 295630 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2590769

New failing tests:
inspector/debugger/breakpoint-action-log.html

[Build Bot]

Created attachment 295641 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Matt Baker]

Created attachment 295661 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 295661 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:5
> +localizedStrings["${exp} = expression"] = "${exp} = expression";

I think "expr" is more canonical than "exp".

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:828
> +        let templateLiteralRegex = /\$\{.*?\}/g;
> +        for (let action of options.actions) {

Style: I'd suggest moving the regex to a `const` and eliminating the `options` local and just inlining it in the loop header.

A regex alone here won't be perfect if we are going to treat the input as a template string.

For example:

    1. Log ${"{world}"}!
           ^--------^   = actual
           ^----------^ = expected

    2. Log ${`{world}`}!
           ^--------^   = actual
           ^----------^ = expected

    3. Log ${ ({a: 1, b: "world"}).world }!
           ^--------------------^          = actual
           ^-----------------------------^ = expected

I'm not sure the right solution here without using a full ECMAScript parser. However I think in practice bad cases (like (2) above) will be rare or nonexistent. So this approach, in general is fine.

At the very least, we should probably be escaping '`' everywhere in the string if we are treating it as a template literal. Otherwise this will be broken:

    4. Log `foo` is ${obj.foo}!

In which case we should probably just do something like:

  - if string contains ${...} => escape possible template string characters
  - otherwise just log it

I think that strikes the right balance of simplicity and functionality.

Thoughts?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:846
> +            action.data = "console.log(`" + templateLiteral + "`)";
> +            action.type = WebInspector.BreakpointAction.Type.Evaluate;

Does this mean we actually modify the real action so that it turns into a console.log in the UI?

> LayoutTests/inspector/debugger/breakpoint-action-log.html:31
> +                breakpoint.createAction(WebInspector.BreakpointAction.Type.Log, breakpoint.actions[0], "Template strings: a = ${a}, b = ${b}.");

You could do ${JSON.stringify(b)} to show more meaningful output in the results and to do a more complex expression.

[Matt Baker]

Comment on attachment 295661 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:846
>> +            action.type = WebInspector.BreakpointAction.Type.Evaluate;
> 
> Does this mean we actually modify the real action so that it turns into a console.log in the UI?

Yes

[Matt Baker]

Comment on attachment 295661 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:846
>>> +            action.type = WebInspector.BreakpointAction.Type.Evaluate;
>> 
>> Does this mean we actually modify the real action so that it turns into a console.log in the UI?
> 
> Yes

Just to clarify, the breakpoint itself is not modified just the data sent to the backend. The original breakpoint is displayed and serialized.

[Matt Baker]

Comment on attachment 295661 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:5
>> +localizedStrings["${exp} = expression"] = "${exp} = expression";
> 
> I think "expr" is more canonical than "exp".

Xcode uses "exp", I'm fine with either!

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:828
>> +        for (let action of options.actions) {
> 
> Style: I'd suggest moving the regex to a `const` and eliminating the `options` local and just inlining it in the loop header.
> 
> A regex alone here won't be perfect if we are going to treat the input as a template string.
> 
> For example:
> 
>     1. Log ${"{world}"}!
>            ^--------^   = actual
>            ^----------^ = expected
> 
>     2. Log ${`{world}`}!
>            ^--------^   = actual
>            ^----------^ = expected
> 
>     3. Log ${ ({a: 1, b: "world"}).world }!
>            ^--------------------^          = actual
>            ^-----------------------------^ = expected
> 
> I'm not sure the right solution here without using a full ECMAScript parser. However I think in practice bad cases (like (2) above) will be rare or nonexistent. So this approach, in general is fine.
> 
> At the very least, we should probably be escaping '`' everywhere in the string if we are treating it as a template literal. Otherwise this will be broken:
> 
>     4. Log `foo` is ${obj.foo}!
> 
> In which case we should probably just do something like:
> 
>   - if string contains ${...} => escape possible template string characters
>   - otherwise just log it
> 
> I think that strikes the right balance of simplicity and functionality.
> 
> Thoughts?

Another example:

    5. Log ${1 + ${1 + 1}}

Nested template literals are also broken.

Will fix and add these test cases to breakpoint-action-log.html.

[Joseph Pecoraro]

> Another example:
> 
>     5. Log ${1 + ${1 + 1}}
> 
> Nested template literals are also broken.
> 
> Will fix and add these test cases to breakpoint-action-log.html.

I think you meant something like:

    5. Log ${1 + `${1 + 1}`}

But yes, this nested case is for sure going to need a full parser.

In a way we have multiple parsers for this. Both CodeMirror, Esprima, and JavaScriptCore (Runtime.parse). After having thought about it in my previous comment I concluded it was so rare that the extra overheard might not be warranted. But perhaps there is a way we could trick this.

One thing I thought about was when you close "Log" it would start out with a:

    `log`

Template String which the user could edit:

    `log ${expr}!`

Which would have the benefit of CodeMirror syntax highlighting and code completion.

[Joseph Pecoraro]

I guess the nested case could mean either nested "${}" or nested template strings. Both are relevant here if we are doing a pseudo-template string =)

[Radar WebKit Bug Importer]

<rdar://problem/29464765>

[Matt Baker]

Created attachment 296230 [details]
Patch

[WebKit Commit Bot]

Attachment 296230 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:147:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matt Baker]

(In reply to comment #23)
> Attachment 296230 [details] did not pass style-queue:
> 
> 
> ERROR:
> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:
> 147:  Line contains single-quote character.  [js/syntax] [5]
> Total errors found: 1 in 13 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

check-webkit-style shouldn't complain about a single quote in a RegExp literal.

[Matt Baker]

(In reply to comment #24)
> (In reply to comment #23)
> > Attachment 296230 [details] did not pass style-queue:
> > 
> > 
> > ERROR:
> > Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:
> > 147:  Line contains single-quote character.  [js/syntax] [5]
> > Total errors found: 1 in 13 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> check-webkit-style shouldn't complain about a single quote in a RegExp
> literal.

Interesting! The RegExp scenario is mentioned in the patch comments:
https://bugs.webkit.org/show_bug.cgi?id=128422#c5

[Mark Lam]

Comment on attachment 296230 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:81
> +     _finishPlainText()

FYI, there's an extra space char before _finishPlainText().

[Matt Baker]

(In reply to comment #23)
> Attachment 296230 [details] did not pass style-queue:
> 
> 
> ERROR:
> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:
> 147:  Line contains single-quote character.  [js/syntax] [5]
> Total errors found: 1 in 13 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

A bug in the style checker is to blame:
https://bugs.webkit.org/show_bug.cgi?id=165453

[Joseph Pecoraro]

Comment on attachment 296230 [details]
Patch

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

r=me! Add a few more tests below and I think we're good.

> Source/WebInspectorUI/ChangeLog:10
> +        New string "${exp} = expression" for breakpoint popover.

Nit: "expr" to match the updated code.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:26
> +WebInspector.BreakpointLogMessageLexer = class BreakpointLogMessageLexer extends WebInspector.Object

Nifty! I had figured this would be a lot of complexity to add, but in the end it ends up pretty simple. Sounds good to me.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:38
> +        this._stateFunctions = {};
> +
> +        this._stateFunctions[WebInspector.BreakpointLogMessageLexer.State.EscapeCharacter] = this._escapeCharacter;
> +        this._stateFunctions[WebInspector.BreakpointLogMessageLexer.State.Expression] = this._expression;
> +        this._stateFunctions[WebInspector.BreakpointLogMessageLexer.State.PlainText] = this._plainText;
> +        this._stateFunctions[WebInspector.BreakpointLogMessageLexer.State.PossiblePlaceholder] = this._possiblePlaceholder;
> +        this._stateFunctions[WebInspector.BreakpointLogMessageLexer.State.RegExpOrStringLiteral] = this._regExpOrStringLiteral;

Style: You could simplify this quite a bit with computed properties:

    this._stateFunctions = {
        [WebInspector.BreakpointLogMessageLexer.State.EscapeCharacter]: this._escapeCharacter,
        [WebInspector.BreakpointLogMessageLexer.State.Expression]: this._expression,
        ...
    };

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:104
> +        let token = {type};
> +        if (data)
> +            token.data = data;
> +        this._tokens.push(token);

This if (data) is redundant. Both callers have already if checked that value. I'd say just inline this above or perhaps simplifying:

    _finishPlainText()
    {
        this._appendToken(.PlainType);
    }

    _finishExpression()
    {
        this._appendToken(.Expression);
    }

    _appendToken(type)
    {
        if (!this._buffer)
            return;
        this._tokens.push({type, data: this._buffer});
        this._buffer = "";
    }

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:111
> +        console.assert(this._index < this._input.length);
> +        if (this._index >= this._input.length)
> +            return null;

This bail is probably worth dropping since we have the assert. It seems code below would already work without exception if we were already behaving incorrectly.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:128
> +        this._states.pop();

I think before each pop you can assert what it should be. Here we should be in EscapeCharacter.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:138
> +                this._states.pop();

I think before each pop you can assert what it should be. Here we should be in Expression.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:147
> +        if (/[\/'"]/.test(character)) {

I think it might be clearer to just compare the 3 characters then use a regexp here.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:159
> +        if (character === "$")
> +            this._states.push(WebInspector.BreakpointLogMessageLexer.State.PossiblePlaceholder);

When does this get popped?

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:168
> +        let character = this._consume();

Nit: console.assert(character === "$")

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:170
> +

I think somewhere in here we should be popping the PossiblePlaceholder state. We are either advancing to Expression, or returning back to PlainText.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:187
> +        if (character === "\\") {
> +            this._states.push(WebInspector.BreakpointLogMessageLexer.State.EscapeCharacter);
> +            return;
> +        }

This boils down to: Whenever we encounter a '\' we can consume the next character no matter what.

It would be nice to just do that (consume). Instead we have an extra tokenizer state (push(Escape); loop; consume; pop(Escape)).

I'd want to see:

    if (character === "\\")
        this._consume();
        return;
    }

But an issue is that if "\" is the last character we would assert in consume. So we could do:

    if (character === "\\") {
        if (this._peek() !== null)
            this._consume();
        return;
    }

Or:

    if (character === "\\") {
        if (!this._atEnd())
            this._consume();
        return;
    }

Which I think are simpler to follow than an extra Escape state.

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointLogMessageLexer.js:190
> +        if (character === this._literalStartCharacter)
> +            this._states.pop();

I think before each pop you can assert what it should be. Here we should be in RegExpOrStringLiteral.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:848
> +                else if (token.type === WebInspector.BreakpointLogMessageLexer.TokenType.Expression)

Style: Drop the `else`.

> Source/WebInspectorUI/UserInterface/Test.html:195
> +    <script src="Controllers/BreakpointLogMessageLexer.js"></script>

Nit: This should go at the top of the earlier group, to closer match Main.html include order.

> LayoutTests/inspector/debugger/breakpoint-action-log.html:5
> +<script type="text/javascript" src="../../http/tests/inspector/resources/inspector-test.js"></script>
> +<script type="text/javascript" src="resources/script-for-breakpoint-actions.js"></script>

Style: Drop unnecessary type attribute.

> LayoutTests/inspector/debugger/breakpoint-action-log.html:41
> +                addLogAction("close curly brace in RegExp: ${/\\}/.test('}')}");

Would be good for a test with multiple expressions:

    addLogAction("${a} and ${JSON.stringify(b)}")

> LayoutTests/inspector/unit-tests/breakpoint-log-message-lexer.html:5
> +<script type="text/javascript" src="../../http/tests/inspector/resources/inspector-test.js"></script>
> +<script type="text/javascript" src="resources/script-for-breakpoint-actions.js"></script>

Style: We normally drop type="text/javascript" in our tests.

> LayoutTests/inspector/unit-tests/breakpoint-log-message-lexer.html:34
> +    let testCases = [
> +        {
> +            name: "EmptyMessage",
> +            input: "",
> +            expectedTokens: []
> +        },

Style: I don't find this better than our usual style:

    addTestCase({
        name: "EmptyMessage",
        input: "",
        expectedTokens: [],
    });

> LayoutTests/inspector/unit-tests/breakpoint-log-message-lexer.html:112
> +        }

Please add tests for:

    Multiple Expressions
    input: "${1} ${2}"
    expected: [ (Expression, "1"), (Text, " "), (Expression, "2") ]

    Expression Followed By Braces
    input: "${1}{2}"
    expected: [ (Expression, "1"), (Text, "{2}") ]

I think the second of these will be broken right now. Maybe both.

[Matt Baker]

Created attachment 296903 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 296903 [details]
Patch for landing

Clearing flags on attachment: 296903

Committed r209706: <http://trac.webkit.org/changeset/209706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 296903 [details]
Patch for landing

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

> LayoutTests/inspector/debugger/breakpoint-action-log.html:18
> +        description: ".",

Doh.

[Matt Baker]

(In reply to comment #32)
> Comment on attachment 296903 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296903&action=review
> 
> > LayoutTests/inspector/debugger/breakpoint-action-log.html:18
> > +        description: ".",
> 
> Doh.

Oops! Nice catch, fixed: https://trac.webkit.org/changeset/209735