Bug 66224 - Web Inspector: extract breakpoint management code to a helper class and add tests.
Summary: Web Inspector: extract breakpoint management code to a helper class and add t...
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: 66623
Blocks:
  Show dependency tree / graph
 
Reported: 2011-08-15 07:34 PDT by Pavel Podivilov
Modified: 2011-08-19 23:31 PDT (History)
14 users (show)

See Also:

Attachments
Patch (28.77 KB, patch)
2011-08-15 07:35 PDT, Pavel Podivilov 		no flags Details | Formatted Diff | Diff
Patch (29.88 KB, patch)
2011-08-16 05:23 PDT, Pavel Podivilov 		no flags Details | Formatted Diff | Diff
Patch (33.67 KB, patch)
2011-08-18 04:50 PDT, Pavel Podivilov 		pfeldman: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.