Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

third_party/WebKit/Source/core/dom/IntersectionObservation.cpp

Index: third_party/WebKit/Source/core/dom/IntersectionObservation.cpp
diff --git a/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp b/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp
index 8a35d27e6dcdd306ff4eb6c895ac4dc6d1438b10..7367ed270a7c02102f4b889dc9b37835369a48c7 100644
--- a/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp
+++ b/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp
@@ -34,19 +34,20 @@ void IntersectionObservation::applyRootMargin(LayoutRect& rect) const
         m_observer->applyRootMargin(rect);
 }
 
+void IntersectionObservation::initializeGeometry(IntersectionGeometry& geometry) const
+{
+    initializeTargetRect(geometry.targetRect);
+    geometry.intersectionRect = geometry.targetRect;
+    initializeRootRect(geometry.rootRect);
+    geometry.doesIntersect = true;

[ojan, 2016/03/21 22:23:05]

Why do you need this line? It looks like it always gets overridden later.

[szager1, 2016/03/22 00:55:48]

No reason other than my instinctive aversion to leaving struct fields
uninitialized in a method with "initialize" in the name.

+}
+
 void IntersectionObservation::initializeTargetRect(LayoutRect& rect) const
 {
     ASSERT(m_target);
     LayoutObject* targetLayoutObject = target()->layoutObject();
     ASSERT(targetLayoutObject && targetLayoutObject->isBoxModelObject());
     rect = toLayoutBoxModelObject(targetLayoutObject)->visualOverflowRect();
-
-    // TODO(szager): Properly support intersection observations for zero-area targets
-    //   by using edge-inclusive geometry.
-    if (!rect.size().width())
-        rect.setWidth(LayoutUnit(1));
-    if (!rect.size().height())
-        rect.setHeight(LayoutUnit(1));
 }
 
 void IntersectionObservation::initializeRootRect(LayoutRect& rect) const
@@ -62,7 +63,7 @@ void IntersectionObservation::initializeRootRect(LayoutRect& rect) const
     applyRootMargin(rect);
 }
 
-void IntersectionObservation::clipToRoot(LayoutRect& rect, const LayoutRect& rootRect) const
+void IntersectionObservation::clipToRoot(IntersectionGeometry& geometry) const
 {
     // Map and clip rect into root element coordinates.
     // TODO(szager): the writing mode flipping needs a test.
@@ -70,10 +71,12 @@ void IntersectionObservation::clipToRoot(LayoutRect& rect, const LayoutRect& roo
     LayoutObject* rootLayoutObject = m_observer->rootLayoutObject();
     LayoutObject* targetLayoutObject = target()->layoutObject();
 
-    targetLayoutObject->mapToVisibleRectInAncestorSpace(toLayoutBoxModelObject(rootLayoutObject), rect, nullptr);
-    LayoutRect rootClipRect(rootRect);
+    geometry.doesIntersect = targetLayoutObject->mapToVisibleRectInAncestorSpace(toLayoutBoxModelObject(rootLayoutObject), geometry.intersectionRect, nullptr, true);
+    if (!geometry.doesIntersect)
+        return;
+    LayoutRect rootClipRect(geometry.rootRect);
     toLayoutBox(rootLayoutObject)->flipForWritingMode(rootClipRect);
-    rect.intersect(rootClipRect);
+    geometry.doesIntersect &= geometry.intersectionRect.inclusiveIntersect(rootClipRect);
 }
 
 static void mapRectUpToDocument(LayoutRect& rect, const LayoutObject& layoutObject, const Document& document)
@@ -164,22 +167,22 @@ bool IntersectionObservation::computeGeometry(IntersectionGeometry& geometry) co
     if (!isContainingBlockChainDescendant(targetLayoutObject, rootLayoutObject))
         return false;
 
-    initializeTargetRect(geometry.targetRect);
-    geometry.intersectionRect = geometry.targetRect;
-    initializeRootRect(geometry.rootRect);
+    initializeGeometry(geometry);
 
-    clipToRoot(geometry.intersectionRect, geometry.rootRect);
+    clipToRoot(geometry);
 
-    // TODO(szager): there are some simple optimizations that can be done here:
-    //   - Don't transform rootRect if it's not going to be reported
-    //   - Don't transform intersectionRect if it's empty
     mapTargetRectToTargetFrameCoordinates(geometry.targetRect);
-    mapRootRectToTargetFrameCoordinates(geometry.intersectionRect);
-    mapRootRectToRootFrameCoordinates(geometry.rootRect);
 
-    if (geometry.intersectionRect.size().isZero())
+    if (geometry.doesIntersect)
+        mapRootRectToTargetFrameCoordinates(geometry.intersectionRect);
+    else
         geometry.intersectionRect = LayoutRect();

[ojan, 2016/03/21 22:23:05]

Doesn't this get the x/y coordinates of the intersectionRect wrong?

[szager1, 2016/03/22 00:55:48]

If there's no intersection, then there are no right or wrong coordinates.  The
spec doesn't say anything about what intersectionRect should be when root and
target don't overlap.  I consider it safer to do this, rather than possibly leak
information about the root viewport.

Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.

[ojan, 2016/03/22 04:24:47]

Oh, I misread this code. This is only in the non-intersecting case. As far as
the clearing of the rect for prudency, can you verify the correct behavior via
asserts instead of something that will use cycles at runtime?

 
+    if (m_shouldReportRootBounds)
+        mapRootRectToRootFrameCoordinates(geometry.rootRect);
+    else
+        geometry.rootRect = LayoutRect();
+
     return true;
 }
 
@@ -191,9 +194,11 @@ void IntersectionObservation::computeIntersectionObservations(DOMHighResTimeStam
 
     float intersectionArea = geometry.intersectionRect.size().width().toFloat() * geometry.intersectionRect.size().height().toFloat();
     float targetArea = geometry.targetRect.size().width().toFloat() * geometry.targetRect.size().height().toFloat();
-    if (!targetArea)
-        return;
-    float newVisibleRatio = intersectionArea / targetArea;
+    float newVisibleRatio;
+    if (!intersectionArea && geometry.doesIntersect)
+        newVisibleRatio = std::numeric_limits<float>::denorm_min();

[ojan, 2016/03/21 22:23:04]

This can't be 0? /me is probably ignorant about working with floats.

[szager1, 2016/03/22 00:55:48]

Zero is a special value that means "not intersecting," which is distinct from
"has a non-zero intersection."

Basically, we want a value here that is greater than zero, but guaranteed to be
less than or equal to any other threshold value.

+    else
+        newVisibleRatio = intersectionArea / targetArea;
     unsigned newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio);
     IntRect snappedRootBounds = pixelSnappedIntRect(geometry.rootRect);
     IntRect* rootBoundsPointer = m_shouldReportRootBounds ? &snappedRootBounds : nullptr;