Remove LayoutSVGViewportContainer's calcViewport and pointIsInsideViewportClip

third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp

Index: third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp b/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
index a7438c2f6a0eb5a13542a7eaae594d6b26b611df..0c047da7f9a2f87b5165907a967bf15c619a2e10 100644
--- a/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
@@ -44,70 +44,56 @@ void LayoutSVGViewportContainer::determineIfLayoutSizeChanged() {
       toSVGSVGElement(element())->hasRelativeLengths() && selfNeedsLayout();
 }
 
-void LayoutSVGViewportContainer::calcViewport() {
+void LayoutSVGViewportContainer::setNeedsTransformUpdate() {
+  setMayNeedPaintInvalidationSubtree();
+  if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
+    // The transform paint property relies on the SVG transform being up-to-date
+    // (see: PaintPropertyTreeBuilder::updateTransformForNonRootSVG).
+    setNeedsPaintPropertyUpdate();
+  }
+  m_needsTransformUpdate = true;
+}
+
+SVGTransformChange LayoutSVGViewportContainer::calculateLocalTransform() {
   SVGElement* element = this->element();
-  ASSERT(element);
-  if (!isSVGSVGElement(*element))
-    return;
+  CHECK(element && isSVGSVGElement(*element));

[fs, 2016/12/13 10:11:23]

This looks a bit excessive. DCHECK(isSVGSVGElement(element))? (Or element ->
element() even...)

My spider senses tells me there's legacy abound here... We could probably clean
this up more to make it more obvious only SVGSVGElements instantiate these
(codesearch seems to support that...) Oh well, that'll be another CL...

[pdr., 2016/12/13 19:48:55]

Good idea, done.

I agree that this can be cleaned up although it's not immediately clear how. One
slightly strange case here is that SVGSVGElement is the only class to create
viewport containers, but SVGSVGElement only does this for non-root SVG elements.
Maybe something along the names of LayoutNonRootSVGContainer?

Another area of cleanup is removing the call to calculateLocalTransform in favor
of overriding layout.

[fs, 2016/12/14 12:24:45]

I think what I'd like to clean up first is to remove anything that suggests that
this is a "multi-element" LayoutObject - i.e remove any isSVGSVGElement(...) and
just nail down that assumption. The 1:N from the SVGSVGElement I worry less
about.
 
This I've pondered too now and then, but not really seen anything that suggest
it to be worth it.

   SVGSVGElement* svg = toSVGSVGElement(element);
-  FloatRect oldViewport = m_viewport;
 
+  FloatRect oldViewport = m_viewport;
   SVGLengthContext lengthContext(element);
   m_viewport = FloatRect(svg->x()->currentValue()->value(lengthContext),
                          svg->y()->currentValue()->value(lengthContext),
                          svg->width()->currentValue()->value(lengthContext),
                          svg->height()->currentValue()->value(lengthContext));
 
-  if (oldViewport != m_viewport) {
+  // TODO(pdr): Mark this object as needing updates when the viewport values
+  // change instead of waiting for layout.
+  if (m_viewport != oldViewport) {
     setNeedsBoundariesUpdate();
     setNeedsTransformUpdate();
   }
-}
 
-void LayoutSVGViewportContainer::setNeedsTransformUpdate() {
-  setMayNeedPaintInvalidationSubtree();
-  if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
-    // The transform paint property relies on the SVG transform being up-to-date
-    // (see: PaintPropertyTreeBuilder::updateTransformForNonRootSVG).
-    setNeedsPaintPropertyUpdate();
-  }
-  m_needsTransformUpdate = true;
-}
-
-SVGTransformChange LayoutSVGViewportContainer::calculateLocalTransform() {
   if (!m_needsTransformUpdate)
     return SVGTransformChange::None;
 
   SVGTransformChangeDetector changeDetector(m_localToParentTransform);
   m_localToParentTransform =
       AffineTransform::translation(m_viewport.x(), m_viewport.y()) *
-      viewportTransform();
+      svg->viewBoxToViewTransform(m_viewport.width(), m_viewport.height());
   m_needsTransformUpdate = false;
   return changeDetector.computeChange(m_localToParentTransform);
 }
 
-AffineTransform LayoutSVGViewportContainer::viewportTransform() const {
-  ASSERT(element());
-  if (isSVGSVGElement(*element())) {
-    SVGSVGElement* svg = toSVGSVGElement(element());
-    return svg->viewBoxToViewTransform(m_viewport.width(), m_viewport.height());
+bool LayoutSVGViewportContainer::nodeAtFloatPoint(
+    HitTestResult& result,
+    const FloatPoint& pointInParent,
+    HitTestAction action) {
+  // Respect the viewport clip which is in parent coordinates.
+  if (SVGLayoutSupport::isOverflowHidden(this)) {
+    if (!m_viewport.contains(pointInParent))
+      return false;
   }
-
-  return AffineTransform();
-}
-
-bool LayoutSVGViewportContainer::pointIsInsideViewportClip(
-    const FloatPoint& pointInParent) {
-  // Respect the viewport clip (which is in parent coords)
-  if (!SVGLayoutSupport::isOverflowHidden(this))
-    return true;
-
-  return m_viewport.contains(pointInParent);
-}
-
-void LayoutSVGViewportContainer::paint(const PaintInfo& paintInfo,
-                                       const LayoutPoint& paintOffset) const {
-  SVGContainerPainter(*this).paint(paintInfo);
+  return LayoutSVGContainer::nodeAtFloatPoint(result, pointInParent, action);
 }
 
 }  // namespace blink