DevTools: Implement evaluation on async call frames.

DevTools: Implement evaluation on async call frames.

We emulate evaluation on an async call frame by doing evaluation on the global object with an extended context derived from the async call frame scope chain.

BUG=272416
R=yurys

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165738

[aandrey, 2014-01-21 09:45:44]

ping

[yurys, 2014-01-21 11:56:52]

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScript.h (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScript.h:69: void
evaluateOnCallFrame(ErrorString*, const ScriptValue& callFrames, const
Vector<ScriptValue>& asyncCallStacks, const String& callFrameId, const String&
expression, const String& objectGroup, bool includeCommandLineAPI, bool
returnByValue, bool generatePreview, RefPtr<TypeBuilder::Runtime::RemoteObject>*
result, TypeBuilder::OptOutput<bool>* wasThrown);
Please keep vertical formatting for better readability.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:664: return
this._evaluateAndWrap(topCallFrame.evaluateGlobal, topCallFrame, expression,
objectGroup, true, injectCommandLineAPI, returnByValue, generatePreview,
InjectedScript.CallFrameProxy._toProtoChainedScope(callFrame));
Why do we use topCallFrame as object argument but not callFrame here ? We should
be evaluating either in the context of current frame or in an async frame I
believe.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:764: var asyncOrdinal =
parsedCallFrameId["asyncOrdinal"] || 0; // 1-based index
You don't really need  "|| 0" part.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:1220:
Object.prototype.__defineGetter__.call(proxy, name, bind(getter, object, name));
Why not simply copy the value: proxy[name] = o[name] ?

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/JavaSc...
File Source/core/inspector/JavaScriptCallFrame.cpp (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/JavaSc...
Source/core/inspector/JavaScriptCallFrame.cpp:164: return
ScriptValue(evalFunction->Call(callFrame, 2, argv), m_isolate);
Please use WTF_ARRAY_LENGTH

[aandrey, 2014-01-21 13:30:21]

PTAL

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScript.h (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScript.h:69: void
evaluateOnCallFrame(ErrorString*, const ScriptValue& callFrames, const
Vector<ScriptValue>& asyncCallStacks, const String& callFrameId, const String&
expression, const String& objectGroup, bool includeCommandLineAPI, bool
returnByValue, bool generatePreview, RefPtr<TypeBuilder::Runtime::RemoteObject>*
result, TypeBuilder::OptOutput<bool>* wasThrown);
On 2014/01/21 11:56:52, yurys wrote:
> Please keep vertical formatting for better readability.

Done.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:664: return
this._evaluateAndWrap(topCallFrame.evaluateGlobal, topCallFrame, expression,
objectGroup, true, injectCommandLineAPI, returnByValue, generatePreview,
InjectedScript.CallFrameProxy._toProtoChainedScope(callFrame));
On 2014/01/21 11:56:52, yurys wrote:
> Why do we use topCallFrame as object argument but not callFrame here ? We
should
> be evaluating either in the context of current frame or in an async frame I
> believe.

We can evaluate only on the call frames of the current call stack while paused,
thus callFrame.evaluate and callFrame.evaluateGlobal would throw, given that
callFrame is async.

The topCallFrame.evaluateGlobal runs global eval rather than eval-on-call-frame.
All evaluateGlobal methods of the valid ("sync") call frames behave in the same
way.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:764: var asyncOrdinal =
parsedCallFrameId["asyncOrdinal"] || 0; // 1-based index
On 2014/01/21 11:56:52, yurys wrote:
> You don't really need  "|| 0" part.

Done.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:1220:
Object.prototype.__defineGetter__.call(proxy, name, bind(getter, object, name));
On 2014/01/21 11:56:52, yurys wrote:
> Why not simply copy the value: proxy[name] = o[name] ?

To avoid side effects.
For example, accessing window["webkitStorageInfo"] may trigger a warning:

'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/JavaSc...
File Source/core/inspector/JavaScriptCallFrame.cpp (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/JavaSc...
Source/core/inspector/JavaScriptCallFrame.cpp:164: return
ScriptValue(evalFunction->Call(callFrame, 2, argv), m_isolate);
On 2014/01/21 11:56:52, yurys wrote:
> Please use WTF_ARRAY_LENGTH

Done.

[yurys, 2014-01-21 13:54:43]

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:664: return
this._evaluateAndWrap(topCallFrame.evaluateGlobal, topCallFrame, expression,
objectGroup, true, injectCommandLineAPI, returnByValue, generatePreview,
InjectedScript.CallFrameProxy._toProtoChainedScope(callFrame));
On 2014/01/21 13:30:22, aandrey wrote:
> On 2014/01/21 11:56:52, yurys wrote:
> > Why do we use topCallFrame as object argument but not callFrame here ? We
> should
> > be evaluating either in the context of current frame or in an async frame I
> > believe.
> 
> We can evaluate only on the call frames of the current call stack while
paused,
> thus callFrame.evaluate and callFrame.evaluateGlobal would throw, given that
> callFrame is async.
> 
> The topCallFrame.evaluateGlobal runs global eval rather than
eval-on-call-frame.
> All evaluateGlobal methods of the valid ("sync") call frames behave in the
same
> way.

Sound like we should have a version of evaluateGlobal that would work on pause
rather than abusing CallFrame.evaluateGlobal. It should be a straightforward
change on V8 side and would make this code simpler.

https://codereview.chromium.org/136333007/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:1220:
Object.prototype.__defineGetter__.call(proxy, name, bind(getter, object, name));
On 2014/01/21 13:30:22, aandrey wrote:
> On 2014/01/21 11:56:52, yurys wrote:
> > Why not simply copy the value: proxy[name] = o[name] ?
> 
> To avoid side effects.
> For example, accessing window["webkitStorageInfo"] may trigger a warning:
> 
> 'window.webkitStorageInfo' is deprecated. Please use
> 'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
> instead. 

I'm concerned about the performance here. We may end up adding wrapping many
properties of a global object. On the other hand, if it happens only for
evaluation on asyn call frame it might be OK.

What if instead we passed chain of extension objects  into evaluateGlobal and
have it construct a several with blocks for each of the extension object, would
it solve this problem?

[aandrey, 2014-01-22 11:04:34]

Removed evaluateGlobal, using with(...) {} approach.
PTAL.

[yurys, 2014-01-23 15:32:26]

https://codereview.chromium.org/136333007/diff/180001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/136333007/diff/180001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:640:
inspectedWindow._scopeChainForEval = scopeChain;
Please put this into inspectedWindow.console._scopeChainForEval instead.

[aandrey, 2014-01-23 15:40:42]

https://codereview.chromium.org/136333007/diff/180001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/136333007/diff/180001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:640:
inspectedWindow._scopeChainForEval = scopeChain;
On 2014/01/23 15:32:26, yurys wrote:
> Please put this into inspectedWindow.console._scopeChainForEval instead.

Done.

[aandrey, 2014-01-24 05:40:58]

Re-uploaded

[yurys, 2014-01-24 08:01:06]

lgtm

[commit-bot: I haz the power, 2014-01-24 08:03:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aandrey@chromium.org/136333007/290001

[commit-bot: I haz the power, 2014-01-24 08:03:40]

Failed to apply patch for
LayoutTests/inspector/console/console-format-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/inspector/console/console-format-expected.txt
  Hunk #1 FAILED at 129.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/inspector/console/console-format-expected.txt.rej

Patch:       LayoutTests/inspector/console/console-format-expected.txt
Index: LayoutTests/inspector/console/console-format-expected.txt
diff --git a/LayoutTests/inspector/console/console-format-expected.txt
b/LayoutTests/inspector/console/console-format-expected.txt
index
7dcaedbdd7eb1c82ab6e399c80c69dc0b112b02f..b6a382cf2e7829c8aea4367ebb123319708c134f
100644
--- a/LayoutTests/inspector/console/console-format-expected.txt
+++ b/LayoutTests/inspector/console/console-format-expected.txt
@@ -129,7 +129,7 @@ undefined console-format.html:12
 [undefined] console-format.html:13
 globals[12]
 undefined
-Attr.specified is deprecated. Its value is always true. VM:448
+Attr.specified is deprecated. Its value is always true. VM:449
 attr="" console-format.html:12
 [attr]0: attrlength: 1__proto__: Array[0] console-format.html:13
 globals[13]

[commit-bot: I haz the power, 2014-01-24 10:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aandrey@chromium.org/136333007/350001

[commit-bot: I haz the power, 2014-01-24 13:31:59]

Change committed as 165738