Bindings: Manage multiple DOMWrapperWorlds for worklets

Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |worldMap| to manage multiple worker-or-worklet
worlds on a single thread for the issue 1), and teaches the main thread that
there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
Review-Url: https://codereview.chromium.org/2735823006
Cr-Commit-Position: refs/heads/master@{#456683}
Committed: https://chromium.googlesource.com/chromium/src/+/0fe6bc1c1b65086da86d225ac7dfe9b00a317a1e

[nhiroki, 2017-03-07 09:08:11]

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

[commit-bot: I haz the power, 2017-03-07 09:08:40]

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

[nhiroki, 2017-03-07 09:09:35]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there were 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread This didn't
   work when multiple worklets can be created on the same thread.
2) DOMWrapperWorld assumed that a world for a worker-or-worklet is never created
   on the main thread, but actually it's possible to create a world for the main
   thread worklet (i.e., PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple worlds for
workers-or-worklets for the issue 1), and teaches the main thread that there can
be multiple worker worlds for the issue 2).

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there were 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread. This didn't
   work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumed that a world for a worker-or-worklet is never created
   on the main thread, but actually it's possible to create a world for the main
   thread worklet (i.e., PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple worlds for
workers-or-worklets for the issue 1), and teaches the main thread that there can
be multiple worker worlds for the issue 2).

BUG=697622, 697629
==========

[nhiroki, 2017-03-07 09:15:10]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there were 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread. This didn't
   work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumed that a world for a worker-or-worklet is never created
   on the main thread, but actually it's possible to create a world for the main
   thread worklet (i.e., PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple worlds for
workers-or-worklets for the issue 1), and teaches the main thread that there can
be multiple worker worlds for the issue 2).

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there were 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This didn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumed that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple
worker-or-worklet worlds on a single thread for the issue 1), and teaches the
main thread that there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

[nhiroki, 2017-03-07 09:57:36]

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

[nhiroki, 2017-03-07 09:57:37]

PTAL. Is this a right fix?

[commit-bot: I haz the power, 2017-03-07 10:00:08]

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

[commit-bot: I haz the power, 2017-03-07 10:00:09]

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

[Yuki, 2017-03-07 10:17:15]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2017-03-07 10:17:16]

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
map.insert(worldId, this);
Does this support multiple worlds in a thread?

IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're always using
the same world id.

[nhiroki, 2017-03-07 10:45:15]

Thanks for your comment!

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
map.insert(worldId, this);
On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> Does this support multiple worlds in a thread?
> 
> IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're always
using
> the same world id.

You're right. This doesn't make sense. Is it ok to reserve the identifier space
for worklets like IsolatedWorldIdLimit?

[nhiroki, 2017-03-07 10:51:40]

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
map.insert(worldId, this);
On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > Does this support multiple worlds in a thread?
> > 
> > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're always
> using
> > the same world id.
> 
> You're right. This doesn't make sense. Is it ok to reserve the identifier
space
> for worklets like IsolatedWorldIdLimit?

Or, how about separating WorldIdConstants into tags of world types and world
ids?

[Yuki, 2017-03-07 11:51:26]

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
map.insert(worldId, this);
On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > Does this support multiple worlds in a thread?
> > > 
> > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're always
> > using
> > > the same world id.
> > 
> > You're right. This doesn't make sense. Is it ok to reserve the identifier
> space
> > for worklets like IsolatedWorldIdLimit?
> 
> Or, how about separating WorldIdConstants into tags of world types and world
> ids?

I'm personally okay to introduce IsolatedWorldIdLimit, but we need to be super
careful not to break things.  IMHO, it has been a kind of fundamental assumption
that we only have a single world for worker.  It wouldn't be easy to change (but
I'm supportive).

haraken@, what do you think?

[haraken, 2017-03-07 11:57:33]

On 2017/03/07 11:51:26, Yuki(slow_til_mar08) wrote:
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> map.insert(worldId, this);
> On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > Does this support multiple worlds in a thread?
> > > > 
> > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
always
> > > using
> > > > the same world id.
> > > 
> > > You're right. This doesn't make sense. Is it ok to reserve the identifier
> > space
> > > for worklets like IsolatedWorldIdLimit?
> > 
> > Or, how about separating WorldIdConstants into tags of world types and world
> > ids?
> 
> I'm personally okay to introduce IsolatedWorldIdLimit, but we need to be super
> careful not to break things.  IMHO, it has been a kind of fundamental
assumption
> that we only have a single world for worker.  It wouldn't be easy to change
(but
> I'm supportive).
> 
> haraken@, what do you think?

I'm confused.

WorkerOrWorkletScriptController already has its own m_world. Doesn't it mean
that each Worklet already has its own world?

[nhiroki, 2017-03-07 15:12:45]

On 2017/03/07 11:57:33, haraken wrote:
> On 2017/03/07 11:51:26, Yuki(slow_til_mar08) wrote:
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > map.insert(worldId, this);
> > On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > > Does this support multiple worlds in a thread?
> > > > > 
> > > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
> always
> > > > using
> > > > > the same world id.
> > > > 
> > > > You're right. This doesn't make sense. Is it ok to reserve the
identifier
> > > space
> > > > for worklets like IsolatedWorldIdLimit?
> > > 
> > > Or, how about separating WorldIdConstants into tags of world types and
world
> > > ids?
> > 
> > I'm personally okay to introduce IsolatedWorldIdLimit, but we need to be
super
> > careful not to break things.  IMHO, it has been a kind of fundamental
> assumption
> > that we only have a single world for worker.  It wouldn't be easy to change
> (but
> > I'm supportive).
> > 
> > haraken@, what do you think?
> 
> I'm confused.
> 
> WorkerOrWorkletScriptController already has its own m_world. Doesn't it mean
> that each Worklet already has its own world?

Yes, each Worklet already has its own world.

In the current implementation, DOMWrapperWorld assumes 1) there is no
WorkerWorld on the main thread, and 2) there is only one WorkerWorld on a worker
thread. These assumption are no longer correct because multiple Worklets can run
on both the main thread and worker thread. This CL fixes the invalid
assumptions.

[nhiroki, 2017-03-07 15:13:13]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there were 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This didn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumed that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple
worker-or-worklet worlds on a single thread for the issue 1), and teaches the
main thread that there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple
worker-or-worklet worlds on a single thread for the issue 1), and teaches the
main thread that there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

[peria, 2017-03-08 02:14:51]

On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> These assumption are no longer correct because multiple Worklets can run
> on both the main thread and worker thread.

Then, you can't use thread specific maps in workerWorldMap() or something
like it, right?
I put this comment just in case if you'll update this CL.

[peria, 2017-03-08 02:17:00]

On 2017/03/07 10:51:40, nhiroki (slow) wrote:
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> 
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> map.insert(worldId, this);
> On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > Does this support multiple worlds in a thread?
> > > 
> > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're always
> > using
> > > the same world id.
> > 
> > You're right. This doesn't make sense. Is it ok to reserve the identifier
> space
> > for worklets like IsolatedWorldIdLimit?
> 
> Or, how about separating WorldIdConstants into tags of world types and world
> ids?

I think it better to have such tags beside of world IDs.

[nhiroki, 2017-03-08 02:34:14]

On 2017/03/08 02:17:00, peria wrote:
> On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > map.insert(worldId, this);
> > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > Does this support multiple worlds in a thread?
> > > > 
> > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
always
> > > using
> > > > the same world id.
> > > 
> > > You're right. This doesn't make sense. Is it ok to reserve the identifier
> > space
> > > for worklets like IsolatedWorldIdLimit?
> > 
> > Or, how about separating WorldIdConstants into tags of world types and world
> > ids?
> 
> I think it better to have such tags beside of world IDs.

[nhiroki, 2017-03-08 02:50:50]

On 2017/03/08 02:14:51, peria wrote:
> On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> > These assumption are no longer correct because multiple Worklets can run
> > on both the main thread and worker thread.
> 
> Then, you can't use thread specific maps in workerWorldMap() or something
> like it, right?
> I put this comment just in case if you'll update this CL.

I think the thread-specific map is available here because Worlds are the
per-thread concept, right?

On 2017/03/08 02:17:00, peria wrote:
> On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > map.insert(worldId, this);
> > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > Does this support multiple worlds in a thread?
> > > > 
> > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
always
> > > using
> > > > the same world id.
> > > 
> > > You're right. This doesn't make sense. Is it ok to reserve the identifier
> > space
> > > for worklets like IsolatedWorldIdLimit?
> > 
> > Or, how about separating WorldIdConstants into tags of world types and world
> > ids?
> 
> I think it better to have such tags beside of world IDs.

Do you mean it's better to manage the tags and world IDs in the single enum like
the current implementation? Can you explain the reason why you think so? IMHO,
it'd be better to separate them in terms of extensibility and readability. For
example, in the current implementation, we need to estimate how many worklets
(WorkerWorlds) will be created on a thread for deciding the size of
WorkerWorldID space (Note that I don't know how difficult to do this change :p)

[peria, 2017-03-08 03:39:49]

On 2017/03/08 02:50:50, nhiroki (slow) wrote:
> On 2017/03/08 02:14:51, peria wrote:
> > On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> > > These assumption are no longer correct because multiple Worklets can run
> > > on both the main thread and worker thread.
> > 
> > Then, you can't use thread specific maps in workerWorldMap() or something
> > like it, right?
> > I put this comment just in case if you'll update this CL.
> 
> I think the thread-specific map is available here because Worlds are the
> per-thread concept, right?
> 

For the record of offline chat;
- Each worklet is run on a thread.
- A thread (including the main thread) can have multiple worklets.
Thus having thread specific maps is reasonable.


> On 2017/03/08 02:17:00, peria wrote:
> > On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > > map.insert(worldId, this);
> > > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > > Does this support multiple worlds in a thread?
> > > > > 
> > > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
> always
> > > > using
> > > > > the same world id.
> > > > 
> > > > You're right. This doesn't make sense. Is it ok to reserve the
identifier
> > > space
> > > > for worklets like IsolatedWorldIdLimit?
> > > 
> > > Or, how about separating WorldIdConstants into tags of world types and
world
> > > ids?
> > 
> > I think it better to have such tags beside of world IDs.
> 
> Do you mean it's better to manage the tags and world IDs in the single enum
like
> the current implementation? Can you explain the reason why you think so? IMHO,
> it'd be better to separate them in terms of extensibility and readability. For
> example, in the current implementation, we need to estimate how many worklets
> (WorkerWorlds) will be created on a thread for deciding the size of
> WorkerWorldID space (Note that I don't know how difficult to do this change
:p)

I meant to have them separately.
It doesn't make sense to me that an ID has multiple meanings.

[nhiroki, 2017-03-08 03:40:26]

On 2017/03/08 02:50:50, nhiroki (slow) wrote:
> On 2017/03/08 02:14:51, peria wrote:
> > On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> > > These assumption are no longer correct because multiple Worklets can run
> > > on both the main thread and worker thread.
> > 
> > Then, you can't use thread specific maps in workerWorldMap() or something
> > like it, right?
> > I put this comment just in case if you'll update this CL.
> 
> I think the thread-specific map is available here because Worlds are the
> per-thread concept, right?
> 
> On 2017/03/08 02:17:00, peria wrote:
> > On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > > map.insert(worldId, this);
> > > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > > Does this support multiple worlds in a thread?
> > > > > 
> > > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
> always
> > > > using
> > > > > the same world id.
> > > > 
> > > > You're right. This doesn't make sense. Is it ok to reserve the
identifier
> > > space
> > > > for worklets like IsolatedWorldIdLimit?
> > > 
> > > Or, how about separating WorldIdConstants into tags of world types and
world
> > > ids?
> > 
> > I think it better to have such tags beside of world IDs.
> 
> Do you mean it's better to manage the tags and world IDs in the single enum
like
> the current implementation? Can you explain the reason why you think so? IMHO,
> it'd be better to separate them in terms of extensibility and readability. For
> example, in the current implementation, we need to estimate how many worklets
> (WorkerWorlds) will be created on a thread for deciding the size of
> WorkerWorldID space (Note that I don't know how difficult to do this change
:p)

I had offline chat with peria@ and confirmed that we're on the same page, that
is, he also think it's better to separate them.

[nhiroki, 2017-03-08 06:47:18]

On 2017/03/08 03:40:26, nhiroki (slow) wrote:
> On 2017/03/08 02:50:50, nhiroki (slow) wrote:
> > On 2017/03/08 02:14:51, peria wrote:
> > > On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> > > > These assumption are no longer correct because multiple Worklets can run
> > > > on both the main thread and worker thread.
> > > 
> > > Then, you can't use thread specific maps in workerWorldMap() or something
> > > like it, right?
> > > I put this comment just in case if you'll update this CL.
> > 
> > I think the thread-specific map is available here because Worlds are the
> > per-thread concept, right?
> > 
> > On 2017/03/08 02:17:00, peria wrote:
> > > On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > > > map.insert(worldId, this);
> > > > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > > > Does this support multiple worlds in a thread?
> > > > > > 
> > > > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
> > always
> > > > > using
> > > > > > the same world id.
> > > > > 
> > > > > You're right. This doesn't make sense. Is it ok to reserve the
> identifier
> > > > space
> > > > > for worklets like IsolatedWorldIdLimit?
> > > > 
> > > > Or, how about separating WorldIdConstants into tags of world types and
> world
> > > > ids?
> > > 
> > > I think it better to have such tags beside of world IDs.
> > 
> > Do you mean it's better to manage the tags and world IDs in the single enum
> like
> > the current implementation? Can you explain the reason why you think so?
IMHO,
> > it'd be better to separate them in terms of extensibility and readability.
For
> > example, in the current implementation, we need to estimate how many
worklets
> > (WorkerWorlds) will be created on a thread for deciding the size of
> > WorkerWorldID space (Note that I don't know how difficult to do this change
> :p)
> 
> I had offline chat with peria@ and confirmed that we're on the same page, that
> is, he also think it's better to separate them.

I'm now thinking to work on worklet support first without this refactoring and
try it later with a more concrete design because this seems to be more
complicated than I expected (especially, WorldId handling for IsolatedWorlds).

[haraken, 2017-03-08 09:33:35]

On 2017/03/08 06:47:18, nhiroki (slow) wrote:
> On 2017/03/08 03:40:26, nhiroki (slow) wrote:
> > On 2017/03/08 02:50:50, nhiroki (slow) wrote:
> > > On 2017/03/08 02:14:51, peria wrote:
> > > > On 2017/03/07 15:12:45, nhiroki (slow) wrote:
> > > > > These assumption are no longer correct because multiple Worklets can
run
> > > > > on both the main thread and worker thread.
> > > > 
> > > > Then, you can't use thread specific maps in workerWorldMap() or
something
> > > > like it, right?
> > > > I put this comment just in case if you'll update this CL.
> > > 
> > > I think the thread-specific map is available here because Worlds are the
> > > per-thread concept, right?
> > > 
> > > On 2017/03/08 02:17:00, peria wrote:
> > > > On 2017/03/07 10:51:40, nhiroki (slow) wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > > > File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp
> > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2735823006/diff/1/third_party/WebKit/Source/b...
> > > > > third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:106:
> > > > > map.insert(worldId, this);
> > > > > On 2017/03/07 10:45:14, nhiroki (slow) wrote:
> > > > > > On 2017/03/07 10:17:16, Yuki(slow_til_mar08) wrote:
> > > > > > > Does this support multiple worlds in a thread?
> > > > > > > 
> > > > > > > IIUC, worldId == m_worldId == WorkerWorldId == 1<<29 + 3, so we're
> > > always
> > > > > > using
> > > > > > > the same world id.
> > > > > > 
> > > > > > You're right. This doesn't make sense. Is it ok to reserve the
> > identifier
> > > > > space
> > > > > > for worklets like IsolatedWorldIdLimit?
> > > > > 
> > > > > Or, how about separating WorldIdConstants into tags of world types and
> > world
> > > > > ids?
> > > > 
> > > > I think it better to have such tags beside of world IDs.
> > > 
> > > Do you mean it's better to manage the tags and world IDs in the single
enum
> > like
> > > the current implementation? Can you explain the reason why you think so?
> IMHO,
> > > it'd be better to separate them in terms of extensibility and readability.
> For
> > > example, in the current implementation, we need to estimate how many
> worklets
> > > (WorkerWorlds) will be created on a thread for deciding the size of
> > > WorkerWorldID space (Note that I don't know how difficult to do this
change
> > :p)
> > 
> > I had offline chat with peria@ and confirmed that we're on the same page,
that
> > is, he also think it's better to separate them.
> 
> I'm now thinking to work on worklet support first without this refactoring and
> try it later with a more concrete design because this seems to be more
> complicated than I expected (especially, WorldId handling for IsolatedWorlds).

FWIW, the world ID is exposed to the Chromium side, which is assuming the
uniqueness of the ID. i.e.,  we shouldn't assign the same ID on two worlds.

[nhiroki, 2017-03-09 11:59:05]

Patchset #3 (id:40001) has been deleted

[nhiroki, 2017-03-09 12:02:52]

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

[commit-bot: I haz the power, 2017-03-09 12:03:10]

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

[nhiroki, 2017-03-09 12:10:36]

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

[commit-bot: I haz the power, 2017-03-09 12:12:00]

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-03-09 12:55:16]

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

[commit-bot: I haz the power, 2017-03-09 12:55:17]

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

[nhiroki, 2017-03-09 15:14:34]

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

[commit-bot: I haz the power, 2017-03-09 15:14:50]

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-03-09 15:32:18]

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

[commit-bot: I haz the power, 2017-03-09 15:32:19]

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

[nhiroki, 2017-03-10 01:53:05]

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

[commit-bot: I haz the power, 2017-03-10 01:53:39]

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-03-10 03:36:27]

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

[commit-bot: I haz the power, 2017-03-10 03:36:29]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-03-12 03:25:13]

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

[nhiroki, 2017-03-12 03:25:19]

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

[commit-bot: I haz the power, 2017-03-12 03:25:25]

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-03-12 05:03:08]

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

[commit-bot: I haz the power, 2017-03-12 05:03:09]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-03-14 05:58:09]

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

[commit-bot: I haz the power, 2017-03-14 05:58:26]

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

[nhiroki, 2017-03-14 06:17:05]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |workerWorldMap| to manage multiple
worker-or-worklet worlds on a single thread for the issue 1), and teaches the
main thread that there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |worldMap| to manage multiple worker-or-worklet
worlds on a single thread for the issue 1), and teaches the main thread that
there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

[nhiroki, 2017-03-14 06:17:52]

Patchset #8 (id:160001) has been deleted

[nhiroki, 2017-03-14 06:25:07]

Updated the whole patch based on my cleanup (
https://codereview.chromium.org/2735973006/). Please take another look.

The latest patchset-8 assigns a new world id every time a new worker world is
created so that the id conflict yukishiino@ pointed out should no longer happen.

Thanks.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:168: const
ScriptWrappableVisitor* visitor) {
Question: AFAICS, this function doesn't mark wrappers in the main world. Is this
expected behavior?

[peria, 2017-03-14 07:10:48]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2017-03-14 07:10:49]

over all, it looks good to me. :)

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:135: }
Add
default: NOTREACHED();
to catch unexpected behaviors just in case.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:168: const
ScriptWrappableVisitor* visitor) {
On 2017/03/14 06:25:07, nhiroki (slow) wrote:
> Question: AFAICS, this function doesn't mark wrappers in the main world. Is
this
> expected behavior?

scriptWrappable->markWrapper(visitor) does.
IIUC, each ScriptWrappble object holds a (weak persistent) pointer to its
wrapper in the main world, and the main world exists only in the main thread.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:170:
scriptWrappable->markWrapper(visitor);
can we put this under the if branch on #181?
Then we don't need #169.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:345: // The last
used world ID. Must be used with atomic operations because this is
code health: remove the first sentence. It explains no additional information.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: static int
s_lastUsedWorldId =
First ID to be used is UnspecifiedWorldIdStart, and it is not "last used world
ID" before the first  assignment.
Please rename this static variable.

[commit-bot: I haz the power, 2017-03-14 07:26:33]

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

[commit-bot: I haz the power, 2017-03-14 07:26:34]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-03-14 08:06:00]

LGTM.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: static int
s_lastUsedWorldId =
nit: Better to put this variable inside generateWorldIdForType so that no one
can access to this by accident.  Should we use DEFINE_THREAD_SAFE_STATIC_LOCAL
then?

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:363: return
WTF::atomicIncrement(&s_lastUsedWorldId);
nit: I understand it's super unlikely, but should we check an overflow?  I'm
okay to crash if we hit an overflow.

[nhiroki, 2017-03-14 08:52:53]

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

[commit-bot: I haz the power, 2017-03-14 08:53:03]

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

[haraken, 2017-03-14 08:53:59]

Why do we need

[haraken, 2017-03-14 08:54:29]

Why do we need to differentiate isolatedWorldMap() from worldMap()? Can we use
the same map for all non-main worlds?

[Yuki, 2017-03-14 09:05:21]

I think that haraken's idea is good, but also think that we can do it in a
separate CL.

https://codereview.chromium.org/2735823006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: int,
s_lastUsedWorldId,
nit: s/lastUsed/lastGenerated/

[nhiroki, 2017-03-14 09:14:39]

Thank you for your comments!

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:135: }
On 2017/03/14 07:10:49, peria wrote:
> Add
> default: NOTREACHED();
> to catch unexpected behaviors just in case.

This unexpected behavior could happen only when static_cast<> is used, right?

I'd rather not add 'default' because it disables the compiler's branch coverage
check and would be more error-prone than the case.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:168: const
ScriptWrappableVisitor* visitor) {
On 2017/03/14 07:10:49, peria wrote:
> On 2017/03/14 06:25:07, nhiroki (slow) wrote:
> > Question: AFAICS, this function doesn't mark wrappers in the main world. Is
> this
> > expected behavior?
> 
> scriptWrappable->markWrapper(visitor) does.
> IIUC, each ScriptWrappble object holds a (weak persistent) pointer to its
> wrapper in the main world, and the main world exists only in the main thread.

Thank you for the clarification.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:170:
scriptWrappable->markWrapper(visitor);
On 2017/03/14 07:10:49, peria wrote:
> can we put this under the if branch on #181?
> Then we don't need #169.

Moved it back (I wrongly thought this is necessary before marking wrappers on
the main thread, so move it to here :p)

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:345: // The last
used world ID. Must be used with atomic operations because this is
On 2017/03/14 07:10:49, peria wrote:
> code health: remove the first sentence. It explains no additional information.

Done.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: static int
s_lastUsedWorldId =
On 2017/03/14 07:10:49, peria wrote:
> First ID to be used is UnspecifiedWorldIdStart, and it is not "last used world
> ID" before the first  assignment.
> Please rename this static variable.

Replaced UnspecifiedWorldIdStart with IsolatedWorldIdLimit instead of renaming
the variable because I didn't come up with a better name...

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: static int
s_lastUsedWorldId =
On 2017/03/14 08:06:00, Yuki wrote:
> nit: Better to put this variable inside generateWorldIdForType so that no one
> can access to this by accident.  Should we use DEFINE_THREAD_SAFE_STATIC_LOCAL
> then?

Done.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:363: return
WTF::atomicIncrement(&s_lastUsedWorldId);
On 2017/03/14 08:06:00, Yuki wrote:
> nit: I understand it's super unlikely, but should we check an overflow?  I'm
> okay to crash if we hit an overflow.

Done.

https://codereview.chromium.org/2735823006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:347: int,
s_lastUsedWorldId,
On 2017/03/14 09:05:20, Yuki wrote:
> nit: s/lastUsed/lastGenerated/

Done.

[nhiroki, 2017-03-14 09:15:10]

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

[commit-bot: I haz the power, 2017-03-14 09:15:21]

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

[haraken, 2017-03-14 09:18:20]

Assuming that the worldMap is gone in a follow-up CL, looks good.

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:348: new
int(DOMWrapperWorld::WorldId::IsolatedWorldIdLimit));

I think you could just use ThreadSpecific. The world ID needs to be unique per
thread, not per process.

[nhiroki, 2017-03-14 09:37:56]

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

[commit-bot: I haz the power, 2017-03-14 09:38:08]

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

[nhiroki, 2017-03-14 09:39:05]

Thank you.

On 2017/03/14 09:18:20, haraken wrote:
> Assuming that the worldMap is gone in a follow-up CL, looks good.

Yeah, merging the maps is feasible and clearer. I'll work on it in a separate
CL.

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:348: new
int(DOMWrapperWorld::WorldId::IsolatedWorldIdLimit));
On 2017/03/14 09:18:20, haraken wrote:
> 
> I think you could just use ThreadSpecific. The world ID needs to be unique per
> thread, not per process.

Done.

[haraken, 2017-03-14 09:47:06]

LGTM

[peria, 2017-03-14 09:52:46]

non OWNER lgtm.

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:135: }
On 2017/03/14 09:14:38, nhiroki (slow) wrote:
> This unexpected behavior could happen only when static_cast<> is used, right?
>
> I'd rather not add 'default' because it disables the compiler's branch
coverage
> check and would be more error-prone than the case.

Thank you for the comment. Totally agreed.

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:147: static
unsigned s_numberOfNonMainWorldsInMainThread;
optional: Can this static variable be replaced with isolatedWorldMap().size() +
worldMap().size() if its performance is acceptable and it is accessed only in
the main thread?

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:348:
*s_nextWorldId = DOMWrapperWorld::WorldId::UnspecifiedWorldIdStart;
s/DOMWrapperWorld:://

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:362:
CHECK_GT(worldId, 0);
CHECK_GE(worldId, WorldId::UnspecifiedWorldIdStart);

[nhiroki, 2017-03-14 10:53:30]

Patchset #13 (id:280001) has been deleted

[nhiroki, 2017-03-14 10:58:28]

Thank you!

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/2735823006/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h:147: static
unsigned s_numberOfNonMainWorldsInMainThread;
On 2017/03/14 09:52:46, peria wrote:
> optional: Can this static variable be replaced with isolatedWorldMap().size()
+
> worldMap().size() if its performance is acceptable and it is accessed only in
> the main thread?

It'd be feasible. I'll work on it in a follow-up CL that merges
isolatedWorldMap() into worldMap().

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:348:
*s_nextWorldId = DOMWrapperWorld::WorldId::UnspecifiedWorldIdStart;
On 2017/03/14 09:52:46, peria wrote:
> s/DOMWrapperWorld:://

Done.

https://codereview.chromium.org/2735823006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:362:
CHECK_GT(worldId, 0);
On 2017/03/14 09:52:46, peria wrote:
> CHECK_GE(worldId, WorldId::UnspecifiedWorldIdStart);

Done.

[nhiroki, 2017-03-14 10:58:49]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-03-14 10:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, haraken@chromium.org, peria@chromium.org
Link to the patchset: https://codereview.chromium.org/2735823006/#ps300001
(title: "address review comments")

[commit-bot: I haz the power, 2017-03-14 10:59:10]

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-03-14 13:22:59]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1489489129727940,
"parent_rev": "34cad83d81a686a09b2350ef31fdb1e2279bc770", "commit_rev":
"0fe6bc1c1b65086da86d225ac7dfe9b00a317a1e"}

[commit-bot: I haz the power, 2017-03-14 13:23:41]

Description was changed from

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |worldMap| to manage multiple worker-or-worklet
worlds on a single thread for the issue 1), and teaches the main thread that
there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629
==========

to

==========
Bindings: Manage multiple DOMWrapperWorlds for worklets

Before this CL, there are 2 issues:

1) DOMWrapperWorld for a worker-or-worklet is managed as per-thread singleton.
   This doesn't work when multiple worklets are created on the same thread.
2) DOMWrapperWorld assumes that a worker-or-worklet world is never created on
   the main thread, but actually it's possible for the main thread worklet
   (i.e. PaintWorklet).

This CL introduces a per-thread |worldMap| to manage multiple worker-or-worklet
worlds on a single thread for the issue 1), and teaches the main thread that
there can be worker-or-worklet worlds for the issue 2).

BUG=697622, 697629

Review-Url: https://codereview.chromium.org/2735823006
Cr-Commit-Position: refs/heads/master@{#456683}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fe6bc1c1b65086da86d225ac7df...
==========

[commit-bot: I haz the power, 2017-03-14 13:23:43]

Committed patchset #13 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/0fe6bc1c1b65086da86d225ac7df...