Worklet: Lazily create PaintWorkletGlobalScopes

Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
DesignDoc: https://docs.google.com/document/d/1cgLcrua7H_7x_o5GlzYrAi2qt-TqTzgtOeixFAugR6g/edit?usp=sharing

BUG=627945
Review-Url: https://codereview.chromium.org/2871513002
Cr-Commit-Position: refs/heads/master@{#475337}
Committed: https://chromium.googlesource.com/chromium/src/+/cb9819bdbebd43f5f2a9219dcb127a5368965dcb

[nhiroki, 2017-05-08 09:17:51]

Description was changed from

==========
Worklet: Lazily create MainThreadWorkletGlobalScopes

Before this CL, MainThreadWorkletGlobalScope is created on the ctor of
MainThreadWorklet. This behavior is not compatible with the Worklet spec.
WorkletGlobalScopes should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create WorkletGlobalScopes.

BUG=627945
==========

to

==========
Worklet: Lazily create MainThreadWorkletGlobalScopes

Before this CL, MainThreadWorkletGlobalScope is created on the ctor of
MainThreadWorklet. This behavior is not compatible with the Worklet spec.
WorkletGlobalScopes should be lazily created when addModule() is called.
To fix it, this CL changes a timing to create WorkletGlobalScopes.

In addition, this CL enables MainThreadWorklet have multiple
WorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

[nhiroki, 2017-05-08 09:19:45]

Description was changed from

==========
Worklet: Lazily create MainThreadWorkletGlobalScopes

Before this CL, MainThreadWorkletGlobalScope is created on the ctor of
MainThreadWorklet. This behavior is not compatible with the Worklet spec.
WorkletGlobalScopes should be lazily created when addModule() is called.
To fix it, this CL changes a timing to create WorkletGlobalScopes.

In addition, this CL enables MainThreadWorklet have multiple
WorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

to

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called. To fix it, this CL changes
a timing to create PaintWorkletGlobalScopes.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

[nhiroki, 2017-05-08 09:19:54]

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

[commit-bot: I haz the power, 2017-05-08 09:20:29]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-08 09:22:08]

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

[nhiroki, 2017-05-08 09:22:09]

PTAL, thanks!

[nhiroki, 2017-05-08 09:22:20]

Description was changed from

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called. To fix it, this CL changes
a timing to create PaintWorkletGlobalScopes.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

to

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called. To fix it, this CL changes
a timing to create PaintWorkletGlobalScopes.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

[commit-bot: I haz the power, 2017-05-08 11:27:17]

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

[commit-bot: I haz the power, 2017-05-08 11:27:18]

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

[kouhei (in TOK), 2017-05-08 13:13:19]

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:44: virtual void
AddWorkletGlobalScopesIfNeeded() = 0;
Can we have a virtual WorkletGlobalScopeProxy* CreateWorkletGlobalScope() = 0;
instead and have AddWorkletGlobalScopesIfNeeded + global_scope_proxies_
non-virtual?
This is based on assumption that every AddWorkletGlobalScopesIfNeeded will
actually global_scope_proxies_.insert(std::move(proxy)) at the end.

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:52: // persistent
holder for PaintWorkletGlobalscope).
+1. Can we give a crbug # to this?

[nhiroki, 2017-05-09 00:10:07]

Thanks for your review comments! I'll address them soon.

By the way, I noticed this change fails a csspaint layout test. Apparently, the
current PaintWorklet impl assumes there is a PaintWorkletGlobalScope before
addModule(). I'll investigate whether this is a spec issue or impl issue...

[nhiroki, 2017-05-09 02:02:15]

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

[commit-bot: I haz the power, 2017-05-09 02:04:57]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-09 02:25:58]

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

[commit-bot: I haz the power, 2017-05-09 02:27:05]

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-09 04:00:25]

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

[commit-bot: I haz the power, 2017-05-09 04:00:26]

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

[nhiroki, 2017-05-10 02:30:35]

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

[commit-bot: I haz the power, 2017-05-10 02:32:35]

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-10 02:39:18]

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

[commit-bot: I haz the power, 2017-05-10 02:39:19]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[nhiroki, 2017-05-10 03:19:51]

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

[commit-bot: I haz the power, 2017-05-10 03:20:55]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-10 03:36:08]

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

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

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-10 03:39:43]

Description was changed from

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called. To fix it, this CL changes
a timing to create PaintWorkletGlobalScopes.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

to

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

[ikilpatrick, 2017-05-10 03:47:43]

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

[ikilpatrick, 2017-05-10 03:47:44]

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:29: //
TODO(nhiroki): Support the case where there are multiple global scopes.
so what we'll want to do for paint is basically switch global scopes every N
frames for the moment. (N ~= say 60). It may make sense to remove FindDefinition
here? Thoughts?

[commit-bot: I haz the power, 2017-05-10 04:01:22]

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

[commit-bot: I haz the power, 2017-05-10 04:01:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nhiroki, 2017-05-10 09:01:32]

Thank you! Updated.

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:44: virtual void
AddWorkletGlobalScopesIfNeeded() = 0;
On 2017/05/08 13:13:19, kouhei (on transit) wrote:
> Can we have a virtual WorkletGlobalScopeProxy* CreateWorkletGlobalScope() = 0;
> instead and have AddWorkletGlobalScopesIfNeeded + global_scope_proxies_
> non-virtual?
> This is based on assumption that every AddWorkletGlobalScopesIfNeeded will
> actually global_scope_proxies_.insert(std::move(proxy)) at the end.

Replaced this with NeedsToCreateGlobalScope() and CreateGlobalScope().

https://codereview.chromium.org/2871513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:52: // persistent
holder for PaintWorkletGlobalscope).
On 2017/05/08 13:13:19, kouhei (on transit) wrote:
> +1. Can we give a crbug # to this?

Added a crbug link.

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:29: //
TODO(nhiroki): Support the case where there are multiple global scopes.
On 2017/05/10 03:47:44, ikilpatrick wrote:
> so what we'll want to do for paint is basically switch global scopes every N
> frames for the moment. (N ~= say 60). It may make sense to remove
FindDefinition
> here? Thoughts?

I'm a bit confused. Does it mean to remove PaintWorklet::FindDefinition() call
in CSSPaintImageGeneratorImpl::Create() and always add a newly created generator
into the pending registry? If so, how/when do we associate the pending generator
with an existing CSSPaintDefinition?

Or, does it mean to remove a CSSPaintDefinition returned from FindDefinition()
from WorkletGlobalScope's DefinitionMap?

[nhiroki, 2017-05-10 09:15:29]

On 2017/05/09 00:10:07, nhiroki wrote:
> Thanks for your review comments! I'll address them soon.
> 
> By the way, I noticed this change fails a csspaint layout test. Apparently,
the
> current PaintWorklet impl assumes there is a PaintWorkletGlobalScope before
> addModule(). I'll investigate whether this is a spec issue or impl issue...

This would be an implementation issue. I introduced
PaintWorkletPendingGeneratorRegistry to keep pending CSSPaintImageGeneratorImpls
until the first addModule() call. First, the registry is owned by PaintWorklet,
and then its ownership is transferred to PaintWorkletGlobalScope once it's
created.

[nhiroki, 2017-05-10 14:34:45]

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

[commit-bot: I haz the power, 2017-05-10 14:35:08]

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-10 16:41:50]

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

[commit-bot: I haz the power, 2017-05-10 16:41: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_...)

[nhiroki, 2017-05-12 09:38:41]

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

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

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-12 09:51:26]

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

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

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

[nhiroki, 2017-05-12 12:29:19]

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

[commit-bot: I haz the power, 2017-05-12 12:29:36]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-12 12:38:08]

Description was changed from

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

BUG=627945
==========

to

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
DesignDoc:
https://docs.google.com/document/d/1cgLcrua7H_7x_o5GlzYrAi2qt-TqTzgtOeixFAugR...

BUG=627945
==========

[commit-bot: I haz the power, 2017-05-12 14:29:57]

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

[commit-bot: I haz the power, 2017-05-12 14:29:58]

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

[kouhei (in TOK), 2017-05-12 16:37:10]

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:39:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>>& GetProxies() {
I'd want to avoid this as much as possible.

Please create a collection wrapper class which has_a
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> and expose that.

[falken, 2017-05-15 05:14:49]

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:39:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>>& GetProxies() {
On 2017/05/12 16:37:10, kouhei (travelling MTV) wrote:
> I'd want to avoid this as much as possible.
> 
> Please create a collection wrapper class which has_a
> WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> and expose that.

For my edification, what is "this" that you want to avoid?

[nhiroki, 2017-05-22 00:52:33]

Thank you for your comments. I'm now back from OOO.

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:39:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>>& GetProxies() {
On 2017/05/15 05:14:48, falken wrote:
> On 2017/05/12 16:37:10, kouhei (travelling MTV) wrote:
> > I'd want to avoid this as much as possible.
> > 
> > Please create a collection wrapper class which has_a
> > WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> and expose that.
> 
> For my edification, what is "this" that you want to avoid?

I guess he'd like to avoid exposing a reference to an internal collection
(kouhei@, is this correct?)

By the way, I think such a wrapper class still needs to expose a reference or an
iterator to pick up an appropriate global scope in PaintWorklet::FindDefinition
etc. These proxies are accessed only from sub-classes of MainThreadWorklet or
tests, so making the proxies "protected" and removing GetProxies() would be a
simpler solution in this case. What do you think?

[nhiroki, 2017-05-22 00:53:34]

Patchset #10 (id:180001) has been deleted

[kinuko, 2017-05-22 02:51:04]

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

[kinuko, 2017-05-22 02:51:04]

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:39:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>>& GetProxies() {
On 2017/05/22 00:52:33, nhiroki (slow) wrote:
> On 2017/05/15 05:14:48, falken wrote:
> > On 2017/05/12 16:37:10, kouhei (travelling MTV) wrote:
> > > I'd want to avoid this as much as possible.
> > > 
> > > Please create a collection wrapper class which has_a
> > > WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> and expose that.
> > 
> > For my edification, what is "this" that you want to avoid?
> 
> I guess he'd like to avoid exposing a reference to an internal collection
> (kouhei@, is this correct?)
> 
> By the way, I think such a wrapper class still needs to expose a reference or
an
> iterator to pick up an appropriate global scope in
PaintWorklet::FindDefinition
> etc. These proxies are accessed only from sub-classes of MainThreadWorklet or
> tests, so making the proxies "protected" and removing GetProxies() would be a
> simpler solution in this case. What do you think?

Fwiw I think it's still possible to expose this kind of things with a bit
stricter, more explicit interface, e.g. const method for size and mutable
methods to insert new one / to return one proxy (if the required behavior here
is to return one of the two in alternative way intuitively it seems such logic
could be implemented in the wrapper to hide details?)

(Sounds also ok to expose this only to subclasses, but felt that it'd be nice to
clarify what interactions we want to have with that... my 2 cents)

[ikilpatrick, 2017-05-24 03:47:07]

lgtm

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:29: //
TODO(nhiroki): Support the case where there are multiple global scopes.
On 2017/05/10 09:01:32, nhiroki (slow) wrote:
> On 2017/05/10 03:47:44, ikilpatrick wrote:
> > so what we'll want to do for paint is basically switch global scopes every N
> > frames for the moment. (N ~= say 60). It may make sense to remove
> FindDefinition
> > here? Thoughts?
> 
> I'm a bit confused. Does it mean to remove PaintWorklet::FindDefinition() call
> in CSSPaintImageGeneratorImpl::Create() and always add a newly created
generator
> into the pending registry? If so, how/when do we associate the pending
generator
> with an existing CSSPaintDefinition?
> 
> Or, does it mean to remove a CSSPaintDefinition returned from FindDefinition()
> from WorkletGlobalScope's DefinitionMap?

Ah sorry, we'll have to work on this a bit more, the CSS Paint API actually has
two types of registeries/definitions to deal with this, a:
DocumentPaintRegistry
PaintWorkletGlobalScopeRegistry

The document one is used for the pending generator case, i.e. the document wants
to see what input properties it should invalidate on etc.

The worklet GS one is used for the invoking paint, etc upon.

We can refactor this later.

[nhiroki, 2017-05-24 05:06:13]

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

[commit-bot: I haz the power, 2017-05-24 05:07:07]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-24 05:12:22]

Thank you for your comments! Updated.

kouhei@, kinuko@, falken@: Can you take another look?

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:29: //
TODO(nhiroki): Support the case where there are multiple global scopes.
On 2017/05/24 03:47:07, ikilpatrick wrote:
> On 2017/05/10 09:01:32, nhiroki (slow) wrote:
> > On 2017/05/10 03:47:44, ikilpatrick wrote:
> > > so what we'll want to do for paint is basically switch global scopes every
N
> > > frames for the moment. (N ~= say 60). It may make sense to remove
> > FindDefinition
> > > here? Thoughts?
> > 
> > I'm a bit confused. Does it mean to remove PaintWorklet::FindDefinition()
call
> > in CSSPaintImageGeneratorImpl::Create() and always add a newly created
> generator
> > into the pending registry? If so, how/when do we associate the pending
> generator
> > with an existing CSSPaintDefinition?
> > 
> > Or, does it mean to remove a CSSPaintDefinition returned from
FindDefinition()
> > from WorkletGlobalScope's DefinitionMap?
> 
> Ah sorry, we'll have to work on this a bit more, the CSS Paint API actually
has
> two types of registeries/definitions to deal with this, a:
> DocumentPaintRegistry
> PaintWorkletGlobalScopeRegistry
> 
> The document one is used for the pending generator case, i.e. the document
wants
> to see what input properties it should invalidate on etc.
> 
> The worklet GS one is used for the invoking paint, etc upon.
> 
> We can refactor this later.

Acknowledged. Thank you for the clarification.

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:39:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>>& GetProxies() {
On 2017/05/22 02:51:04, kinuko wrote:
> On 2017/05/22 00:52:33, nhiroki (slow) wrote:
> > On 2017/05/15 05:14:48, falken wrote:
> > > On 2017/05/12 16:37:10, kouhei (travelling MTV) wrote:
> > > > I'd want to avoid this as much as possible.
> > > > 
> > > > Please create a collection wrapper class which has_a
> > > > WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> and expose that.
> > > 
> > > For my edification, what is "this" that you want to avoid?
> > 
> > I guess he'd like to avoid exposing a reference to an internal collection
> > (kouhei@, is this correct?)
> > 
> > By the way, I think such a wrapper class still needs to expose a reference
or
> an
> > iterator to pick up an appropriate global scope in
> PaintWorklet::FindDefinition
> > etc. These proxies are accessed only from sub-classes of MainThreadWorklet
or
> > tests, so making the proxies "protected" and removing GetProxies() would be
a
> > simpler solution in this case. What do you think?
> 
> Fwiw I think it's still possible to expose this kind of things with a bit
> stricter, more explicit interface, e.g. const method for size and mutable
> methods to insert new one / to return one proxy (if the required behavior here
> is to return one of the two in alternative way intuitively it seems such logic
> could be implemented in the wrapper to hide details?)

Fair enough.

> (Sounds also ok to expose this only to subclasses, but felt that it'd be nice
to
> clarify what interactions we want to have with that... my 2 cents)

This is the list of known interactions for now:

- Get the number of global scopes (proxies)
- Insert a new global scope.
- Request all global scopes to fetch and evaluate module scripts.
- Request all global scopes to terminate themselves.
- Pick up one of global scopes to run a task.

Based on them, I introduced WorkletGlobalScopeManager in the patchset 10. How
does it look to you?

[kinuko, 2017-05-24 06:53:40]

Thanks!  A little more bike shedding comments...

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h (right):

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h:29: void
AddGlobalScope(std::unique_ptr<WorkletGlobalScopeProxy>);
nit: it looks we can remove 'GlobalScope' part from the method names here and
below

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h:42: size_t
GetNumberOfGlobalScopes() const { return proxies_.size(); }
It looks we can also simply let MainThreadWorklet hold the HashSet proxies_, but
only expose 'GetNumberOfGlobalScopes()' and 'FindAvailableGlobalScope()' methods
to subclasses as protected, without introducing this wrapper class?  (To be
clear, I did want to clarify the interface to proxies, while was neutral on
introducing a wrapper class)

(Either works for me, but in the current code having both 'CreateGlobalScope'
and 'AddGlobalScope' exposed to subclasses feels a bit less tight to me.  With
the anyone can add whatever global scope to the proxies without implementing
CreateGlobalScope.  If we'll have a plan to further extend this class / add more
code in this class I'm fine with having this wrapper class)

[commit-bot: I haz the power, 2017-05-24 07:31:42]

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

[commit-bot: I haz the power, 2017-05-24 07:31:43]

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

[nhiroki, 2017-05-24 07:59:25]

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

[commit-bot: I haz the power, 2017-05-24 07:59:41]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-24 08:10:32]

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

[commit-bot: I haz the power, 2017-05-24 08:10:58]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-24 08:11:23]

Thank you! Updated.

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h (right):

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h:29: void
AddGlobalScope(std::unique_ptr<WorkletGlobalScopeProxy>);
On 2017/05/24 06:53:40, kinuko wrote:
> nit: it looks we can remove 'GlobalScope' part from the method names here and
> below

The latest patchset removed this and other functions in this class.

https://codereview.chromium.org/2871513002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkletGlobalScopeManager.h:42: size_t
GetNumberOfGlobalScopes() const { return proxies_.size(); }
On 2017/05/24 06:53:39, kinuko wrote:
> It looks we can also simply let MainThreadWorklet hold the HashSet proxies_,
but
> only expose 'GetNumberOfGlobalScopes()' and 'FindAvailableGlobalScope()'
methods
> to subclasses as protected, without introducing this wrapper class?  (To be
> clear, I did want to clarify the interface to proxies, while was neutral on
> introducing a wrapper class)

Done.

> (Either works for me, but in the current code having both 'CreateGlobalScope'
> and 'AddGlobalScope' exposed to subclasses feels a bit less tight to me.  With
> the anyone can add whatever global scope to the proxies without implementing
> CreateGlobalScope.  If we'll have a plan to further extend this class / add
more
> code in this class I'm fine with having this wrapper class)

I don't have a strong opinion whether to introduce the wrapper class and also
don't have an extension plan of that. The latest patchset re-merges the wrapper
class into MainThreadWorklet and reduces # of public/protected functions around
the proxy collection.

[kinuko, 2017-05-24 08:21:33]

Thanks! Works for me (lgtm)

[commit-bot: I haz the power, 2017-05-24 12:16:41]

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

[commit-bot: I haz the power, 2017-05-24 12:16:42]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-05-24 14:31:13]

lgtm

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:64: if
(GetNumberOfGlobalScopes() == 0) {
DCHECK(pending_generator_registry_)

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:68: }
else DCHECK(!pending_generator_registry_)

[nhiroki, 2017-05-24 14:42:00]

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

[nhiroki, 2017-05-24 14:42:09]

Thank you!

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:64: if
(GetNumberOfGlobalScopes() == 0) {
On 2017/05/24 14:31:13, kouhei (back in TOK) wrote:
> DCHECK(pending_generator_registry_)

Done.

https://codereview.chromium.org/2871513002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:68: }
On 2017/05/24 14:31:13, kouhei (back in TOK) wrote:
> else DCHECK(!pending_generator_registry_)

Done (I omitted "else")

[commit-bot: I haz the power, 2017-05-24 14:42: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-05-24 16:38:34]

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

[commit-bot: I haz the power, 2017-05-24 16:38:35]

Dry run: This issue passed the CQ dry run.

[falken, 2017-05-25 00:30:58]

Sorry for the delay, I defer to others here. Let me know if there's something
specific you wanted me to look at.

[haraken, 2017-05-25 01:04:38]

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

[haraken, 2017-05-25 01:04:39]

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);

I think this is the point of this CL but would you help me understand why we
need to transfer the ownership to the global scope? Why can't we simply keep the
registry in PaintWorklet?

[nhiroki, 2017-05-25 06:23:23]

Thank you!

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 01:04:39, haraken wrote:
> 
> I think this is the point of this CL but would you help me understand why we
> need to transfer the ownership to the global scope? Why can't we simply keep
the
> registry in PaintWorklet?
> 

PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not implemented
yet though). IIUC, a pending generator is a per-global-scope thing and should be
exclusively owned by one of the global scopes, so transferring the ownership
seems like a natural way here.

[haraken, 2017-05-25 06:28:54]

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 06:23:23, nhiroki wrote:
> On 2017/05/25 01:04:39, haraken wrote:
> > 
> > I think this is the point of this CL but would you help me understand why we
> > need to transfer the ownership to the global scope? Why can't we simply keep
> the
> > registry in PaintWorklet?
> > 
> 
> PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not implemented
> yet though). IIUC, a pending generator is a per-global-scope thing and should
be
> exclusively owned by one of the global scopes, so transferring the ownership
> seems like a natural way here.

Does it mean that the pending generators can go to any arbitrary global scope?

Or your intention is to move all the pending generators to the first global
scope?

[nhiroki, 2017-05-25 07:24:08]

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 06:28:53, haraken wrote:
> On 2017/05/25 06:23:23, nhiroki wrote:
> > On 2017/05/25 01:04:39, haraken wrote:
> > > 
> > > I think this is the point of this CL but would you help me understand why
we
> > > need to transfer the ownership to the global scope? Why can't we simply
keep
> > the
> > > registry in PaintWorklet?
> > > 
> > 
> > PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not
implemented
> > yet though). IIUC, a pending generator is a per-global-scope thing and
should
> be
> > exclusively owned by one of the global scopes, so transferring the ownership
> > seems like a natural way here.
> 
> Does it mean that the pending generators can go to any arbitrary global scope?
> 
> Or your intention is to move all the pending generators to the first global
> scope?

Hmmm..., I'm losing the confidence of this comment: "IIUC, a pending generator
is a per-global-scope thing and should be exclusively owned by one of the global
scopes,"

ikilpatrick@, should be pending generators sent to (shared among) all global
scopes? I'm now having a feeling that should be so because switching global
scopes (every 60 frames as you commented) seems to require they are also
identical in terms of the generators. Am I missing something?

haraken@, sorry for confusing you and thank you for pointing it out. I'm also
now learning design/implementation of PaintAPI :p

[nhiroki, 2017-05-25 07:37:45]

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 07:24:08, nhiroki wrote:
> On 2017/05/25 06:28:53, haraken wrote:
> > On 2017/05/25 06:23:23, nhiroki wrote:
> > > On 2017/05/25 01:04:39, haraken wrote:
> > > > 
> > > > I think this is the point of this CL but would you help me understand
why
> we
> > > > need to transfer the ownership to the global scope? Why can't we simply
> keep
> > > the
> > > > registry in PaintWorklet?
> > > > 
> > > 
> > > PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not
> implemented
> > > yet though). IIUC, a pending generator is a per-global-scope thing and
> should
> > be
> > > exclusively owned by one of the global scopes, so transferring the
ownership
> > > seems like a natural way here.
> > 
> > Does it mean that the pending generators can go to any arbitrary global
scope?
> > 
> > Or your intention is to move all the pending generators to the first global
> > scope?
> 
> Hmmm..., I'm losing the confidence of this comment: "IIUC, a pending generator
> is a per-global-scope thing and should be exclusively owned by one of the
global
> scopes,"
> 
> ikilpatrick@, should be pending generators sent to (shared among) all global
> scopes? I'm now having a feeling that should be so because switching global
> scopes (every 60 frames as you commented) seems to require they are also
> identical in terms of the generators. Am I missing something?

If this is correct, we may need to keep pending generators in PaintWorklet as
haraken@ pointed out, and associate them with definitions every time a global
scope is newly created.

> haraken@, sorry for confusing you and thank you for pointing it out. I'm also
> now learning design/implementation of PaintAPI :p

[ikilpatrick, 2017-05-25 21:58:53]

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 07:37:44, nhiroki wrote:
> On 2017/05/25 07:24:08, nhiroki wrote:
> > On 2017/05/25 06:28:53, haraken wrote:
> > > On 2017/05/25 06:23:23, nhiroki wrote:
> > > > On 2017/05/25 01:04:39, haraken wrote:
> > > > > 
> > > > > I think this is the point of this CL but would you help me understand
> why
> > we
> > > > > need to transfer the ownership to the global scope? Why can't we
simply
> > keep
> > > > the
> > > > > registry in PaintWorklet?
> > > > > 
> > > > 
> > > > PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not
> > implemented
> > > > yet though). IIUC, a pending generator is a per-global-scope thing and
> > should
> > > be
> > > > exclusively owned by one of the global scopes, so transferring the
> ownership
> > > > seems like a natural way here.
> > > 
> > > Does it mean that the pending generators can go to any arbitrary global
> scope?
> > > 
> > > Or your intention is to move all the pending generators to the first
global
> > > scope?
> > 
> > Hmmm..., I'm losing the confidence of this comment: "IIUC, a pending
generator
> > is a per-global-scope thing and should be exclusively owned by one of the
> global
> > scopes,"
> > 
> > ikilpatrick@, should be pending generators sent to (shared among) all global
> > scopes? I'm now having a feeling that should be so because switching global
> > scopes (every 60 frames as you commented) seems to require they are also
> > identical in terms of the generators. Am I missing something?
> 
> If this is correct, we may need to keep pending generators in PaintWorklet as
> haraken@ pointed out, and associate them with definitions every time a global
> scope is newly created.
> 
> > haraken@, sorry for confusing you and thank you for pointing it out. I'm
also
> > now learning design/implementation of PaintAPI :p
> 

Yeah we need to re-work this a little. We'll need to implement the following to
make this correct:

PaintWorklet
  Add document_paint_definitions_ member
(https://drafts.css-houdini.org/css-paint-api/#document-paint-definition)

PaintWorkletGlobalScope
  Add paint_definitions_ member
(https://drafts.css-houdini.org/css-paint-api/#paint-definition)

The PaintWorklet also needs to keep track of the pending_generator_registery_
which is being discussed here.

The way that it works, is when a new PaintWorkletGlobalScope is created it adds
the paint_definition to its own set, and then tries to add it to the
PaintWorklet (document paint definition) one. If they differ (e.g. different
input arguments), it gets "poisoned" and it can't be used anymore.

But yeah, this now belongs on the PaintWorklet.

[nhiroki, 2017-05-29 05:50:48]

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

[commit-bot: I haz the power, 2017-05-29 05:50:55]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-29 05:54:39]

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

[commit-bot: I haz the power, 2017-05-29 05:54:46]

Dry run: CQ is trying da patch.

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

[nhiroki, 2017-05-29 05:57:31]

haraken@: Can you take another look? I made PaintWorklet keep the pending
generator registry based on ikilpatrick@'s comment. Thanks!

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:67:
pending_generator_registry_);
On 2017/05/25 21:58:52, ikilpatrick wrote:
> On 2017/05/25 07:37:44, nhiroki wrote:
> > On 2017/05/25 07:24:08, nhiroki wrote:
> > > On 2017/05/25 06:28:53, haraken wrote:
> > > > On 2017/05/25 06:23:23, nhiroki wrote:
> > > > > On 2017/05/25 01:04:39, haraken wrote:
> > > > > > 
> > > > > > I think this is the point of this CL but would you help me
understand
> > why
> > > we
> > > > > > need to transfer the ownership to the global scope? Why can't we
> simply
> > > keep
> > > > > the
> > > > > > registry in PaintWorklet?
> > > > > > 
> > > > > 
> > > > > PaintWorklet can have multiple PaintWorkletGlobalScopes (it's not
> > > implemented
> > > > > yet though). IIUC, a pending generator is a per-global-scope thing and
> > > should
> > > > be
> > > > > exclusively owned by one of the global scopes, so transferring the
> > ownership
> > > > > seems like a natural way here.
> > > > 
> > > > Does it mean that the pending generators can go to any arbitrary global
> > scope?
> > > > 
> > > > Or your intention is to move all the pending generators to the first
> global
> > > > scope?
> > > 
> > > Hmmm..., I'm losing the confidence of this comment: "IIUC, a pending
> generator
> > > is a per-global-scope thing and should be exclusively owned by one of the
> > global
> > > scopes,"
> > > 
> > > ikilpatrick@, should be pending generators sent to (shared among) all
global
> > > scopes? I'm now having a feeling that should be so because switching
global
> > > scopes (every 60 frames as you commented) seems to require they are also
> > > identical in terms of the generators. Am I missing something?
> > 
> > If this is correct, we may need to keep pending generators in PaintWorklet
as
> > haraken@ pointed out, and associate them with definitions every time a
global
> > scope is newly created.
> > 
> > > haraken@, sorry for confusing you and thank you for pointing it out. I'm
> also
> > > now learning design/implementation of PaintAPI :p
> > 
> 
> Yeah we need to re-work this a little. We'll need to implement the following
to
> make this correct:
> 
> PaintWorklet
>   Add document_paint_definitions_ member
> (https://drafts.css-houdini.org/css-paint-api/#document-paint-definition)
> 
> PaintWorkletGlobalScope
>   Add paint_definitions_ member
> (https://drafts.css-houdini.org/css-paint-api/#paint-definition)
> 
> The PaintWorklet also needs to keep track of the pending_generator_registery_
> which is being discussed here.
> 
> The way that it works, is when a new PaintWorkletGlobalScope is created it
adds
> the paint_definition to its own set, and then tries to add it to the
> PaintWorklet (document paint definition) one. If they differ (e.g. different
> input arguments), it gets "poisoned" and it can't be used anymore.
> 
> But yeah, this now belongs on the PaintWorklet.

Thank you for the clarification. I made PaintWorklet keep the pending generator
registry, and added some TODO/implementation comments about the defintion map.

haraken@, can you take another look?

[commit-bot: I haz the power, 2017-05-29 07:37:00]

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

[commit-bot: I haz the power, 2017-05-29 07:37:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-29 07:38:48]

LGTM

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

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:79: DCHECK_LT(0u,
GetNumberOfGlobalScopes());

DCHECK_EQ(1, GetNumberOfGlobalScopes()) at the moment?

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:62:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> proxies_;

WTF:: is not needed.

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:45: // "The user
agent must have, and select from at least two

I'm just curious but why at least two? (This is a question to the spec though.)

[nhiroki, 2017-05-29 08:52:03]

Thank you!

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

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:79: DCHECK_LT(0u,
GetNumberOfGlobalScopes());
On 2017/05/29 07:38:48, haraken wrote:
> 
> DCHECK_EQ(1, GetNumberOfGlobalScopes()) at the moment?

Done.

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:62:
WTF::HashSet<std::unique_ptr<WorkletGlobalScopeProxy>> proxies_;
On 2017/05/29 07:38:48, haraken wrote:
> 
> WTF:: is not needed.

Done.

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp (right):

https://codereview.chromium.org/2871513002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp:45: // "The user
agent must have, and select from at least two
On 2017/05/29 07:38:48, haraken wrote:
> 
> I'm just curious but why at least two? (This is a question to the spec
though.)

Spec issue: https://github.com/w3c/css-houdini-drafts/issues/327
Relevant spec texts: https://drafts.css-houdini.org/worklets/#code-idempotency

Worklet can parallelize tasks over multiple threads or move tasks between
threads as required. Therefore, web application developers must not depend on
the global state. By making 2 global scopes by default, we could prevent such
the abuse.

[nhiroki, 2017-05-29 08:53:15]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-05-29 08:53:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org, kouhei@chromium.org, kinuko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2871513002/#ps380001
(title: "address review comments")

[commit-bot: I haz the power, 2017-05-29 08:53:33]

CQ is trying da patch.

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

[nhiroki, 2017-05-29 09:39:34]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2017-05-29 09:40:15]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-05-29 09:40:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org, haraken@chromium.org, kouhei@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2871513002/#ps400001
(title: "fix wrong dcheck")

[commit-bot: I haz the power, 2017-05-29 09:40:28]

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-29 11:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1496050815031050,
"parent_rev": "f76c29b63b40b0c45132a37ac8e296cc2973b322", "commit_rev":
"cb9819bdbebd43f5f2a9219dcb127a5368965dcb"}

[commit-bot: I haz the power, 2017-05-29 11:51:45]

Description was changed from

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
DesignDoc:
https://docs.google.com/document/d/1cgLcrua7H_7x_o5GlzYrAi2qt-TqTzgtOeixFAugR...

BUG=627945
==========

to

==========
Worklet: Lazily create PaintWorkletGlobalScopes

Before this CL, PaintWorkletGlobalScope is created on the ctor of PaintWorklet.
This behavior is not compatible with the Worklet spec. PaintWorkletGlobalScopes
should be lazily created when addModule() is called.

To fix it, this CL changes a timing to create PaintWorkletGlobalScopes and
introduces PaintWorkletPendingGeneratorRegistry to keep pending generators until
the first global scope is created.

In addition, this CL enables PaintWorklet have multiple
PaintWorkletGlobalScopes. This is also required by the spec.

Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
DesignDoc:
https://docs.google.com/document/d/1cgLcrua7H_7x_o5GlzYrAi2qt-TqTzgtOeixFAugR...

BUG=627945

Review-Url: https://codereview.chromium.org/2871513002
Cr-Commit-Position: refs/heads/master@{#475337}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb9819bdbebd43f5f2a9219dcb12...
==========

[commit-bot: I haz the power, 2017-05-29 11:51:47]

Committed patchset #20 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/cb9819bdbebd43f5f2a9219dcb12...