LayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace()

third_party/WebKit/Source/core/layout/LayoutBox.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 382501f6a7694e0a02dc01d7e081da30305af60f..ef1dfd9007ab3cf737e6ca01feec0470c447b158 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -958,11 +958,12 @@ IntSize LayoutBox::scrolledContentOffset() const
     return flooredIntSize(getScrollableArea()->scrollOffset());
 }
 
-void LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect) const
+bool LayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace(LayoutRect& rect, VisibleRectFlags visibleRectFlags) const
 {
-    ASSERT(hasLayer());
-    ASSERT(hasOverflowClip());
+    if (!hasOverflowClip())
+        return true;
 
+    // Apply reverse of scroll offset.
     LayoutSize offset = LayoutSize(-scrolledContentOffset());
     if (UNLIKELY(hasFlippedBlocksWritingMode())) {
         if (isHorizontalWritingMode())
@@ -971,18 +972,12 @@ void LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect) const
             offset.setWidth(-offset.width());
     }
     rect.move(offset);
-}
 
-bool LayoutBox::applyOverflowClip(LayoutRect& rect, VisibleRectFlags visibleRectFlags) const
-{
-    ASSERT(hasLayer());
-    ASSERT(hasOverflowClip());
+    if (usesCompositedScrolling())

[chrishtr, 2016/03/28 16:34:31]

I don't think this isn't right. These methods should not refer to compositing
state.
Same goes for PaintInvalidationState.

[Xianzhu, 2016/03/28 17:35:21]

Why? I think we should treat composited scrolling box not clipped, otherwise
we'll invalidate contents of a composited scrolling box on each scroll.

[chrishtr, 2016/03/28 18:02:04]

Because the concept of a visible rect should not take into account compositing.
I looked elsewhere
and see we do have special-casing in PaintInvalidaitonState, so this is not a
new introduction.

For the use case you're describing, the space in which visible rects are
computed would be the scrolling
element, right? In which case we should not apply that clip since it's the
ancestor. Maybe right now the code does apply that clip, but should it? This is
similar to the code/discussion about root-layer-scrolls.

[Xianzhu, 2016/03/28 18:15:37]

Our current condition of applying clipping is "container != ancestor" (old code
line 1998). This works for composited-scrolling container, but not for
non-composited-scrolling container (crbug.com/597902).

Cases:
1. container == ancestor:
  1a. container is composited-scrolling: should not clip
  1b. container is not composited-scrolling: should clip
2. container != ancestor (implies not composited-scrolling): should clip

For now the problem is about 1b. Slow-path doesn't clip (using condition
container == ancestor) while fast-path clips (using condition
usesCompositedScrolling).

[chrishtr, 2016/03/28 19:28:53]

I don't think case 1b is required at this point. if a LayoutObject is composited
for some other reason then it gets accelerated composited scrolling as well,
even on a low-DPI screen. Is there another use case for it?

[Xianzhu, 2016/03/28 19:42:10]

The case is overflow:hidden which seems to to cause composited scrolling.

[Xianzhu, 2016/03/28 19:43:01]

The case is overflow:hidden which seems not to cause composited scrolling.

[chrishtr, 2016/03/28 20:38:58]

How about:

if (container == ancestor)
  if (container scrolls overflow)
    don't clip
  else
    clip
else
  clip

[Xianzhu, 2016/03/28 21:12:08]

It should work. It just looks weird to pass 'ancestor' parameter here unless we
give it a good meaning. Haven't got an idea for now.

[chrishtr, 2016/03/28 22:09:58]

So you think it's good to try that in this patch?

+        return true;
 
+    // Apply clip.
     flipForWritingMode(rect);
-
-    // size() is inaccurate if we're in the middle of a layout of this LayoutBox, so use the
-    // layer's size instead. Even if the layer's size is wrong, the layer itself will issue paint invalidations
-    // anyway if its size does change.
     LayoutRect clipRect(LayoutPoint(), LayoutSize(layer()->size()));
     bool doesIntersect;
     if (visibleRectFlags & EdgeInclusive) {
@@ -1942,8 +1937,7 @@ bool LayoutBox::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ance
     // offset corner for the enclosing container). This allows for a fully RL or BT document to issue paint invalidations

[chrishtr, 2016/03/28 18:02:04]

The sentence "This allows..." refers to invalidating during layout, which is no
longer done since
repaint-after-layout. Remove this sentence if you confirm it is obsolete.

[Xianzhu, 2016/03/28 21:12:08]

Acknowledged.

     // properly even during layout, since the rect remains flipped all the way until the end.
     //
-    // LayoutView::computeRectForPaintInvalidation then converts the rect to physical coordinates. We also convert to
-    // physical when we hit the ancestor. Therefore the final rect returned is always in the
+    // We convert to physical when we hit the ancestor. Therefore the final rect returned is always in the
     // physical coordinate space of the ancestor.
     const ComputedStyle& styleToUse = styleRef();
 
@@ -1954,20 +1948,14 @@ bool LayoutBox::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ance
     // included into the visual overflow for paint invalidation, we wouldn't have this issue.
     inflatePaintInvalidationRectForReflectionAndFilter(rect);
 
-    if (ancestor == this) {
-        if (ancestor->style()->isFlippedBlocksWritingMode())
-            flipForWritingMode(rect);
+    if (ancestor == this)
         return true;
-    }
 
     bool containerSkipped;
     LayoutObject* container = this->container(ancestor, &containerSkipped);
     if (!container)
         return true;
 
-    if (isWritingModeRoot())
-        flipForWritingMode(rect);
-
     LayoutPoint topLeft = rect.location();
     topLeft.move(locationOffset());
 
@@ -1989,21 +1977,22 @@ bool LayoutBox::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ance
         topLeft += layer()->offsetForInFlowPosition();
     }
 
-    // FIXME: We ignore the lightweight clipping rect that controls use, since if |o| is in mid-layout,
-    // its controlClipRect will be wrong. For overflow clip we use the values cached by the layer.
     rect.setLocation(topLeft);
-    if (container->hasOverflowClip()) {
+
+    if (container->isBox()) {
         LayoutBox* containerBox = toLayoutBox(container);
-        containerBox->mapScrollingContentsRectToBoxSpace(rect);
-        if (container != ancestor && !containerBox->applyOverflowClip(rect, visibleRectFlags))
+        if (!containerBox->mapContentsRectToVisibleRectInBorderBoxSpace(rect, visibleRectFlags))
             return false;
+
+        if (containerBox == ancestor || containerBox->isWritingModeRoot())

[chrishtr, 2016/03/28 16:34:31]

Why also if containerBox == ancestor? Why does this flipping have to happen here
at all if it's
(sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace?

[Xianzhu, 2016/03/28 17:35:21]

1. The function is to map the rect in physical coordinates in ancestor, but
during recursion, if several boxes in the ancestor chain have the same writing
mode, we'll keep the rect unflipped until we meet a writing mode root (writing
mode changes) or the ancestor. This is mentioned in the comments at line
1935-1940 above.

2. mapContentsRectToVisibleRectInBorderBoxSpace() doesn't actually flip. It just
flips temporarily for clipping: it flips before applying clipping and flip again
to revert the flip after clipping.

Another method is to keep the rect always in physical coordinates during the
recursion. This requires us to add different functions which accept input rect
in different coordinates. (A question: what terms do we use to mention the
original and flipped coordinate spaces?)

[chrishtr, 2016/03/28 18:02:04]

Makes sense, thanks for explaining.

[Xianzhu, 2016/03/28 18:15:37]

I'd like to explore always-physical-coordinates way. One question is how to
mention the "natural coordinate space" that LayoutBox etc. are using (e.g. when
we say 'in local coordinates', 'in border box coordinates', etc., and the
coordinate space of location(), frameRect(), visualOverflowRect(), etc.).

+            containerBox->flipForWritingMode(rect);
     }
 
     if (containerSkipped) {
-        // If the paintInvalidationContainer is below o, then we need to map the rect into paintInvalidationContainer's coordinates.
+        // If the ancestor is below o, then we need to map the rect into paintInvalidationContainer's coordinates.
         LayoutSize containerOffset = ancestor->offsetFromAncestorContainer(container);
         rect.move(-containerOffset);
-        // If the paintInvalidationContainer is fixed, then the rect is already in its coordinates so doesn't need viewport-adjusting.
+        // If the ancestor is fixed, then the rect is already in its coordinates so doesn't need viewport-adjusting.
         if (ancestor->style()->position() != FixedPosition && container->isLayoutView() && position == FixedPosition)
             toLayoutView(container)->adjustOffsetForFixedPosition(rect);
         return true;