[webkit-reviews] review denied: [Bug 50567] Web Inspector: introduce DebuggerModel class representing InspectorDebuggerAgent state : [Attachment 75686] Patch.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 50567: Web Inspector: introduce DebuggerModel class representing
InspectorDebuggerAgent state
https://bugs.webkit.org/show_bug.cgi?id=50567

Attachment 75686: Patch.
https://bugs.webkit.org/attachment.cgi?id=75686&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?