[webkit-reviews] review denied: [Bug 85612] Web Inspector: console should allow JS execution in the context of an isolated world : [Attachment 140200] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 85612: Web Inspector: console should allow JS execution in the context of
an isolated world
https://bugs.webkit.org/show_bug.cgi?id=85612

Attachment 140200: Patch
https://bugs.webkit.org/attachment.cgi?id=140200&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140200&action=review


> Source/WebCore/bindings/js/ScriptController.cpp:332
> +    return 0;

Please file a bug and insert FIXME here.

> Source/WebCore/bindings/v8/V8Proxy.cpp:637
> +	   return v8::Local<v8::Context>();

Can this happen?

> Source/WebCore/bindings/v8/V8Proxy.h:255
> +#if ENABLE(INSPECTOR)

Chrome(i)um does not compile without inspector.

> Source/WebCore/inspector/CodeGeneratorInspector.py:188
> +    skip_js_bind_domains = set(["DOMDebugger"])

What does it mean?

> Source/WebCore/inspector/Inspector.json:432
> +		   "id": "IsolatedContext",

EvaluationContext ?

> Source/WebCore/inspector/Inspector.json:436
> +		       { "name": "id", "type": "integer", "description": "World
id. It can be used to specify in which world script evaluation should be
performed." },

I would rename it to "contextId" or type "ContextId".

> Source/WebCore/inspector/Inspector.json:438
> +		       { "name": "frameId", "$ref": "Network.FrameId",
"description": "Id of the owning frame." }

reorder by importance?

> Source/WebCore/inspector/Inspector.json:453
> +		       { "name": "contextId", "type": "integer", "optional":
true, "description": "Specifies in which isolated context to perform
evaluation. Each content script lives in an isolated context and this parameter
may be used to specify on of those contexts. If the parameter is omitted or 0
the evaluation will be performed in the context of the inspected page.",
"hidden": true },

I think it is better to combine the frame and context id into the single
contextId and stick to that term in runtime / debugger.