DevTools: Make WorkerThreadDebugger available for Worklets

DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc: https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx1k/edit

BUG=646559
Committed: https://crrev.com/46d040e09230a0a04c61fc06d88636b9c3dfb627
Cr-Commit-Position: refs/heads/master@{#430558}

[nhiroki, 2016-10-27 02:10:24]

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

[commit-bot: I haz the power, 2016-10-27 02:10:43]

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-10-27 02:21:52]

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

[commit-bot: I haz the power, 2016-10-27 02:21:53]

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

[nhiroki, 2016-11-02 09:33:20]

Description was changed from

==========
(WIP) Worker: Support WorkerThreadDebugger for Worklets

BUG=
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, we need to (1) expand 1:1 relationship among WorkerThreadDebugger
and WorkerThread to 1:M relationship, and (2) decouple lifetime of
WorkerThreadDebugger from lifetime of WorkerThread.

Regarding (1), this CL allocate an identifier per WorkerThread and make
WorkerThreadDebugger manage ID-WorkerThread map.

Regarding (2), before this CL, WorkerThreadDebugger has the same lifetime of
WorkerThread. This prevent WorkerThreadDebugger from hosting multiple
WorkerThreads. After this CL, WorkerThreadDebugger has the same lifetime of
WorkerBackingThread. WorkerThreadDebugger is attached with WorkerThread when
WorkerThread is created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[nhiroki, 2016-11-02 09:55:41]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, we need to (1) expand 1:1 relationship among WorkerThreadDebugger
and WorkerThread to 1:M relationship, and (2) decouple lifetime of
WorkerThreadDebugger from lifetime of WorkerThread.

Regarding (1), this CL allocate an identifier per WorkerThread and make
WorkerThreadDebugger manage ID-WorkerThread map.

Regarding (2), before this CL, WorkerThreadDebugger has the same lifetime of
WorkerThread. This prevent WorkerThreadDebugger from hosting multiple
WorkerThreads. After this CL, WorkerThreadDebugger has the same lifetime of
WorkerBackingThread. WorkerThreadDebugger is attached with WorkerThread when
WorkerThread is created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, we need to (1) expand 1:1 relationship among WorkerThreadDebugger
and WorkerThread to 1:M relationship, and (2) decouple lifetime of
WorkerThreadDebugger from lifetime of WorkerThread.

Regarding (1), this CL allocate an identifier per WorkerThread and make
WorkerThreadDebugger manage ID-WorkerThread map.

Regarding (2), before this CL, WorkerThreadDebugger has the same lifetime of
WorkerThread. This prevent WorkerThreadDebugger from hosting multiple
WorkerThreads. After this CL, WorkerThreadDebugger has the same lifetime of
WorkerBackingThread. WorkerThreadDebugger is attached with WorkerThread when
WorkerThread is created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[nhiroki, 2016-11-02 09:56:53]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2016-11-02 09:57:02]

Patchset #2 (id:40001) has been deleted

[nhiroki, 2016-11-02 09:57:10]

Patchset #2 (id:60001) has been deleted

[nhiroki, 2016-11-02 09:57:19]

Patchset #2 (id:80001) has been deleted

[nhiroki, 2016-11-02 10:02:40]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-11-02 10:03:00]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2016-11-02 10:03:11]

Patchset #1 (id:120001) has been deleted

[nhiroki, 2016-11-02 10:03:18]

Patchset #1 (id:140001) has been deleted

[nhiroki, 2016-11-02 10:17:46]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, we need to (1) expand 1:1 relationship among WorkerThreadDebugger
and WorkerThread to 1:M relationship, and (2) decouple lifetime of
WorkerThreadDebugger from lifetime of WorkerThread.

Regarding (1), this CL allocate an identifier per WorkerThread and make
WorkerThreadDebugger manage ID-WorkerThread map.

Regarding (2), before this CL, WorkerThreadDebugger has the same lifetime of
WorkerThread. This prevent WorkerThreadDebugger from hosting multiple
WorkerThreads. After this CL, WorkerThreadDebugger has the same lifetime of
WorkerBackingThread. WorkerThreadDebugger is attached with WorkerThread when
WorkerThread is created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[nhiroki, 2016-11-02 10:23:56]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra cannot host
multiple worklet execution contexts on a single thread.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra(*) cannot
host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

After this CL, Worklets can at least output console messages.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[nhiroki, 2016-11-02 10:29:26]

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

[nhiroki, 2016-11-02 10:29:27]

Hi, can you review this? I'll add layout tests in this CL later. Thanks.

[dgozman, 2016-11-02 17:23:38]

Overall, looks very good!

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:158:
DCHECK(m_workerThreads.contains(contextGroupId));
Just set m_pausedContentGroupId in runMessageLoopOnPause and use it here?

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:160:
->stopRunningDebuggerTasksOnPauseOnWorkerThread();
I wonder whether we should now move inspector tasks to WorkerBackingThread,
since we should pause all possible worklets running on the single thread, and
thus blocking the thread makes sense on the WorkerBackingThread level. WDYT?

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h (right):

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h:55: int
contextGroupId(WorkerOrWorkletGlobalScope*);
First thing we do in all these methods is converting global scope into worker
thread. Maybe just pass worker thread instead?

[nhiroki, 2016-11-04 08:00:06]

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

[commit-bot: I haz the power, 2016-11-04 08:00:32]

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-11-04 09:08:47]

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

[commit-bot: I haz the power, 2016-11-04 09:08:47]

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_...)

[nhiroki, 2016-11-04 10:36:14]

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

[commit-bot: I haz the power, 2016-11-04 10:36:36]

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

[nhiroki, 2016-11-04 11:09:29]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets [3]

DevTools is now disabled on Worklets because worker's debugger infra(*) cannot
host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

After this CL, Worklets can at least output console messages.

[1] Chromium: https://codereview.chromium.org/2469233002/
[2] V8: https://codereview.chromium.org/2451893005/
[3] Chromium: THIS PATCH

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559i
==========

[nhiroki, 2016-11-04 11:29:12]

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

[commit-bot: I haz the power, 2016-11-04 11:29:21]

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

[nhiroki, 2016-11-04 11:31:17]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559i
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[nhiroki, 2016-11-04 11:31:37]

Patchset #2 (id:180001) has been deleted

[nhiroki, 2016-11-04 11:42:43]

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

[commit-bot: I haz the power, 2016-11-04 11:42:52]

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

[nhiroki, 2016-11-04 11:43:20]

Patchset #3 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-11-04 13:08:56]

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

[commit-bot: I haz the power, 2016-11-04 13:08:57]

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

[nhiroki, 2016-11-07 02:15:52]

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

[commit-bot: I haz the power, 2016-11-07 02:15:59]

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-11-07 04:09:47]

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

[commit-bot: I haz the power, 2016-11-07 04:09:50]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-11-07 08:23:18]

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

[nhiroki, 2016-11-07 08:23:28]

Patchset #6 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-11-07 08:23:33]

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-11-07 09:35:52]

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

[commit-bot: I haz the power, 2016-11-07 09:35:53]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-11-07 10:06:09]

Thank you for you comments and sorry for my delayed reply. I tackled with
https://crbug.com/662812 and that turned out to be a separate issue :)

The latest patchset-6 addresses your comments and adds layout tests. Can you
take another look?

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:158:
DCHECK(m_workerThreads.contains(contextGroupId));
On 2016/11/02 17:23:37, dgozman wrote:
> Just set m_pausedContentGroupId in runMessageLoopOnPause and use it here?

Done.

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:160:
->stopRunningDebuggerTasksOnPauseOnWorkerThread();
On 2016/11/02 17:23:37, dgozman wrote:
> I wonder whether we should now move inspector tasks to WorkerBackingThread,
> since we should pause all possible worklets running on the single thread, and
> thus blocking the thread makes sense on the WorkerBackingThread level. WDYT?

That sounds reasonable. In the current implementation, inspector tasks are tied
with state management of WorkerThread and I don't think it's easy to move it to
WorkerBackingThread, so I prefer to work on it later as a separate issue.

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h (right):

https://codereview.chromium.org/2456733002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h:55: int
contextGroupId(WorkerOrWorkletGlobalScope*);
On 2016/11/02 17:23:37, dgozman wrote:
> First thing we do in all these methods is converting global scope into worker
> thread. Maybe just pass worker thread instead?

Done.

[nhiroki, 2016-11-07 10:07:37]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org

[nhiroki, 2016-11-07 10:07:38]

+falken@ for reviews from worker's POV

[dgozman, 2016-11-07 18:47:55]

lgtm

We'll need inspector tests later, once we can connect to worklet: something
similar to inspector-protocol/worker/exception-from-worker-contains-stack.html
for example.

https://codereview.chromium.org/2456733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:179:
DCHECK(m_workerThreads.contains(contextGroupId)) << contextGroupId;
Left-over debugging?

[nhiroki, 2016-11-08 01:05:24]

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

[nhiroki, 2016-11-08 01:05:36]

Thank you :)

https://codereview.chromium.org/2456733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:179:
DCHECK(m_workerThreads.contains(contextGroupId)) << contextGroupId;
On 2016/11/07 18:47:55, dgozman wrote:
> Left-over debugging?

Done.

[commit-bot: I haz the power, 2016-11-08 01:05:56]

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

[nhiroki, 2016-11-08 01:06:32]

On 2016/11/07 18:47:55, dgozman wrote:
> lgtm
> 
> We'll need inspector tests later, once we can connect to worklet: something
> similar to inspector-protocol/worker/exception-from-worker-contains-stack.html
> for example.

Acknowledged.

[nhiroki, 2016-11-08 01:07:14]

nhiroki@chromium.org changed reviewers:
+ haraken@chromium.org

[nhiroki, 2016-11-08 01:07:15]

+haraken, can you review bindings/core/v8/WorkerOrWorkletScriptController.cpp?
Thanks.

[haraken, 2016-11-08 01:50:20]

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:128: return
nextWorkerThreadId++;

Shall we add a CHECK to crash if the id overflows?

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:160: WorkerThread*
WorkerThread::from(ExecutionContext* globalScope) {

Should we change ExecutionContext* to WorkerOrWorkletGlobalScope*? It doesn't
really make sense to pass ExecutionContext* to "Worker"Thread::from.

Also I'd prefer introducing WorkerOrWorkletGlobalScope::thread() (a virtual
method) rather than WorkerThread::from.

[commit-bot: I haz the power, 2016-11-08 02:33:39]

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

[commit-bot: I haz the power, 2016-11-08 02:33:40]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-11-08 05:16:55]

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

[commit-bot: I haz the power, 2016-11-08 05:17:10]

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

[nhiroki, 2016-11-08 06:06:51]

Thank you. Updated.

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:128: return
nextWorkerThreadId++;
On 2016/11/08 01:50:20, haraken wrote:
> 
> Shall we add a CHECK to crash if the id overflows?

Done.

https://codereview.chromium.org/2456733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:160: WorkerThread*
WorkerThread::from(ExecutionContext* globalScope) {
On 2016/11/08 01:50:20, haraken wrote:
> 
> Should we change ExecutionContext* to WorkerOrWorkletGlobalScope*? It doesn't
> really make sense to pass ExecutionContext* to "Worker"Thread::from.
> 
> Also I'd prefer introducing WorkerOrWorkletGlobalScope::thread() (a virtual
> method) rather than WorkerThread::from.
> 

That looks simpler. Added the virtual method in
https://codereview.chromium.org/2481843003/ and rebased this CL on that.

[haraken, 2016-11-08 06:55:36]

LGTM

https://codereview.chromium.org/2456733002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:173: return
scriptState ? scriptState->context() : v8::Local<v8::Context>();

Is it possible that the scriptState is null?

Also it looks strange that "ensureDefaultContext" returns an empty context...

[commit-bot: I haz the power, 2016-11-08 07:52:43]

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

[commit-bot: I haz the power, 2016-11-08 07:52:44]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-11-08 09:38:19]

Thank you for reviewing.

https://codereview.chromium.org/2456733002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right):

https://codereview.chromium.org/2456733002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:173: return
scriptState ? scriptState->context() : v8::Local<v8::Context>();
On 2016/11/08 06:55:35, haraken wrote:
> 
> Is it possible that the scriptState is null?
> 
> Also it looks strange that "ensureDefaultContext" returns an empty context...

Yeah, this looks strange to me, too. I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=663296

[nhiroki, 2016-11-08 09:39:50]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-11-08 09:39:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2456733002/#ps360001
(title: "address review comments")

[commit-bot: I haz the power, 2016-11-08 09:40:12]

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-11-08 09:46:03]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

[commit-bot: I haz the power, 2016-11-08 09:46:05]

Committed patchset #8 (id:360001)

[commit-bot: I haz the power, 2016-11-08 09:49:48]

Description was changed from

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559
==========

to

==========
DevTools: Make WorkerThreadDebugger available for Worklets

DevTools is now disabled on Worklets because worker's debugger infrastructure(*)
cannot host multiple worklet execution contexts on a single thread.
(*) Worklet has been implemented on the worker infrastructure.

To support it, this CL does following things:

- This expands 1:1 relationship between WorkerThreadDebugger and WorkerThread to
  1:M relationship. The debugger has a ID-WorkerThread map to manage multiple
  WorkerThreads. WorkerThread is added when worklet context is created and
  removed when the context is destroyed.

- This decouples lifetime of WorkerThreadDebugger from lifetime of worklet
  context. Before this CL, the debugger has the same lifetime of the context.
  This prevents the debugger from hosting multiple contexts. After this CL,
  the debugger has the same lifetime of WorkerBackingThread, and attaches to
  worklet context when it's created.

DesignDoc:
https://docs.google.com/document/d/1v0dNB6jWMe7w6OeK-Vlnn-Q4qW7KI4m97LGnMefUx...

BUG=646559

Committed: https://crrev.com/46d040e09230a0a04c61fc06d88636b9c3dfb627
Cr-Commit-Position: refs/heads/master@{#430558}
==========

[commit-bot: I haz the power, 2016-11-08 09:49:50]

Patchset 8 (id:??) landed as
https://crrev.com/46d040e09230a0a04c61fc06d88636b9c3dfb627
Cr-Commit-Position: refs/heads/master@{#430558}

[xlai (Olivia), 2016-11-08 16:51:51]

A revert of this CL (patchset #8 id:360001) has been created in
https://codereview.chromium.org/2485633004/ by xlai@chromium.org.

The reason for reverting is: The layout test animation-worklet-console.html is
failing on WebKit Linux Precise Leak bot:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis....