[webkit-reviews] review denied: [Bug 50223] Web Inspector: refactor SourceFrame to simplify implementing of js beautifier : [Attachment 75490] Remove breakpoint storages from SourceFrame.

[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 75490: Remove breakpoint storages from SourceFrame.
https://bugs.webkit.org/attachment.cgi?id=75490&action=review

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

Looks good, but still requires polish.

> WebCore/inspector/front-end/BreakpointManager.js:71
> +    breakpointsForSourceIDs: function(sourceIDs)

It is more efficient, but API method like this does not look natural. It seems
like having this filter belonging to the client would be better (i.e. making
findBreakpoints(filter) public).

> WebCore/inspector/front-end/ScriptView.js:111
> +    _url: function()

I don't see where this is used.

> WebCore/inspector/front-end/SourceFrame.js:43
> +    this._delegate = delegate;

Generic name like "_delegate" with no declared api seems too bad. You should
have defined a class for it. Like WebInspector.SourceFrameDelegate with
constructor that receives all the callbacks..

> WebCore/inspector/front-end/SourceFrame.js:106
>	   if (this._textViewer)

Who is calling this method? Should there be a listener within SourceFrame that
handles it now?

> WebCore/inspector/front-end/SourceFrame.js:431
> +	   var breakpoints = this._delegate.findAllBreakpoints();

findAllBreakpoints -> breakpoints() ?

> WebCore/inspector/front-end/SourceFrame.js:475
> +	   if (!WebInspector.panels.scripts)

We don't seem to depend on panels.scripts anymore. Should be based on delegate

> WebCore/inspector/front-end/SourceView.js:137
> +	   WebInspector.breakpointManager.setBreakpoint(sourceID, this._url(),
line, true, "");

Ok, _url is used here, but this is too bad. In fact, now that ResourceView is
so basic, we can inherit SourceView and ScriptView from each other. Or both
from a single base class.

> WebCore/inspector/front-end/SourceView.js:142
> +    _findAllBreakpoints: function()

It would be nice to extract these into SourceFrameDelegateImpl