Avoid exposing cc::Layer tree to CompositorProxy

third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp

 // 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.
 
 #include "platform/graphics/CompositorMutatorClient.h"
 
+#include <memory>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/trees/layer_tree_impl.h"
+#include "cc/trees/scroll_node.h"
+#include "platform/graphics/CompositorElementId.h"
 #include "platform/graphics/CompositorMutableStateProvider.h"
 #include "platform/graphics/CompositorMutation.h"
 #include "platform/graphics/CompositorMutationsTarget.h"
 #include "platform/graphics/CompositorMutator.h"
 #include "wtf/PtrUtil.h"
-#include <memory>
 
 namespace blink {
+namespace {
+void updateLayerTree(cc::LayerTreeImpl* treeImpl,
+                     uint64_t elementId,
+                     const CompositorMutation* mutation) {
+  cc::LayerImpl* layer = treeImpl->LayerByElementId(

[flackr, 2017/04/03 17:09:19]

Since (I'm assuming) the LayerByElementId requires a lookup whereas isMutated
calls are just checking a bit, we should only do the lookup if one of the
relevant properties has changed (you'll probably need to expose the bitmask to
avoid multiple lookups).

[smcgruer, 2017/04/03 19:37:11]

Done, but it's real ugly now that we're using bitmasks checks rather than calls
into CompositorMutation :(

+      createCompositorElementId(elementId, CompositorSubElementId::Primary));
+  if (layer) {
+    if (mutation->isOpacityMutated()) {
+      treeImpl->property_trees()->effect_tree.OnOpacityAnimated(
+          mutation->opacity(), layer->effect_tree_index(), treeImpl);
+    }
+    if (mutation->isTransformMutated()) {
+      treeImpl->property_trees()->transform_tree.OnTransformAnimated(
+          gfx::Transform(mutation->transform()), layer->transform_tree_index(),
+          treeImpl);
+    }
+  }
+
+  cc::LayerImpl* scrollLayer = treeImpl->LayerByElementId(
+      createCompositorElementId(elementId, CompositorSubElementId::Scroll));
+  if (scrollLayer) {
+    gfx::ScrollOffset offset = scrollLayer->CurrentScrollOffset();
+    if (mutation->isScrollLeftMutated()) {
+      offset.set_x(mutation->scrollLeft());
+    }
+    if (mutation->isScrollTopMutated()) {
+      offset.set_y(mutation->scrollTop());
+    }
+    if (mutation->isScrollLeftMutated() || mutation->isScrollTopMutated()) {
+      treeImpl->property_trees()->scroll_tree.OnScrollOffsetAnimated(
+          scrollLayer->id(), scrollLayer->scroll_tree_index(), offset,
+          treeImpl);
+    }
+  }
+}
+}  // namespace
 
 CompositorMutatorClient::CompositorMutatorClient(
     CompositorMutator* mutator,
     CompositorMutationsTarget* mutationsTarget)
     : m_client(nullptr),
       m_mutationsTarget(mutationsTarget),
       m_mutator(mutator),
       m_mutations(nullptr) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("compositor-worker"),
                "CompositorMutatorClient::CompositorMutatorClient");
 }
 
 CompositorMutatorClient::~CompositorMutatorClient() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("compositor-worker"),
                "CompositorMutatorClient::~CompositorMutatorClient");
 }
 
 bool CompositorMutatorClient::Mutate(base::TimeTicks monotonicTime,
                                      cc::LayerTreeImpl* treeImpl) {
   TRACE_EVENT0("compositor-worker", "CompositorMutatorClient::Mutate");
   double monotonicTimeNow = (monotonicTime - base::TimeTicks()).InSecondsF();
   if (!m_mutations)
     m_mutations = WTF::wrapUnique(new CompositorMutations);
-  CompositorMutableStateProvider compositorState(treeImpl, m_mutations.get());
+
+  ProxyCompositorMutablePropertiesMap inputPropertiesMap;
+  snapshotLayerTree(treeImpl, &inputPropertiesMap);
+
+  CompositorMutations newMutations;
+  CompositorMutableStateProvider compositorState(&inputPropertiesMap,
+                                                 &newMutations);
   bool shouldReinvoke = m_mutator->mutate(monotonicTimeNow, &compositorState);
+
+  for (const auto& entry : newMutations.map) {
+    updateLayerTree(treeImpl, entry.key, entry.value.get());
+  }
+
+  // Currently ScrollTree:OnScrollOffsetAnimated triggers a main frame begin
+  // synchronously, which will call TakeMutations causing m_mutations to be
+  // released. Make sure m_mutations is not null before calling updateMutations.
+  // TODO(smcgruer): Find a way to avoid triggering a main frame begin here.
+  if (!m_mutations)
+    m_mutations = WTF::wrapUnique(new CompositorMutations);

[flackr, 2017/04/03 17:09:19]

With this patch we could limit this workaround to exactly where the problem
occurs, i.e. immediately after calling OnScrollOffsetAnimated, and ensure that
it continues to behave as expected (i.e. DCHECK that m_mutations is always
released).

[smcgruer, 2017/04/03 19:37:11]

Done

+
+  // CompositorMutation objects are moved to updateMutations rather than copied
+  // to reduce the number of allocated objects.
+  for (auto& entry : newMutations.map) {
+    updateMutations(entry.key, std::move(entry.value));
+  }
+
   return shouldReinvoke;
 }
 
 void CompositorMutatorClient::SetClient(cc::LayerTreeMutatorClient* client) {
   TRACE_EVENT0("compositor-worker", "CompositorMutatorClient::SetClient");
   m_client = client;
   setNeedsMutate();
 }
 
 base::Closure CompositorMutatorClient::TakeMutations() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("compositor-worker"),
                "CompositorMutatorClient::TakeMutations");
   if (!m_mutations)
     return base::Closure();
-
   return base::Bind(&CompositorMutationsTarget::applyMutations,
                     base::Unretained(m_mutationsTarget),
                     base::Owned(m_mutations.release()));
 }
 
 void CompositorMutatorClient::setNeedsMutate() {
   TRACE_EVENT0("compositor-worker", "CompositorMutatorClient::setNeedsMutate");
   m_client->SetNeedsMutate();
 }
 
+void CompositorMutatorClient::registerCompositorProxy(
+    uint64_t proxyId,
+    uint64_t elementId,
+    uint32_t mutableProperties) {
+  TRACE_EVENT0("compositor-worker",
+               "CompositorMutatorClient::registerCompositorProxy");
+  m_inputProperties[proxyId] = std::make_pair(elementId, mutableProperties);
+}
+
+void CompositorMutatorClient::unregisterCompositorProxy(uint64_t proxyId) {
+  TRACE_EVENT0("compositor-worker",
+               "CompositorMutatorClient::unregisterCompositorProxy");
+  DCHECK(m_inputProperties.find(proxyId) != m_inputProperties.end());
+  m_inputProperties.erase(proxyId);
+}
+
 void CompositorMutatorClient::setMutationsForTesting(
     std::unique_ptr<CompositorMutations> mutations) {
   m_mutations = std::move(mutations);
 }
 
+void CompositorMutatorClient::snapshotLayerTree(
+    const cc::LayerTreeImpl* treeImpl,
+    ProxyCompositorMutablePropertiesMap* inputMap) const {
+  for (const auto& it : m_inputProperties) {
+    uint64_t proxyId = it.first;
+    const std::pair<uint64_t, uint32_t>& value = it.second;
+    uint64_t elementId = value.first;
+    uint32_t properties = value.second;
+
+    cc::LayerImpl* layer = treeImpl->LayerByElementId(
+        createCompositorElementId(elementId, CompositorSubElementId::Primary));

[flackr, 2017/04/03 17:09:19]

Same comment as above.

[smcgruer, 2017/04/03 19:37:11]

Done, though it's a bit ugly to keep the if (!layer && !scrollLayer) behavior
below.

+    cc::LayerImpl* scrollLayer = treeImpl->LayerByElementId(
+        createCompositorElementId(elementId, CompositorSubElementId::Scroll));
+    if (!layer && !scrollLayer)
+      continue;
+
+    (*inputMap)[proxyId].elementId = elementId;

[flackr, 2017/04/03 17:09:19]

I'm not sure if the repeated calls to (*inputMap)[proxyId] will optimize
correctly. I think you should save and reuse a reference instead.

[smcgruer, 2017/04/03 19:37:11]

Done, I think.

+    (*inputMap)[proxyId].transform = SkMatrix44::I();
+    if (layer) {
+      if (properties & CompositorMutableProperty::kOpacity) {
+        (*inputMap)[proxyId].opacity = layer->Opacity();
+      }
+      if (properties & CompositorMutableProperty::kTransform) {
+        (*inputMap)[proxyId].transform = layer->Transform().matrix();
+      }
+    }
+    if (scrollLayer) {
+      if (properties & CompositorMutableProperty::kScrollLeft) {
+        (*inputMap)[proxyId].scrollLeft =
+            scrollLayer->CurrentScrollOffset().x();
+      }
+      if (properties & CompositorMutableProperty::kScrollTop) {
+        (*inputMap)[proxyId].scrollTop = scrollLayer->CurrentScrollOffset().y();
+      }
+    }
+  }
+}
+
+void CompositorMutatorClient::updateMutations(
+    uint64_t elementId,
+    std::unique_ptr<CompositorMutation> mutation) {
+  if (!m_mutations->map.contains(elementId)) {
+    m_mutations->map.set(elementId, std::move(mutation));
+    return;
+  }
+
+  CompositorMutation* existingMutation = m_mutations->map.at(elementId);
+  if (mutation->isOpacityMutated()) {
+    existingMutation->setOpacity(mutation->opacity());
+  }
+  if (mutation->isTransformMutated()) {
+    existingMutation->setTransform(mutation->transform());
+  }
+  if (mutation->isScrollTopMutated()) {
+    existingMutation->setScrollTop(mutation->scrollTop());
+  }
+  if (mutation->isScrollLeftMutated()) {
+    existingMutation->setScrollLeft(mutation->scrollLeft());
+  }
+}
+
 }  // namespace blink