Bug 66224

Summary: Web Inspector: extract breakpoint management code to a helper class and add tests.
Product: WebKit Reporter: Pavel Podivilov <podivilov>
Component: Web Inspector (Deprecated)Assignee: Pavel Podivilov <podivilov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, aroben, bweinstein, joepeck, keishi, loislo, mrobinson, ossy, pfeldman, pmuellr, rik, rniwa, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66623    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch pfeldman: review+

Description Pavel Podivilov 2011-08-15 07:34:11 PDT
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.
Comment 1 Pavel Podivilov 2011-08-15 07:35:45 PDT
Created attachment 103912 [details]
Patch
Comment 2 Pavel Podivilov 2011-08-15 07:41:09 PDT
For actually switching to BreakpointHelper in DebuggerPresentationModel see https://bugs.webkit.org/show_bug.cgi?id=66225.
Comment 3 Pavel Podivilov 2011-08-16 05:23:36 PDT
Created attachment 104028 [details]
Patch
Comment 4 Pavel Podivilov 2011-08-16 05:24:21 PDT
(In reply to comment #3)
> Created an attachment (id=104028) [details]
> Patch

Split BreakpointHelper into 3 classes and cleanup the code.
Comment 5 Pavel Feldman 2011-08-18 01:56:58 PDT
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.
Comment 6 Pavel Podivilov 2011-08-18 04:50:13 PDT
Created attachment 104324 [details]
Patch
Comment 7 Pavel Feldman 2011-08-18 07:45:16 PDT
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.
Comment 8 Pavel Podivilov 2011-08-19 08:50:49 PDT
Committed r93415: <http://trac.webkit.org/changeset/93415>
Comment 10 Pavel Feldman 2011-08-19 23:31:03 PDT
> I don't even know on which platform this test passes.

I'm rolling it now, sorry for trouble.