Rework inheritance for SVG <pattern>

Switch SVGPatternElement to use an observer scheme similar to that used
by SVGGradientElement - i.e using an IdTargetObserver. Implement an
override of FindCycleFromSelf() on LayoutSVGResourcePattern to handle
the cycle-checking for the inheritance link.
LayoutSVGResourcePattern is refactored a little in the process to allow
reusing the attribute collection phase.
This drops the only remaining user of SVGResources::LinkedResource(), so
it and its associated code can be removed as well.

Bug: 1028063
Change-Id: I228f80fb707921ccab695ee8a573d17c0498fcbe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2584307Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#836111}

third_party/blink/renderer/core/layout/svg/layout_svg_resource_pattern.cc

@@ -26,7 +26,6 @@
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/core/layout/svg/svg_layout_support.h"
 #include "third_party/blink/renderer/core/layout/svg/svg_resources.h"
-#include "third_party/blink/renderer/core/layout/svg/svg_resources_cache.h"
 #include "third_party/blink/renderer/core/paint/svg_object_painter.h"
 #include "third_party/blink/renderer/core/svg/svg_fit_to_view_box.h"
 #include "third_party/blink/renderer/core/svg/svg_pattern_element.h"
@@ -57,6 +56,7 @@ void LayoutSVGResourcePattern::RemoveAllClientsFromCache() {
   NOT_DESTROYED();
   pattern_map_->clear();
   should_collect_pattern_attributes_ = true;
+  To<SVGPatternElement>(*GetElement()).InvalidateDependentPatterns();
   MarkAllClientsForInvalidation(SVGResourceClient::kPaintInvalidation);
 }
 
@@ -70,11 +70,7 @@ bool LayoutSVGResourcePattern::RemoveClientFromCache(
   return true;
 }
 
-std::unique_ptr<PatternData> LayoutSVGResourcePattern::BuildPatternData(
-    const FloatRect& object_bounding_box) {
-  NOT_DESTROYED();
-  auto pattern_data = std::make_unique<PatternData>();
-
+const PatternAttributes& LayoutSVGResourcePattern::EnsureAttributes() const {
   DCHECK(GetElement());
   // Validate pattern DOM state before building the actual pattern. This should
   // avoid tearing down the pattern we're currently working on. Preferably the
@@ -82,11 +78,34 @@ std::unique_ptr<PatternData> LayoutSVGResourcePattern::BuildPatternData(
   if (should_collect_pattern_attributes_) {
     attributes_wrapper_->Set(PatternAttributes());
     auto* pattern_element = To<SVGPatternElement>(GetElement());
-    pattern_element->CollectPatternAttributes(MutableAttributes());
+    pattern_element->CollectPatternAttributes(
+        attributes_wrapper_->Attributes());
     should_collect_pattern_attributes_ = false;
   }
+  return Attributes();
+}
+
+bool LayoutSVGResourcePattern::FindCycleFromSelf(
+    SVGResourcesCycleSolver& solver) const {
+  NOT_DESTROYED();
+  const PatternAttributes& attributes = EnsureAttributes();
+  const SVGPatternElement* content_element = attributes.PatternContentElement();
+  if (!content_element)
+    return false;
+  const LayoutObject* content_object = content_element->GetLayoutObject();
+  DCHECK(content_object);
+  return FindCycleInDescendants(solver, *content_object);
+}
+
+std::unique_ptr<PatternData> LayoutSVGResourcePattern::BuildPatternData(
+    const FloatRect& object_bounding_box) {
+  NOT_DESTROYED();
+  auto pattern_data = std::make_unique<PatternData>();
 
-  const PatternAttributes& attributes = Attributes();
+  const PatternAttributes& attributes = EnsureAttributes();
+  // If there's no content disable rendering of the pattern.
+  if (!attributes.PatternContentElement())
+    return pattern_data;
 
   // Spec: When the geometry of the applicable element has no width or height
   // and objectBoundingBox is specified, then the given effect (e.g. a gradient
@@ -96,10 +115,6 @@ std::unique_ptr<PatternData> LayoutSVGResourcePattern::BuildPatternData(
       object_bounding_box.IsEmpty())
     return pattern_data;
 
-  // If there's no content disable rendering of the pattern.
-  if (!attributes.PatternContentElement())
-    return pattern_data;
-
   // Compute tile metrics.
   FloatRect tile_bounds = SVGLengthContext::ResolveRectangle(
       GetElement(), attributes.PatternUnits(), object_bounding_box,
@@ -161,34 +176,6 @@ bool LayoutSVGResourcePattern::ApplyShader(
   return true;
 }
 
-const LayoutSVGResourceContainer*
-LayoutSVGResourcePattern::ResolveContentElement() const {
-  NOT_DESTROYED();
-  DCHECK(Attributes().PatternContentElement());
-  auto* expected_layout_object = To<LayoutSVGResourceContainer>(
-      Attributes().PatternContentElement()->GetLayoutObject());
-  // No content inheritance - avoid walking the inheritance chain.
-  if (this == expected_layout_object)
-    return this;
-  // Walk the inheritance chain on the LayoutObject-side. If we reach the
-  // expected LayoutObject, all is fine. If we don't, there's a cycle that
-  // the cycle resolver did break, and the resource will be content-less.
-  const LayoutSVGResourceContainer* content_layout_object = this;
-  while (SVGResources* resources =
-             SVGResourcesCache::CachedResourcesForLayoutObject(
-                 *content_layout_object)) {
-    LayoutSVGResourceContainer* linked_resource = resources->LinkedResource();
-    if (!linked_resource)
-      break;
-    if (linked_resource == expected_layout_object)
-      return expected_layout_object;
-    content_layout_object = linked_resource;
-  }
-  // There was a cycle, just use this resource as the "content resource" even
-  // though it will be empty (have no children).
-  return this;
-}
-
 sk_sp<PaintRecord> LayoutSVGResourcePattern::AsPaintRecord(
     const FloatSize& size,
     const AffineTransform& tile_transform) const {
@@ -201,8 +188,8 @@ sk_sp<PaintRecord> LayoutSVGResourcePattern::AsPaintRecord(
     content_transform = tile_transform;
 
   FloatRect bounds(FloatPoint(), size);
-  const LayoutSVGResourceContainer* pattern_layout_object =
-      ResolveContentElement();
+  const auto* pattern_layout_object = To<LayoutSVGResourceContainer>(
+      Attributes().PatternContentElement()->GetLayoutObject());
   DCHECK(pattern_layout_object);
   DCHECK(!pattern_layout_object->NeedsLayout());
 

third_party/blink/renderer/core/layout/svg/layout_svg_resource_pattern.h

@@ -59,24 +59,20 @@ class LayoutSVGResourcePattern final : public LayoutSVGResourcePaintServer {
   }
 
  private:
+  bool FindCycleFromSelf(SVGResourcesCycleSolver&) const override;
   std::unique_ptr<PatternData> BuildPatternData(
       const FloatRect& object_bounding_box);
   sk_sp<PaintRecord> AsPaintRecord(const FloatSize&,
                                    const AffineTransform&) const;
 
-  const LayoutSVGResourceContainer* ResolveContentElement() const;
-
-  bool should_collect_pattern_attributes_ : 1;
+  mutable bool should_collect_pattern_attributes_ : 1;
   Persistent<PatternAttributesWrapper> attributes_wrapper_;
 
-  PatternAttributes& MutableAttributes() {
-    NOT_DESTROYED();
-    return attributes_wrapper_->Attributes();
-  }
   const PatternAttributes& Attributes() const {
     NOT_DESTROYED();
     return attributes_wrapper_->Attributes();
   }
+  const PatternAttributes& EnsureAttributes() const;
 
   // FIXME: we can almost do away with this per-object map, but not quite: the
   // tile size can be relative to the client bounding box, and it gets captured

third_party/blink/renderer/core/layout/svg/svg_resources.cc

@@ -36,7 +36,6 @@
 #include "third_party/blink/renderer/core/style/style_svg_resource.h"
 #include "third_party/blink/renderer/core/svg/graphics/filters/svg_filter_builder.h"
 #include "third_party/blink/renderer/core/svg/svg_filter_primitive_standard_attributes.h"
-#include "third_party/blink/renderer/core/svg/svg_pattern_element.h"
 #include "third_party/blink/renderer/core/svg/svg_resource.h"
 #include "third_party/blink/renderer/core/svg/svg_tree_scope_resources.h"
 #include "third_party/blink/renderer/core/svg/svg_uri_reference.h"
@@ -52,7 +51,7 @@
 
 namespace blink {
 
-SVGResources::SVGResources() : linked_resource_(nullptr) {}
+SVGResources::SVGResources() = default;
 
 SVGElementResourceClient* SVGResources::GetClient(const LayoutObject& object) {
   return To<SVGElement>(object.GetNode())->GetSVGResourceClient();
@@ -146,8 +145,7 @@ static HashSet<AtomicString>& FillAndStrokeTags() {
 }
 
 bool SVGResources::HasResourceData() const {
-  return clipper_filter_masker_data_ || marker_data_ || fill_stroke_data_ ||
-         linked_resource_;
+  return clipper_filter_masker_data_ || marker_data_ || fill_stroke_data_;
 }
 
 static inline SVGResources& EnsureResources(
@@ -223,16 +221,6 @@ std::unique_ptr<SVGResources> SVGResources::BuildResources(
     }
   }
 
-  if (auto* pattern = DynamicTo<SVGPatternElement>(element)) {
-    const SVGPatternElement* directly_referenced_pattern =
-        pattern->ReferencedElement();
-    if (directly_referenced_pattern) {
-      EnsureResources(resources).SetLinkedResource(
-          DynamicTo<LayoutSVGResourceContainer>(
-              directly_referenced_pattern->GetLayoutObject()));
-    }
-  }
-
   return (!resources || !resources->HasResourceData()) ? nullptr
                                                        : std::move(resources);
 }
@@ -242,15 +230,6 @@ void SVGResources::ResourceDestroyed(LayoutSVGResourceContainer* resource) {
   if (!HasResourceData())
     return;
 
-  if (linked_resource_ == resource) {
-    DCHECK(!clipper_filter_masker_data_);
-    DCHECK(!marker_data_);
-    DCHECK(!fill_stroke_data_);
-    linked_resource_->RemoveAllClientsFromCache();
-    linked_resource_ = nullptr;
-    return;
-  }
-
   switch (resource->ResourceType()) {
     case kMaskerResourceType:
       if (!clipper_filter_masker_data_)
@@ -297,14 +276,6 @@ void SVGResources::ResourceDestroyed(LayoutSVGResourceContainer* resource) {
 
 void SVGResources::ClearReferencesTo(LayoutSVGResourceContainer* resource) {
   DCHECK(resource);
-  if (linked_resource_ == resource) {
-    DCHECK(!clipper_filter_masker_data_);
-    DCHECK(!marker_data_);
-    DCHECK(!fill_stroke_data_);
-    linked_resource_ = nullptr;
-    return;
-  }
-
   switch (resource->ResourceType()) {
     case kMaskerResourceType:
       DCHECK(clipper_filter_masker_data_);
@@ -352,14 +323,6 @@ void SVGResources::BuildSetOfResources(
   if (!HasResourceData())
     return;
 
-  if (linked_resource_) {
-    DCHECK(!clipper_filter_masker_data_);
-    DCHECK(!marker_data_);
-    DCHECK(!fill_stroke_data_);
-    set.insert(linked_resource_);
-    return;
-  }
-
   if (clipper_filter_masker_data_) {
     if (clipper_filter_masker_data_->clipper)
       set.insert(clipper_filter_masker_data_->clipper);
@@ -464,14 +427,6 @@ void SVGResources::SetStroke(LayoutSVGResourcePaintServer* stroke) {
   fill_stroke_data_->stroke = stroke;
 }
 
-void SVGResources::SetLinkedResource(
-    LayoutSVGResourceContainer* linked_resource) {
-  if (!linked_resource)
-    return;
-
-  linked_resource_ = linked_resource;
-}
-
 #if DCHECK_IS_ON()
 void SVGResources::Dump(const LayoutObject* object) {
   DCHECK(object);
@@ -517,10 +472,6 @@ void SVGResources::Dump(const LayoutObject* object) {
       fprintf(stderr, " |-> Stroke     : %p (node=%p)\n", stroke,
               stroke->GetElement());
   }
-
-  if (linked_resource_)
-    fprintf(stderr, " |-> xlink:href : %p (node=%p)\n", linked_resource_,
-            linked_resource_->GetElement());
 }
 #endif
 

third_party/blink/renderer/core/layout/svg/svg_resources.h

@@ -107,11 +107,6 @@ class SVGResources {
     return fill_stroke_data_ ? fill_stroke_data_->stroke : nullptr;
   }
 
-  // Chainable resources - linked through xlink:href
-  LayoutSVGResourceContainer* LinkedResource() const {
-    return linked_resource_;
-  }
-
   void BuildSetOfResources(HashSet<LayoutSVGResourceContainer*>&);
 
   // Methods operating on all cached resources
@@ -133,7 +128,6 @@ class SVGResources {
   void SetMasker(LayoutSVGResourceMasker*);
   void SetFill(LayoutSVGResourcePaintServer*);
   void SetStroke(LayoutSVGResourcePaintServer*);
-  void SetLinkedResource(LayoutSVGResourceContainer*);
 
   // From SVG 1.1 2nd Edition
   // clipper: 'container elements' and 'graphics elements'
@@ -186,7 +180,6 @@ class SVGResources {
   std::unique_ptr<ClipperFilterMaskerData> clipper_filter_masker_data_;
   std::unique_ptr<MarkerData> marker_data_;
   std::unique_ptr<FillStrokeData> fill_stroke_data_;
-  LayoutSVGResourceContainer* linked_resource_;
 };
 
 class SVGElementResourceClient final

third_party/blink/renderer/core/svg/svg_pattern_element.cc

@@ -24,16 +24,13 @@
 
 #include "third_party/blink/renderer/core/css/style_change_reason.h"
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
+#include "third_party/blink/renderer/core/dom/id_target_observer.h"
 #include "third_party/blink/renderer/core/layout/svg/layout_svg_resource_pattern.h"
-#include "third_party/blink/renderer/core/layout/svg/svg_resources.h"
-#include "third_party/blink/renderer/core/layout/svg/svg_resources_cache.h"
 #include "third_party/blink/renderer/core/svg/pattern_attributes.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_length.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_preserve_aspect_ratio.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_rect.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_transform_list.h"
-#include "third_party/blink/renderer/core/svg/svg_resource.h"
-#include "third_party/blink/renderer/core/svg/svg_tree_scope_resources.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/transforms/affine_transform.h"
 
@@ -95,7 +92,7 @@ void SVGPatternElement::Trace(Visitor* visitor) const {
   visitor->Trace(pattern_transform_);
   visitor->Trace(pattern_units_);
   visitor->Trace(pattern_content_units_);
-  visitor->Trace(resource_);
+  visitor->Trace(target_id_observer_);
   SVGElement::Trace(visitor);
   SVGURIReference::Trace(visitor);
   SVGTests::Trace(visitor);
@@ -106,28 +103,16 @@ void SVGPatternElement::BuildPendingResource() {
   ClearResourceReferences();
   if (!isConnected())
     return;
-  TreeScope& tree_scope = GetTreeScope();
-  SVGTreeScopeResources& tree_scope_resources =
-      tree_scope.EnsureSVGTreeScopedResources();
-  resource_ = tree_scope_resources.ResourceForId(
-      FragmentIdentifierFromIRIString(HrefString(), tree_scope));
-  if (resource_)
-    resource_->AddClient(EnsureSVGResourceClient());
+  Element* target = ObserveTarget(target_id_observer_, *this);
+  if (auto* pattern = DynamicTo<SVGPatternElement>(target))
+    AddReferenceTo(pattern);
 
   InvalidatePattern(layout_invalidation_reason::kSvgResourceInvalidated);
-  if (auto* layout_object = GetLayoutObject()) {
-    if (!layout_object->Parent())
-      return;
-    SVGResourcesCache::UpdateResources(*layout_object);
-    InvalidateDependentPatterns();
-  }
 }
 
 void SVGPatternElement::ClearResourceReferences() {
-  if (!resource_)
-    return;
-  resource_->RemoveClient(*GetSVGResourceClient());
-  resource_ = nullptr;
+  UnobserveTarget(target_id_observer_);
+  RemoveAllOutgoingReferences();
 }
 
 void SVGPatternElement::CollectStyleForPresentationAttribute(
@@ -265,8 +250,8 @@ static void SetPatternAttributes(const SVGPatternElement& element,
 }
 
 const SVGPatternElement* SVGPatternElement::ReferencedElement() const {
-  return DynamicTo<SVGPatternElement>(resource_ ? resource_->Target()
-                                                : nullptr);
+  return DynamicTo<SVGPatternElement>(
+      TargetElementFromIRIString(HrefString(), GetTreeScope()));
 }
 
 void SVGPatternElement::CollectPatternAttributes(

third_party/blink/renderer/core/svg/svg_pattern_element.h

@@ -35,7 +35,6 @@ namespace blink {
 class PatternAttributes;
 class SVGAnimatedLength;
 class SVGAnimatedTransformList;
-class SVGResource;
 
 class SVGPatternElement final : public SVGElement,
                                 public SVGURIReference,
@@ -110,7 +109,7 @@ class SVGPatternElement final : public SVGElement,
   Member<SVGAnimatedEnumeration<SVGUnitTypes::SVGUnitType>> pattern_units_;
   Member<SVGAnimatedEnumeration<SVGUnitTypes::SVGUnitType>>
       pattern_content_units_;
-  Member<SVGResource> resource_;
+  Member<IdTargetObserver> target_id_observer_;
 };
 
 }  // namespace blink