Bug 120576 - Web Inspector: Breakpoint Actions
Summary: Web Inspector: Breakpoint Actions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 66867 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-09-01 02:16 PDT by Joseph Pecoraro
Modified: 2013-09-05 15:51 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] 1 - Backend and Protocol Tests (28.70 KB, patch)
2013-09-01 02:53 PDT, Joseph Pecoraro
timothy: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
[PATCH] 2 - Frontend (35.60 KB, patch)
2013-09-01 02:53 PDT, Joseph Pecoraro
timothy: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor (34.12 KB, patch)
2013-09-01 02:54 PDT, Joseph Pecoraro
timothy: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[IMAGE] Popover - No Actions (169.07 KB, image/png)
2013-09-01 02:55 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Popover - All Actions (166.42 KB, image/png)
2013-09-01 02:55 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-09-01 02:16:38 PDT
Breakpoints should have actions.

Initial suggestions include:

  - log a message (console.log)
  - evaluate arbitrary JavaScript (at call frame)
  - play a sound (system beep)
Comment 1 Radar WebKit Bug Importer 2013-09-01 02:16:49 PDT
<rdar://problem/14888049>
Comment 2 Joseph Pecoraro 2013-09-01 02:53:04 PDT
Created attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests
Comment 3 Joseph Pecoraro 2013-09-01 02:53:36 PDT
Created attachment 210235 [details]
[PATCH] 2 - Frontend
Comment 4 Joseph Pecoraro 2013-09-01 02:54:29 PDT
Created attachment 210236 [details]
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor
Comment 5 Joseph Pecoraro 2013-09-01 02:55:00 PDT
Created attachment 210237 [details]
[IMAGE] Popover - No Actions
Comment 6 Joseph Pecoraro 2013-09-01 02:55:41 PDT
Created attachment 210238 [details]
[IMAGE] Popover - All Actions
Comment 7 Joseph Pecoraro 2013-09-01 02:58:21 PDT
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.
Comment 8 Early Warning System Bot 2013-09-01 02:58:37 PDT
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
Comment 9 EFL EWS Bot 2013-09-01 03:00:26 PDT
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
Comment 10 Early Warning System Bot 2013-09-01 03:00:42 PDT
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
Comment 11 Joseph Pecoraro 2013-09-01 03:01:00 PDT
*** Bug 66867 has been marked as a duplicate of this bug. ***
Comment 12 EFL EWS Bot 2013-09-01 03:01:12 PDT
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
Comment 13 Build Bot 2013-09-01 03:20:00 PDT
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
Comment 14 Build Bot 2013-09-01 03:36:12 PDT
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
Comment 15 Timothy Hatcher 2013-09-01 06:39:24 PDT
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.
Comment 16 Timothy Hatcher 2013-09-01 07:09:15 PDT
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.
Comment 17 Timothy Hatcher 2013-09-01 07:15:04 PDT
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
> +2013-09-01  Joseph Pecoraro  <pecoraro@apple.com>
> +
> +        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.
Comment 18 Timothy Hatcher 2013-09-01 07:24:52 PDT
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?
Comment 19 Timothy Hatcher 2013-09-01 07:38:32 PDT
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.
Comment 20 Joseph Pecoraro 2013-09-04 18:31:30 PDT
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!
Comment 21 Joseph Pecoraro 2013-09-04 18:44:46 PDT
(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.
Comment 23 Joseph Pecoraro 2013-09-05 11:29:28 PDT
Build fix follow-up:
<http://trac.webkit.org/changeset/155136>
Comment 24 Joseph Pecoraro 2013-09-05 15:51:47 PDT
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.