Bug 48434 – Web Inspector: persist JavaScript breakpoints in frontend settings

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

[Pavel Podivilov]

Web Inspector: persist breakpoints in frontend settings

[Pavel Podivilov]

Created attachment 72042 [details]
Patch.

[Alexander Pavlov (apavlov)]

Comment on attachment 72042 [details]
Patch.

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

> WebCore/inspector/InspectorState.cpp:162
> +    RefPtr<InspectorObject> object;

seems like an unused variable

> WebCore/inspector/InspectorState.cpp:170
> +    i->second.m_value = value;

you can alias i->second for clarity (since it is used 3 times in the function)

> WebCore/inspector/front-end/BreakpointManager.js:138
> +                breakpoints[breakpoint.url][line] = {enabled: breakpoint.enabled, condition: breakpoint.condition};

while we have no consistent opinion on this, having spaces after '{' and before '}' is preferred (as noted by joepeck)

> WebCore/inspector/front-end/Settings.js:91
> +            } catch(e) {

since empty catch blocks are considered a poor practice, please add a comment clarifying why this fall-through is OK

[Pavel Podivilov]

Created attachment 72169 [details]
Patch.

All comments addressed.

[Pavel Podivilov]

Created attachment 77102 [details]
Patch.

[Yury Semikhatsky]

Comment on attachment 77102 [details]
Patch.

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

> WebCore/bindings/js/ScriptDebugServer.cpp:162
> +    intptr_t sourceIDValue = tokens[0].toIntPtr();

Please check for possible parse errors by passing &success flag into toIntPtr.

> WebCore/inspector/Inspector.idl:248
> +        [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out String sourceID, out unsigned int lineNumber, out String condition, out boolean enabled, out unsigned int originalLineNumber);

This method should report breakpointId if we have decided to address breakpoints using ids in other places, otherwise the API is inconsistent.

[Yury Semikhatsky]

Comment on attachment 77102 [details]
Patch.

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

> WebCore/inspector/front-end/BreakpointManager.js:114
> +        this._setNativeBreakpoint(breakpointId, breakpoint, enabled, restored);

this method should be renamed

[Pavel Podivilov]

Created attachment 77124 [details]
Patch.

[Pavel Podivilov]

Created attachment 77125 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #5)
> (From update of attachment 77102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77102&action=review
> 
> > WebCore/bindings/js/ScriptDebugServer.cpp:162
> > +    intptr_t sourceIDValue = tokens[0].toIntPtr();
> 
> Please check for possible parse errors by passing &success flag into toIntPtr.
> 

Done.

> > WebCore/inspector/Inspector.idl:248
> > +        [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out String sourceID, out unsigned int lineNumber, out String condition, out boolean enabled, out unsigned int originalLineNumber);
> 
> This method should report breakpointId if we have decided to address breakpoints using ids in other places, otherwise the API is inconsistent.

We need to know in frontend scriptID and line where breakpoint was resolved by JS engine.

[Yury Semikhatsky]

Comment on attachment 77125 [details]
Patch.

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

> WebCore/inspector/Inspector.idl:248
> +        [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out String sourceID, out unsigned int lineNumber, out String condition, out boolean enabled, out unsigned int originalLineNumber);

no need to pass condition and enable parameters as FE already knows them

[Pavel Podivilov]

Committed r74473: <http://trac.webkit.org/changeset/74473>

[Pavel Podivilov]

(In reply to comment #10)
> (From update of attachment 77125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77125&action=review
> 
> > WebCore/inspector/Inspector.idl:248
> > +        [notify, domain=Debugger] void breakpointResolved(out String breakpointId, out String sourceID, out unsigned int lineNumber, out String condition, out boolean enabled, out unsigned int originalLineNumber);
> 
> no need to pass condition and enable parameters as FE already knows them

We have no guarantee this data would not change on frontend between call to InspectorBackend.setStickyBreakpoints() and the moment when breakpointResolved notification is dispatched on frontend. So we have to pass this data to frontend to be sure that it's up to date.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74473 might have broken Leopard Intel Release (Build) and Leopard Intel Debug (Build)

[Pavel Podivilov]

Committed r74477: <http://trac.webkit.org/changeset/74477>

[Tony Gentilcore]

I believe this is causing inspector/timeline-mark-timeline.html to crash on win debug. I'll add an expectation.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=inspector%2Ftimeline-mark-timeline.html

[Tony Gentilcore]

(In reply to comment #15)
> I believe this is causing inspector/timeline-mark-timeline.html to crash on win debug. I'll add an expectation.
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=inspector%2Ftimeline-mark-timeline.html

Nevermind, just flaky.