* SUMMARY Eliminate the crazy code for evaluateOnCallFrame. While ingenious, it was very complex and difficult and confusing. Move to using a global scope extension and unify as much of the code for Debugger.evaluateOnCallFrame and Runtime.evaluate as possible.
<rdar://problem/26191332>
Created attachment 278484 [details] [PATCH] Proposed Fix One very interesting bit is that in non-strict call frames I'm able to create local variables `var local = 1`, but in strict call frames I'm not. Probably an optimization difference? Or maybe based on the number of locals in the function? In any case, the test discovers this, but the behavior could go either way and I would be fine with it.
Comment on attachment 278484 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review > LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt:40 > +PASS: Script evaluation. Should not be able to access arguments.callee. Nit: 'strict' > LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt:43 > +PASS: Local variable `keys` should be shadowed by CommandLineAPI `keys` function in call frame for `foo`. This description does not match the test case name. > LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:82 > + testEvaluateOnStrictCallFrame("a", (x) => { InspectorTest.expectThat(x === 6, "`a` should be 5 in `bar`."); }); Assertion message doesn't match the condition.
Comment on attachment 278484 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review Looks like a big win to me. I would like Tim or Mark to look at this too. > LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:170 > + InspectorTest.expectThat(resultValue === 123, "Local variable `keys` should be shadowed by CommandLineAPI `keys` function in call frame for `foo`."); See above, this seems wrong. > Source/JavaScriptCore/ChangeLog:19 > + DebuggerCallFrame::evaluteWithScopeExtension. When evaluating globally Typo: evaluateWithScopeExtension > Source/JavaScriptCore/inspector/InjectedScriptSource.js:-490 > - "var __originalEval = global.eval; global.eval = __eval;" + I had no idea this nasty stuff was being done. I wonder if it was hurting performance by doing weird things to globals/builtins. > Source/JavaScriptCore/inspector/JSJavaScriptCallFramePrototype.cpp:-93 > - ASSERT_GC_OBJECT_INHERITS(castedThis, JSJavaScriptCallFrame::info()); The changelog didn't say anything about this change. What's it for?
(In reply to comment #2) > Created attachment 278484 [details] > [PATCH] Proposed Fix > > One very interesting bit is that in non-strict call frames I'm able to > create local variables `var local = 1`, but in strict call frames I'm not. > Probably an optimization difference? Or maybe based on the number of locals > in the function? In any case, the test discovers this, but the behavior > could go either way and I would be fine with it. This sounds like how strict mode eval works in general. In strict mode eval you can not inject variables into your parent var environment. The eval itself creates its own var environment. In sloppy mode eval, all vars declared in the eval are injected into the parent var environment because the eval doesn't have a local var environment.
Comment on attachment 278484 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review So awesome! >> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-490 >> - "var __originalEval = global.eval; global.eval = __eval;" + > > I had no idea this nasty stuff was being done. I wonder if it was hurting performance by doing weird things to globals/builtins. It was all for science…
> > Source/JavaScriptCore/inspector/JSJavaScriptCallFramePrototype.cpp:-93 > > - ASSERT_GC_OBJECT_INHERITS(castedThis, JSJavaScriptCallFrame::info()); > > The changelog didn't say anything about this change. What's it for? This is just asserting the object inherits from JSJavaScriptCallFrame. However the code above it (in all these functions) does a dynamicCast and returns if it isn't that type. So the assert will never fail.
Created attachment 278516 [details] [PATCH] For Landing
Comment on attachment 278516 [details] [PATCH] For Landing Rejecting attachment 278516 [details] from commit-queue. New failing tests: accessibility/meter-element.html Full output: http://webkit-queues.webkit.org/results/1299711
Created attachment 278518 [details] Archive of layout-test-results from webkit-cq-02 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-02 Port: mac-yosemite Platform: Mac OS X 10.10.5
<http://trac.webkit.org/changeset/200634>