Bug 50679

Summary: Web Inspector: eliminate SourceFrameDelegate by passing scripts to SourceFrame constructor
Product: WebKit Reporter: Pavel Podivilov <podivilov>
Component: Web Inspector (Deprecated)Assignee: Pavel Podivilov <podivilov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch.
pfeldman: review-
Patch.
none
Do not regress live edit in ScriptView. pfeldman: review+

Description Pavel Podivilov 2010-12-08 04:15:19 PST
Web Inspector: eliminate SourceFrameDelegate by passing scripts to SourceFrame constructor
Comment 1 Pavel Podivilov 2010-12-08 04:15:56 PST
Created attachment 75887 [details]
Patch.
Comment 2 Pavel Feldman 2010-12-08 04:21:53 PST
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.
Comment 3 Pavel Podivilov 2010-12-08 09:13:28 PST
Created attachment 75914 [details]
Patch.
Comment 4 Pavel Podivilov 2010-12-08 09:15:57 PST
(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.
Comment 5 Pavel Podivilov 2010-12-10 05:39:50 PST
Created attachment 76186 [details]
Do not regress live edit in ScriptView.
Comment 6 Pavel Feldman 2010-12-10 08:11:43 PST
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.
Comment 7 Pavel Podivilov 2010-12-10 08:33:28 PST
Committed r73730: <http://trac.webkit.org/changeset/73730>
Comment 8 Pavel Podivilov 2010-12-10 08:35:52 PST
(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.
Comment 9 Ilya Tikhonovsky 2010-12-15 03:35:37 PST
*** Bug 51101 has been marked as a duplicate of this bug. ***