Bug 53235 - Web Inspector: new api for managing JavaScript breakpoints
Summary: Web Inspector: new api for managing JavaScript breakpoints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-27 04:49 PST by Pavel Podivilov
Modified: 2011-02-03 07:28 PST (History)
10 users (show)

See Also:


Attachments
Patch. (70.84 KB, patch)
2011-01-27 05:35 PST, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase. (70.99 KB, patch)
2011-01-27 06:46 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Patch. (82.17 KB, patch)
2011-02-01 09:32 PST, Pavel Podivilov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2011-01-27 04:49:17 PST
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.
Comment 1 Pavel Podivilov 2011-01-27 05:35:35 PST
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.
Comment 2 Pavel Podivilov 2011-01-27 06:46:25 PST
Created attachment 80330 [details]
Rebase.
Comment 3 Pavel Feldman 2011-01-31 13:49:56 PST
(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.
Comment 4 Pavel Podivilov 2011-02-01 03:03:10 PST
(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?
Comment 5 Pavel Podivilov 2011-02-01 09:32:25 PST
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
Comment 6 Pavel Feldman 2011-02-02 05:37:46 PST
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.
Comment 7 Pavel Podivilov 2011-02-03 07:25:08 PST
Committed r77489: <http://trac.webkit.org/changeset/77489>
Comment 8 Pavel Podivilov 2011-02-03 07:28:34 PST
(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.