[DevTools] Remove [V8] from InspectorInstrumentation.

[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337
Committed: https://crrev.com/c1e6c531c70657b519121ab6b626471a75448a9e
Cr-Commit-Position: refs/heads/master@{#404588}

[dgozman, 2016-06-30 01:28:25]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 01:28:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 01:32:02]

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

[commit-bot: I haz the power, 2016-06-30 01:32:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2016-06-30 17:57:39]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 17:57:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 19:31:05]

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

[commit-bot: I haz the power, 2016-06-30 19:31:05]

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

[dgozman, 2016-06-30 21:03:14]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 21:04:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 22:26:43]

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

[commit-bot: I haz the power, 2016-06-30 22:26:43]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-07-01 00:24:05]

dgozman@chromium.org changed reviewers:
+ haraken@chromium.org, kozyatinskiy@chromium.org

[dgozman, 2016-07-01 00:24:06]

Could you please take a look?

haraken - at bindings, core.
kozyatinskiy - at devtools, inspector.

Thanks,
Dmitry

[haraken, 2016-07-01 02:28:04]

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
(!forceMainWorldInitialization &&
!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())

Hmm, it looks a bit nasty to force the main world initialization in
updateDocument(). Don't we have any explicit timing where the inspector can
force the initialization?

[dgozman, 2016-07-06 17:02:20]

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
(!forceMainWorldInitialization &&
!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
On 2016/07/01 02:28:03, haraken wrote:
> 
> Hmm, it looks a bit nasty to force the main world initialization in
> updateDocument(). Don't we have any explicit timing where the inspector can
> force the initialization?

Alternatives are LocalDOMWindow::installNewDocument,
DocumentLoader::createWriterFor and DocumentLoader::ensureWriter. I don't think
any of them is better.

As for forcing this from inspector, we try to minimize the side-effects of
instrumentation, and instead rely on direct manipulation through settings. This
makes it clear to anyone working on the specific area that such a functionality
exist. Otherwise, it's easy to miss inspector-only side-effect and break things.
WDYT?

[kozy, 2016-07-06 19:38:22]

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:641:
m_v8Session->breakProgramOnException(protocol::Debugger::Paused::ReasonEnum::CSPViolation,
std::move(directive));
Let's move it to "DOMDebugger way" instead of breakProgramOnException.

[dgozman, 2016-07-06 22:52:33]

Please take another look.

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp
(right):

https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:641:
m_v8Session->breakProgramOnException(protocol::Debugger::Paused::ReasonEnum::CSPViolation,
std::move(directive));
On 2016/07/06 19:38:22, kozyatinskiy wrote:
> Let's move it to "DOMDebugger way" instead of breakProgramOnException.

Done.

[dgozman, 2016-07-06 22:53:18]

Description was changed from

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.
Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

BUG=580337
==========

to

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337
==========

[haraken, 2016-07-07 00:43:32]

On 2016/07/06 17:02:20, dgozman wrote:
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):
> 
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
> (!forceMainWorldInitialization &&
> !m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
> On 2016/07/01 02:28:03, haraken wrote:
> > 
> > Hmm, it looks a bit nasty to force the main world initialization in
> > updateDocument(). Don't we have any explicit timing where the inspector can
> > force the initialization?
> 
> Alternatives are LocalDOMWindow::installNewDocument,
> DocumentLoader::createWriterFor and DocumentLoader::ensureWriter. I don't
think
> any of them is better.
> 
> As for forcing this from inspector, we try to minimize the side-effects of
> instrumentation, and instead rely on direct manipulation through settings.
This
> makes it clear to anyone working on the specific area that such a
functionality
> exist. Otherwise, it's easy to miss inspector-only side-effect and break
things.
> WDYT?

Hmm, InspectorSession is already explicitly calling
frame->script().initializeMainWorld() in a couple of places. Can we take the
same approach here? i.e., add some InspectorInstrumentation callback and call
initializeMainWorld() there.

[dgozman, 2016-07-07 00:47:23]

On 2016/07/07 00:43:32, haraken wrote:
> On 2016/07/06 17:02:20, dgozman wrote:
> >
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
> > (!forceMainWorldInitialization &&
> > !m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
> > On 2016/07/01 02:28:03, haraken wrote:
> > > 
> > > Hmm, it looks a bit nasty to force the main world initialization in
> > > updateDocument(). Don't we have any explicit timing where the inspector
can
> > > force the initialization?
> > 
> > Alternatives are LocalDOMWindow::installNewDocument,
> > DocumentLoader::createWriterFor and DocumentLoader::ensureWriter. I don't
> think
> > any of them is better.
> > 
> > As for forcing this from inspector, we try to minimize the side-effects of
> > instrumentation, and instead rely on direct manipulation through settings.
> This
> > makes it clear to anyone working on the specific area that such a
> functionality
> > exist. Otherwise, it's easy to miss inspector-only side-effect and break
> things.
> > WDYT?
> 
> Hmm, InspectorSession is already explicitly calling
> frame->script().initializeMainWorld() in a couple of places. Can we take the
> same approach here? i.e., add some InspectorInstrumentation callback and call
> initializeMainWorld() there.

This is exactly what I'm trying to remove to avoid side-effects of
InspectorInstrumentation.

Ideally, we'd also force contexts in all frames when the setting changes, and
not call initializeMainWorld() from InspectorSession at all. I can do that if
you think it's better.

[kozy, 2016-07-07 00:48:06]

thanks, lgtm

[haraken, 2016-07-07 00:51:43]

On 2016/07/07 00:47:23, dgozman wrote:
> On 2016/07/07 00:43:32, haraken wrote:
> > On 2016/07/06 17:02:20, dgozman wrote:
> > >
> >
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2112593003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:287: if
> > > (!forceMainWorldInitialization &&
> > > !m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
> > > On 2016/07/01 02:28:03, haraken wrote:
> > > > 
> > > > Hmm, it looks a bit nasty to force the main world initialization in
> > > > updateDocument(). Don't we have any explicit timing where the inspector
> can
> > > > force the initialization?
> > > 
> > > Alternatives are LocalDOMWindow::installNewDocument,
> > > DocumentLoader::createWriterFor and DocumentLoader::ensureWriter. I don't
> > think
> > > any of them is better.
> > > 
> > > As for forcing this from inspector, we try to minimize the side-effects of
> > > instrumentation, and instead rely on direct manipulation through settings.
> > This
> > > makes it clear to anyone working on the specific area that such a
> > functionality
> > > exist. Otherwise, it's easy to miss inspector-only side-effect and break
> > things.
> > > WDYT?
> > 
> > Hmm, InspectorSession is already explicitly calling
> > frame->script().initializeMainWorld() in a couple of places. Can we take the
> > same approach here? i.e., add some InspectorInstrumentation callback and
call
> > initializeMainWorld() there.
> 
> This is exactly what I'm trying to remove to avoid side-effects of
> InspectorInstrumentation.
> 
> Ideally, we'd also force contexts in all frames when the setting changes, and
> not call initializeMainWorld() from InspectorSession at all. I can do that if
> you think it's better.

Yeah, it makes more sense. It's not nice that we have two ways to force
initializing contexts. With that change, looks good.

[dgozman, 2016-07-08 00:40:55]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 00:41:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-07-08 00:45:26]

@haraken: please take another look.

[haraken, 2016-07-08 00:47:44]

Thanks, LGTM

[commit-bot: I haz the power, 2016-07-08 02:12:50]

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

[commit-bot: I haz the power, 2016-07-08 02:12:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2016-07-09 00:21:55]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-07-09 00:21:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2112593003/#ps100001
(title: "reverted default setting")

[commit-bot: I haz the power, 2016-07-09 00:22:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-07-09 01:59:50]

The CQ bit was unchecked by dgozman@chromium.org

[dgozman, 2016-07-10 05:09:18]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-07-10 05:09:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2112593003/#ps120001
(title: "migrated to inspector-protocol test")

[commit-bot: I haz the power, 2016-07-10 05:09:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 01:16:50]

Description was changed from

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337
==========

to

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337
==========

[commit-bot: I haz the power, 2016-07-11 01:16:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-11 01:16:56]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 01:19:29]

Description was changed from

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337
==========

to

==========
[DevTools] Remove [V8] from InspectorInstrumentation.

We issue a fine-grained runtimeEnabled/runtimeDiabled notification.

Introduced a new setting forceMainWorldInitialization, which inspector
uses to force contexts in all frames to be able to evaluate there.

Moved scriptBlockedByCSP pause reason to DOMDebugger agent,
similar to scriptFirstStatement.

BUG=580337

Committed: https://crrev.com/c1e6c531c70657b519121ab6b626471a75448a9e
Cr-Commit-Position: refs/heads/master@{#404588}
==========

[commit-bot: I haz the power, 2016-07-11 01:19:31]

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