Worklet: Introduce PaintWorkletGlobalScopeProxy

Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet is tightly coupled with PaintWorkletGlobalScope,
and that is an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

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

[nhiroki, 2017-05-01 03:58:02]

Patchset #1 (id:1) has been deleted

[nhiroki, 2017-05-01 03:58:17]

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

[commit-bot: I haz the power, 2017-05-01 03:58:27]

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-01 04:03:38]

Description was changed from

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

This CL introduces PaintWorkletGlobalScopeProxy to abstract communication
between PaintWorklet and PaintWorkletGlobalScope.

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

to

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

This CL introduces PaintWorkletGlobalScopeProxy to abstract communication
between PaintWorklet and PaintWorkletGlobalScope. This abstraction will be
useful when we unify MainThreadWorklet and ThreadedWorklet.

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

[nhiroki, 2017-05-01 04:11:25]

Description was changed from

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

This CL introduces PaintWorkletGlobalScopeProxy to abstract communication
between PaintWorklet and PaintWorkletGlobalScope. This abstraction will be
useful when we unify MainThreadWorklet and ThreadedWorklet.

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

to

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet was tightly coupled with PaintWorkletGlobalScope,
and that was an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

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

[nhiroki, 2017-05-01 04:16:19]

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

[nhiroki, 2017-05-01 04:16:24]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-01 04:16: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-05-01 04:17:03]

Description was changed from

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet was tightly coupled with PaintWorkletGlobalScope,
and that was an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

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

to

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet is tightly coupled with PaintWorkletGlobalScope,
and that is an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

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

[nhiroki, 2017-05-01 04:17:51]

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

[nhiroki, 2017-05-01 04:17:52]

PTAL, thanks!

[kouhei (in TOK), 2017-05-01 04:48:37]

lgtm
As discussed offline, please put TODO(nhiroki) comments about oilpan gc graph
caveats.

[falken, 2017-05-01 04:52:09]

I'm having trouble understanding the point of these Proxy classes. Why don't the
callers just use the GlobalScope directly?

Is the point to enable other threads to communicate with the GlobalScope, which
always lives in the thread the worklet is on?

If so, could the WorkletGlobalScopeProxy class comment be more clear that the
point is for any thread to communicate with the GlobalScope?

But then why does PaintWorkletGLobalScopeProxy do DCHECKs that it's being called
on the MainThread?

[nhiroki, 2017-05-01 05:39:31]

Thank you. I haven't updated the patch yet.

On 2017/05/01 04:52:09, falken (away May 3-5) wrote:
> I'm having trouble understanding the point of these Proxy classes. Why don't
the
> callers just use the GlobalScope directly?

My motivation is to abstract communication from (Main/Threaded)Worklet to
(Main/Threaded)WorkletGlobalScope as WorkletGlobalScopeProxy so that Worklet
class doesn't have to take care of the thread WorkletGlobalScope runs on and we
can easily consolidate (Main/Threaded)Worklet into Worklet in following CLs.

Before this CL, PaintWorkletGlobalScope implements WorkletGlobalScopeProxy and
PaintWorklet directly uses it, but this makes it difficult to manage the proxies
in the base Worklet class in a universal way, for example,
HashSet<WorkletGlobalScopeProxy>. This is because PaintWorkletGlobalScope is
GC'ed object, but other implementation of the proxy for threaded worklets are
not GC'ed object.

> Is the point to enable other threads to communicate with the GlobalScope,
which
> always lives in the thread the worklet is on?
> 
> If so, could the WorkletGlobalScopeProxy class comment be more clear that the
> point is for any thread to communicate with the GlobalScope?
>
> But then why does PaintWorkletGLobalScopeProxy do DCHECKs that it's being
called
> on the MainThread?

No. Only the main thread can communicate with the global scope via
WorkletGlobalScope. That's the reasons why PaintWorkletGlobalScopeProxy do the
DCHECKs.

[falken, 2017-05-01 06:02:39]

OK, I think I see. Should WorkletGlobalScopeProxy have a comment like:
"abstracts communication from (Main/Threaded)Worklet to
(Main/Threaded)WorkletGlobalScope so that Worklet class doesn't have to care
about the thread WorkletGlobalScope runs on"?

lgtm with some more class-level comments

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.h (right):

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.h:17: 
// Manages a paint worklet:
https://drafts.css-houdini.org/css-paint-api/#paintworkletglobalscope?

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
(right):

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h:17: 
// A proxy for Worklet to talk to PaintWorkletGlobalScope?

[commit-bot: I haz the power, 2017-05-01 06:12:45]

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

[commit-bot: I haz the power, 2017-05-01 06:12:47]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-05-01 07:30:07]

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

[commit-bot: I haz the power, 2017-05-01 07:30:18]

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-01 07:30:33]

Thank you!

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorklet.h (right):

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorklet.h:17: 
On 2017/05/01 06:02:38, falken (away May 3-5) wrote:
> // Manages a paint worklet:
> https://drafts.css-houdini.org/css-paint-api/#paintworkletglobalscope?

Done.

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
(right):

https://codereview.chromium.org/2853743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h:17: 
On 2017/05/01 06:02:38, falken (away May 3-5) wrote:
> // A proxy for Worklet to talk to PaintWorkletGlobalScope?

Done.

[nhiroki, 2017-05-01 07:31:19]

On 2017/05/01 04:48:37, kouhei wrote:
> lgtm
> As discussed offline, please put TODO(nhiroki) comments about oilpan gc graph
> caveats.

Done

On 2017/05/01 06:02:39, falken (away May 3-5) wrote:
> OK, I think I see. Should WorkletGlobalScopeProxy have a comment like:
> "abstracts communication from (Main/Threaded)Worklet to
> (Main/Threaded)WorkletGlobalScope so that Worklet class doesn't have to care
> about the thread WorkletGlobalScope runs on"?

Done.

[nhiroki, 2017-05-01 07:32:27]

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

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

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-01 07:33:17]

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

[nhiroki, 2017-05-01 07:33:18]

+haraken@, can you review changes in modules/csspaint/? Thanks.

[haraken, 2017-05-01 08:43:47]

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp
(right):

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:18:
return WTF::WrapUnique(new PaintWorkletGlobalScopeProxy(frame));

MakeUnique

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:29:
DCHECK(WTF::IsMainThread());

WTF:: wouldn't be needed.

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:30:
global_scope_->FetchAndInvokeScript(module_url_record, pending_tasks);

Just to confirm: You can simply redirect the method to global_scope_ because
PaintWorklet is guaranteed to run on the main thread, right?

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
(right):

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h:41:
Persistent<PaintWorkletGlobalScope> global_scope_;

Are you pretty sure that this persistent handle doesn't create a reference
cycle?

  PaintWorklet => PaintWorkletGlobalScopeProxy => PaintWorkletGlobalScope => ...
=> PaintWorklet ?

[kouhei (in TOK), 2017-05-01 09:09:01]

On 2017/05/01 04:52:09, falken (away May 3-5) wrote:
> I'm having trouble understanding the point of these Proxy classes. Why don't
the
> callers just use the GlobalScope directly?

I have to admit the CL was hard to review w/o offline design intro.
After all I was convinced w/ the design, but I'd really want to see design doc
for this in near future. Just a diagram/slide is enough to start with :)

[commit-bot: I haz the power, 2017-05-01 09:24:22]

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

[commit-bot: I haz the power, 2017-05-01 09:24:23]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-05-02 04:34:53]

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

[commit-bot: I haz the power, 2017-05-02 04:35:11]

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-02 04:45:24]

Thank you for your comments. Updated.

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp
(right):

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:18:
return WTF::WrapUnique(new PaintWorkletGlobalScopeProxy(frame));
On 2017/05/01 08:43:46, haraken wrote:
> 
> MakeUnique

To change this to MakeUnique, we need to make the ctor public. It'd be odd to
make both Create() and the ctor public, so I removed Create() and expose the
ctor instead.

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:29:
DCHECK(WTF::IsMainThread());
On 2017/05/01 08:43:46, haraken wrote:
> 
> WTF:: wouldn't be needed.

Done.

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp:30:
global_scope_->FetchAndInvokeScript(module_url_record, pending_tasks);
On 2017/05/01 08:43:47, haraken wrote:
> 
> Just to confirm: You can simply redirect the method to global_scope_ because
> PaintWorklet is guaranteed to run on the main thread, right?

Yes, that's right.

For other threaded worklets, thread hop (main thread -> worker thread) is done
in this WorkletGlobalScopeProxy layer.

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
(right):

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h:41:
Persistent<PaintWorkletGlobalScope> global_scope_;
On 2017/05/01 08:43:47, haraken wrote:
> 
> Are you pretty sure that this persistent handle doesn't create a reference
> cycle?
> 
>   PaintWorklet => PaintWorkletGlobalScopeProxy => PaintWorkletGlobalScope =>
...
> => PaintWorklet ?

Hmmm... I'm not 100% sure that this never cause the reference cycle.

To make sure to avoid it, I made TerminateWorkletGlobalScope() nullify the
handle. It's called during Document's ContextDestroyed(). Does this make sense?

[nhiroki, 2017-05-02 04:47:15]

On 2017/05/01 09:09:01, kouhei (on vacation) wrote:
> On 2017/05/01 04:52:09, falken (away May 3-5) wrote:
> > I'm having trouble understanding the point of these Proxy classes. Why don't
> the
> > callers just use the GlobalScope directly?
> 
> I have to admit the CL was hard to review w/o offline design intro.
> After all I was convinced w/ the design, but I'd really want to see design doc
> for this in near future. Just a diagram/slide is enough to start with :)

Sorry for the lack of the design diagram. I'm now refreshing the outdated design
doc:
https://docs.google.com/document/d/1cgLcrua7H_7x_o5GlzYrAi2qt-TqTzgtOeixFAugR...

[haraken, 2017-05-02 04:49:44]

LGTM

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
(right):

https://codereview.chromium.org/2853743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h:41:
Persistent<PaintWorkletGlobalScope> global_scope_;
On 2017/05/02 04:45:24, nhiroki (ooo May 3-7) wrote:
> On 2017/05/01 08:43:47, haraken wrote:
> > 
> > Are you pretty sure that this persistent handle doesn't create a reference
> > cycle?
> > 
> >   PaintWorklet => PaintWorkletGlobalScopeProxy => PaintWorkletGlobalScope =>
> ...
> > => PaintWorklet ?
> 
> Hmmm... I'm not 100% sure that this never cause the reference cycle.
> 
> To make sure to avoid it, I made TerminateWorkletGlobalScope() nullify the
> handle. It's called during Document's ContextDestroyed(). Does this make
sense?

Sounds good.

[nhiroki, 2017-05-02 04:56:52]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2017-05-02 04:56:57]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-05-02 04:56:57]

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

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

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-02 06:18:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493701017279600,
"parent_rev": "6d749aecf75ec843731bf88438ed540bba793bb4", "commit_rev":
"70506ba46cddc70df56252e365866361e7712466"}

[commit-bot: I haz the power, 2017-05-02 06:18:25]

Description was changed from

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet is tightly coupled with PaintWorkletGlobalScope,
and that is an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

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

to

==========
Worklet: Introduce PaintWorkletGlobalScopeProxy

This is split from https://codereview.chromium.org/2840523002/

Before this CL, PaintWorklet is tightly coupled with PaintWorkletGlobalScope,
and that is an obstacle to unifying MainThreadWorklet and ThreadedWorklet.

This CL introduces PaintWorkletGlobalScopeProxy to separate
PaintWorkletGlobalScope from PaintWorklet and makes it easier to abstract
communication between Worklet and WorkletGlobalScope in future CLs.

BUG=627945

Review-Url: https://codereview.chromium.org/2853743002
Cr-Commit-Position: refs/heads/master@{#468572}
Committed:
https://chromium.googlesource.com/chromium/src/+/70506ba46cddc70df56252e36586...
==========

[commit-bot: I haz the power, 2017-05-02 06:18:26]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/70506ba46cddc70df56252e36586...