[compositorworker] compositor proxy mutation updates underlying layers

[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ee4cbfea75629407af2b0377ca2567540081430d
Cr-Commit-Position: refs/heads/master@{#400082}

[majidvp, 2016-06-07 15:49:46]

Description was changed from

==========
[compositorworker] pass in layertree to compositor mutations

- Pass in layer tree to compositor mutator
- ensure each proxy takes its own mutable state and any mutation is applied
  to the state

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

to

==========
[compositorworker] pass in layertree to compositor mutations

- Pass in layer tree to compositor mutator
- ensure each proxy takes its own mutable state and any mutation is applied
  to the state

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[majidvp, 2016-06-07 15:50:39]

Description was changed from

==========
[compositorworker] pass in layertree to compositor mutations

- Pass in layer tree to compositor mutator
- ensure each proxy takes its own mutable state and any mutation is applied
  to the state

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[compositorworker] pass in layer tree to compositor mutatator

- Pass in layer tree to compositor mutator
- ensure each proxy takes its own mutable state and any mutation is applied
  to the state

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[majidvp, 2016-06-07 23:06:56]

Description was changed from

==========
[compositorworker] pass in layer tree to compositor mutatator

- Pass in layer tree to compositor mutator
- ensure each proxy takes its own mutable state and any mutation is applied
  to the state

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[majidvp, 2016-06-07 23:06:56]

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

[jbroman, 2016-06-08 14:59:38]

some initial comments, but this depends on the other CL, of course

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:211: if (!m_state.get())
nit: both std::unique_ptr and WTF::OwnPtr have operator!; you can just write
"!m_state" without the explicit get(). (Here and elsewhere)

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

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:15: #include
"wtf/PtrUtil.h"
Unused header?

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:55: void
takeCompositorMutableState(std::unique_ptr<CompositorMutableState>);
yutak will switch us to unique_ptr shortly, but for now I think OwnPtr<> is
still the preferred unique-owner smart pointer in Source/core/.

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMMatrix.cpp:22: return new
DOMMatrix(transformationMatrix, true);
the boolean here is "is2D"; what justification does this constructor have for
believing that the SkMatrix44 it is given is 2D?

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h
(right):

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h:29:
std::unique_ptr<CompositorMutableState> getMutableStateFor(uint64_t elementId);
Why all this code changing from OwnPtr to std::unique_ptr?

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

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h:28: bool
Mutate(base::TimeTicks monotonicTime, cc::LayerTreeImpl* state) override;
"state" seems an odd name for a LayerTreeImpl* (here and at the interface method
definition); what does this mean?

[majidvp, 2016-06-08 21:50:10]

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

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:15: #include
"wtf/PtrUtil.h"
On 2016/06/08 14:59:38, jbroman wrote:
> Unused header?

Done.

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:55: void
takeCompositorMutableState(std::unique_ptr<CompositorMutableState>);
On 2016/06/08 14:59:38, jbroman wrote:
> yutak will switch us to unique_ptr shortly, but for now I think OwnPtr<> is
> still the preferred unique-owner smart pointer in Source/core/.

There was a PSA earlier that we should start using them for new code in blink.
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/unique_pt...

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMMatrix.cpp:22: return new
DOMMatrix(transformationMatrix, true);
On 2016/06/08 14:59:38, jbroman wrote:
> the boolean here is "is2D"; what justification does this constructor have for
> believing that the SkMatrix44 it is given is 2D?

Done.
You are absolutely right. Not the bit is only set if the TransformationMatrix is
affine which is equivalent to a 2D DOMMatrix;

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h
(right):

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h:29:
std::unique_ptr<CompositorMutableState> getMutableStateFor(uint64_t elementId);
On 2016/06/08 14:59:38, jbroman wrote:
> Why all this code changing from OwnPtr to std::unique_ptr?

I am using unique_ptr for the only call site of this function so I wanted
to be consistent.

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

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h:28: bool
Mutate(base::TimeTicks monotonicTime, cc::LayerTreeImpl* state) override;
On 2016/06/08 14:59:38, jbroman wrote:
> "state" seems an odd name for a LayerTreeImpl* (here and at the interface
method
> definition); what does this mean?

This is following the convention used in CompositorMutableStateProvider.

I think the intention has been that in this context layer tree impl is the 
underlying state we are mutating. But I agree that it is perhaps better to just
use |tree_impl| and reserve state for referring to its wrapper which 
is CompositorMutableState.
wdyt?

[jbroman, 2016-06-08 21:54:56]

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

https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:55: void
takeCompositorMutableState(std::unique_ptr<CompositorMutableState>);
On 2016/06/08 at 21:50:09, majidvp wrote:
> On 2016/06/08 14:59:38, jbroman wrote:
> > yutak will switch us to unique_ptr shortly, but for now I think OwnPtr<> is
> > still the preferred unique-owner smart pointer in Source/core/.
> 
> There was a PSA earlier that we should start using them for new code in blink.
>
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/unique_pt...

OK, I'd missed that PSA, and was assuming it would be handled the same way as
scoped_ptr, which is for the old thing to be used until the One Great Rewrite
(tm). Proceed. :)

[majidvp, 2016-06-13 18:53:28]

jbroman@: PTAL

On 2016/06/08 21:54:56, jbroman wrote:
>
https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/CompositorProxy.h (right):
> 
>
https://codereview.chromium.org/2041193005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/CompositorProxy.h:55: void
> takeCompositorMutableState(std::unique_ptr<CompositorMutableState>);
> On 2016/06/08 at 21:50:09, majidvp wrote:
> > On 2016/06/08 14:59:38, jbroman wrote:
> > > yutak will switch us to unique_ptr shortly, but for now I think OwnPtr<>
is
> > > still the preferred unique-owner smart pointer in Source/core/.
> > 
> > There was a PSA earlier that we should start using them for new code in
blink.
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/unique_pt...
> 
> OK, I'd missed that PSA, and was assuming it would be handled the same way as
> scoped_ptr, which is for the old thing to be used until the One Great Rewrite
> (tm). Proceed. :)

[jbroman, 2016-06-13 19:45:17]

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html
(right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:27:
var test = async_test('Tests that a change from the main thread is received on
the compositor thread');
This test is a little weird to read, because of the asynchronous control flow.
Have you considered writing a promise test instead? Something like:

function postMessageAndAwaitReply(worker, message) {
  worker.postMessage(message);
  return new Promise((resolve, reject) => {
    worker.addEventListener(function listener(e) {
      resolve(e.data);
      worker.removeEventListener(listener);
    });
  });
}

function requestAnimationFramePromise() {
  return new Promise((resolve, reject) => requestAnimationFrame(ts =>
resolve(ts)));
}

promise_test(function() {
  var worker = new CompositorWorker(...);
  var proxy = new CompositorProxy(document.getElementById('box'));
  var keys = ['opacity', 'transform', 'scrollTop', 'scrollLeft'];
  return postMessageAndAwaitReply(worker, {proxy, keys}).then(data => {
    assert_equals(data.opacity, 1);
    // ...
    element.scrollLeft = 10;
    // ...
    return requestAnimationFramePromise().then(ts =>
postMessageAndAwaitReply(worker, {});
  }).then(data => {
    assert_equals(data.opacity, 0.5);
    // ..
  });
}, 'Tests that...');

I personally prefer this (removes the need to remember to put step_func in the
right places due to promise rejection propagation, and the order of events reads
from top to bottom). But I won't insist on this.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:46:
var element = document.getElementById('box');
nit: inconsistent indentation (I think Blink still prefers 4 in JS, but
regardless this should be consistent)

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:62:
setTimeout(function(){
Are you sure you want setTimeout and not requestAnimationFrame or
testRunner.layoutAndPaintAsyncThen?

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:181: if (!m_state.get())
Should these be returning zero? What does it mean for the proxy to be queried
when it has no state? Should an exception be thrown in this case instead? How
could clients distinguish missing state from something actually having zero
opacity, zero scroll offset, etc.?

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.idl (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.idl:12: readonly attribute
boolean initialized;
Is there a doc/spec that explains what it means for a CompositorProxy to be
"initialized"?

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:16:
CompositorMutableStateProvider::CompositorMutableStateProvider(cc::LayerTreeImpl*
tree_impl, CompositorMutations* mutations)
nit: "treeImpl"

Blink style still uses camelCase variable names.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:55:
updateMutableStateForCompositorProxies(stateProvider);
Mind clarifying what's going on here? I think I get it (the state is only
available during the animation frame callbacks), but I think it bears some
explanation.

[majidvp, 2016-06-13 21:52:51]

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html
(right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:27:
var test = async_test('Tests that a change from the main thread is received on
the compositor thread');
On 2016/06/13 19:45:17, jbroman wrote:
> This test is a little weird to read, because of the asynchronous control flow.
> Have you considered writing a promise test instead? Something like:
> 
> function postMessageAndAwaitReply(worker, message) {
>   worker.postMessage(message);
>   return new Promise((resolve, reject) => {
>     worker.addEventListener(function listener(e) {
>       resolve(e.data);
>       worker.removeEventListener(listener);
>     });
>   });
> }
> 
> function requestAnimationFramePromise() {
>   return new Promise((resolve, reject) => requestAnimationFrame(ts =>
> resolve(ts)));
> }
> 
> promise_test(function() {
>   var worker = new CompositorWorker(...);
>   var proxy = new CompositorProxy(document.getElementById('box'));
>   var keys = ['opacity', 'transform', 'scrollTop', 'scrollLeft'];
>   return postMessageAndAwaitReply(worker, {proxy, keys}).then(data => {
>     assert_equals(data.opacity, 1);
>     // ...
>     element.scrollLeft = 10;
>     // ...
>     return requestAnimationFramePromise().then(ts =>
> postMessageAndAwaitReply(worker, {});
>   }).then(data => {
>     assert_equals(data.opacity, 0.5);
>     // ..
>   });
> }, 'Tests that...');
> 
> I personally prefer this (removes the need to remember to put step_func in the
> right places due to promise rejection propagation, and the order of events
reads
> from top to bottom). But I won't insist on this.

Acknowledged.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:46:
var element = document.getElementById('box');
On 2016/06/13 19:45:17, jbroman wrote:
> nit: inconsistent indentation (I think Blink still prefers 4 in JS, but
> regardless this should be consistent)

Done.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/basic-plumbing-main-to-worker.html:62:
setTimeout(function(){
On 2016/06/13 19:45:17, jbroman wrote:
> Are you sure you want setTimeout and not requestAnimationFrame or
> testRunner.layoutAndPaintAsyncThen?

Actually, setTimeout is not needed here. Removed.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:181: if (!m_state.get())
On 2016/06/13 19:45:17, jbroman wrote:
> Should these be returning zero? What does it mean for the proxy to be queried
> when it has no state? Should an exception be thrown in this case instead? How
> could clients distinguish missing state from something actually having zero
> opacity, zero scroll offset, etc.?

Not having a state is equivalent to the underlying layer not being present in
the tree. At the moment, any mutation of the proxy is a no-op as you see.

vollick@: do you have any preference on the failure mode when a proxy in not
initialized?

I believed, our past discussion lead to the conclusion that eventually we should
just hold onto those mutations even when disconnected and apply them once the
layer is activated. So I think not throwing is the right approach even though 
we don't do the correct thing here yet.

This means that modifying an uninitialized proxy is *not* illegal, only that 
those modifications will not be seen in the the composited result.

Or should we just throw until the correct behaviour is implemented?

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.idl (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.idl:12: readonly attribute
boolean initialized;
On 2016/06/13 19:45:17, jbroman wrote:
> Is there a doc/spec that explains what it means for a CompositorProxy to be
> "initialized"?

I don't think so yet :(. Let me find out where is the most accurate source of
truth
for CW API and I will add it there.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:16:
CompositorMutableStateProvider::CompositorMutableStateProvider(cc::LayerTreeImpl*
tree_impl, CompositorMutations* mutations)
On 2016/06/13 19:45:17, jbroman wrote:
> nit: "treeImpl"
> 
> Blink style still uses camelCase variable names.

Done.

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:55:
updateMutableStateForCompositorProxies(stateProvider);
On 2016/06/13 19:45:17, jbroman wrote:
> Mind clarifying what's going on here? I think I get it (the state is only
> available during the animation frame callbacks), but I think it bears some
> explanation.

Yes. Proxies may only mutate the tree during rAF. I will add a comment.

[Ian Vollick, 2016-06-14 11:24:22]

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

[Ian Vollick, 2016-06-14 11:24:29]

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:181: if (!m_state.get())
On 2016/06/13 at 21:52:51, majidvp wrote:
> On 2016/06/13 19:45:17, jbroman wrote:
> > Should these be returning zero? What does it mean for the proxy to be
queried
> > when it has no state? Should an exception be thrown in this case instead?
How
> > could clients distinguish missing state from something actually having zero
> > opacity, zero scroll offset, etc.?
> 
> Not having a state is equivalent to the underlying layer not being present in
> the tree. At the moment, any mutation of the proxy is a no-op as you see.
> 
> vollick@: do you have any preference on the failure mode when a proxy in not
> initialized?
> 
> I believed, our past discussion lead to the conclusion that eventually we
should
> just hold onto those mutations even when disconnected and apply them once the
> layer is activated. So I think not throwing is the right approach even though 
> we don't do the correct thing here yet.
> 
> This means that modifying an uninitialized proxy is *not* illegal, only that 
> those modifications will not be seen in the the composited result.
> 
> Or should we just throw until the correct behaviour is implemented?

There's no harm in throwing in the getter and I think we should probably do
that.

As for the setter, I would indeed like it to be legal to write while
disconnected, but I think that throwing until it works correctly is a good idea.

[Ian Vollick, 2016-06-14 11:24:47]

vollick@chromium.org changed reviewers:
- vollick@chromium.org

[majidvp, 2016-06-14 17:25:58]

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2041193005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:181: if (!m_state.get())
On 2016/06/14 11:24:28, vollick wrote:
> On 2016/06/13 at 21:52:51, majidvp wrote:
> > On 2016/06/13 19:45:17, jbroman wrote:
> > > Should these be returning zero? What does it mean for the proxy to be
> queried
> > > when it has no state? Should an exception be thrown in this case instead?
> How
> > > could clients distinguish missing state from something actually having
zero
> > > opacity, zero scroll offset, etc.?
> > 
> > Not having a state is equivalent to the underlying layer not being present
in
> > the tree. At the moment, any mutation of the proxy is a no-op as you see.
> > 
> > vollick@: do you have any preference on the failure mode when a proxy in not
> > initialized?
> > 
> > I believed, our past discussion lead to the conclusion that eventually we
> should
> > just hold onto those mutations even when disconnected and apply them once
the
> > layer is activated. So I think not throwing is the right approach even
though 
> > we don't do the correct thing here yet.
> > 
> > This means that modifying an uninitialized proxy is *not* illegal, only that

> > those modifications will not be seen in the the composited result.
> > 
> > Or should we just throw until the correct behaviour is implemented?
> 
> There's no harm in throwing in the getter and I think we should probably do
> that.
> 
> As for the setter, I would indeed like it to be legal to write while
> disconnected, but I think that throwing until it works correctly is a good
idea.

Done.

[jbroman, 2016-06-15 08:55:50]

lgtm once you fix the trybot failures (which look real)

https://codereview.chromium.org/2041193005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:76:
proxy->takeCompositorMutableState(stateProvider ?
stateProvider->getMutableStateFor(proxy->elementId()) : nullptr);
By the way, what happens if there are multiple CompositorWorkers with proxies to
the same element (or one worker with multiple proxies)? This _looks_ like one of
them will "win" (i.e. read the original values, and have its values take
effect). Is this intentional?

(Or am I guessing wrong what the behaviour will be? Is there a layout test for
this?)

[majidvp, 2016-06-15 15:38:03]

Description was changed from

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern 

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[majidvp, 2016-06-15 15:46:53]

Fixed failing test and refactor the layout tests to
use a similar promise based pattern for waiting on
proxy initialization.

https://codereview.chromium.org/2041193005/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:76:
proxy->takeCompositorMutableState(stateProvider ?
stateProvider->getMutableStateFor(proxy->elementId()) : nullptr);
On 2016/06/15 08:55:50, jbroman wrote:
> By the way, what happens if there are multiple CompositorWorkers with proxies
to
> the same element (or one worker with multiple proxies)? This _looks_ like one
of
> them will "win" (i.e. read the original values, and have its values take
> effect). Is this intentional?
> 
> (Or am I guessing wrong what the behaviour will be? Is there a layout test for
> this?)

Yes this is the current design. It is similar to mutating the same
property in multiple rAF callback. The last callback mutation wins.

The only difference is that unlike regular rAF callbacks there is no
specified order among callbacks that belong to different workers. 

No layout test covering this but we should add one.

[majidvp, 2016-06-15 15:47:37]

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

[majidvp, 2016-06-15 15:47:38]

aelias@: Please review changes in cc/ and Source/web

[aelias_OOO_until_Jul13, 2016-06-15 18:40:54]

https://codereview.chromium.org/2041193005/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:57:
updateMutableStateForCompositorProxies(stateProvider);
Please make a small RAII class called like ScopedCompositorMutableState for this
instead.

[majidvp, 2016-06-16 02:53:54]

https://codereview.chromium.org/2041193005/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2041193005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:57:
updateMutableStateForCompositorProxies(stateProvider);
On 2016/06/15 18:40:54, aelias wrote:
> Please make a small RAII class called like ScopedCompositorMutableState for
this
> instead.

Added a class please take a look.

[aelias_OOO_until_Jul13, 2016-06-16 03:09:08]

lgtm

[majidvp, 2016-06-16 03:16:18]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-06-16 03:16:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2041193005/#ps300001
(title: "Use RAII pattern")

[commit-bot: I haz the power, 2016-06-16 03:16:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041193005/300001

[commit-bot: I haz the power, 2016-06-16 04:10:19]

Description was changed from

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern 

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern 

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-16 04:10:20]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-06-16 04:10:25]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 04:11:49]

Description was changed from

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern 

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[compositorworker] compositor proxy mutation updates underlying layers

The CL makes it so that changes to compositor proxies in  compositor worker to
actually mutate the underlying layers.

This is achieved by passing a reference to active layer tree to
LayerTreeMutator. This reference encapsulated by  CompositorMutableStateProvider
which is responsible for handing out  individual CompositorMutableState for each
proxy during rAF mutation.

Any mutation on the proxy updates the mutable states which in turn is reflected
on the underlying layers (main, scroll).

major changes:
- pass active layer tree impl to mutator
- set mutable state for compositor proxy during rAF
- add 'initialized' attribute to compositor worker to signal its activation
- add layout tests covering visual updates from worker
- update most of compositor worker layout tests to use a
common pattern

TEST=virtual/threaded/fast/compositorworker
BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ee4cbfea75629407af2b0377ca2567540081430d
Cr-Commit-Position: refs/heads/master@{#400082}
==========

[commit-bot: I haz the power, 2016-06-16 04:11:51]

Patchset 16 (id:??) landed as
https://crrev.com/ee4cbfea75629407af2b0377ca2567540081430d
Cr-Commit-Position: refs/heads/master@{#400082}