Don't expose a LayerImpl reference to CompositorProxy.

Instead of exposing a LayerImpl reference to CompositorProxy, create a
snapshot of the LayerImpl and pass the snapshot down to CompositorProxy.  Then
use the CompositorMutations to figure out what updates should be made to the
LayerTree.

BUG=694532

[Scott Funkenhauser, 2017-03-17 19:08:04]

Description was changed from

==========
DO NOT SUBMIT WIP

Animation Worklet

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

to

==========
Instead of exposing a LayerImpl reference to CompositorProxy, create a
snapshot of the LayerImpl and pass the snapshot down to CompositorProxy.  Then
use the CompositorMutations to figure out what updates should be made to the
LayerTree.

BUG=694532
==========

[Scott Funkenhauser, 2017-03-17 20:02:40]

sfunkenhauser@chromium.org changed reviewers:
+ flackr@chromium.org, smcgruer@chromium.org

[Scott Funkenhauser, 2017-03-17 20:02:40]

sfunkenhauser@chromium.org changed required reviewers:
+ flackr@chromium.org, smcgruer@chromium.org

[Scott Funkenhauser, 2017-03-17 20:03:48]

I'll be adding tests soon, hopefully early next week.

[smcgruer, 2017-03-20 14:31:51]

A few mostly high level thoughts.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:40: uint64_t proxyId()
const { return (uint64_t)this; }
I think this should probably be a static uint64_t that is incremented and
assigned in the constructor, unless flackr has a better idea of how to assign an
id to each proxy.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:23:
m_properties->opacity = opacity;
I may be thinking about this incorrectly, but I believe this CL still changes
the code behavior when there are multiple CompositorProxy objects for the same
element.

Previously, all CompositorProxy objects for the same element would share not
only the same CompositorMutation*, but also the same cc::LayerImpl for
main/scroll. Therefore, if proxy A updated the opacity and then proxy B read it,
proxy B would get the value written by A.

With this CL, each CompositorProxy object for the same element shares only the
CompositorMutation*, and has a unique CompositorMutableProperties object that is
snapshotted at the beginning of the Mutate() call-chain. In this scenario, any
updates from proxy A to the elements opacity would not be seen by proxy B.
Correct?

This behavior change might be fine, I just want to make sure that we are aware
it is happening (if my interpretation of the code is correct :))

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:45: //
Populate ProxyCompositorMutablePropertiesMap.
Nit; this comment isn't useful, the code is clear enough

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:59: //
Currently ScrollTree:OnScrollOffsetAnimted triggers a main frame
1. Typo in OnScrollOffsetAnimated

2. It may be more correct to say it triggers a main frame begin synchronously.
Only main can actually trigger a main frame afaik.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:61: //
released.  Make sure m_mutations is not null before calling
Nit; I think lines 61,62 would fit on one line if you got rid of the
double-space after released.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:154:
void CompositorMutatorClient::updateLayerTree(
Could this be an anon-namespace method? I don't see any member accesses.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h:53:
std::map<uint64_t, std::pair<uint64_t, uint32_t>> m_inputProperties;
What threads access this? I suspect that
CompositorMutatorClient::[un]registerCompositorProxy might be called on the main
thread?

[Scott Funkenhauser, 2017-03-21 19:31:12]

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:40: uint64_t proxyId()
const { return (uint64_t)this; }
On 2017/03/20 14:31:50, smcgruer wrote:
> I think this should probably be a static uint64_t that is incremented and
> assigned in the constructor, unless flackr has a better idea of how to assign
an
> id to each proxy.

Done.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:23:
m_properties->opacity = opacity;
Yes your interpretation is correct.

Since it was easier to isolate input between different proxies, and that's the
direction we want to move to eventually, I didn't think it made sense to retain
the same code behavior.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:45: //
Populate ProxyCompositorMutablePropertiesMap.
On 2017/03/20 14:31:51, smcgruer wrote:
> Nit; this comment isn't useful, the code is clear enough

Done.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:59: //
Currently ScrollTree:OnScrollOffsetAnimted triggers a main frame
On 2017/03/20 14:31:51, smcgruer wrote:
> 1. Typo in OnScrollOffsetAnimated
> 
> 2. It may be more correct to say it triggers a main frame begin synchronously.
> Only main can actually trigger a main frame afaik.

Done.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:61: //
released.  Make sure m_mutations is not null before calling
On 2017/03/20 14:31:51, smcgruer wrote:
> Nit; I think lines 61,62 would fit on one line if you got rid of the
> double-space after released.

Done.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:154:
void CompositorMutatorClient::updateLayerTree(
On 2017/03/20 14:31:51, smcgruer wrote:
> Could this be an anon-namespace method? I don't see any member accesses.

Done.

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h
(right):

https://codereview.chromium.org/2756703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h:53:
std::map<uint64_t, std::pair<uint64_t, uint32_t>> m_inputProperties;
It looks like registerCompositorProxy is called on the Compositor thread:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Compo...

UnregisterCompositorProxy is called from CompositorProxy::disconnect(), but I'm
not sure how to track down what thread that is called on.

I'm assuming unregisterCompositorProxy is called from the Compositor thread as
well, otherwise there is also an issue in CompositorWorkerProxyClientImpl.  In
the same [un]register calls the proxy is added / removed from (what appears to
me) a non-thread safe map.

[smcgruer, 2017-03-22 14:59:40]

Thanks for your efforts Scott. I have taken over this CL in
https://codereview.chromium.org/2765053002/, can you close this issue?