[DevTools] Support CommandLineAPI in workers and Node.js

[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI methods are installed to global object as non enumerable, replaceble properties. Method can be overriden during evaluation and doesn't override existing property if it's presented.

BUG=607748, 595206
R=dgozman@chromium.org, pfeldman@chromium.org

Committed: https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e
Cr-Commit-Position: refs/heads/master@{#399560}

[kozy, 2016-05-31 23:44:00]

Description was changed from

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection was moved to v8_inspector. CommandLineAPI methods are
installed to global object as non enumerable, replaceble properties. Method can
be overriden during evaluation and doesn't override existing property if it's
presented.

BUG=607748
R=dgozman@chromium.org, pfeldman@chromium.org
==========

to

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection was moved to v8_inspector. CommandLineAPI methods are
installed to global object as non enumerable, replaceble properties. Method can
be overriden during evaluation and doesn't override existing property if it's
presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

[kozy, 2016-05-31 23:44:19]

Description was changed from

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection was moved to v8_inspector. CommandLineAPI methods are
installed to global object as non enumerable, replaceble properties. Method can
be overriden during evaluation and doesn't override existing property if it's
presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

to

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI
methods are installed to global object as non enumerable, replaceble properties.
Method can be overriden during evaluation and doesn't override existing property
if it's presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

[kozy, 2016-06-01 00:34:24]

Patchset #1 (id:1) has been deleted

[kozy, 2016-06-01 00:34:31]

Patchset #1 (id:20001) has been deleted

[kozy, 2016-06-01 00:35:06]

Dmitry, please take a look!

[dgozman, 2016-06-01 01:25:40]

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:705: static void
setterCallback(const v8::FunctionCallbackInfo<v8::Value>& info)
Why do we need this indirection?

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:726: if
(!commandLineAPI->GetOwnPropertyNames(context).ToLocal(&commandLineAPIMethodNames))
Perhaps, it makes sense to migrate command line api object to map as well.

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:776: if
(!commandLineAPIValue->Equals(context, objectMethodValue).FromMaybe(false))
This is kind of scary. What about having a setter for each property, which will
remove it from a list of installed and set user-passed value?

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.h (right):

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.h:22: static void
clearCommandLineAPI(v8::Local<v8::Context>, v8::Local<v8::Object>,
v8::Local<v8::Object> installedMethods);
- let's name second parameter "global";
- let's use Map for installedMethods so that intention is clear.

[kozy, 2016-06-01 18:49:23]

Dmitry, please take a look.

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:705: static void
setterCallback(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2016/06/01 01:25:40, dgozman wrote:
> Why do we need this indirection?

We'd like to make CommandLineAPI methods replaceable on window object.

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:726: if
(!commandLineAPI->GetOwnPropertyNames(context).ToLocal(&commandLineAPIMethodNames))
On 2016/06/01 01:25:40, dgozman wrote:
> Perhaps, it makes sense to migrate command line api object to map as well.

I'll make it in follow up.

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:776: if
(!commandLineAPIValue->Equals(context, objectMethodValue).FromMaybe(false))
On 2016/06/01 01:25:40, dgozman wrote:
> This is kind of scary. What about having a setter for each property, which
will
> remove it from a list of installed and set user-passed value?

Done.

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.h (right):

https://codereview.chromium.org/2030453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.h:22: static void
clearCommandLineAPI(v8::Local<v8::Context>, v8::Local<v8::Object>,
v8::Local<v8::Object> installedMethods);
On 2016/06/01 01:25:40, dgozman wrote:
> - let's name second parameter "global";
> - let's use Map for installedMethods so that intention is clear.

Done.

[dgozman, 2016-06-01 19:41:28]

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:725:
DCHECK(!scope);
DCHECK(scope)

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:733: if
(isCommandLineAPIGetter(toProtocolStringWithTypeCheck(name)) &&
value->IsFunction()) {
It must be a function.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:748: if
(!info.Holder()->CreateDataProperty(context, name, value).FromMaybe(false))
Let's add "var dir = 1" to the test.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:767: if
(m_global->Has(context, name).FromMaybe(true))
Do we account for non-enumerable properties here? Don't we hide any bindings?

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:771: if
(!m_global->SetAccessor(context, v8::Local<v8::Name>::Cast(name),
CommandLineAPIScope::accessorGetterCallback,
name.As<> ?

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:772:
CommandLineAPIScope::accessorSetterCallback,
v8::External::New(context->GetIsolate(), this),
Can we have a single external |this| per scope?

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:788: bool removed
= m_global->Delete(m_context, name).FromMaybe(false);
Let's try to definePorperty in the test and check we don't remove it.

[kozy, 2016-06-02 00:50:43]

All done. please take a look!

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:725:
DCHECK(!scope);
On 2016/06/01 19:41:28, dgozman wrote:
> DCHECK(scope)

Done.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:733: if
(isCommandLineAPIGetter(toProtocolStringWithTypeCheck(name)) &&
value->IsFunction()) {
On 2016/06/01 19:41:28, dgozman wrote:
> It must be a function.

Done.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:748: if
(!info.Holder()->CreateDataProperty(context, name, value).FromMaybe(false))
On 2016/06/01 19:41:28, dgozman wrote:
> Let's add "var dir = 1" to the test.

Done.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:767: if
(m_global->Has(context, name).FromMaybe(true))
On 2016/06/01 19:41:28, dgozman wrote:
> Do we account for non-enumerable properties here? Don't we hide any bindings?

We don't hide bindings here.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:771: if
(!m_global->SetAccessor(context, v8::Local<v8::Name>::Cast(name),
CommandLineAPIScope::accessorGetterCallback,
On 2016/06/01 19:41:28, dgozman wrote:
> name.As<> ?

We can't use As for function arguments because it produces temporary variable.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:772:
CommandLineAPIScope::accessorSetterCallback,
v8::External::New(context->GetIsolate(), this),
On 2016/06/01 19:41:28, dgozman wrote:
> Can we have a single external |this| per scope?

Done.

https://codereview.chromium.org/2030453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:788: bool removed
= m_global->Delete(m_context, name).FromMaybe(false);
On 2016/06/01 19:41:28, dgozman wrote:
> Let's try to definePorperty in the test and check we don't remove it.

Done.
When defineProperty is called with value then it will go accessor otherwise it
will add setter or getter.

[dgozman, 2016-06-02 22:42:36]

lgtm

[pfeldman, 2016-06-02 23:10:56]

lgtm

[kozy, 2016-06-02 23:11:08]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-06-02 23:11:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2030453002/80001

[commit-bot: I haz the power, 2016-06-03 00:42:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 00:42:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kozy, 2016-06-10 18:27:21]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-06-10 18:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2030453002/#ps140001
(title: "")

[commit-bot: I haz the power, 2016-06-10 18:28:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/140001

[commit-bot: I haz the power, 2016-06-10 22:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-10 22:52:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-06-10 23:10:53]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-06-10 23:11:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/140001

[commit-bot: I haz the power, 2016-06-11 02:19:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-11 02:19:44]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-06-11 02:20:31]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-06-11 02:20:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/140001

[commit-bot: I haz the power, 2016-06-11 06:13:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-11 06:13:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-06-11 06:24:57]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-06-11 06:25:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/140001

[commit-bot: I haz the power, 2016-06-11 10:30:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-11 10:30:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-06-11 14:49:12]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-06-11 14:49:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/140001

[commit-bot: I haz the power, 2016-06-11 18:47:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-11 18:47:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-06-13 18:27:27]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-06-13 18:27:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2030453002/#ps160001
(title: "rebased")

[commit-bot: I haz the power, 2016-06-13 18:28:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030453002/160001

[commit-bot: I haz the power, 2016-06-13 21:53:47]

Description was changed from

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI
methods are installed to global object as non enumerable, replaceble properties.
Method can be overriden during evaluation and doesn't override existing property
if it's presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

to

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI
methods are installed to global object as non enumerable, replaceble properties.
Method can be overriden during evaluation and doesn't override existing property
if it's presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

[commit-bot: I haz the power, 2016-06-13 21:53:49]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-06-13 21:53:55]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 21:56:03]

Description was changed from

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI
methods are installed to global object as non enumerable, replaceble properties.
Method can be overriden during evaluation and doesn't override existing property
if it's presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org
==========

to

==========
[DevTools] Support CommandLineAPI in workers and Node.js

CommandLineAPI injection is moved to v8_inspector in this CL. CommandLineAPI
methods are installed to global object as non enumerable, replaceble properties.
Method can be overriden during evaluation and doesn't override existing property
if it's presented.

BUG=607748,595206
R=dgozman@chromium.org, pfeldman@chromium.org

Committed: https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e
Cr-Commit-Position: refs/heads/master@{#399560}
==========

[commit-bot: I haz the power, 2016-06-13 21:56:05]

Patchset 7 (id:??) landed as
https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e
Cr-Commit-Position: refs/heads/master@{#399560}