[inspector] Support multiple sessions per context group

[inspector] Support multiple sessions per context group

This patch adds ability to connect multiple sessions to a single context group. This is an experimental feature, which is already supported in test harness.

So far covered runtime domain with tests (and found a bug thanks to the test). More tests to follow in next patches, probably with code adjustments as well.

BUG=chromium:590878
Review-Url: https://codereview.chromium.org/2906153002
Cr-Commit-Position: refs/heads/master@{#45667}
Committed: https://chromium.googlesource.com/v8/v8/+/375bea1c45e4ce1f71d9a2ab3a3c6e38a5833eed

[dgozman, 2017-05-26 21:39:14]

Description was changed from

==========
[inspector] Support multiple clients per context group

BUG=chromium:590878
==========

to

==========
[inspector] Support multiple sessions per context group

BUG=chromium:590878
==========

[dgozman, 2017-05-30 23:30:13]

Description was changed from

==========
[inspector] Support multiple sessions per context group

BUG=chromium:590878
==========

to

==========
[inspector] Support multiple sessions per context group

This patch adds ability to connect multiple sessions to a single context group.
This is an experimental feature, which is already supported in test harness.

So far covered runtime domain with tests (and found a bug thanks to the test).
More tests to follow in next patches, probably with code adjustments as well.

BUG=chromium:590878
==========

[dgozman, 2017-05-30 23:36:03]

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

[dgozman, 2017-05-30 23:36:03]

Alexei, could you please take a look?

[dgozman, 2017-05-31 19:03:42]

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

[commit-bot: I haz the power, 2017-05-31 19:03:53]

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, 2017-05-31 19:31:59]

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

[commit-bot: I haz the power, 2017-05-31 19:32:00]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-31 23:39:50]

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

[commit-bot: I haz the power, 2017-05-31 23:39:56]

Dry run: CQ is trying da patch.

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

[dgozman, 2017-05-31 23:43:00]

Alexei, could you please take a look at this? It's ready for review!

[commit-bot: I haz the power, 2017-06-01 00:10:47]

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

[commit-bot: I haz the power, 2017-06-01 00:10:48]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-06-01 09:43:45]

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
File src/inspector/inspected-context.h (right):

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
src/inspector/inspected-context.h:35: bool isReported(int session_id) { return
m_reported[session_id]; }
auto it = m_reported.find(session_id);
return it != m_reported.end() && *it;

to save a const :)

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
src/inspector/inspected-context.h:58: std::map<int, bool> m_reported;
can it be a set of session_ids where context was reported?

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/v8-conso...
File src/inspector/v8-console.cc (right):

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/v8-conso...
src/inspector/v8-console.cc:277:
session->debuggerAgent()->breakProgramOnException(
It looks like we'll run nested message loop for first agent, report paused to
all enabled agents, resume in one of the agents and will run nested message loop
again, will we?
Does it mean that for break on assert I'll need to click resume as many times as
many enabled agents we have?
Should we break only in first agent where break actually happens?

[dgozman, 2017-06-01 16:06:48]

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
File src/inspector/inspected-context.h (right):

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
src/inspector/inspected-context.h:58: std::map<int, bool> m_reported;
On 2017/06/01 09:43:45, kozy wrote:
> can it be a set of session_ids where context was reported?

Totally!

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/v8-conso...
File src/inspector/v8-console.cc (right):

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/v8-conso...
src/inspector/v8-console.cc:277:
session->debuggerAgent()->breakProgramOnException(
On 2017/06/01 09:43:45, kozy wrote:
> It looks like we'll run nested message loop for first agent, report paused to
> all enabled agents, resume in one of the agents and will run nested message
loop
> again, will we?
> Does it mean that for break on assert I'll need to click resume as many times
as
> many enabled agents we have?
> Should we break only in first agent where break actually happens?

Nice catch! We should report to all, but break once. I will refactor this in a
follow up and add a test.

[kozy, 2017-06-01 16:09:11]

lgtm

[dgozman, 2017-06-01 21:04:59]

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
File src/inspector/inspected-context.h (right):

https://codereview.chromium.org/2906153002/diff/100001/src/inspector/inspecte...
src/inspector/inspected-context.h:35: bool isReported(int session_id) { return
m_reported[session_id]; }
On 2017/06/01 09:43:45, kozy wrote:
> auto it = m_reported.find(session_id);
> return it != m_reported.end() && *it;
> 
> to save a const :)

Done.

[dgozman, 2017-06-01 21:05:06]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-06-01 21:05:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2906153002/#ps120001
(title: "using set per kozy@")

[commit-bot: I haz the power, 2017-06-01 21:05:17]

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, 2017-06-01 21:34:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496351106221180,
"parent_rev": "fa89ce53496e189218774c359b9b9ca22dfec2d1", "commit_rev":
"375bea1c45e4ce1f71d9a2ab3a3c6e38a5833eed"}

[commit-bot: I haz the power, 2017-06-01 21:34:07]

Description was changed from

==========
[inspector] Support multiple sessions per context group

This patch adds ability to connect multiple sessions to a single context group.
This is an experimental feature, which is already supported in test harness.

So far covered runtime domain with tests (and found a bug thanks to the test).
More tests to follow in next patches, probably with code adjustments as well.

BUG=chromium:590878
==========

to

==========
[inspector] Support multiple sessions per context group

This patch adds ability to connect multiple sessions to a single context group.
This is an experimental feature, which is already supported in test harness.

So far covered runtime domain with tests (and found a bug thanks to the test).
More tests to follow in next patches, probably with code adjustments as well.

BUG=chromium:590878

Review-Url: https://codereview.chromium.org/2906153002
Cr-Commit-Position: refs/heads/master@{#45667}
Committed:
https://chromium.googlesource.com/v8/v8/+/375bea1c45e4ce1f71d9a2ab3a3c6e38a58...
==========

[commit-bot: I haz the power, 2017-06-01 21:34:08]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/375bea1c45e4ce1f71d9a2ab3a3c6e38a58...