Bug 50679 – Web Inspector: eliminate SourceFrameDelegate by passing scripts to SourceFrame constructor

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

[Pavel Podivilov]

Web Inspector: eliminate SourceFrameDelegate by passing scripts to SourceFrame constructor

[Pavel Podivilov]

Created attachment 75887 [details]
Patch.

[Pavel Feldman]

Comment on attachment 75887 [details]
Patch.

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

> WebCore/inspector/front-end/Resource.js:512
>          script.resource = this;

Do we actually use this link?

> WebCore/inspector/front-end/Script.js:88
> +        this.dispatchEventToListeners(WebInspector.Script.Events.SourceChanged, revertCallback || function() {});

callback should not be a part of the event data - what if there are several listeners? what should they do to callback? r- for this.

[Pavel Podivilov]

Created attachment 75914 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #2)
> (From update of attachment 75887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75887&action=review
> 
> > WebCore/inspector/front-end/Resource.js:512
> >          script.resource = this;
> 
> Do we actually use this link?
It's extensively used by ScriptsPanel.

> 
> > WebCore/inspector/front-end/Script.js:88
> > +        this.dispatchEventToListeners(WebInspector.Script.Events.SourceChanged, revertCallback || function() {});
> 
> callback should not be a part of the event data - what if there are several listeners? what should they do to callback? r- for this.

In new patch resource is updated directly like it was done in original code. When we'll move scripts and live edit to DebuggerModel we may eliminate script.resource link.

[Pavel Podivilov]

Created attachment 76186 [details]
Do not regress live edit in ScriptView.

[Pavel Feldman]

Comment on attachment 76186 [details]
Do not regress live edit in ScriptView.

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

> WebCore/inspector/front-end/SourceFrame.js:811
> +        if (script.resource)

I think this can be encapsulated in script.source setter.

> WebCore/inspector/front-end/SourceFrame.js:826
> +        return WebInspector.debuggerModel.findBreakpoints(function(b) { return b.sourceID in scripts; });

nit: findBreakpoints -> queryBreakpoints.

[Pavel Podivilov]

Committed r73730: <http://trac.webkit.org/changeset/73730>

[Pavel Podivilov]

(In reply to comment #6)
> (From update of attachment 76186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76186&action=review
> 
> > WebCore/inspector/front-end/SourceFrame.js:811
> > +        if (script.resource)
> 
> I think this can be encapsulated in script.source setter.
There is revertEditLineCallback argument, so we can't encapsulate this in setter.

> 
> > WebCore/inspector/front-end/SourceFrame.js:826
> > +        return WebInspector.debuggerModel.findBreakpoints(function(b) { return b.sourceID in scripts; });
> 
> nit: findBreakpoints -> queryBreakpoints.

Done.

[Ilya Tikhonovsky]

*** Bug 51101 has been marked as a duplicate of this bug. ***