Bug 89678 – Web Inspector: Support 'Restart frame' in inspector frontend

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

[Peter Rybin]

'Restart frame' action is supported in protocol. Add a UI for this feature to WebInspector.

[Peter Rybin]

Created attachment 148840 [details]
Patch

[Pavel Feldman]

Comment on attachment 148840 [details]
Patch

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

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:110
> +    contributeToContextMenu: function(contextMenu) {

{ should go to the next line

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:111
>          contextMenu.appendItem(WebInspector.UIString("Copy Stack Trace"), this._copyStackTrace.bind(this));

I'd rather move this into placard. You can call _copyStackTrace from there (it is private to this file).

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:188
> +    _placardContextMenu: function(event) {

{ on the next line

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:190
> +        contextMenu.appendItem(WebInspector.UIString("Restart Frame"), this._restartFrame.bind(this), !WebInspector.debuggerModel.canSetScriptSource());

canSetScriptSource is constant for the port. You should not disable it on that basis, you should just add the item depending on the value.

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:196
> +    _restartFrame: function() {

{ on the next line

> Source/WebCore/inspector/front-end/DebuggerModel.js:552
> +    callStackModified: function(newCallFrames, details) {

{ on the next line

> Source/WebCore/inspector/front-end/DebuggerModel.js:554
> +        if (details && details["stack_update_needs_step_in"]) {

No need for { } for the upper branch

> Source/WebCore/inspector/front-end/DebuggerModel.js:555
> +            DebuggerAgent.stepInto();

You should do this in the backend.

> Source/WebCore/inspector/front-end/DebuggerModel.js:740
> +            if (!error) {

no need for { }

> Source/WebCore/inspector/front-end/DebuggerModel.js:743
> +            if (callback) {

ditto

[Vsevolod Vlasov]

Comment on attachment 148840 [details]
Patch

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

> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:192
> +        this._pane.contributeToContextMenu(contextMenu);

I would keep context menu population in one method of CallStackSidebarPane.
E.g. in eclipse "restart frame" is in the middle of context menu which would be hard to implement with current approach.

[Peter Rybin]

Created attachment 148863 [details]
Patch

[Peter Rybin]

Comment on attachment 148840 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DebuggerModel.js:552
>> +    callStackModified: function(newCallFrames, details) {
> 
> { on the next line

Done

>> Source/WebCore/inspector/front-end/DebuggerModel.js:554
>> +        if (details && details["stack_update_needs_step_in"]) {
> 
> No need for { } for the upper branch

Done. Could have done for the lower branch as well.

>> Source/WebCore/inspector/front-end/DebuggerModel.js:555
>> +            DebuggerAgent.stepInto();
> 
> You should do this in the backend.

It's a tricky thing. This would make command optionally changing backend state from paused to resumed. Let's not do it right now.

>> Source/WebCore/inspector/front-end/DebuggerModel.js:740
>> +            if (!error) {
> 
> no need for { }

Done

>> Source/WebCore/inspector/front-end/DebuggerModel.js:743
>> +            if (callback) {
> 
> ditto

Done

[Peter Rybin]

(In reply to comment #3)
> (From update of attachment 148840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148840&action=review
> 
> > Source/WebCore/inspector/front-end/CallStackSidebarPane.js:192
> > +        this._pane.contributeToContextMenu(contextMenu);
> 
> I would keep context menu population in one method of CallStackSidebarPane.
> E.g. in eclipse "restart frame" is in the middle of context menu which would be hard to implement with current approach.

Done

[Pavel Feldman]

Comment on attachment 148863 [details]
Patch

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

> Source/WebCore/inspector/front-end/DebuggerModel.js:733
> +    restart: function(callback) {

Please put { on the next line.

[Peter Rybin]

(In reply to comment #7)
> (From update of attachment 148863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148863&action=review
> 
> > Source/WebCore/inspector/front-end/DebuggerModel.js:733
> > +    restart: function(callback) {
> 
> Please put { on the next line.

Damn
Done
Thank you

[Peter Rybin]

Created attachment 148946 [details]
Patch

[Andrey Adaikin]

BTW, I find "Restart Frame" a bit confusing. If what you're doing is similar to invoking frame.contentWindow.location.reload(), then IMO "Reload Frame" would be more appropriate, no?

[Andrey Adaikin]

ah, call frame, not an iframe :) nevermind.

[WebKit Review Bot]

Comment on attachment 148946 [details]
Patch

Clearing flags on attachment: 148946

Committed r121021: <http://trac.webkit.org/changeset/121021>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.