[webkit-reviews] review denied: [Bug 56748] Web Inspector: move content loading logic to a new SourceFile class. : [Attachment 86323] Patch.

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 56748: Web Inspector: move content loading logic to a new SourceFile class.
https://bugs.webkit.org/show_bug.cgi?id=56748

Attachment 86323: Patch.
https://bugs.webkit.org/attachment.cgi?id=86323&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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?