Bug 40063 - Web Inspector: make SourceFrame BreakpointManager's listener
Summary: Web Inspector: make SourceFrame BreakpointManager's listener
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-02 08:14 PDT by Yury Semikhatsky
Modified: 2011-02-08 11:24 PST (History)
8 users (show)

See Also:


Attachments
Patch (18.14 KB, patch)
2010-06-03 11:09 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (17.66 KB, patch)
2010-06-04 06:26 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2010-06-02 08:14:37 PDT
1. Make SourceFrame BreakpointManager's listener
1.1. Make SourceFrame responsible for filling breakpoints on creation (few addBreakpoint snippets will go away from ScriptsPanel)
2. Remove _sourceFrameForScriptOrResource from the ScriptsPanel(most of its usages are needed to populate new SourceView with existing breakpoints).
Comment 1 Yury Semikhatsky 2010-06-03 11:09:09 PDT
Created attachment 57791 [details]
Patch
Comment 2 Pavel Feldman 2010-06-03 11:45:12 PDT
Comment on attachment 57791 [details]
Patch

WebCore/inspector/front-end/ScriptsPanel.js:279
 +              if (script._scriptView) {
Nit: encapsulate this into script.disposeView();

WebCore/inspector/front-end/ScriptsPanel.js:448
 +                      object._scriptView.willRemoveScript();
Why is this needed?

WebCore/inspector/front-end/SourceFrame.js:198
 +          if (!(breakpoint.sourceID in this._scriptIds)) {
So you ended up with making SourceFrame both sourceID and sourceURL aware without external binding. It might be simple, not sure it is good design-wise though.

WebCore/inspector/front-end/SourceView.js:129
 +              this.sourceFrame.addBreakpointsForURL(resource.sourceURL);
I thought the point was to make SourceFrame breakpoint manager's listener so that breakpoints are not populated from outside.

WebCore/inspector/front-end/SourceView.js:240
 +              this.localSourceFrame = new WebInspector.SourceFrame(this.localContentElement, null, null, null, null);
just pass single parameter.
Comment 3 Yury Semikhatsky 2010-06-04 06:25:16 PDT
(In reply to comment #2)
> (From update of attachment 57791 [details])
> WebCore/inspector/front-end/ScriptsPanel.js:279
>  +              if (script._scriptView) {
> Nit: encapsulate this into script.disposeView();
> 
Script and Resource are not aware of there views (despite they have _sourceView and _scriptView fields) that is why I didn't add disposeView to them.


> WebCore/inspector/front-end/ScriptsPanel.js:448
>  +                      object._scriptView.willRemoveScript();
> Why is this needed?
> 
To remove SourceFrame from BreakpointManager listeners list.


> WebCore/inspector/front-end/SourceFrame.js:198
>  +          if (!(breakpoint.sourceID in this._scriptIds)) {
> So you ended up with making SourceFrame both sourceID and sourceURL aware without external binding. It might be simple, not sure it is good design-wise though.
> 
We need either them or a filter to detect breakpoints that should be displayed.


> WebCore/inspector/front-end/SourceView.js:129
>  +              this.sourceFrame.addBreakpointsForURL(resource.sourceURL);
> I thought the point was to make SourceFrame breakpoint manager's listener so that breakpoints are not populated from outside.
> 
Adding listeners to BreakpointManager is not enough because we need to pull breakpoints that already exist when SourceFrame is being created. I'm open to alternative solutions.


> WebCore/inspector/front-end/SourceView.js:240
>  +              this.localSourceFrame = new WebInspector.SourceFrame(this.localContentElement, null, null, null, null);
> just pass single parameter.
Done.
Comment 4 Yury Semikhatsky 2010-06-04 06:26:44 PDT
Created attachment 57868 [details]
patch
Comment 5 Pavel Feldman 2010-07-18 12:18:28 PDT
Comment on attachment 57868 [details]
patch

As we agreed, we need a better solution. Clearing r? for now.
Comment 6 Pavel Feldman 2011-02-08 11:24:17 PST
Obsolete.