Bug 81669 - Web Inspector: Extract WebInspector.UIBreakpoint from WebInspector.Breakpoint.
Summary: Web Inspector: Extract WebInspector.UIBreakpoint from WebInspector.Breakpoint.
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: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-20 08:49 PDT by Vsevolod Vlasov
Modified: 2012-03-22 02:57 PDT (History)
12 users (show)

See Also:

Attachments
Patch (32.32 KB, patch)
2012-03-20 09:27 PDT, Vsevolod Vlasov 		no flags Details | Formatted Diff | Diff
Patch (32.29 KB, patch)
2012-03-21 05:12 PDT, Vsevolod Vlasov 		no flags Details | Formatted Diff | Diff
Patch (31.38 KB, patch)
2012-03-21 11:03 PDT, Vsevolod Vlasov 		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 Vsevolod Vlasov 2012-03-20 08:49:10 PDT 
Currently persisted breakpoint can potentially be used in zero to many uiSourceCodes.
Splitting WebInspector.Breakpoint into WebInspector.Breakpoint and WebInspector.UIBreakpoint and moving some logic to new objects will make code simpler and cleaner.
Comment 1 Vsevolod Vlasov 2012-03-20 09:27:03 PDT 
Created attachment 132838 [details]
Patch
Comment 2 Pavel Feldman 2012-03-20 12:20:56 PDT 
I guess Pavel P. should take a look first.
Comment 3 Vsevolod Vlasov 2012-03-21 05:12:56 PDT 
Created attachment 133015 [details]
Patch
Comment 4 Pavel Podivilov 2012-03-21 06:50:26 PDT 
Comment on attachment 133015 [details]
Patch

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

Could you please extract DPM-related part into a separate change?

> Source/WebCore/inspector/front-end/BreakpointManager.js:47
> +    this._uiBreakpointsByUILocation = {};

Please use UISourceCode as key type.

> Source/WebCore/inspector/front-end/BreakpointManager.js:-61
> -        if (!this._breakpoint(breakpoint.uiSourceCodeId, breakpoint.lineNumber))

It checks for duplicates, please keep it as is.

> Source/WebCore/inspector/front-end/BreakpointManager.js:92
> +        var breakpoints = this._breakpoints(uiSourceCode.id);

Could you iterate over uiBreakpointsByUILocation map instead?

> Source/WebCore/inspector/front-end/BreakpointManager.js:184
>      _addBreakpointToUI: function(breakpoint)

Please inline this method.

> Source/WebCore/inspector/front-end/BreakpointManager.js:220
> +        if (this._breakpoint(breakpoint.id, lineNumber))

breakpoint.id should be breakpoint.uiSourceCodeId

> Source/WebCore/inspector/front-end/BreakpointManager.js:434
> +        for (var id in this._uiBreakpointsByUILocation) {

You should just clear this map.

> Source/WebCore/inspector/front-end/BreakpointManager.js:489
> +    createUIBreakpoint: function(uiSourceCode)

Please make private.

> Source/WebCore/inspector/front-end/BreakpointManager.js:524
> +    return new WebInspector.Breakpoint(uiSourceCode.id, lineNumber, condition, enabled, !!uiSourceCode.url);

Please inline.

> Source/WebCore/inspector/front-end/BreakpointManager.js:550
> +    someFunction: function()

?
Comment 5 Vsevolod Vlasov 2012-03-21 11:03:16 PDT 
Created attachment 133076 [details]
Patch
Comment 6 Pavel Podivilov 2012-03-21 11:48:18 PDT 
Comment on attachment 133076 [details]
Patch

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

looks good.

> Source/WebCore/inspector/front-end/BreakpointManager.js:45
> +     * @type {Object.<WebInspector.UISourceCode, Object.<string,WebInspector.UIBreakpoint>>}

Please fix the annotation.
Comment 7 Vsevolod Vlasov 2012-03-21 12:43:08 PDT 
Committed r111595: <http://trac.webkit.org/changeset/111595>
Comment 8 Kristóf Kosztyó 2012-03-22 02:57:08 PDT 
Hi

The expected for the inspector/debugger/breakpoint-manager.html was wrong for every port except the chromium, so I rebaseline it at r111666. http://trac.webkit.org/changeset/111666