DevTools: introduce TargetBreakpoints as a presentation of breakpoint and its state within target

DevTools: introduce TargetBreakpoints as a presentation of breakpoint and its state within target

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175665

[vsevik, 2014-06-02 13:37:59]

https://chromiumcodereview.appspot.com/310463003/diff/1/Source/devtools/front...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/1/Source/devtools/front...
Source/devtools/front_end/sdk/BreakpointManager.js:49: this._breakpoints = [];
Let's only keep provisional breakpoints there.

https://chromiumcodereview.appspot.com/310463003/diff/1/Source/devtools/front...
Source/devtools/front_end/sdk/BreakpointManager.js:713:
this._breakpoint._fakeBreakpointAtPrimaryLocation();
TargetBreakpoint should know nothing about faking breakpoint at primary
location.

[vsevik, 2014-06-04 08:15:22]

https://chromiumcodereview.appspot.com/310463003/diff/20001/LayoutTests/inspe...
File LayoutTests/inspector/sources/debugger/breakpoint-manager.html (right):

https://chromiumcodereview.appspot.com/310463003/diff/20001/LayoutTests/inspe...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:9:
setTimeout(InspectorTest.completeTest, 2000);
Redundant

https://chromiumcodereview.appspot.com/310463003/diff/20001/LayoutTests/inspe...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:296:
mockTarget.debuggerModel = debuggerModel;
This one is redundant

[vsevik, 2014-06-04 16:28:52]

https://chromiumcodereview.appspot.com/310463003/diff/40001/LayoutTests/inspe...
File LayoutTests/inspector/sources/debugger/breakpoint-manager.html (right):

https://chromiumcodereview.appspot.com/310463003/diff/40001/LayoutTests/inspe...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:9:
setTimeout(InspectorTest.completeTest, 2000);
please remove

https://chromiumcodereview.appspot.com/310463003/diff/40001/LayoutTests/inspe...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:296:
mockTarget.debuggerModel = debuggerModel;
still redundant

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:97:
removeProvisionalBreakpointsForTest: function(target)
This one is only used for final clean up, so you could probably remov eit for
all targets here.

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:101:
breakpoints[i].removeProvisionalBreakpointsForTest(target);
removeProvisionalForTest

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:178:
breakpoints[i]._resetLocations();
Is this change important?

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:439: /** @type
{!WebInspector.UILocation|undefined} */ this._fakeBreakpointPrimaryLocation;
I would replace this with primayrLocation() getter and isFakedAtPrimaryLocation
field.
It's actually essential that breakpoint does not have any persistent reference
to uiSourceCodes/uiLocations.

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:444:
this._setNewState(condition, enabled);
updateState

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:453:
target.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.BreakpointResolved,
this._breakpointResolved, this);
This should be in TargetBreakpoint constructor

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:462:
target.debuggerModel.removeEventListener(WebInspector.DebuggerModel.Events.BreakpointResolved,
this._breakpointResolved, this);
This should be in TargetBreakpoint.dispose()

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:464:
targetBreakpoint._resetLocations();
This should be in TargetBreakpoint.dispose()

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:474:
target.debuggerModel.removeBreakpoint(debuggerId);
I would delegate this call to TargetBreakpoint.

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:527:
this._removeFakeBreakpointAtPrimaryLocation();
this one seems redundant.

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:614:
this._targetBreakpoints.get(targets[i])._removeFromDebugger();
Again ,let's put all this stuff into TargetBreakpoint.dispose()

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:660: _breakpointResolved:
function(event)
This method belongs to TargetBreakpoint

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
File Source/devtools/front_end/sdk/Target.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/40001/Source/devtools/f...
Source/devtools/front_end/sdk/Target.js:241: removeTargetObserver:
function(targetObserver)
unobserveTargets or stopObservingTargets for consistency.

[sergeyv, 2014-06-05 12:59:51]

https://codereview.chromium.org/310463003/diff/40001/LayoutTests/inspector/so...
File LayoutTests/inspector/sources/debugger/breakpoint-manager.html (right):

https://codereview.chromium.org/310463003/diff/40001/LayoutTests/inspector/so...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:9:
setTimeout(InspectorTest.completeTest, 2000);
On 2014/06/04 16:28:53, vsevik wrote:
> please remove

Done.

https://codereview.chromium.org/310463003/diff/40001/LayoutTests/inspector/so...
LayoutTests/inspector/sources/debugger/breakpoint-manager.html:296:
mockTarget.debuggerModel = debuggerModel;
On 2014/06/04 16:28:53, vsevik wrote:
> still redundant

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:97:
removeProvisionalBreakpointsForTest: function(target)
On 2014/06/04 16:28:53, vsevik wrote:
> This one is only used for final clean up, so you could probably remov eit for
> all targets here.

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:101:
breakpoints[i].removeProvisionalBreakpointsForTest(target);
On 2014/06/04 16:28:53, vsevik wrote:
> removeProvisionalForTest

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:178:
breakpoints[i]._resetLocations();
On 2014/06/04 16:28:53, vsevik wrote:
> Is this change important?

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:444:
this._setNewState(condition, enabled);
On 2014/06/04 16:28:53, vsevik wrote:
> updateState

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:453:
target.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.BreakpointResolved,
this._breakpointResolved, this);
On 2014/06/04 16:28:53, vsevik wrote:
> This should be in TargetBreakpoint constructor

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:462:
target.debuggerModel.removeEventListener(WebInspector.DebuggerModel.Events.BreakpointResolved,
this._breakpointResolved, this);
On 2014/06/04 16:28:53, vsevik wrote:
> This should be in TargetBreakpoint.dispose()

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:464:
targetBreakpoint._resetLocations();
On 2014/06/04 16:28:53, vsevik wrote:
> This should be in TargetBreakpoint.dispose()

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:474:
target.debuggerModel.removeBreakpoint(debuggerId);
On 2014/06/04 16:28:53, vsevik wrote:
> I would delegate this call to TargetBreakpoint.

Done. But instead of it, I just call remove method on breakpoints that are
stored in provisionalBreakpoints

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:527:
this._removeFakeBreakpointAtPrimaryLocation();
On 2014/06/04 16:28:53, vsevik wrote:
> this one seems redundant.

Well, it is not :) since here we add a real location, we should remove a fake
one.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:614:
this._targetBreakpoints.get(targets[i])._removeFromDebugger();
On 2014/06/04 16:28:53, vsevik wrote:
> Again ,let's put all this stuff into TargetBreakpoint.dispose()

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:660: _breakpointResolved:
function(event)
On 2014/06/04 16:28:53, vsevik wrote:
> This method belongs to TargetBreakpoint

Done.

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/310463003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Target.js:241: removeTargetObserver:
function(targetObserver)
On 2014/06/04 16:28:53, vsevik wrote:
> unobserveTargets or stopObservingTargets for consistency.

Done.

[vsevik, 2014-06-05 13:19:10]

https://chromiumcodereview.appspot.com/310463003/diff/80001/Source/devtools/f...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/80001/Source/devtools/f...
Source/devtools/front_end/sdk/BreakpointManager.js:436: /** @type
{!WebInspector.UILocation|undefined} */ this._fakeBreakpointPrimaryLocation;
nit: this._fakePrimaryLocation

[sergeyv, 2014-06-05 13:25:53]

https://codereview.chromium.org/310463003/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://codereview.chromium.org/310463003/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/BreakpointManager.js:436: /** @type
{!WebInspector.UILocation|undefined} */ this._fakeBreakpointPrimaryLocation;
On 2014/06/05 13:19:10, vsevik wrote:
> nit: this._fakePrimaryLocation

Done.

[vsevik, 2014-06-05 13:47:16]

lgtm

[vsevik, 2014-06-05 13:56:54]

As discussed offline we need a test for this feature but since this requires
refactoring of existing test we can add it in a separate patch.

Also this change leads to N^2 time when all breakpoints are resolving on page
navigation since we add a breakpointResolved listener for each breakpoint. This
could be addressed by adding a DebuggerModel.addBreakpointResolvedListener.

[vsevik, 2014-06-05 15:48:29]

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
Source/devtools/front_end/sdk/BreakpointManager.js:685: _removeFromDebugger:
function(callCallbackImmediately)
callbackImmediately

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
Source/devtools/front_end/sdk/BreakpointManager.js:732:
this.target().debuggerModel.registerBreakpointListenerForId(this._debuggerId,
this);
addBreakpointListener(id, this._breakpointResolved)

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
Source/devtools/front_end/sdk/BreakpointManager.js:781: _dispose: function()
Looks like this method is now redundant.

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
Source/devtools/front_end/sdk/BreakpointManager.js:783:
this._removeFromDebugger();
This should not be done on TargetRemoved

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
File Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://chromiumcodereview.appspot.com/310463003/diff/140001/Source/devtools/...
Source/devtools/front_end/sdk/DebuggerModel.js:52: this._breakpointListenerById
= new StringMap();
Let's have this._breakpointResolvedEventTarget = new EventTarget() instead.

[sergeyv, 2014-06-06 11:44:53]

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/BreakpointManager.js (right):

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/sdk/BreakpointManager.js:685: _removeFromDebugger:
function(callCallbackImmediately)
On 2014/06/05 15:48:29, vsevik wrote:
> callbackImmediately

Done.

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/sdk/BreakpointManager.js:732:
this.target().debuggerModel.registerBreakpointListenerForId(this._debuggerId,
this);
On 2014/06/05 15:48:29, vsevik wrote:
> addBreakpointListener(id, this._breakpointResolved)

Done.

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/sdk/BreakpointManager.js:781: _dispose: function()
On 2014/06/05 15:48:29, vsevik wrote:
> Looks like this method is now redundant.

Done.

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/sdk/BreakpointManager.js:783:
this._removeFromDebugger();
On 2014/06/05 15:48:29, vsevik wrote:
> This should not be done on TargetRemoved

Done.

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/310463003/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/sdk/DebuggerModel.js:52: this._breakpointListenerById
= new StringMap();
On 2014/06/05 15:48:29, vsevik wrote:
> Let's have this._breakpointResolvedEventTarget = new EventTarget() instead.

Done.

[sergeyv, 2014-06-06 11:45:00]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2014-06-06 11:46:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyv@chromium.org/310463003/150001

[commit-bot: I haz the power, 2014-06-06 13:05:37]

Change committed as 175665