Improve handling of duplicate id's for SVG resources

third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp

Index: third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp b/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
index 2dddc1bee94d4365a630291d6d36bc96f5516dd7..adc9fe6908f8b9db06a5f46b64bf55a87c60eb6e 100644
--- a/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
@@ -13,7 +13,8 @@
 
 namespace blink {
 
-SVGTreeScopeResources::SVGTreeScopeResources(TreeScope* treeScope) {
+SVGTreeScopeResources::SVGTreeScopeResources(TreeScope* treeScope)
+    : m_treeScope(treeScope) {
   // Whenever an object of SVGTreeScopeResources is created, to keep the code
   // behave as before,
   // the document should also have an instance of SVGDocumentExtensions created.
@@ -23,41 +24,97 @@ SVGTreeScopeResources::SVGTreeScopeResources(TreeScope* treeScope) {
 
 SVGTreeScopeResources::~SVGTreeScopeResources() = default;
 
+static LayoutSVGResourceContainer* lookupResource(TreeScope& treeScope,
+                                                  const AtomicString& id) {
+  Element* element = treeScope.getElementById(id);
+  if (!element)
+    return nullptr;
+  LayoutObject* layoutObject = element->layoutObject();
+  if (!layoutObject || !layoutObject->isSVGResourceContainer())
+    return nullptr;
+  return toLayoutSVGResourceContainer(layoutObject);
+}
+
 void SVGTreeScopeResources::updateResource(
     const AtomicString& id,
     LayoutSVGResourceContainer* resource) {
   DCHECK(resource);
-  if (id.isEmpty())
+  if (resource->isRegistered() || id.isEmpty())

[Stephen Chennney, 2017/02/27 16:21:22]

This check is the only use of "resource" and I find it confusing to pass it into
this method just for that purpose. As I understand it, the passed resource is
the old resource for that id and if it's registered then it's still the right
one to use so we shouldn't update. Is that right?

Could this if be moved to the caller? Or maybe rename either the method or the
argument or both to make it clear what's going on? e.g. updateResourceIfNeeded?

[fs, 2017/02/27 17:47:31]

It may or may not be the old resource. What we know is that it's _a_resource_
with the passed 'id'. As stated in the description, the flag-check is an
optimization.
It could. The reason for having it here is to have all the flag mongering in one
place, and make LayoutSVGResourceContainer not care about it at all. In the
future "registering" would be made implicit and the flag dropped entirely.

     return;
-  // Replaces resource if already present, to handle potential id changes.
-  m_resources.set(id, resource);
+  // Lookup the current resource. (Could differ from what's in the map if an
+  // element was just added/removed.)
+  LayoutSVGResourceContainer* currentResource =
+      lookupResource(*m_treeScope, id);
+  // Lookup the currently registered resource.
+  auto it = m_resources.find(id);
+  if (it != m_resources.end()) {
+    // Is the local map up-to-date already?
+    if (it->value == currentResource)
+      return;
+    unregisterResource(it);
+  }
+  if (currentResource)
+    registerResource(id, currentResource);
+}
 
-  SVGPendingElements* pendingElements = m_pendingResources.take(id);
-  if (!pendingElements)
+void SVGTreeScopeResources::updateResource(
+    const AtomicString& oldId,
+    const AtomicString& newId,
+    LayoutSVGResourceContainer* resource) {
+  removeResource(oldId, resource);
+  updateResource(newId, resource);
+}
+
+void SVGTreeScopeResources::removeResource(
+    const AtomicString& id,
+    LayoutSVGResourceContainer* resource) {
+  DCHECK(resource);
+  if (!resource->isRegistered() || id.isEmpty())
     return;
-  // Update cached resources of pending clients.
-  for (Element* clientElement : *pendingElements) {
-    DCHECK(clientElement->hasPendingResources());
-    clearHasPendingResourcesIfPossible(*clientElement);
+  auto it = m_resources.find(id);
+  // If this is not the currently registered resource for this id, then do
+  // nothing.
+  if (it == m_resources.end() || it->value != resource)
+    return;
+  unregisterResource(it);
+  // If the layout tree is being torn down, then don't attempt to update the
+  // map, since that layout object is likely to be stale already.
+  if (resource->documentBeingDestroyed())
+    return;
+  // Another resource could now be current. Perform a lookup and potentially
+  // update the map.
+  LayoutSVGResourceContainer* currentResource =
+      lookupResource(*m_treeScope, id);
+  if (!currentResource)
+    return;
+  // Since this is a removal, don't allow re-adding the resource.
+  if (currentResource == resource)
+    return;
+  registerResource(id, currentResource);
+}
 
-    LayoutObject* layoutObject = clientElement->layoutObject();
-    if (!layoutObject)
-      continue;
-    DCHECK(layoutObject->isSVG());
+void SVGTreeScopeResources::registerResource(
+    const AtomicString& id,
+    LayoutSVGResourceContainer* resource) {
+  DCHECK(!id.isEmpty());
+  DCHECK(resource);
+  DCHECK(!resource->isRegistered());
 
-    StyleDifference diff;
-    diff.setNeedsFullLayout();
-    SVGResourcesCache::clientStyleChanged(layoutObject, diff,
-                                          layoutObject->styleRef());
-    layoutObject->setNeedsLayoutAndFullPaintInvalidation(
-        LayoutInvalidationReason::SvgResourceInvalidated);
-  }
+  m_resources.set(id, resource);
+  resource->setRegistered(true);
+
+  notifyPendingClients(id);
 }
 
-void SVGTreeScopeResources::removeResource(const AtomicString& id) {
-  if (id.isEmpty())
-    return;
-  m_resources.erase(id);
+void SVGTreeScopeResources::unregisterResource(ResourceMap::iterator it) {
+  LayoutSVGResourceContainer* resource = it->value;
+  DCHECK(resource);
+  DCHECK(resource->isRegistered());
+
+  resource->detachAllClients(it->key);
+
+  resource->setRegistered(false);
+  m_resources.remove(it);
 }
 
 LayoutSVGResourceContainer* SVGTreeScopeResources::resourceById(
@@ -125,6 +182,30 @@ void SVGTreeScopeResources::removeElementFromPendingResources(
   clearHasPendingResourcesIfPossible(element);
 }
 
+void SVGTreeScopeResources::notifyPendingClients(const AtomicString& id) {
+  DCHECK(!id.isEmpty());
+  SVGPendingElements* pendingElements = m_pendingResources.take(id);
+  if (!pendingElements)
+    return;
+  // Update cached resources of pending clients.
+  for (Element* clientElement : *pendingElements) {
+    DCHECK(clientElement->hasPendingResources());
+    clearHasPendingResourcesIfPossible(*clientElement);
+
+    LayoutObject* layoutObject = clientElement->layoutObject();
+    if (!layoutObject)
+      continue;
+    DCHECK(layoutObject->isSVG());
+
+    StyleDifference diff;
+    diff.setNeedsFullLayout();
+    SVGResourcesCache::clientStyleChanged(layoutObject, diff,
+                                          layoutObject->styleRef());
+    layoutObject->setNeedsLayoutAndFullPaintInvalidation(
+        LayoutInvalidationReason::SvgResourceInvalidated);
+  }
+}
+
 void SVGTreeScopeResources::notifyResourceAvailable(const AtomicString& id) {
   if (id.isEmpty())
     return;
@@ -152,5 +233,6 @@ void SVGTreeScopeResources::notifyResourceAvailable(const AtomicString& id) {
 
 DEFINE_TRACE(SVGTreeScopeResources) {
   visitor->trace(m_pendingResources);
+  visitor->trace(m_treeScope);
 }
 }