[webkit-reviews] review denied: [Bug 89678] Web Inspector: Support 'Restart frame' in inspector frontend : [Attachment 148840] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Peter Rybin
<prybin at chromium.org>'s request for review:
Bug 89678: Web Inspector: Support 'Restart frame' in inspector frontend
https://bugs.webkit.org/show_bug.cgi?id=89678

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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