Bug 55823 - Web Inspector: re-implement breakpoints sidebar pane based on debugger presentation model.
Summary: Web Inspector: re-implement breakpoints sidebar pane based on debugger presen...
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: Pavel Podivilov
URL:
Keywords:
: 55822 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-03-05 06:46 PST by Pavel Podivilov
Modified: 2011-03-10 04:26 PST (History)
10 users (show)

See Also:

Attachments
Patch. (20.56 KB, patch)
2011-03-05 06:47 PST, Pavel Podivilov 		pfeldman: review-
Details | Formatted Diff | Diff
Patch. (17.98 KB, patch)
2011-03-09 04:42 PST, Pavel Podivilov 		pfeldman: review-
Details | Formatted Diff | Diff
Patch. (22.14 KB, patch)
2011-03-09 06:25 PST, Pavel Podivilov 		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 Pavel Podivilov 2011-03-05 06:46:32 PST 
Web Inspector: re-implement breakpoints sidebar pane as a simple ui component.

The code would be cleaner if scripts panel is the only debugger model's client.
Comment 1 Pavel Podivilov 2011-03-05 06:47:21 PST 
Created attachment 84863 [details]
Patch.
Comment 2 Pavel Feldman 2011-03-06 10:50:26 PST 
Comment on attachment 84863 [details]
Patch.

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

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1056
> +    _javaScriptBreakpointsSidebarPaneBreakpointClicked: function(event)

I think the code would be much cleaner if you had a Breakpoint class with corresponding methods on it. Did you consider this option? It would also result it way less changes.
Comment 3 Pavel Podivilov 2011-03-09 04:42:23 PST 
Created attachment 85159 [details]
Patch.
Comment 4 Pavel Podivilov 2011-03-09 04:56:53 PST 
(In reply to comment #2)
> (From update of attachment 84863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84863&action=review
> 
> > Source/WebCore/inspector/front-end/ScriptsPanel.js:1056
> > +    _javaScriptBreakpointsSidebarPaneBreakpointClicked: function(event)
> 
> I think the code would be much cleaner if you had a Breakpoint class with corresponding methods on it. Did you consider this option? It would also result it way less changes.

Moved event handlers back to JavaScriptBreakpointsSidebarPane class.
I don't think Breakpoint class would make the code any cleaner, it just shadows the dependencies.
Comment 5 Pavel Feldman 2011-03-09 05:12:13 PST 
Comment on attachment 85159 [details]
Patch.

Could you please explain why are you doing this refactoring? Bug title says: "re-implement breakpoints sidebar pane as a simple ui component", but I see this._model being introduced in the sidebar section class. I.e. the opposite is happening. I am fine with having model in the sidebar, but I am confused wrt what exactly is happening and why.
Comment 6 Pavel Podivilov 2011-03-09 06:25:29 PST 
Created attachment 85169 [details]
Patch.
Comment 7 Pavel Podivilov 2011-03-09 06:26:06 PST 
(In reply to comment #5)
> (From update of attachment 85159 [details])
> Could you please explain why are you doing this refactoring? Bug title says: "re-implement breakpoints sidebar pane as a simple ui component", but I see this._model being introduced in the sidebar section class. I.e. the opposite is happening. I am fine with having model in the sidebar, but I am confused wrt what exactly is happening and why.

Sorry about that, forgot to update the title.
Comment 8 Pavel Podivilov 2011-03-09 06:27:14 PST 
*** Bug 55822 has been marked as a duplicate of this bug. ***
Comment 9 Pavel Podivilov 2011-03-10 04:26:55 PST 
Committed r80705: <http://trac.webkit.org/changeset/80705>