[webkit-reviews] review denied: [Bug 14190] Breakpoints should persist across launches : [Attachment 57318] Breakpoints adding/removing logic refactored.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  review:
Bug 14190: Breakpoints should persist across launches
https://bugs.webkit.org/show_bug.cgi?id=14190

Attachment 57318: Breakpoints adding/removing logic refactored. 
https://bugs.webkit.org/attachment.cgi?id=57318&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
A bunch of renames below. Otherwise looks very good.

WebCore/inspector/front-end/BreakpointManager.js:2
 +   * Copyright (C) 2008 Apple Inc. All Rights Reserved.
Please append Google copyright 2010 string.

WebCore/inspector/front-end/BreakpointManager.js:34
 +	    return new WebInspector.Breakpoint(sourceID, sourceURL, line,
enabled, condition, this);
Nit: 'this' is usually a first parameter in the list.

WebCore/inspector/front-end/BreakpointManager.js:40
 +	       return;
Poor indentation

WebCore/inspector/front-end/BreakpointManager.js:55
 +	getBreakpointsBySourceID: function(sourceID) {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:64
 +	getBreakpointsByURL: function(url) {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:73
 +	reset: function() {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:98
 +	this._manager = manager;
this._breakpointManager

WebCore/inspector/front-end/BreakpointManager.js:77
 +	_saveBreakpointToBackend: function(breakpoint)
_setBreakpointOnBackend

WebCore/inspector/front-end/BreakpointManager.js:82
 +	_removeBreakpointFromBackend: function(breakpoint)
_removeBreakpointFromBackend

WebCore/inspector/front-end/BreakpointManager.js:64
 +	getBreakpointsByURL: function(url) {
In WebCore, get is not used, so this one would be breakpointsForURL


WebCore/inspector/front-end/BreakpointManager.js:32
 +	createBreakpoint: function(sourceID, sourceURL, line, enabled,
condition)
You always add breakpoints after creation. Consider merging these methods.

WebCore/inspector/front-end/ScriptsPanel.js:346
 +	    var breakpoints =
WebInspector.breakpointManager.getBreakpointsBySourceID(sourceID);
You only use getBreakpointsBySourceID here. Consider encapsulating the whole
routine (removeBreakpointsWithSourceID).