Refactor transform update in LayoutSVGBlock subclasses

The transform is already stored in LayoutSVGBlock, so move the
|needs_transform_update_| flag there as well as the setter. Move the
actual update of the transform to a new helper method on LayoutSVGBlock
named UpdateTransformAfterLayout(). This should make it easier to fix
the referenced bug - which affects both of the subclasses of
LayoutSVGBlock.

Add a TODO about the early transform update in LayoutSVGForeignObject.

Bug: 1094020
Change-Id: Iaa351e06aaafa9dddba868d36ea7841c4d4653d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2247788Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#779221}

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

@@ -34,7 +34,7 @@
 namespace blink {
 
 LayoutSVGBlock::LayoutSVGBlock(SVGElement* element)
-    : LayoutBlockFlow(element) {}
+    : LayoutBlockFlow(element), needs_transform_update_(true) {}
 
 SVGElement* LayoutSVGBlock::GetElement() const {
   return To<SVGElement>(LayoutObject::GetNode());
@@ -51,6 +51,15 @@ void LayoutSVGBlock::UpdateFromStyle() {
   SetFloating(false);
 }
 
+bool LayoutSVGBlock::UpdateTransformAfterLayout() {
+  if (!needs_transform_update_)
+    return false;
+  local_transform_ =
+      GetElement()->CalculateTransform(SVGElement::kIncludeMotionTransform);
+  needs_transform_update_ = false;
+  return true;
+}
+
 void LayoutSVGBlock::StyleDidChange(StyleDifference diff,
                                     const ComputedStyle* old_style) {
   // Since layout depends on the bounds of the filter, we need to force layout

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

@@ -50,6 +50,7 @@ class LayoutSVGBlock : public LayoutBlockFlow {
       LayoutGeometryMap&) const final;
 
   AffineTransform LocalSVGTransform() const final { return local_transform_; }
+  void SetNeedsTransformUpdate() override { needs_transform_update_ = true; }
 
   PaintLayerType LayerTypeRequired() const override { return kNoPaintLayer; }
 
@@ -63,11 +64,13 @@ class LayoutSVGBlock : public LayoutBlockFlow {
       VisualRectFlags = kDefaultVisualRectFlags) const final;
 
   AffineTransform local_transform_;
+  bool needs_transform_update_ : 1;
 
   bool IsOfType(LayoutObjectType type) const override {
     return type == kLayoutObjectSVG || LayoutBlockFlow::IsOfType(type);
   }
 
+  bool UpdateTransformAfterLayout();
   void StyleDidChange(StyleDifference, const ComputedStyle* old_style) override;
 
  private:

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

@@ -32,7 +32,7 @@
 namespace blink {
 
 LayoutSVGForeignObject::LayoutSVGForeignObject(SVGForeignObjectElement* node)
-    : LayoutSVGBlock(node), needs_transform_update_(true) {}
+    : LayoutSVGBlock(node) {}
 
 LayoutSVGForeignObject::~LayoutSVGForeignObject() = default;
 
@@ -92,12 +92,15 @@ void LayoutSVGForeignObject::UpdateLayout() {
 
   auto* foreign = To<SVGForeignObjectElement>(GetElement());
 
-  bool update_cached_boundaries_in_parents = false;
+  // Update our transform before layout, in case any of our descendants rely on
+  // the transform being somewhat accurate.  The |needs_transform_update_| flag
+  // will be cleared after layout has been performed.
+  // TODO(fs): Remove this. AFAICS in all cases where we ancestors compute some
+  // form of CTM, they stop at their nearest ancestor LayoutSVGRoot, and thus
+  // will not care about this value.
   if (needs_transform_update_) {
     local_transform_ =
         foreign->CalculateTransform(SVGElement::kIncludeMotionTransform);
-    needs_transform_update_ = false;
-    update_cached_boundaries_in_parents = true;
   }
 
   LayoutRect old_viewport = FrameRect();
@@ -110,19 +113,24 @@ void LayoutSVGForeignObject::UpdateLayout() {
   SetX(ElementX());
   SetY(ElementY());
 
-  bool layout_changed = EverHadLayout() && SelfNeedsLayout();
+  const bool layout_changed = EverHadLayout() && SelfNeedsLayout();
   LayoutBlock::UpdateLayout();
   DCHECK(!NeedsLayout());
+  const bool bounds_changed = old_viewport != FrameRect();
 
-  // If our bounds changed, notify the parents.
-  if (!update_cached_boundaries_in_parents)
-    update_cached_boundaries_in_parents = old_viewport != FrameRect();
-  if (update_cached_boundaries_in_parents)
+  bool update_parent_boundaries = bounds_changed;
+  if (UpdateTransformAfterLayout())
+    update_parent_boundaries = true;
+
+  // Notify ancestor about our bounds changing.
+  if (update_parent_boundaries)
     LayoutSVGBlock::SetNeedsBoundariesUpdate();
 
   // Invalidate all resources of this client if our layout changed.
   if (layout_changed)
     SVGResourcesCache::ClientLayoutChanged(*this);
+
+  DCHECK(!needs_transform_update_);
 }
 
 bool LayoutSVGForeignObject::NodeAtPointFromSVG(

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

@@ -81,8 +81,6 @@ class LayoutSVGForeignObject final : public LayoutSVGBlock {
            LayoutSVGBlock::IsOfType(type);
   }
 
-  void SetNeedsTransformUpdate() override { needs_transform_update_ = true; }
-
   PaintLayerType LayerTypeRequired() const override;
 
   bool CreatesNewFormattingContext() const final {
@@ -101,8 +99,6 @@ class LayoutSVGForeignObject final : public LayoutSVGBlock {
                             LayoutUnit logical_top,
                             LogicalExtentComputedValues&) const override;
   void StyleDidChange(StyleDifference, const ComputedStyle* old_style) override;
-
-  bool needs_transform_update_;
 };
 
 template <>

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

@@ -67,7 +67,6 @@ LayoutSVGText::LayoutSVGText(SVGTextElement* node)
     : LayoutSVGBlock(node),
       needs_reordering_(false),
       needs_positioning_values_update_(false),
-      needs_transform_update_(true),
       needs_text_metrics_update_(false) {}
 
 LayoutSVGText::~LayoutSVGText() {
@@ -248,19 +247,19 @@ void LayoutSVGText::UpdateLayout() {
 
   needs_reordering_ = false;
 
-  FloatRect new_boundaries = ObjectBoundingBox();
-  bool bounds_changed = old_boundaries != new_boundaries;
+  const bool bounds_changed = old_boundaries != ObjectBoundingBox();
 
   // Update the transform after laying out. Update if the bounds
   // changed too, since the transform could depend on the bounding
   // box.
-  if (bounds_changed || needs_transform_update_) {
-    local_transform_ =
-        GetElement()->CalculateTransform(SVGElement::kIncludeMotionTransform);
-    needs_transform_update_ = false;
+  if (bounds_changed) {
+    needs_transform_update_ = true;
     update_parent_boundaries = true;
   }
 
+  if (UpdateTransformAfterLayout())
+    update_parent_boundaries = true;
+
   ClearLayoutOverflow();
 
   // Invalidate all resources of this client if our layout changed.

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

@@ -39,7 +39,6 @@ class LayoutSVGText final : public LayoutSVGBlock {
   void SetNeedsPositioningValuesUpdate() {
     needs_positioning_values_update_ = true;
   }
-  void SetNeedsTransformUpdate() override { needs_transform_update_ = true; }
   void SetNeedsTextMetricsUpdate() { needs_text_metrics_update_ = true; }
   FloatRect VisualRectInLocalSVGCoordinates() const override;
   FloatRect ObjectBoundingBox() const override;
@@ -97,7 +96,6 @@ class LayoutSVGText final : public LayoutSVGBlock {
 
   bool needs_reordering_ : 1;
   bool needs_positioning_values_update_ : 1;
-  bool needs_transform_update_ : 1;
   bool needs_text_metrics_update_ : 1;
   Vector<LayoutSVGInlineText*> descendant_text_nodes_;
 };