Bug 50745 – Web Inspector: refactoring: make selected call frame a debugger model property

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

[Pavel Podivilov]

Make selected call frame a debugger model property to weaken the coupling between different UI elements that depend on selected call frame.

[Pavel Podivilov]

Created attachment 76034 [details]
Patch.

[Yury Semikhatsky]

Comment on attachment 76034 [details]
Patch.

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

> WebCore/inspector/front-end/CallStackSidebarPane.js:31
> +    WebInspector.debuggerModel.addEventListener("selected-call-frame-changed", this._selectedCallFrameChanged, this);

Selected call frame is a property of CallStackSideBarPane there is no need in moving it into the debuggerModel. If its owner(ScriptsPanel) needs that information it can add "selected-call-frame-changed" listener to the sidebar.

> WebCore/inspector/front-end/CallStackSidebarPane.js:39
> +        // FIXME: remove this when we'll have scripts in debugger model

Just get the map from the ScriptsPanel when you need it in the sidebar pane, otherwise it's easy to break it by creating new sourceIDMap in the panel and forget to update it here.

> WebCore/inspector/front-end/DebuggerModel.js:169
> +    selectedCallFrameIndex: function()

Consider changing these methods into getters for consistency with the rest of the front-end code.

[Pavel Podivilov]

Created attachment 77215 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #2)
> (From update of attachment 76034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76034&action=review
> 
> > WebCore/inspector/front-end/CallStackSidebarPane.js:31
> > +    WebInspector.debuggerModel.addEventListener("selected-call-frame-changed", this._selectedCallFrameChanged, this);
> 
> Selected call frame is a property of CallStackSideBarPane there is no need in moving it into the debuggerModel. If its owner(ScriptsPanel) needs that information it can add "selected-call-frame-changed" listener to the sidebar.

There are several views depending on selected call frame - SourceFrame, ConsoleView, ScriptsPanel, ScopeChain, WatchExpressions, CallStackSidebarPane. Moving selected call frame property to model would help us to eliminate unwanted dependencies between views.

> 
> > WebCore/inspector/front-end/CallStackSidebarPane.js:39
> > +        // FIXME: remove this when we'll have scripts in debugger model
> 
> Just get the map from the ScriptsPanel when you need it in the sidebar pane, otherwise it's easy to break it by creating new sourceIDMap in the panel and forget to update it here.

No need for this map since we have scripts in DebuggerModel.

> 
> > WebCore/inspector/front-end/DebuggerModel.js:169
> > +    selectedCallFrameIndex: function()
> 
> Consider changing these methods into getters for consistency with the rest of the front-end code.

Done.

[Yury Semikhatsky]

Comment on attachment 77215 [details]
Patch.

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

> WebCore/inspector/front-end/CallStackSidebarPane.js:81
> +        if (WebInspector.debuggerModel.currentlyHitBreakpoint())

I think isOnBreakpoint would be a better name than currentlyHitBreakpoint

> WebCore/inspector/front-end/DebuggerModel.js:192
> +        this._callFrames = callFrames;

this._selectedCallFrameIndex should be reset when this._callFrames changes, r- for this.

[Pavel Podivilov]

Created attachment 77227 [details]
All comments addressed.

[Yury Semikhatsky]

Comment on attachment 77227 [details]
All comments addressed.

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

> WebCore/inspector/front-end/DebuggerModel.js:193
> +        this._selectedCallFrameIndex = 0;

These two lines can be replaced with this.selectedCallFrameIndex = 0;

[Pavel Feldman]

Comment on attachment 77227 [details]
All comments addressed.

This looks really strange - why should debugger model have user-configurable state? I don't see clients putting it into a particular state (selecting call frame) and executing its commands in that state.

[Pavel Feldman]

Comment on attachment 77227 [details]
All comments addressed.

Clearing r+ from the obsolete attachment.

[Pavel Feldman]

This has been on hold for a long time. Closing it as obsolete.