Introduce AnimationWorkletProxyClient and necessary plumbing to get it in worklet messaging proxy.

Introduce AnimationWorkletProxyClient and necessary plumbing to get it in worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient interface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001
Review-Url: https://codereview.chromium.org/2515363002
Cr-Commit-Position: refs/heads/master@{#449619}
Committed: https://chromium.googlesource.com/chromium/src/+/4f443d0a5d9fb3dfd0f8bbbd91b9511fef7c54f8

[majidvp, 2016-11-22 14:29:06]

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

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

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

[majidvp, 2016-11-22 14:31:24]

Description was changed from

==========
Introduce AnimationWorkletProxyClient



The following refactoring are added to allow this
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorklet to also have become a CompositorProxyClient.

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get is in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorklet to also have become a CompositorProxyClient.
- Create a factory for CompositorProxyClients

BUG=666001
==========

[commit-bot: I haz the power, 2016-11-22 14:32:23]

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

[commit-bot: I haz the power, 2016-11-22 14:32:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[majidvp, 2016-11-22 15:31:51]

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

[commit-bot: I haz the power, 2016-11-22 15:32:13]

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

[commit-bot: I haz the power, 2016-11-22 15:51:33]

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

[commit-bot: I haz the power, 2016-11-22 15:51:33]

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

[majidvp, 2016-11-22 17:18:28]

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

[commit-bot: I haz the power, 2016-11-22 17:18:54]

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

[commit-bot: I haz the power, 2016-11-22 18:15:22]

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

[commit-bot: I haz the power, 2016-11-22 18:15:22]

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

[majidvp, 2016-11-22 19:09:48]

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

[commit-bot: I haz the power, 2016-11-22 19:10:06]

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

[majidvp, 2016-11-22 19:28:06]

Description was changed from

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get is in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorklet to also have become a CompositorProxyClient.
- Create a factory for CompositorProxyClients

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorklet to also have become a CompositorProxyClient.
- Create a factory for CompositorProxyClients

BUG=666001
==========

[commit-bot: I haz the power, 2016-11-22 19:37:49]

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

[commit-bot: I haz the power, 2016-11-22 19:37:50]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[majidvp, 2016-11-23 16:50:32]

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

[commit-bot: I haz the power, 2016-11-23 16:51:02]

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

[commit-bot: I haz the power, 2016-11-23 18:11:52]

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

[commit-bot: I haz the power, 2016-11-23 18:11:53]

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

[majidvp, 2016-11-23 18:58:47]

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

[commit-bot: I haz the power, 2016-11-23 18:59:37]

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

[majidvp, 2016-11-23 19:04:15]

Description was changed from

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorklet to also have become a CompositorProxyClient.
- Create a factory for CompositorProxyClients

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now returns a factory for CompositorProxyClient

BUG=666001
==========

[majidvp, 2016-11-23 19:04:16]

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

[commit-bot: I haz the power, 2016-11-23 20:27:04]

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

[commit-bot: I haz the power, 2016-11-23 20:27:05]

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

[ikilpatrick, 2016-11-23 22:39:20]

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

[ikilpatrick, 2016-11-23 22:39:21]

+nhiroki

[nhiroki, 2016-11-24 06:59:01]

LGTM with minor comments

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h:24: #endif  //
AnimationWorkerProxyClient_h
AnimationWorkletProxyClient_h

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.h
(right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.h:23:
explicit AnimationWorkletMessagingProxy(ExecutionContext*,
Can you remove 'explicit'?

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.h
(right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.h:8:
#include "core/dom/CompositorWorkerProxyClient.h"
Can we move this into .cpp?

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h (right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:19:
CompositorProxyClientFactoryImpl(CompositorMutatorImpl*);
explicit

[majidvp, 2016-11-28 14:43:21]

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h:24: #endif  //
AnimationWorkerProxyClient_h
On 2016/11/24 06:59:00, nhiroki (slow-sheriff) wrote:
> AnimationWorkletProxyClient_h

Done.

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.h
(right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.h:23:
explicit AnimationWorkletMessagingProxy(ExecutionContext*,
On 2016/11/24 06:59:00, nhiroki (slow-sheriff) wrote:
> Can you remove 'explicit'?

Done.

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.h
(right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.h:8:
#include "core/dom/CompositorWorkerProxyClient.h"
On 2016/11/24 06:59:00, nhiroki (slow-sheriff) wrote:
> Can we move this into .cpp?

Done.

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h (right):

https://codereview.chromium.org/2515363002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:19:
CompositorProxyClientFactoryImpl(CompositorMutatorImpl*);
On 2016/11/24 06:59:00, nhiroki (slow-sheriff) wrote:
> explicit

Done.

[majidvp, 2016-11-28 14:46:08]

majidvp@chromium.org changed reviewers:
+ dcheng@chromium.org

[majidvp, 2016-11-28 14:46:09]

+dcheng@: Please review Source/core and Source/web

[ikilpatrick, 2016-11-28 16:43:42]

lgtm

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorWorkerProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorWorkerProxyClient.h:13: #include
<v8.h>
not needed?

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorWorkerProxyClientImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorWorkerProxyClientImpl.h:34:
CompositorWorkerProxyClientImpl(CompositorMutatorImpl*);
explicit?

[dcheng, 2016-11-28 21:17:32]

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxyClient.h:15: class CORE_EXPORT
CompositorProxyClient : public GarbageCollectedMixin {
Nit: it might be slightly simpler just to make this a GarbageCollectedFinalized:
it feels weird for derived classes to have to call
CompositorProxyClient::trace() when nothing is ever traced.

Alternatively... would it make sense to just have createWorkerClient() and
createWorkletClient() here and just have it return an
AnimationProxyWorkletClient / CompositorProxyWorkerClient here? Is there code
that will need to work with either one in a transparent manner?

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxyClient.h:17: enum Type {
kWorkerClient, kWorkletClient };
Nit: can we document the distinction between 'worker' and 'worklet' somewhere? I
don't actually know what the difference is...

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.h:21: // Should be
accessed only on the compositor thread.
This comment seems to be directly in opposition to the DCHECK() in the ctor that
it's executing on the main thread...

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.h:30:
AnimationWorkletProxyClientImpl(CompositorMutatorImpl*);
Nit: explicit

Or even better, add |CompositorMutatorImpl*| in the future CL where it will be
used (since it's not obvious why this needs to be a cross-thread persistent)

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:20:
CompositorProxyClient* create(CompositorProxyClient::Type) override;
Nit: It's nice to annotate this with something like:

// CompositorProxyClientFactory overrides:

Since many classes also have a factory create() method, so it's a bit confusing
here.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:25:
CrossThreadPersistent<CompositorMutatorImpl> m_mutator;
This code has a lot of CrossThreadPersistents...

Is there any documentation that describes how all these different pieces work
together?

[haraken, 2016-11-29 02:35:35]

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

[haraken, 2016-11-29 02:35:36]

LGTM

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp:37:
m_proxies.add(proxy);

DCHECK(!isMainThread()) ?

I'm a fan of having DCHECK(xxxThread()) on worker-related classes for
documentation purpose.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp:42:
m_proxies.remove(proxy);

DCHECK(!isMainThread()) ?

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:25:
CrossThreadPersistent<CompositorMutatorImpl> m_mutator;
On 2016/11/28 21:17:32, dcheng wrote:
> This code has a lot of CrossThreadPersistents...
> 
> Is there any documentation that describes how all these different pieces work
> together?

Yeah, I agree that this needs to be CrossThreadPersistent but am wondering why
we need to have m_mutator in so many places.

[majidvp, 2017-01-16 19:01:51]

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

[commit-bot: I haz the power, 2017-01-16 19:02:03]

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-01-16 19:03:57]

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

[commit-bot: I haz the power, 2017-01-16 19:03:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[majidvp, 2017-01-17 21:28:23]

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

[commit-bot: I haz the power, 2017-01-17 21:28:42]

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-01-17 22:48:52]

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

[commit-bot: I haz the power, 2017-01-17 22:48:53]

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

[majidvp, 2017-01-18 16:55:28]

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

[commit-bot: I haz the power, 2017-01-18 16:55: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-01-18 18:11:40]

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

[commit-bot: I haz the power, 2017-01-18 18:11:42]

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

[majidvp, 2017-01-20 15:40:16]

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

[commit-bot: I haz the power, 2017-01-20 15:40:25]

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

[majidvp, 2017-01-20 15:46:35]

Description was changed from

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now returns a factory for CompositorProxyClient

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001
==========

[majidvp, 2017-01-20 15:46:49]

Description was changed from

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient intereface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient interface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001
==========

[majidvp, 2017-01-20 15:48:57]

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxyClient.h:15: class CORE_EXPORT
CompositorProxyClient : public GarbageCollectedMixin {
On 2016/11/28 21:17:31, dcheng wrote:
> Nit: it might be slightly simpler just to make this a
GarbageCollectedFinalized:
> it feels weird for derived classes to have to call
> CompositorProxyClient::trace() when nothing is ever traced.

Good idea. Done!

> Alternatively... would it make sense to just have createWorkerClient() and
> createWorkletClient() here and just have it return an
> AnimationProxyWorkletClient / CompositorProxyWorkerClient here? Is there code
> that will need to work with either one in a transparent manner?

I don't think I can construct the clients here. To construct one, we need
to have a reference to CompositorMutatorImpl which is available in WebViewImpl
which is why the clients are constructed there.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxyClient.h:17: enum Type {
kWorkerClient, kWorkletClient };
On 2016/11/28 21:17:31, dcheng wrote:
> Nit: can we document the distinction between 'worker' and 'worklet' somewhere?
I
> don't actually know what the difference is...

I don't know if there is a design doc for worklets but ikilpatrick@ maybe able
to
point one out. Here is the spec which talks about the difference between worker
and a worklet https://drafts.css-houdini.org/worklets/.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorWorkerProxyClient.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorWorkerProxyClient.h:13: #include
<v8.h>
On 2016/11/28 16:43:42, ikilpatrick wrote:
> not needed?

Done.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp:37:
m_proxies.add(proxy);
On 2016/11/29 02:35:36, haraken wrote:
> 
> DCHECK(!isMainThread()) ?
> 
> I'm a fan of having DCHECK(xxxThread()) on worker-related classes for
> documentation purpose.

Done.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.cpp:42:
m_proxies.remove(proxy);
On 2016/11/29 02:35:36, haraken wrote:
> 
> DCHECK(!isMainThread()) ?

Done.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AnimationWorkletProxyClientImpl.h:21: // Should be
accessed only on the compositor thread.
On 2016/11/28 21:17:31, dcheng wrote:
> This comment seems to be directly in opposition to the DCHECK() in the ctor
that
> it's executing on the main thread...

I clarified the comment. This is similar to a worker client as it is 
constructed on main thread but its methods are used in the worklet
backing thread which in this case is not the main thread.

Added DCHECK per haraken@ suggestions that asserts the above statement.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:20:
CompositorProxyClient* create(CompositorProxyClient::Type) override;
On 2016/11/28 21:17:32, dcheng wrote:
> Nit: It's nice to annotate this with something like:
> 
> // CompositorProxyClientFactory overrides:
> 
> Since many classes also have a factory create() method, so it's a bit
confusing
> here.

I ended up removing the factory. So this is no longer an issue.

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientFactoryImpl.h:25:
CrossThreadPersistent<CompositorMutatorImpl> m_mutator;
On 2016/11/29 02:35:36, haraken wrote:
> On 2016/11/28 21:17:32, dcheng wrote:
> > This code has a lot of CrossThreadPersistents...
> > 
> > Is there any documentation that describes how all these different pieces
work
> > together?
> 
> Yeah, I agree that this needs to be CrossThreadPersistent but am wondering why
> we need to have m_mutator in so many places.
> 

OK. I realized the factory is not really doing much and may not be worth
introducing yet another CrossThreadPersistent for it. so I simply went with
existing design (a method on WebViewImp) but only added a new param to it.

I just created a doc for the animation worklet design. It is very basebone at
this point but I added a diagram that captures changes related to this patch.

https://docs.google.com/a/chromium.org/document/d/1MdpvGtnK_A2kTzLeTd07NUevMO...

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorWorkerProxyClientImpl.h (right):

https://codereview.chromium.org/2515363002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorWorkerProxyClientImpl.h:34:
CompositorWorkerProxyClientImpl(CompositorMutatorImpl*);
On 2016/11/28 16:43:42, ikilpatrick wrote:
> explicit?

Done.

[commit-bot: I haz the power, 2017-01-20 16:48:38]

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

[commit-bot: I haz the power, 2017-01-20 16:48:39]

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

[majidvp, 2017-01-23 21:50:10]

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

[commit-bot: I haz the power, 2017-01-23 21:50:57]

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-01-23 23:22:17]

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

[commit-bot: I haz the power, 2017-01-23 23:22:18]

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

[dcheng, 2017-01-24 22:56:26]

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp
(right):

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp:40:
static_cast<AnimationWorkletProxyClient*>(client);
Rather than downcasting here and below, we should just have two separate
functions IMO. While it costs us an extra vtable entry, it's more robust against
programmer error.

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:430: case
CompositorProxyClient::kWorkerClient:
Internally, we can just share |m_mutator| initialization and construct the
appropriate object depending on which overload is called.

[majidvp, 2017-01-26 15:00:04]

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp
(right):

https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp:40:
static_cast<AnimationWorkletProxyClient*>(client);
On 2017/01/24 22:56:26, dcheng wrote:
> Rather than downcasting here and below, we should just have two separate
> functions IMO. While it costs us an extra vtable entry, it's more robust
against
> programmer error.

This means adding two methods to more than a few places (ChromeClient and its
impls RenderWidgetFrameBase and its impls). Perhaps we can achieve the same
robustness by doing safer typecasts via DEFINE_TYPE_CASTS? This seems to be a
fairly typical pattern in Blink. I prefer this option if you are open to it.

[dcheng, 2017-01-27 08:58:16]

On 2017/01/26 15:00:04, majidvp wrote:
>
https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp
> (right):
> 
>
https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp:40:
> static_cast<AnimationWorkletProxyClient*>(client);
> On 2017/01/24 22:56:26, dcheng wrote:
> > Rather than downcasting here and below, we should just have two separate
> > functions IMO. While it costs us an extra vtable entry, it's more robust
> against
> > programmer error.
> 
> This means adding two methods to more than a few places (ChromeClient and its
> impls RenderWidgetFrameBase and its impls). Perhaps we can achieve the same
> robustness by doing safer typecasts via DEFINE_TYPE_CASTS? This seems to be a
> fairly typical pattern in Blink. I prefer this option if you are open to it.

I think that would make more sense if there's something that wants to use the
underlying interface directly. But it seems like that's not the case here--every
location that calls this immediately downcasts. So in that case,
DEFINE_TYPE_CASTS and using toFoo() doesn't make as much sense.

[majidvp, 2017-02-06 19:08:19]

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

[commit-bot: I haz the power, 2017-02-06 19:09: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-02-06 19:12:07]

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

[commit-bot: I haz the power, 2017-02-06 19:12:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[majidvp, 2017-02-06 20:24:48]

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

[commit-bot: I haz the power, 2017-02-06 20:25:27]

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-02-06 21:26:27]

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

[commit-bot: I haz the power, 2017-02-06 21:26:28]

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

[majidvp, 2017-02-07 13:53:33]

On 2017/01/27 08:58:16, dcheng wrote:
> On 2017/01/26 15:00:04, majidvp wrote:
> >
>
https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2515363002/diff/210001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp:40:
> > static_cast<AnimationWorkletProxyClient*>(client);
> > On 2017/01/24 22:56:26, dcheng wrote:
> > > Rather than downcasting here and below, we should just have two separate
> > > functions IMO. While it costs us an extra vtable entry, it's more robust
> > against
> > > programmer error.
> > 
> > This means adding two methods to more than a few places (ChromeClient and
its
> > impls RenderWidgetFrameBase and its impls). Perhaps we can achieve the same
> > robustness by doing safer typecasts via DEFINE_TYPE_CASTS? This seems to be
a
> > fairly typical pattern in Blink. I prefer this option if you are open to it.
> 
> I think that would make more sense if there's something that wants to use the
> underlying interface directly. But it seems like that's not the case
here--every
> location that calls this immediately downcasts. So in that case,
> DEFINE_TYPE_CASTS and using toFoo() doesn't make as much sense.

Done.

[dcheng, 2017-02-08 03:16:36]

LGTM

[majidvp, 2017-02-09 16:49:13]

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

[commit-bot: I haz the power, 2017-02-09 16:49: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-02-09 16:54:39]

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

[commit-bot: I haz the power, 2017-02-09 16:54:41]

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...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[majidvp, 2017-02-09 17:03:56]

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

[commit-bot: I haz the power, 2017-02-09 17:04:29]

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-02-09 18:37:22]

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

[commit-bot: I haz the power, 2017-02-09 18:37:24]

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

[majidvp, 2017-02-09 20:01:51]

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

[commit-bot: I haz the power, 2017-02-09 20:03:11]

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-02-09 22:04:37]

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

[commit-bot: I haz the power, 2017-02-09 22:04:39]

Dry run: This issue passed the CQ dry run.

[majidvp, 2017-02-10 14:42:31]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2017-02-10 14:42:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, haraken@chromium.org, ikilpatrick@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2515363002/#ps310001
(title: "Remove unnecessary PLATFORM_EXPORT")

[commit-bot: I haz the power, 2017-02-10 14:42:52]

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-02-10 14:48:59]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1486737751118870,
"parent_rev": "28ec0e0c2425d480693e8d40510b8093cef1f89f", "commit_rev":
"4f443d0a5d9fb3dfd0f8bbbd91b9511fef7c54f8"}

[commit-bot: I haz the power, 2017-02-10 14:49:35]

Description was changed from

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient interface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001
==========

to

==========
Introduce AnimationWorkletProxyClient and necessary plumbing to get it in
worklet messaging proxy. 

The following refactoring were made to allow this:
- CompositoraMutatorImpl now uses CompositorAnimator interface and no longer
  depends on CPCI directly.
- Divide CompositorProxyClient interface into two unit separating functionality
  related to CompositorProxy from functionality related to Worker. This allows
  AnimationWorkletProxyClient to also be a CompositorProxyClient.
- WebView now can create two different types of CompositorProxyClient
  for compositor worker and animation worklet.

BUG=666001

Review-Url: https://codereview.chromium.org/2515363002
Cr-Commit-Position: refs/heads/master@{#449619}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f443d0a5d9fb3dfd0f8bbbd91b9...
==========

[commit-bot: I haz the power, 2017-02-10 14:49:38]

Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/4f443d0a5d9fb3dfd0f8bbbd91b9...