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.
Created attachment 75686 [details] Patch.
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")
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?
Created attachment 75807 [details] Comments addressed.
(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
(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.
Committed r73501: <http://trac.webkit.org/changeset/73501>