[compositorworker] Register compositor proxies with proxy client

third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp

Index: third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp
diff --git a/third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp b/third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp
index 785f096b53c210fa47430bbbf0c614d36fad27c0..883ea21352636a176304ed08ace173512285acfb 100644
--- a/third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp
+++ b/third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp
@@ -23,6 +23,7 @@ DEFINE_TRACE(CompositorProxyClientImpl)
     CompositorProxyClient::trace(visitor);
     visitor->trace(m_mutator);
     visitor->trace(m_globalScope);
+    visitor->trace(m_proxyMap);
 }
 
 void CompositorProxyClientImpl::setGlobalScope(WorkerGlobalScope* scope)
@@ -64,4 +65,23 @@ bool CompositorProxyClientImpl::executeAnimationFrameCallbacks(double monotonicT
     return shouldReinvoke;
 }
 
+void CompositorProxyClientImpl::registerCompositorProxy(CompositorProxy* proxy)
+{
+    uint64_t elementId = proxy->elementId();
+    ProxyMap::AddResult entry = m_proxyMap.add(elementId, new ProxySet);

[jbroman, 2016/06/10 17:45:42]

This will create a spurious "new ProxySet" every time, which you don't want to
do. Instead, only allocate it if there wasn't one previously. Similarly, the
"contains" check is redundant: it's legal (and faster) to unconditionally add it
(the set itself will not contain duplicate entries, and will only scan the
hashtable once this way).

ProxyMap::AddResult entry = m_proxyMap.add(proxy->elementId(), nullptr);
Member<ProxySet>& proxies = entry.storedValue->value;
if (entry.isNewEntry)
    proxies = new ProxySet;
proxies->add(proxy);

[majidvp, 2016/06/10 19:17:51]

Great! I like this pattern of using isNewEntry. Thanks.

+    ProxySet* proxies = entry.storedValue->value;
+    if (!proxies->contains(proxy))
+        proxies->add(proxy);
+}
+
+void CompositorProxyClientImpl::unregisterCompositorProxy(CompositorProxy* proxy)
+{
+    uint64_t elementId = proxy->elementId();
+    if (ProxySet* proxies = m_proxyMap.get(elementId)) {
+        proxies->remove(proxy);
+        if (proxies->isEmpty())
+            m_proxyMap.remove(elementId);

[jbroman, 2016/06/10 17:45:42]

This case is probably less worrisome, but this is still one more hashtable scan
than necessary. You might consider this pattern instead (slightly more
complicated, but only one line longer):

ProxyMap::iterator it = m_proxyMap.find(proxy->elementId());
if (it == m_proxyMap.end())
    return;

ProxySet* proxies = it->value;
proxies->remove(proxy);
if (proxies->isEmpty())
    m_proxyMap.remove(it);

...but I'm less worried about this than the extra allocation above.

[majidvp, 2016/06/10 19:17:51]

Done.

+    }
+}
+
 } // namespace blink