compositor-worker: Initialize CW machinery plumbing to compositor and fire CW rAF callbacks.

third_party/WebKit/Source/web/CompositorMutatorImpl.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CompositorMutatorImpl_h
+#define CompositorMutatorImpl_h
+
+#include "platform/graphics/CompositorMutator.h"
+#include "platform/heap/Handle.h"
+#include "wtf/Noncopyable.h"
+#include "wtf/OwnPtr.h"
+#include "wtf/PassOwnPtr.h"
+
+#include <set>
+
+namespace blink {
+
+class CompositorProxy;
+class CompositorProxyClient;
+class CompositorWorkerGlobalScope;
+class CompositorMutatorClient;
+class CustomCompositorAnimationManager;
+class WaitableEvent;
+
+class CompositorMutatorImpl final : public CompositorMutator {

[jbroman, 2016/04/20 19:24:51]

It sounds like there are threading concerns with this class; would you mind a
comment explaining not only what it's for, but which threads are expected to use
which parts of it? I'm doing a fair amount of archaeology to try and figure this
out.

+    WTF_MAKE_NONCOPYABLE(CompositorMutatorImpl);
+
+public:
+    static PassOwnPtr<CompositorMutatorImpl> create();
+    ~CompositorMutatorImpl() override;
+
+    // CompositorMutator implementation.
+    bool mutate(double timeNow) override;
+
+    void registerClient(CompositorProxyClient*);

[jbroman, 2016/04/20 21:06:42]

nit: This name is confusing given that client() also exists, but is a different
kind of thing. You say "proxy client" elsewhere in this file; that seems
clearer.

Also, is there any reason not to use the concrete type,
CompositorProxyClientImpl*, here?

[flackr, 2016/04/25 14:06:28]

Done and done.

+    void unregisterClient(CompositorProxyClient*);
+
+    CompositorProxyClient* createCompositorProxyClient();

[jbroman, 2016/04/20 21:06:42]

What value does this function add, compared callers just writing "new
CompositorProxyClientImpl(mutator)"? Callers have to be able to access
Source/web/ already.

[flackr, 2016/04/25 14:06:28]

Doesn't seem to be adding any value. I've moved this construction to callers.

+    CompositorMutatorClient* client() { return m_client.get(); }
+
+    void setNeedsMutate();
+
+private:
+    CompositorMutatorImpl();
+
+    // We use a standard container instead of oil-pan wtf ones as
+    // the latter seems to mess up oil-pan garbage collection. I believe this is
+    // due to the multi-threaded situation and the fact that CC thread does not
+    // participate in oil-pan GC.
+    // This should not be an issue, as the pointer does not need to be traced
+    // here because the CompositorProxyClientImpl lifetime is already managed by
+    // oilpan and we update the Set ourself via register/unregister and do not
+    // need oilpan to take any extra actions.
+    // TODO(majidvp): Look into details to see why we crash if
+    // PersistentHeapHashSet<Member<>> is used
+    using ProxyClients = std::set<CompositorProxyClient*>;

[haraken, 2016/04/19 23:35:52]

Can you make CompositorMutator GarbageCollected? Then you can just use
HeapHashSet<Member<>> here.

Then you won't need a pre-finalizer in CompositorWorkerGlobalScope.

[jbroman, 2016/04/20 19:24:51]

+1 An STL container holding raw pointers into the Oilpan heap is very scary.

[jbroman, 2016/04/20 21:06:42]

Also, can the pointed-to type ever be anything other than
CompositorProxyClientImpl? If not, perhaps we can refer to the concrete type and
avoid exposing some virtual methods on the base class (e.g.
CompositorProxyClient::mutate).

[flackr, 2016/04/25 14:06:28]

Still working on getting this right. Somehow naively converting this I am
getting a crash during the trace of the HeapHashSet from the
Persistent<CompositorMutatorImpl>.

On 2016/04/20 at 21:06:42, jbroman wrote:
Done.

+    ProxyClients m_proxyClients;
+
+    OwnPtr<CustomCompositorAnimationManager> m_animationManager;
+    OwnPtr<CompositorMutatorClient> m_client;
+};
+
+} // namespace blink
+
+#endif // CompositorMutatorImpl_h