Bug 81669

Summary: Web Inspector: Extract WebInspector.UIBreakpoint from WebInspector.Breakpoint.
Product: WebKit Reporter: Vsevolod Vlasov <vsevik>
Component: Web Inspector (Deprecated)Assignee: Vsevolod Vlasov <vsevik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, kkristof, loislo, pfeldman, pmuellr, podivilov, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch pfeldman: review+

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