Web Inspector: Breakpoint Actions

[Joseph Pecoraro]

Breakpoints should have actions.

Initial suggestions include:

  - log a message (console.log)
  - evaluate arbitrary JavaScript (at call frame)
  - play a sound (system beep)

[Radar WebKit Bug Importer]

<rdar://problem/14888049>

[Joseph Pecoraro]

Created attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

[Joseph Pecoraro]

Created attachment 210235 [details]
[PATCH] 2 - Frontend

[Joseph Pecoraro]

Created attachment 210236 [details]
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor

[Joseph Pecoraro]

Created attachment 210237 [details]
[IMAGE] Popover - No Actions

[Joseph Pecoraro]

Created attachment 210238 [details]
[IMAGE] Popover - All Actions

[Joseph Pecoraro]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

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

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:445
> +    // ASSERT(m_currentCallFrame);

So, I have to debug this. It happens almost 100% running LayoutTest/inspector-protocol/debugger/setBreakpoint-options-exception.html. But things seem to work as expected.

[Early Warning System Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1656607

[EFL EWS Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1656608

[Early Warning System Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1641640

[Joseph Pecoraro]

*** Bug 66867 has been marked as a duplicate of this bug. ***

[EFL EWS Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1609550

[Build Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1657569

[Build Bot]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

Attachment 210234 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1675455

[Timothy Hatcher]

Comment on attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests

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

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:205
> +    case ScriptBreakpointActionTypeSound: {
> +        systemBeep();
> +        break;
> +    }

No need for the extra braces here.

>> Source/WebCore/bindings/js/ScriptDebugServer.cpp:445
>> +    // ASSERT(m_currentCallFrame);
> 
> So, I have to debug this. It happens almost 100% running LayoutTest/inspector-protocol/debugger/setBreakpoint-options-exception.html. But things seem to work as expected.

Perhaps the nested evaluate or reportException calls are confusing the debugger?

> Source/WebCore/inspector/Inspector.json:2667
> +                    { "name": "type", "type": "string", "enum": ["log", "eval", "sound"], "description": "Different kinds of breakpoint actions." },

"evaluate"? It would match the others better and other protocol methods that spell it out.

[Timothy Hatcher]

Comment on attachment 210235 [details]
[PATCH] 2 - Frontend

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

Nice an clean!

> Source/WebInspectorUI/UserInterface/Breakpoint.js:43
> +            actions[i] = WebInspector.BreakpointAction.fromProtocol(this, actions[i]);

I wouldn't call this "fromProtocol". It is really about save and restore from localStorage. So it is really "fromInfo" given the rename I also suggest for the protocolObject getter. I would prefer to special case the WebInspector.BreakpointAction constructor to check the type of the second argument and do either normal or info object construction. I'm not a fan of separate "from" constructor helpers if they can be avoided. (Call me jaded by the old way "fromPayload" helpers leaked protocol data all over the Inspector code.)

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:41
> +WebInspector.BreakpointAction.fromProtocol = function(breakpoint, breakpointActionObject)
> +{
> +    return new WebInspector.BreakpointAction(breakpoint, breakpointActionObject.type, breakpointActionObject.data);
> +}

See previous comment.

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:73
> +    get protocolObject()

I'd call this info() to match Breakpoint since this is used for both the protocol and saving to localStorage and add a comment here that it is used for those.

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:49
> +    background-image: url(Images/BreakpointActionAdd.svg), -webkit-linear-gradient(top, rgb(250, 250, 250), rgb(200, 200, 200));

Why does the gradient need to be here? Can it be moved to the SVG? I'm not sure how this clips the gradient to the SVG circle!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:72
> +    -webkit-appearance: textfield;

Clever! But this adds some padding at the top and bottom that push the line gutter down when it is showing. Not an issue in this patch since it is disabled, but something to think about. The CodeMirror instance might be able to set negative top and bottom margins to compensate.

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78
> +    width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */

calc()?

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:83
> +    width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */

calc() here too?

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:128
> +        case DebuggerAgent.BreakpointActionType.Log:

Yay enums!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:152
> +                indentWithTabs: true,
> +                indentUnit: 4,
> +                matchBrackets: true,

I wonder if there is a way to make these default so we don't need to set them everywhere?

> Source/WebInspectorUI/UserInterface/Images/BreakpointActionAdd.svg:5
> +    <path stroke="#F1F2F3" stroke-width="2" d="M 4 9 L 12 9 M 8 5 L 8 13"/>
> +    <path stroke="#818181" stroke-width="2" d="M 4 8 L 12 8 M 8 4 L 8 12"/>

I like rgb() in the SVGs.

[Timothy Hatcher]

Comment on attachment 210236 [details]
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor

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

> Source/WebInspectorUI/ChangeLog:13
> +        The logic was inside of JavaScriptLogViewController but was already
> +        entirely independent. Factor it out into its own class and plug it into
> +        CodeMirrorCompletionController as a "CompletionsProvider".

Nice!

> Source/WebInspectorUI/ChangeLog:56
> [phone]  Joseph Pecoraro  <[e-mail]>
> +
> +        Web Inspector: Breakpoint Actions
> +        https://bugs.webkit.org/show_bug.cgi?id=120576
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Oops!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:160
> +            completionController.setExtendedCompletionProvider("javascript", WebInspector.JavaScriptRuntimeCompletionProvider.instance);

I think WebInspector.JavaScriptRuntimeCompletionProvider.instance is odd. I would expect WebInspector.globalJavaScriptRuntimeCompletionProvider or something.

> Source/WebInspectorUI/UserInterface/CodeMirrorCompletionController.js:95
> +    setExtendedCompletionProvider: function(modeName, provider)
> +    {
> +        this._extendedCompletionProviders[modeName] = provider;
> +    },

I would almost call this "addExtendedCompletionProvider" so it can be multiple. But that is just me wanting to avoid "set" which makes me want this to be a property.

[Timothy Hatcher]

Comment on attachment 210236 [details]
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor

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

>> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:160
>> +            completionController.setExtendedCompletionProvider("javascript", WebInspector.JavaScriptRuntimeCompletionProvider.instance);
> 
> I think WebInspector.JavaScriptRuntimeCompletionProvider.instance is odd. I would expect WebInspector.globalJavaScriptRuntimeCompletionProvider or something.

I guess "global" is redundant since it is on the global namespace. WebInspector.javaScriptRuntimeCompletionProvider?

[Timothy Hatcher]

Comment on attachment 210235 [details]
[PATCH] 2 - Frontend

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

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:75
> +        var obj = {type: this._type};

I'm kind of bothered the type comes from the protocol and not another enum from the frontend. This can lead to a compatibility issue since this is stored in localStorage for an indefinite time and the enums can never change then.

Ideally the DebuggerManager would be the gatekeeper for all the Breakpoint protocol translation.

[Joseph Pecoraro]

Comment on attachment 210235 [details]
[PATCH] 2 - Frontend

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

>> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:49
>> +    background-image: url(Images/BreakpointActionAdd.svg), -webkit-linear-gradient(top, rgb(250, 250, 250), rgb(200, 200, 200));
> 
> Why does the gradient need to be here? Can it be moved to the SVG? I'm not sure how this clips the gradient to the SVG circle!

I didn't want to duplicate the gradient in both SVG files. I was hoping I could share the gradient here. Though it isn't completely shared here at least they are close enough together you can compare at a glance. And finally, gradients in CSS are much shorter then their SVG counterparts.

>> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78
>> +    width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */
> 
> calc()?

Ahh, apparently width: 100% does what I want if I put it on both. Much easier!

[Joseph Pecoraro]

(In reply to comment #19)
> (From update of attachment 210235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review
> 
> > Source/WebInspectorUI/UserInterface/BreakpointAction.js:75
> > +        var obj = {type: this._type};
> 
> I'm kind of bothered the type comes from the protocol and not another enum from the frontend. This can lead to a compatibility issue since this is stored in localStorage for an indefinite time and the enums can never change then.
> 
> Ideally the DebuggerManager would be the gatekeeper for all the Breakpoint protocol translation.

So I went with a patch that makes the frontend use only its own enum:

  WebInspector.BreakpointAction.Type = { Log: "log", … }

And DebuggerManager converts it to the protocol enum when sending (setBreakpoint).

I'm going to rebase and re-run tests. If it all checks out I will land.

[Joseph Pecoraro]

Committed:
<http://trac.webkit.org/changeset/155132>
<http://trac.webkit.org/changeset/155133>
<http://trac.webkit.org/changeset/155134>

[Joseph Pecoraro]

Build fix follow-up:
<http://trac.webkit.org/changeset/155136>

[Joseph Pecoraro]

Comment on attachment 210235 [details]
[PATCH] 2 - Frontend

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

>>> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78
>>> +    width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */
>> 
>> calc()?
> 
> Ahh, apparently width: 100% does what I want if I put it on both. Much easier!

Ahh, I found the case where this doesn't work. 1 long line of text wraps incorrectly and even expands if width is ever % based (e.g. 100% or calc(100%-30px)).

I don't yet know if this is an engine bug or behaving correctly. I'll address this issue in a follow-up bug.