Bug 112470 – Web Inspector: support changing local variables in frontend

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

[Peter Rybin]

V8/Backend/protocol support changing local variables (see https://bugs.webkit.org/show_bug.cgi?id=107829 )

However Frontend is yet to support it.

[Peter Rybin]

Created attachment 193379 [details]
Patch

[Yury Semikhatsky]

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.

[Peter Rybin]

Created attachment 193849 [details]
Patch

[Peter Rybin]

> 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

[Build Bot]

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

[WebKit Review Bot]

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

[Peter Rybin]

Created attachment 193918 [details]
Patch

[Yury Semikhatsky]

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.

[Peter Rybin]

Created attachment 194123 [details]
Patch

[Peter Rybin]

(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

[Yury Semikhatsky]

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.

[Peter Rybin]

Created attachment 194268 [details]
Patch

[Peter Rybin]

> 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

[Yury Semikhatsky]

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?

[Peter Rybin]

Created attachment 194551 [details]
Patch

[Peter Rybin]

(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

[Peter Rybin]

Created attachment 194553 [details]
Patch

[WebKit Review Bot]

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

[Peter Rybin]

Created attachment 194662 [details]
Rebase

[Peter Rybin]

Hope this rebase patch will pass.

[WebKit Review Bot]

Comment on attachment 194662 [details]
Rebase

Clearing flags on attachment: 194662

Committed r146759: <http://trac.webkit.org/changeset/146759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.