[webkit-reviews] review denied: [Bug 54645] Web Inspector: move source mapping from SourceFrame to ScriptsPanel. : [Attachment 83178] Patch.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 54645: Web Inspector: move source mapping from SourceFrame to ScriptsPanel.
https://bugs.webkit.org/show_bug.cgi?id=54645

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

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

> LayoutTests/http/tests/inspector/debugger-test.js:3
> +InspectorTest.runDebuggerTestSuite = function(steps)

There already is a runDebuggerTestSuite, why not to use existing one?

> LayoutTests/inspector/debugger/source-frame.html:16
> +	   function(next) {

A bunch of nits:
- { on the next line
- Tests should have names
- Only tests should be first class citizens in the test suite, not their steps.
How many tests are here?

> LayoutTests/inspector/debugger/source-frame.html:22
> +	   function(next, sourceFrame) {

what is sourceFrame? where is it taken from?

> LayoutTests/inspector/debugger/source-frame.html:35
> +	       InspectorTest.completeDebuggerTest();

This is not neeeded in the existing runDebuggerTestSuite.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:36
> +   
WebInspector.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.B
reakpointAdded, this._breakpointAdded, this);

If debugger presentation model is the only listener of the debugger model, why
does it listen to addition of the breakpoints? I don't think you addressed my
only concern.