Bug 112470 - Web Inspector: support changing local variables in frontend
Summary: Web Inspector: support changing local variables in frontend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-15 15:00 PDT by Peter Rybin
Modified: 2013-03-25 05:41 PDT (History)
12 users (show)

See Also:


Attachments
Patch (12.32 KB, patch)
2013-03-15 15:08 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (12.95 KB, patch)
2013-03-19 10:06 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2013-03-19 14:15 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (16.47 KB, patch)
2013-03-20 14:18 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.60 KB, patch)
2013-03-21 08:36 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.59 KB, patch)
2013-03-22 08:12 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.59 KB, patch)
2013-03-22 08:21 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Rebase (15.61 KB, patch)
2013-03-22 17:17 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Rybin 2013-03-15 15:00:24 PDT
V8/Backend/protocol support changing local variables (see https://bugs.webkit.org/show_bug.cgi?id=107829 )

However Frontend is yet to support it.
Comment 1 Peter Rybin 2013-03-15 15:08:07 PDT
Created attachment 193379 [details]
Patch
Comment 2 Yury Semikhatsky 2013-03-18 06:43:07 PDT
Comment on attachment 193379 [details]
Patch

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

Maybe test?

> Source/WebCore/inspector/front-end/RemoteObject.js:59
> +        this._scopeRef = scopeRef;

Why not do this._scopeRef = scopeRef; unconditionally ?

> Source/WebCore/inspector/front-end/RemoteObject.js:122
> +    console.assert(typeof payload === "object", "Remote object payload should only be an object");

Do really want to keep this check in production code?

> Source/WebCore/inspector/front-end/RemoteObject.js:286
> +        

Remove empty line.

> Source/WebCore/inspector/front-end/RemoteObject.js:452
> +    this.scopeHolder = scopeHolder;

I'd merge these two objects.
Comment 3 Peter Rybin 2013-03-19 10:06:12 PDT
Created attachment 193849 [details]
Patch
Comment 4 Peter Rybin 2013-03-19 10:07:58 PDT
> Maybe test?

I included it into existing test.

> > Source/WebCore/inspector/front-end/RemoteObject.js:59
> > +        this._scopeRef = scopeRef;
> Why not do this._scopeRef = scopeRef; unconditionally ?
Done

> > Source/WebCore/inspector/front-end/RemoteObject.js:122
> > +    console.assert(typeof payload === "object", "Remote object payload should only be an object");
> Do really want to keep this check in production code?
Done

> > Source/WebCore/inspector/front-end/RemoteObject.js:286
> > +        
> Remove empty line.
Done

> > Source/WebCore/inspector/front-end/RemoteObject.js:452
> > +    this.scopeHolder = scopeHolder;
> I'd merge these two objects.
Done
Comment 5 Build Bot 2013-03-19 10:32:37 PDT
Comment on attachment 193849 [details]
Patch

Attachment 193849 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17242090

New failing tests:
inspector/debugger/debugger-eval-while-paused.html
Comment 6 WebKit Review Bot 2013-03-19 10:55:39 PDT
Comment on attachment 193849 [details]
Patch

Attachment 193849 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17244029

New failing tests:
inspector/debugger/debugger-eval-while-paused.html
Comment 7 Peter Rybin 2013-03-19 14:15:59 PDT
Created attachment 193918 [details]
Patch
Comment 8 Yury Semikhatsky 2013-03-20 08:16:27 PDT
Comment on attachment 193918 [details]
Patch

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

> LayoutTests/inspector/debugger/debugger-eval-while-paused-expected.txt:1
> +Tests that evaluation in console works fine when script is paused. It also checks that stack and global variables are accessible from the console. Additionally it tries to modify local variable value and checks that it works.

Please put this in a separate test.
Comment 9 Peter Rybin 2013-03-20 14:18:24 PDT
Created attachment 194123 [details]
Patch
Comment 10 Peter Rybin 2013-03-20 14:20:46 PDT
(In reply to comment #8)
> (From update of attachment 193918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193918&action=review
> 
> > LayoutTests/inspector/debugger/debugger-eval-while-paused-expected.txt:1
> > +Tests that evaluation in console works fine when script is paused. It also checks that stack and global variables are accessible from the console. Additionally it tries to modify local variable value and checks that it works.
> 
> Please put this in a separate test.

Done
Comment 11 Yury Semikhatsky 2013-03-21 06:21:23 PDT
Comment on attachment 194123 [details]
Patch

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

> LayoutTests/inspector/debugger/debugger-change-variable.html:45
> +        InspectorTest.evaluateInConsole("localObject.a + globalVar.b", step5);

Steps 1-4 is just a clone of those in debugger-eval-while-paused.html and not related to the tested feature, please remove them.
Comment 12 Peter Rybin 2013-03-21 08:36:15 PDT
Created attachment 194268 [details]
Patch
Comment 13 Peter Rybin 2013-03-21 08:36:57 PDT
> Steps 1-4 is just a clone of those in debugger-eval-while-paused.html and not related to the tested feature, please remove them.

Done
Comment 14 Yury Semikhatsky 2013-03-22 08:07:50 PDT
Comment on attachment 194268 [details]
Patch

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

> LayoutTests/inspector/debugger/debugger-change-variable.html:37
> +        InspectorTest.evaluateInConsole("localObject.a + 10", step4);

Why +10?
Comment 15 Peter Rybin 2013-03-22 08:12:11 PDT
Created attachment 194551 [details]
Patch
Comment 16 Peter Rybin 2013-03-22 08:12:48 PDT
(In reply to comment #14)
> (From update of attachment 194268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194268&action=review
> 
> > LayoutTests/inspector/debugger/debugger-change-variable.html:37
> > +        InspectorTest.evaluateInConsole("localObject.a + 10", step4);
> 
> Why +10?

Done
Comment 17 Peter Rybin 2013-03-22 08:21:23 PDT
Created attachment 194553 [details]
Patch
Comment 18 WebKit Review Bot 2013-03-22 08:47:15 PDT
Comment on attachment 194553 [details]
Patch

Rejecting attachment 194553 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 194553, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 14015 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 14015.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17291066
Comment 19 Peter Rybin 2013-03-22 17:17:23 PDT
Created attachment 194662 [details]
Rebase
Comment 20 Peter Rybin 2013-03-22 17:25:08 PDT
Hope this rebase patch will pass.
Comment 21 WebKit Review Bot 2013-03-25 05:41:16 PDT
Comment on attachment 194662 [details]
Rebase

Clearing flags on attachment: 194662

Committed r146759: <http://trac.webkit.org/changeset/146759>
Comment 22 WebKit Review Bot 2013-03-25 05:41:20 PDT
All reviewed patches have been landed.  Closing bug.