Bug 54645 – Web Inspector: move source mapping from SourceFrame to ScriptsPanel.

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

[Pavel Podivilov]

Web Inspector: move source mapping from SourceFrame to ScriptsPanel.
This is needed to map one script to several source files.

[Pavel Podivilov]

Created attachment 82787 [details]
Patch.

[Pavel Feldman]

Comment on attachment 82787 [details]
Patch.

Please provide layout test for this change.

[Pavel Podivilov]

Created attachment 82951 [details]
Patch + test.

[Pavel Feldman]

Comment on attachment 82951 [details]
Patch + test.

You seem to introduce a whole new functionality in the ScriptsPanel with couple of new maps. It would be great to see them implemented in a separate module.

[Pavel Podivilov]

Created attachment 82953 [details]
Patch + test.

[Pavel Podivilov]

This is the first step of scripts panel's refactoring (as described in bug 51175).

[Pavel Feldman]

Comment on attachment 82953 [details]
Patch + test.

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

> LayoutTests/http/tests/inspector/debugger-test.js:123
> +        InspectorTest._addSniffer(WebInspector.panels.scripts, "_addScriptToFilesMenu", InspectorTest.showScriptSource.bind(InspectorTest, scriptName, callback));

Nit: It is not clear what part of the functionality you intend to test: debugger model or scripts panel with this harness.

> LayoutTests/http/tests/inspector/inspector-test.js:138
> +InspectorTest.runTest = function(steps)

Please use runTestSuite or runDebuggerTestSuite.

> LayoutTests/http/tests/inspector/inspector-test.js:149
> +                InspectorTest.completeTest();

What if that was a debugger test?

> Source/WebCore/inspector/front-end/ScriptsPanel.js:192
> +    WebInspector.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.BreakpointAdded, this._breakpointAdded, this);

It is strange that the client that adds breakpoints is listening to the breakpoints addition. r- for that.

[Pavel Podivilov]

Created attachment 83178 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #7)
> (From update of attachment 82953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82953&action=review
> 
> > LayoutTests/http/tests/inspector/debugger-test.js:123
> > +        InspectorTest._addSniffer(WebInspector.panels.scripts, "_addScriptToFilesMenu", InspectorTest.showScriptSource.bind(InspectorTest, scriptName, callback));
> 
> Nit: It is not clear what part of the functionality you intend to test: debugger model or scripts panel with this harness.

This is a helper method, it doesn't intend to test any functionality.

> 
> > LayoutTests/http/tests/inspector/inspector-test.js:138
> > +InspectorTest.runTest = function(steps)
> 
> Please use runTestSuite or runDebuggerTestSuite.

Done.

> 
> > LayoutTests/http/tests/inspector/inspector-test.js:149
> > +                InspectorTest.completeTest();
> 
> What if that was a debugger test?

Done.

> 
> > Source/WebCore/inspector/front-end/ScriptsPanel.js:192
> > +    WebInspector.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.BreakpointAdded, this._breakpointAdded, this);
> 
> It is strange that the client that adds breakpoints is listening to the breakpoints addition. r- for that.

We have two clients - SourceFrame and BreakpointsSidebarPane. It's impossible to fix this right now.

[Pavel Feldman]

Comment on attachment 83178 [details]
Patch.

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.BreakpointAdded, 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.

[Pavel Podivilov]

Created attachment 83181 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #10)
> (From update of attachment 83178 [details])
> 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.
> 

Done.

> > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:36
> > +    WebInspector.debuggerModel.addEventListener(WebInspector.DebuggerModel.Events.BreakpointAdded, 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.

There are currently two clients who use debugger model directly. We should update them to use DPM and eliminate those listeners in a separate change.

[Pavel Podivilov]

Committed r79311: <http://trac.webkit.org/changeset/79311>