Avoid exposing cc::Layer tree to CompositorProxy

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

Index: third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp b/third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
index f0344194d387cffe88dfb9a80c86baf90bcde12d..61a04066ca368fec956f115954ae8d763c1eec9c 100644
--- a/third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
+++ b/third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
@@ -4,18 +4,57 @@
 
 #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,
@@ -39,8 +78,32 @@ bool CompositorMutatorClient::Mutate(base::TimeTicks monotonicTime,
   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;
 }
 
@@ -55,7 +118,6 @@ base::Closure CompositorMutatorClient::TakeMutations() {
                "CompositorMutatorClient::TakeMutations");
   if (!m_mutations)
     return base::Closure();
-
   return base::Bind(&CompositorMutationsTarget::applyMutations,
                     base::Unretained(m_mutationsTarget),
                     base::Owned(m_mutations.release()));
@@ -66,9 +128,86 @@ void 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