WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89678
Web Inspector: Support 'Restart frame' in inspector frontend
https://bugs.webkit.org/show_bug.cgi?id=89678
Summary
Web Inspector: Support 'Restart frame' in inspector frontend
Peter Rybin
Reported
2012-06-21 10:57:48 PDT
'Restart frame' action is supported in protocol. Add a UI for this feature to WebInspector.
Attachments
Patch
(8.43 KB, patch)
2012-06-21 11:02 PDT
,
Peter Rybin
no flags
Details
Formatted Diff
Diff
Patch
(8.37 KB, patch)
2012-06-21 12:23 PDT
,
Peter Rybin
no flags
Details
Formatted Diff
Diff
Patch
(8.37 KB, patch)
2012-06-21 19:49 PDT
,
Peter Rybin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Peter Rybin
Comment 1
2012-06-21 11:02:51 PDT
Created
attachment 148840
[details]
Patch
Pavel Feldman
Comment 2
2012-06-21 11:11:46 PDT
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 3
2012-06-21 11:24:03 PDT
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
Comment 4
2012-06-21 12:23:29 PDT
Created
attachment 148863
[details]
Patch
Peter Rybin
Comment 5
2012-06-21 12:34:25 PDT
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
Comment 6
2012-06-21 12:34:44 PDT
(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 7
2012-06-21 18:27:23 PDT
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
Comment 8
2012-06-21 19:48:28 PDT
(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
Comment 9
2012-06-21 19:49:56 PDT
Created
attachment 148946
[details]
Patch
Andrey Adaikin
Comment 10
2012-06-21 20:42:12 PDT
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
Comment 11
2012-06-21 20:47:17 PDT
ah, call frame, not an iframe :) nevermind.
WebKit Review Bot
Comment 12
2012-06-22 06:56:33 PDT
Comment on
attachment 148946
[details]
Patch Clearing flags on attachment: 148946 Committed
r121021
: <
http://trac.webkit.org/changeset/121021
>
WebKit Review Bot
Comment 13
2012-06-22 06:56:38 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug