Rename and change parameter type of some GeometryMapper methods

third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "platform/graphics/paint/GeometryMapper.h"
 
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/geometry/LayoutRect.h"
 
 namespace blink {
 
-FloatRect GeometryMapper::mapToVisualRectInDestinationSpace(
-    const FloatRect& rect,
-    const PropertyTreeState& sourceState,
-    const PropertyTreeState& destinationState,
-    bool& success) {
-  FloatRect result = localToVisualRectInAncestorSpace(
-      rect, sourceState, destinationState, success);
-  if (success)
-    return result;
-  return slowMapToVisualRectInDestinationSpace(rect, sourceState,
-                                               destinationState, success);
-}
-
-FloatRect GeometryMapper::mapRectToDestinationSpace(
+FloatRect GeometryMapper::sourceToDestinationVisualRect(
     const FloatRect& rect,
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState,
     bool& success) {
   FloatRect result =
-      localToAncestorRect(rect, sourceState, destinationState, success);
+      localToAncestorVisualRect(rect, sourceState, destinationState, success);
   if (success)
     return result;
-  return slowMapRectToDestinationSpace(rect, sourceState, destinationState,
-                                       success);
+  return slowSourceToDestinationVisualRect(rect, sourceState, destinationState,
+                                           success);
 }
 
-FloatRect GeometryMapper::slowMapToVisualRectInDestinationSpace(
+FloatRect GeometryMapper::sourceToDestinationRect(
+    const FloatRect& rect,
+    const TransformPaintPropertyNode* sourceTransformNode,
+    const TransformPaintPropertyNode* destinationTransformNode,
+    bool& success) {
+  FloatRect result = localToAncestorRect(rect, sourceTransformNode,
+                                         destinationTransformNode, success);
+  if (success)
+    return result;
+  return slowSourceToDestinationRect(rect, sourceTransformNode,
+                                     destinationTransformNode, success);
+}
+
+FloatRect GeometryMapper::slowSourceToDestinationVisualRect(
     const FloatRect& rect,
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState,
     bool& success) {
   const TransformPaintPropertyNode* lcaTransform = leastCommonAncestor(
       sourceState.transform(), destinationState.transform());
   DCHECK(lcaTransform);
 
   // Assume that the clip of destinationState is an ancestor of the clip of
   // sourceState and is under the space of lcaTransform. Otherwise
   // localToAncestorClipRect() will fail.
   PropertyTreeState lcaState = destinationState;
   lcaState.setTransform(lcaTransform);
 
-  const auto clipRect = localToAncestorClipRect(sourceState, lcaState, success);
+  FloatRect result =
+      localToAncestorVisualRect(rect, sourceState, lcaState, success);
   if (!success)
-    return rect;
+    return result;
 
-  FloatRect result = localToAncestorRect(rect, sourceState, lcaState, success);

[chrishtr, 2017/01/11 23:23:09]

Why remove this?

[Xianzhu, 2017/01/11 23:54:43]

Line 53-59 now becomes a call to localToAncestorVisualRect() which is almost
equivalent to the original code. One difference is about the special handling of
'success' in localToAncestorVisualRect() which is actually also needed here for
spv1.

-  DCHECK(success);
-  result.intersect(clipRect);
-
-  return ancestorToLocalRect(result, destinationState, lcaState, success);
+  return ancestorToLocalRect(result, lcaTransform, destinationState.transform(),
+                             success);
 }
 
-FloatRect GeometryMapper::slowMapRectToDestinationSpace(
+FloatRect GeometryMapper::slowSourceToDestinationRect(
     const FloatRect& rect,
-    const PropertyTreeState& sourceState,
-    const PropertyTreeState& destinationState,
+    const TransformPaintPropertyNode* sourceTransformNode,
+    const TransformPaintPropertyNode* destinationTransformNode,
     bool& success) {
-  const TransformPaintPropertyNode* lcaTransform = leastCommonAncestor(
-      sourceState.transform(), destinationState.transform());
+  const TransformPaintPropertyNode* lcaTransform =
+      leastCommonAncestor(sourceTransformNode, destinationTransformNode);
   DCHECK(lcaTransform);
-  PropertyTreeState lcaState = sourceState;
-  lcaState.setTransform(lcaTransform);
 
-  FloatRect result = localToAncestorRect(rect, sourceState, lcaState, success);
+  FloatRect result =
+      localToAncestorRect(rect, sourceTransformNode, lcaTransform, success);
   DCHECK(success);
 
-  return ancestorToLocalRect(result, destinationState, lcaState, success);
+  return ancestorToLocalRect(result, lcaTransform, destinationTransformNode,
+                             success);
 }
 
-FloatRect GeometryMapper::localToVisualRectInAncestorSpace(
+FloatRect GeometryMapper::localToAncestorVisualRect(
     const FloatRect& rect,
     const PropertyTreeState& localState,
     const PropertyTreeState& ancestorState,
     bool& success) {
   if (localState == ancestorState) {
     success = true;
     return rect;
   }
 
-  const auto& transformMatrix =
-      localToAncestorMatrix(localState.transform(), ancestorState, success);
+  const auto& transformMatrix = localToAncestorMatrix(
+      localState.transform(), ancestorState.transform(), success);
   if (!success)
     return rect;
 
   FloatRect mappedRect = transformMatrix.mapRect(rect);
 
   const auto clipRect =
       localToAncestorClipRect(localState, ancestorState, success);
 
   if (success) {
     mappedRect.intersect(clipRect);
   } else if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
     // On SPv1 we may fail when the paint invalidation container creates an
     // overflow clip (in ancestorState) which is not in localState of an
     // out-of-flow positioned descendant. See crbug.com/513108 and layout test
     // compositing/overflow/handle-non-ancestor-clip-parent.html (run with
     // --enable-prefer-compositing-to-lcd-text) for details.
     // Ignore it for SPv1 for now.
     success = true;
-  } else {

[chrishtr, 2017/01/11 23:23:09]

Why remove this?

[Xianzhu, 2017/01/11 23:54:43]

Now slowSourceToDestinationVisualRect (previously
slowMapToVisualRectInDestinationSpace) calls this function. Some unit tests
(e.g. GeometryMapperTest, SiblingTransformsWithClip) expect success==false
instead of DCHECK failure here.

[chrishtr, 2017/01/12 18:07:12]

The DCHECK is to ensure that it's actually an ancestor. Are the unittests at
fault here? Is the assumption that there is no use case
for failure in SPv2 incorrect?

[Xianzhu, 2017/01/12 18:39:17]

No DCHECK(success) here is mainly a requirement of
slowSourceToDestinationVisualRect() because ancestorState here contains
lcaTransform and destinationState.clip(), and destinationState.clip() may not be
an ancestor of localState.clip().

Is the |success| parameter just for testing and nested calls from other
GeometryMapper methods? If yes, can we add several internal methods having the
parameter, and remove the parameter from public methods which will
DCHECK(success) in them?

-    DCHECK(success);
   }
 
   return mappedRect;
 }
 
 FloatRect GeometryMapper::localToAncestorRect(
     const FloatRect& rect,
-    const PropertyTreeState& localState,
-    const PropertyTreeState& ancestorState,
+    const TransformPaintPropertyNode* localTransformNode,
+    const TransformPaintPropertyNode* ancestorTransformNode,
     bool& success) {
-  if (localState.transform() == ancestorState.transform()) {
+  if (localTransformNode == ancestorTransformNode) {
     success = true;
     return rect;
   }
 
   const auto& transformMatrix =
-      localToAncestorMatrix(localState.transform(), ancestorState, success);
+      localToAncestorMatrix(localTransformNode, ancestorTransformNode, success);
   if (!success)
     return rect;
   return transformMatrix.mapRect(rect);
 }
 
 FloatRect GeometryMapper::ancestorToLocalRect(
     const FloatRect& rect,
-    const PropertyTreeState& localState,
-    const PropertyTreeState& ancestorState,
+    const TransformPaintPropertyNode* ancestorTransformNode,
+    const TransformPaintPropertyNode* localTransformNode,
     bool& success) {
-  if (localState.transform() == ancestorState.transform()) {
+  if (localTransformNode == ancestorTransformNode) {
     success = true;
     return rect;
   }
 
   const auto& transformMatrix =
-      localToAncestorMatrix(localState.transform(), ancestorState, success);
+      localToAncestorMatrix(localTransformNode, ancestorTransformNode, success);
   if (!success)
     return rect;
 
   if (!transformMatrix.isInvertible()) {
     success = false;
     return rect;
   }
   success = true;
 
   // TODO(chrishtr): Cache the inverse?
   return transformMatrix.inverse().mapRect(rect);
 }
 
 PrecomputedDataForAncestor& GeometryMapper::getPrecomputedDataForAncestor(
-    const PropertyTreeState& ancestorState) {
-  auto addResult = m_data.add(ancestorState.transform(), nullptr);
+    const TransformPaintPropertyNode* ancestorTransformNode) {
+  auto addResult = m_data.add(ancestorTransformNode, nullptr);
   if (addResult.isNewEntry)
     addResult.storedValue->value = PrecomputedDataForAncestor::create();
   return *addResult.storedValue->value;
 }
 
 FloatRect GeometryMapper::localToAncestorClipRect(
     const PropertyTreeState& localState,
     const PropertyTreeState& ancestorState,
     bool& success) {
   FloatRect clip(LayoutRect::infiniteIntRect());
   if (localState.clip() == ancestorState.clip()) {
     success = true;
     return clip;
   }
 
   PrecomputedDataForAncestor& precomputedData =
-      getPrecomputedDataForAncestor(ancestorState);
+      getPrecomputedDataForAncestor(ancestorState.transform());
   const ClipPaintPropertyNode* clipNode = localState.clip();
   Vector<const ClipPaintPropertyNode*> intermediateNodes;
 
-  bool found = false;
   // Iterate over the path from localState.clip to ancestorState.clip. Stop if
   // we've found a memoized (precomputed) clip for any particular node.
-  while (clipNode) {
+  while (clipNode && clipNode != ancestorState.clip()) {
     auto it = precomputedData.toAncestorClipRects.find(clipNode);
     if (it != precomputedData.toAncestorClipRects.end()) {
       clip = it->value;
-      found = true;
       break;
     }
     intermediateNodes.push_back(clipNode);
-
-    if (clipNode == ancestorState.clip())
-      break;
-
     clipNode = clipNode->parent();
   }
-  if (clipNode != ancestorState.clip() && !found) {
+  if (!clipNode) {

[chrishtr, 2017/01/11 23:23:09]

What about if clipNode == ancestorState.clip()?

[Xianzhu, 2017/01/11 23:54:43]

In the case we'll execute the loop below, the same as before. (!clipNode) is
equivalent to the original (clipNode != ancestorState.clip() && !found).

These changes are possible because now we early return if localState.clip() ==
ancestorState.clip(). Previously the two loops needed to handle the case by
putting infinite clip rect into the map and returning the entry from the map.

     success = false;
     return clip;
   }
 
   // Iterate down from the top intermediate node found in the previous loop,
   // computing and memoizing clip rects as we go.
   for (auto it = intermediateNodes.rbegin(); it != intermediateNodes.rend();
        ++it) {
-    if ((*it) != ancestorState.clip()) {
-      success = false;
-      const TransformationMatrix& transformMatrix = localToAncestorMatrix(
-          (*it)->localTransformSpace(), ancestorState, success);
-      if (!success)
-        return clip;
-      FloatRect mappedRect = transformMatrix.mapRect((*it)->clipRect().rect());
-      clip.intersect(mappedRect);
-    }
-
+    success = false;
+    const TransformationMatrix& transformMatrix = localToAncestorMatrix(
+        (*it)->localTransformSpace(), ancestorState.transform(), success);
+    if (!success)
+      return clip;
+    FloatRect mappedRect = transformMatrix.mapRect((*it)->clipRect().rect());
+    clip.intersect(mappedRect);
     precomputedData.toAncestorClipRects.set(*it, clip);
   }
 
   success = true;
   return precomputedData.toAncestorClipRects.find(localState.clip())->value;
 }
 
 const TransformationMatrix& GeometryMapper::localToAncestorMatrix(
     const TransformPaintPropertyNode* localTransformNode,
-    const PropertyTreeState& ancestorState,
+    const TransformPaintPropertyNode* ancestorTransformNode,
     bool& success) {
-  if (localTransformNode == ancestorState.transform()) {
+  if (localTransformNode == ancestorTransformNode) {
     success = true;
     return m_identity;
   }
 
   PrecomputedDataForAncestor& precomputedData =
-      getPrecomputedDataForAncestor(ancestorState);
+      getPrecomputedDataForAncestor(ancestorTransformNode);
 
   const TransformPaintPropertyNode* transformNode = localTransformNode;
   Vector<const TransformPaintPropertyNode*> intermediateNodes;
   TransformationMatrix transformMatrix;
 
   // Iterate over the path from localTransformNode to ancestorState.transform.
   // Stop if we've found a memoized (precomputed) transform for any particular
   // node.
-  while (transformNode) {
+  while (transformNode && transformNode != ancestorTransformNode) {
     auto it = precomputedData.toAncestorTransforms.find(transformNode);
     if (it != precomputedData.toAncestorTransforms.end()) {
       transformMatrix = it->value;
       break;
     }
-
-    if (transformNode == ancestorState.transform())
-      break;
-
     intermediateNodes.push_back(transformNode);
     transformNode = transformNode->parent();
   }
   if (!transformNode) {
     success = false;
     return m_identity;
   }
 
   // Iterate down from the top intermediate node found in the previous loop,
   // computing and memoizing transforms as we go.
@@ skipping 42 matching lines @@
 
   // Walk up until we find the ancestor.
   while (a != b) {
     a = a->parent();
     b = b->parent();
   }
   return a;
 }
 
 }  // namespace blink