Bug 53235 – Web Inspector: new api for managing JavaScript breakpoints

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Podivilov]

In new protocol, there should be three methods related to JavaScript breakpoints:
[domain=Debugger] void setJavaScriptBreakpoint(in Object breakpoint, out String breakpointId, out Array locations);
[domain=Debugger] void removeJavaScriptBreakpoint(in String breakpointId);
[notify, domain=Debugger] void breakpointResolved(out String breakpointId, out Object location);

breakpoint argument has the following properties:
1. url (optional)
2. sourceID (optional)
3. lineNumber
4. columnNumber (optional, defaults to 1)
5. condition (optional)
6. enabled (optional, defaults to true)
At least one of url and sourceID should be provided.

location specifies actual location where breakpoint was resolved by VM, location has 3 properties: sourceID, lineNumber, columnNumber.

When frontend enables debugger, it should set all JavaScript breakpoints one by one using setJavaScriptBreakpoint method. After that, breakpoints will be resolved in parsed scripts automatically during debugging session (until frontend is closed).
When provisional breakpoint is resolved (e.g. on navigation), breakpointResolved notification is used to update frontend with the new breakpoint location.

[Pavel Podivilov]

Created attachment 80325 [details]
Patch.

Single protocol breakpoint (e.g. set by url) is mapped on zero or more VM breakpoints (set by sourceID).
removeJavaScriptBreakpoint(breakpointId) removes breakpoint and all linked VM breakpoints.
Since UI uses VM breakpoint location rather then protocol breakpoint location, all resolved breakpoints locations are passed to frontend.

SourceFrame is now aware of whether breakpoint is resolved or not and may display it accordingly.
JavaScriptBreakpointsSidebarPane filters out breakpoints set on nonexistent scripts to avoid UI cluttering.

[Pavel Podivilov]

Created attachment 80330 [details]
Rebase.

[Pavel Feldman]

(In reply to comment #0)
> In new protocol, there should be three methods related to JavaScript breakpoints:
> [domain=Debugger] void setJavaScriptBreakpoint(in Object breakpoint, out String breakpointId, out Array locations);

Why not to list breakpoint properties explicitly? Note that the actual protocol sends arguments as named properties.

> [domain=Debugger] void removeJavaScriptBreakpoint(in String breakpointId);
> [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out Object location);
> 
> breakpoint argument has the following properties:
> 1. url (optional)
> 2. sourceID (optional)
> 3. lineNumber
> 4. columnNumber (optional, defaults to 1)
> 5. condition (optional)
> 6. enabled (optional, defaults to true)
> At least one of url and sourceID should be provided.
> 

We need to make sure that optional parameters are marked as such in the IDL. Otherwise protocol checks won't let them through.

> location specifies actual location where breakpoint was resolved by VM, location has 3 properties: sourceID, lineNumber, columnNumber.
> 

I don't understand the locations Array. I would expect that a breakpoint results in a single location. Breakpoints set by sourceID should result in location with sourceID, breakpoints set by URL should result in location with URL. Clients should have a choice of whether working in terms of IDs or URLs. We should not mix those.

[Pavel Podivilov]

(In reply to comment #3)
> (In reply to comment #0)
> > In new protocol, there should be three methods related to JavaScript breakpoints:
> > [domain=Debugger] void setJavaScriptBreakpoint(in Object breakpoint, out String breakpointId, out Array locations);
> 
> Why not to list breakpoint properties explicitly? Note that the actual protocol sends arguments as named properties.
> 
> > [domain=Debugger] void removeJavaScriptBreakpoint(in String breakpointId);
> > [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out Object location);
> > 
> > breakpoint argument has the following properties:
> > 1. url (optional)
> > 2. sourceID (optional)
> > 3. lineNumber
> > 4. columnNumber (optional, defaults to 1)
> > 5. condition (optional)
> > 6. enabled (optional, defaults to true)
> > At least one of url and sourceID should be provided.
> > 
> 
> We need to make sure that optional parameters are marked as such in the IDL. Otherwise protocol checks won't let them through.

We may use empty strings for omitted parameters and hide this behind sdk.

> 
> > location specifies actual location where breakpoint was resolved by VM, location has 3 properties: sourceID, lineNumber, columnNumber.
> > 
> 
> I don't understand the locations Array. I would expect that a breakpoint results in a single location. Breakpoints set by sourceID should result in location with sourceID, breakpoints set by URL should result in location with URL. Clients should have a choice of whether working in terms of IDs or URLs. We should not mix those.

When you set breakpoint by url, it may be resolved into several scripts that have the same url. Locations array is used to provide sourceId:line:column for each of this scripts. Alternatively, we may return only the first location, but such protocol looks inconsistent to me, since we address scripts by sourceId, but hide sourceId info when setting breakpoint by url. It would also prevent us from debugging several scripts with the same url (we wouldn't be able to display breakpoints correctly). What do you think?

[Pavel Podivilov]

Created attachment 80772 [details]
Patch.

Update after offline discussion.

[domain=Debugger] void setJavaScriptBreakpoint(in String url, in int lineNumber, in int columnNumber, in String condition, in boolean enabled, out String breakpointId, out Array locations);
[domain=Debugger] void setJavaScriptBreakpointBySourceId(in String sourceId, in int lineNumber, in int columnNumber, in String condition, in boolean enabled, out String breakpointId, out int actualLineNumber, out int actualColumnNumber);
[domain=Debugger] void removeJavaScriptBreakpoint(in String breakpointId);
[notify, domain=Debugger] void breakpointResolved(out String breakpointId, out String sourceId, out int lineNumber, out int columnNumber);

There are two methods for setting a breakpoint:
1. setJavaScriptBreakpoint - sets provisional breakpoint by url, backend would set it in all scripts with matching url and notify front-end using breakpointResolved notification
2. setJavaScriptBreakpointBySourceId - sets a breakpoint into a single script by sourceId, breakpoint won't be restored after navigation

[Pavel Feldman]

Comment on attachment 80772 [details]
Patch.

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

> Source/WebCore/inspector/InspectorAgent.cpp:1012
> +void InspectorAgent::enableDebugger(bool newDebuggingSession)

newDebuggingSession -> eraseStickyBreakpoints

> Source/WebCore/inspector/InspectorAgent.h:244
> +    void enableDebugger(bool newDebuggingSession = false);

We should not have default values.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:100
> +    struct Script {

You can't do structs. Yury says that Adam will come and you will get beaten.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:116
> +        int lineOffset;

I don't see why this is here.

[Pavel Podivilov]

Committed r77489: <http://trac.webkit.org/changeset/77489>

[Pavel Podivilov]

(In reply to comment #6)
> (From update of attachment 80772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80772&action=review
> 
> > Source/WebCore/inspector/InspectorAgent.cpp:1012
> > +void InspectorAgent::enableDebugger(bool newDebuggingSession)
> 
> newDebuggingSession -> eraseStickyBreakpoints
Done.
> 
> > Source/WebCore/inspector/InspectorAgent.h:244
> > +    void enableDebugger(bool newDebuggingSession = false);
> 
> We should not have default values.
Done.
> 
> > Source/WebCore/inspector/InspectorDebuggerAgent.h:100
> > +    struct Script {
> 
> You can't do structs. Yury says that Adam will come and you will get beaten.
Done.
> 
> > Source/WebCore/inspector/InspectorDebuggerAgent.h:116
> > +        int lineOffset;
> 
> I don't see why this is here.
This is needed to fix old and nasty bug with multiplying breakpoints in JSC debugger.