Bug 50567 – Web Inspector: introduce DebuggerModel class representing InspectorDebuggerAgent state

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

[Pavel Podivilov]

Web Inspector: introduce DebuggerModel class representing InspectorDebuggerAgent state.
Debugging-related code moved from BreakpointManager to DebuggerModel. The next step would be to extract model aspects such as parsed scripts and current execution line from ScriptsPanel to DebuggerModel.

[Pavel Podivilov]

Created attachment 75686 [details]
Patch.

[Alexander Pavlov (apavlov)]

Comment on attachment 75686 [details]
Patch.

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

Not checking the overall logic as I'm not quite familiar with it.

> WebCore/ChangeLog:8
> +        * English.lproj/localizedStrings.js:

For this file's diff to be found in the patch, you need to use the "--binary" flag.

> WebCore/WebCore.vcproj/WebCore.vcproj:65215
> +        <File

You seem to have spaces rather than tabs here. The file should only use tabs as leading indents, and all the stuff should be aligned.

> WebCore/inspector/front-end/DebuggerModel.js:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY

You should use the correct, up-to-date license boilerplate. Make use of the one found e.g. in ExtensionPanel.js

> WebCore/inspector/front-end/inspector.js:1306
> +    this.debuggerModel.restoredBreakpoint(sourceID, sourceURL, line, enabled, condition);

The method had better been called "breakpointRestored", as this seems to be an event notification, right? (akin to "debuggerResumed")

[Pavel Feldman]

Comment on attachment 75686 [details]
Patch.

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

What about WebInspector.parsedScriptSource, debuggerWasEnabled, debuggerWasDisabled, activateBreakpoints, stepOverStatement, etc.? They all should go through the DebuggerModel, right?

> WebCore/inspector/front-end/DebuggerModel.js:27
> +WebInspector.DebuggerModel = function()

Is this only a rename for BreakpointManager? Could you describe changes in the ChangeLog?

> WebCore/inspector/front-end/DebuggerModel.js:143
> +        breakpoint.dispatchEventToListeners("hit-state-changed");

Breakpoint event dispatching should be encapsulated in the hit setter.

> WebCore/inspector/front-end/DebuggerModel.js:145
> +        this.dispatchEventToListeners("script-breakpoint-hit", breakpoint);

This method fires way too many events. Events-driven operation brings in a lot of indirection. It should only be used when necessary (i.e. breaking unhealthy dependencies).

> WebCore/inspector/front-end/DebuggerModel.js:162
> +WebInspector.Breakpoint = function(debuggerModel, sourceID, url, line, enabled, condition)

Should this be extracted into a separate file?

[Pavel Podivilov]

Created attachment 75807 [details]
Comments addressed.

[Pavel Podivilov]

(In reply to comment #2)
> (From update of attachment 75686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75686&action=review
> 
> Not checking the overall logic as I'm not quite familiar with it.
> 
> > WebCore/ChangeLog:8
> > +        * English.lproj/localizedStrings.js:
> 
> For this file's diff to be found in the patch, you need to use the "--binary" flag.

cq doesn't process binary diffs.

> 
> > WebCore/WebCore.vcproj/WebCore.vcproj:65215
> > +        <File
> 
> You seem to have spaces rather than tabs here. The file should only use tabs as leading indents, and all the stuff should be aligned.
> 
> > WebCore/inspector/front-end/DebuggerModel.js:14
> > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
> 
> You should use the correct, up-to-date license boilerplate. Make use of the one found e.g. in ExtensionPanel.js
> 
> > WebCore/inspector/front-end/inspector.js:1306
> > +    this.debuggerModel.restoredBreakpoint(sourceID, sourceURL, line, enabled, condition);
> 
> The method had better been called "breakpointRestored", as this seems to be an event notification, right? (akin to "debuggerResumed")

done

[Pavel Podivilov]

(In reply to comment #3)
> (From update of attachment 75686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75686&action=review
> 
> What about WebInspector.parsedScriptSource, debuggerWasEnabled, debuggerWasDisabled, activateBreakpoints, stepOverStatement, etc.? They all should go through the DebuggerModel, right?

Yes. I'll extract stuff from ScriptsPanel as a separate patch.

> 
> > WebCore/inspector/front-end/DebuggerModel.js:27
> > +WebInspector.DebuggerModel = function()
> 
> Is this only a rename for BreakpointManager? Could you describe changes in the ChangeLog?

Not exactly, debugging-related code (that mirrors InspectorDebuggerAgent) moved from BreakpointManager to DebuggerModel. Updated ChangeLog.

> 
> > WebCore/inspector/front-end/DebuggerModel.js:143
> > +        breakpoint.dispatchEventToListeners("hit-state-changed");
> 
> Breakpoint event dispatching should be encapsulated in the hit setter.
> 

Done.

> > WebCore/inspector/front-end/DebuggerModel.js:145
> > +        this.dispatchEventToListeners("script-breakpoint-hit", breakpoint);
> 
> This method fires way too many events. Events-driven operation brings in a lot of indirection. It should only be used when necessary (i.e. breaking unhealthy dependencies).

debuggerPaused event has a lot of effects on different parts of UI. We may merge some events together if it appears to be a performance problem.

> 
> > WebCore/inspector/front-end/DebuggerModel.js:162
> > +WebInspector.Breakpoint = function(debuggerModel, sourceID, url, line, enabled, condition)
> 
> Should this be extracted into a separate file?

Done.

[Pavel Podivilov]

Committed r73501: <http://trac.webkit.org/changeset/73501>