Bug 56748 – Web Inspector: move content loading logic to a new SourceFile class.

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

[Pavel Podivilov]

Web Inspector: move content loading logic to a new SourceFile class.

[Pavel Podivilov]

Created attachment 86323 [details]
Patch.

[WebKit Review Bot]

Attachment 86323 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:66823:  not well-formed (invalid token)  [xml/syntax] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yury Semikhatsky]

Comment on attachment 86323 [details]
Patch.

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

Several minor comments, otherwise looks good.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:89
>          sourceFile.id = sourceFileId;

Move this into SourceFile constructor.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:90
> +        sourceFile._breakpoints = [];

Move this into SourceFile constructor.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:95
> +            if (!(id in this._presentationBreakpoints))

Why do we have to iterate though all breakpoints when a script is being added? Is it possible to update only breakpoints related to the script?

> Source/WebCore/inspector/front-end/SourceFile.js:31
> +WebInspector.SourceFile = function(script, contentChangedDelegate)

I don't like the name, but I don't have a better suggestion at the moment.

> Source/WebCore/inspector/front-end/SourceFile.js:111
> +        // FIXME: move provider's load functions here.

Can you fix this as part of this change?

[Pavel Podivilov]

(In reply to comment #3)
> (From update of attachment 86323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86323&action=review
> 
> Several minor comments, otherwise looks good.
> 
> > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:89
> >          sourceFile.id = sourceFileId;
> 
> Move this into SourceFile constructor.
Done.

> 
> > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:90
> > +        sourceFile._breakpoints = [];
> 
> Move this into SourceFile constructor.
Done.

> 
> > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:95
> > +            if (!(id in this._presentationBreakpoints))
> 
> Why do we have to iterate though all breakpoints when a script is being added? Is it possible to update only breakpoints related to the script?
DebuggerModel contains all breakpoints from local storage, but we want to show only those that have corresponding scripts. When the new script is added, we have to find all breakpoints matching the script and show them in UI.

> 
> > Source/WebCore/inspector/front-end/SourceFile.js:31
> > +WebInspector.SourceFile = function(script, contentChangedDelegate)
> 
> I don't like the name, but I don't have a better suggestion at the moment.
> 
> > Source/WebCore/inspector/front-end/SourceFile.js:111
> > +        // FIXME: move provider's load functions here.
> 
> Can you fix this as part of this change?

There is already a separate change for this - https://bugs.webkit.org/show_bug.cgi?id=56756.

[Pavel Podivilov]

Created attachment 86451 [details]
Patch.

[Pavel Podivilov]

Committed r81767: <http://trac.webkit.org/changeset/81767>