[webkit-reviews] review denied: [Bug 50223] Web Inspector: refactor SourceFrame to simplify implementing of js beautifier : [Attachment 75673] Comments addressed.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 50223: Web Inspector: refactor SourceFrame to simplify implementing of js
beautifier
https://bugs.webkit.org/show_bug.cgi?id=50223

Attachment 75673: Comments addressed.
https://bugs.webkit.org/attachment.cgi?id=75673&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75673&action=review

> WebCore/inspector/front-end/ScriptView.js:39
> +    this.script.addEventListener("source-changed",
this._scriptSourceChanged, this);

You are introducing a lot of named events. You should start using constants for
them.

> WebCore/inspector/front-end/ScriptsPanel.js:293
> +	   var breakpoints =
WebInspector.breakpointManager.findBreakpoints(function(b) { return b.sourceID
=== editData.sourceID });

Sound like this should belong to source frame now?

> WebCore/inspector/front-end/SourceFrame.js:780
> +WebInspector.SourceFrameDelegate.prototype = {

Most of these methods should belong to SourceFrame in order to reduce the
delegate API surface.

> WebCore/inspector/front-end/SourceView.js:247
> +WebInspector.ResourceFrameDelegate.prototype = {

SourceFrame does not display arbitrary resources, why is it
ResourceFrameDelegate? And hat is this _canEditScripts check used for?