Bug 66224 – Web Inspector: extract breakpoint management code to a helper class and add tests.

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

[Pavel Podivilov]

Web Inspector: extract breakpoint management code to a helper class and add tests.

Our breakpoint management code is quite complex, why don't we move it to a separate class and add a set of unit tests for it?
The next step would be to dispatch breakpoint events on uiSourceCode objects directly and thus completely decouple breakpoint management code from the rest of presentation model's code.

[Pavel Podivilov]

Created attachment 103912 [details]
Patch

[Pavel Podivilov]

For actually switching to BreakpointHelper in DebuggerPresentationModel see https://bugs.webkit.org/show_bug.cgi?id=66225.

[Pavel Podivilov]

Created attachment 104028 [details]
Patch

[Pavel Podivilov]

(In reply to comment #3)
> Created an attachment (id=104028) [details]
> Patch

Split BreakpointHelper into 3 classes and cleanup the code.

[Pavel Feldman]

Comment on attachment 104028 [details]
Patch

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

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:705
> +WebInspector.BreakpointHelper = function(debuggerModel, breakpoints, breakpointAddedDelegate, breakpointRemovedDelegate)

Class with two inner classes deserves to be defined in a separate file.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:720
> +    registerUISourceCode: function(uiSourceCodeId, rawSourceCode, persistent)

Can we pass real instance here? Also, remove <persist>.
This could be called uiSourceCodeAdded

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:722
> +        this._uiSourceCodeInfo[uiSourceCodeId] = { rawSourceCode: rawSourceCode, persistent: persistent };

This mapping should be bi-directional, should be defined in the RawSourceCode <-> UISourceCode terms.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:745
> +        this._uiBreakpoints.addBreakpoint(breakpoint);

Either pass all the parameters into addBreakpoint or define a class for a breakpoint.

[Pavel Podivilov]

Created attachment 104324 [details]
Patch

[Pavel Feldman]

Comment on attachment 104324 [details]
Patch

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

> Source/WebCore/inspector/front-end/BreakpointManager.js:31
> +WebInspector.BreakpointManager = function(debuggerModel, breakpoints, breakpointAddedDelegate, breakpointRemovedDelegate)

Please remove the breakpoints parameter and replace it with the "setBreakpointsForTest" method.

[Pavel Podivilov]

Committed r93415: <http://trac.webkit.org/changeset/93415>

[Ryosuke Niwa]

The test added by this patch (inspector/debugger/breakpoint-manager.html) is failing everywhere:
http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r93465%20(31415)/inspector/debugger/breakpoint-manager-pretty-diff.html
http://build.webkit.org/results/Qt%20Linux%20Release/r93416%20(36643)/inspector/debugger/breakpoint-manager-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r93461%20(1777)/inspector/debugger/breakpoint-manager-pretty-diff.html

I don't even know on which platform this test passes.

[Pavel Feldman]

> I don't even know on which platform this test passes.

I'm rolling it now, sorry for trouble.