Bug 40063 – Web Inspector: make SourceFrame BreakpointManager's listener

• Component: Web Inspector (Deprecated)
• Status: RESOLVED WONTFIX

[Yury Semikhatsky]

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).

[Yury Semikhatsky]

Created attachment 57791 [details]
Patch

[Pavel Feldman]

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.

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 57868 [details]
patch

[Pavel Feldman]

Comment on attachment 57868 [details]
patch

As we agreed, we need a better solution. Clearing r? for now.

[Pavel Feldman]

Obsolete.