Bug 50567 - Web Inspector: introduce DebuggerModel class representing InspectorDebuggerAgent state
Summary: Web Inspector: introduce DebuggerModel class representing InspectorDebuggerAg...
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: 2010-12-06 06:19 PST by Pavel Podivilov
Modified: 2010-12-08 03:18 PST (History)
10 users (show)

See Also:


Attachments
Patch. (34.51 KB, patch)
2010-12-06 06:20 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Comments addressed. (40.66 KB, patch)
2010-12-07 06:30 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 2010-12-06 06:19:17 PST
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.
Comment 1 Pavel Podivilov 2010-12-06 06:20:23 PST
Created attachment 75686 [details]
Patch.
Comment 2 Alexander Pavlov (apavlov) 2010-12-06 06:38:07 PST
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 3 Pavel Feldman 2010-12-07 05:32:23 PST
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?
Comment 4 Pavel Podivilov 2010-12-07 06:30:11 PST
Created attachment 75807 [details]
Comments addressed.
Comment 5 Pavel Podivilov 2010-12-07 06:31:03 PST
(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
Comment 6 Pavel Podivilov 2010-12-07 06:36:43 PST
(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.
Comment 7 Pavel Podivilov 2010-12-08 03:18:15 PST
Committed r73501: <http://trac.webkit.org/changeset/73501>