Bug 44424

Summary: Web Inspector: show DOM breakpoints in sidebar pane
Product: WebKit Reporter: Pavel Podivilov <podivilov>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, pfeldman, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Proposed patch.
none
Screenshot.
none
Add check boxes.
none
Screenshot.
none
Add check boxes.
none
Proposed patch.
none
What's changed from previous patch. none

Description Pavel Podivilov 2010-08-23 07:52:33 PDT
Web Inspector: show DOM breakpoints in sidebar pane
Comment 1 Pavel Podivilov 2010-08-23 08:50:49 PDT
Created attachment 65123 [details]
Proposed patch.
Comment 2 Pavel Podivilov 2010-08-23 09:11:52 PDT
Created attachment 65129 [details]
Screenshot.
Comment 3 Pavel Podivilov 2010-08-24 02:36:06 PDT
Created attachment 65246 [details]
Add check boxes.
Comment 4 Pavel Podivilov 2010-08-24 02:41:04 PDT
Created attachment 65247 [details]
Screenshot.
Comment 5 Pavel Podivilov 2010-08-24 03:17:55 PDT
Created attachment 65252 [details]
Add check boxes.
Comment 6 Pavel Feldman 2010-08-24 05:09:48 PDT
Comment on attachment 65252 [details]
Add check boxes.

Great job! Please fix style nits and we can land this.

General nit: Instead of setting listeners to the individual breakpoints, you should add single one to the breakpoint manager.WebCore/inspector/front-end/BreakpointsSidebarPane.js:202
(can be done later)

 +  WebInspector.DOMBreakpointItem.Labels[WebInspector.DOMBreakpoint.Types.SubtreeModified] = "Subtree Modified";
It is better to move UIString call here.

WebCore/inspector/front-end/BreakpointsSidebarPane.js:212
 +      element: WebInspector.JSBreakpointItem.prototype.element,
You should either inherit or aggregate.
WebCore/inspector/front-end/inspector.js:203
 +          WebInspector.breakpointManager.addEventListener("breakpoint-added", function(event)
Please follow style guidelines when declaring functions.

WebCore/inspector/front-end/inspector.js:213
 +          WebInspector.domBreakpointManager.addEventListener("dom-breakpoint-added", function(event)
ditto
Comment 7 Pavel Podivilov 2010-08-24 06:34:31 PDT
Created attachment 65264 [details]
Proposed patch.
Comment 8 Pavel Podivilov 2010-08-24 06:36:08 PDT
Created attachment 65265 [details]
What's changed from previous patch.
Comment 9 Yury Semikhatsky 2010-08-24 07:58:40 PDT
Comment on attachment 65264 [details]
Proposed patch.

WebCore/inspector/front-end/BreakpointsSidebarPane.js:203
 +              return this._breakpoint.type < other._breakpoint.type ? -1 : 1;
You may replace this with
return this._breakpoint.type - other._breakpoint.type;

WebCore/inspector/front-end/DOMAgent.js:703
 +          for (var type in nodeBreakpoints)
This will fail if nodeBreakpoints is undefined.
Comment 10 WebKit Commit Bot 2010-08-24 15:17:35 PDT
Comment on attachment 65264 [details]
Proposed patch.

Clearing flags on attachment: 65264

Committed r65939: <http://trac.webkit.org/changeset/65939>
Comment 11 WebKit Commit Bot 2010-08-24 15:17:40 PDT
All reviewed patches have been landed.  Closing bug.