From 7681dc8f970a252f64dfb42e98907b65b5ab3b04 Mon Sep 17 00:00:00 2001
From: Robert O'Callahan <robert@ocallahan.org>
Date: Mon, 7 Sep 2009 12:35:14 +1200
Subject: [PATCH] Bug 513082. Round out 'region of moving visible content' to
 device pixels to reduce incidence of stray subpixel rectangles causing
 unnecessary painting. r=dbaron

--HG--
extra : rebase_source : a976be084de5b610089cebe1da9a9c5a2e580cb2
---
 layout/base/tests/scrolling_helper.html | 101 +++++++++++++++++++++---
 view/src/nsScrollPortView.cpp           |  91 ++++++++++-----------
 2 files changed, 128 insertions(+), 64 deletions(-)

diff --git a/layout/base/tests/scrolling_helper.html b/layout/base/tests/scrolling_helper.html
index 6ab4d171fe77..3072fe7ce3a9 100644
--- a/layout/base/tests/scrolling_helper.html
+++ b/layout/base/tests/scrolling_helper.html
@@ -20,7 +20,8 @@ body > div {
      tests which expect to observe scrolling happening. -->
 
 <!-- Each of the DIV children of the BODY is one test. We scroll that DIV's
-     scrollTop from 0 to 20 and then call the function given by the DIV's id,
+     scrollTop (or scrollLeft, if the DIV's class is 'horizontal')
+     from 0 to 20 and then call the function given by the DIV's id,
      passing the blit region and the paint region as a parameters. -->
 
 <div id="testSimpleScroll">
@@ -70,6 +71,22 @@ body > div {
   <div style="height:10px; margin-top:210px; background:-moz-linear-gradient(top, bottom, from(rgba(0,0,0,0.7)), to(rgba(255,0,0,0.7)));"></div>
 </div>
 
+<div id="testSimpleScrollWithSubpixelOffset1" style="top:0.2px">
+  <div style="height:300px; background:-moz-linear-gradient(top, bottom, from(red), to(black));"></div>
+</div>
+
+<div id="testSimpleScrollWithSubpixelOffset2" style="top:0.8px">
+  <div style="height:300px; background:-moz-linear-gradient(top, bottom, from(red), to(black));"></div>
+</div>
+
+<div id="testSimpleScrollWithSubpixelOffset3" style="left:0.2px" class="horizontal">
+  <div style="width:300px; height:200px; background:-moz-linear-gradient(left, right, from(red), to(black));"></div>
+</div>
+
+<div id="testSimpleScrollWithSubpixelOffset4" style="left:0.8px" class="horizontal">
+  <div style="width:300px; height:200px; background:-moz-linear-gradient(left, right, from(red), to(black));"></div>
+</div>
+
 <script>
 var tests = document.querySelectorAll("body>div");
 var currentTest = -1;
@@ -91,18 +108,18 @@ window.onerror = function (event) { window.opener.onerror(event); window.close()
 // the content that has scrolled into view.
 function testSimpleScroll(blitRegion, paintRegion) {
   ok(blitRegion.equalsRegion(new Region([[0,0,200,180]])),
-     "Should blit everything that was already visible");
+     "Should blit everything that was already visible: " + blitRegion.toString());
   ok(paintRegion.equalsRegion(new Region([[0,180,200,200]])),
-     "Should repaint area that was scrolled into view");
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
 }
 
 // Check that scrolling visible content over background-attachment:fixed
 // content repaints everything
 function testFixedBackground(blitRegion, paintRegion) {
   ok(blitRegion.isEmpty(),
-     "Shouldn't blit anything");
+     "Shouldn't blit anything: " + blitRegion.toString());
   ok(paintRegion.equalsRegion(new Region([[0,0,200,200]])),
-     "Should repaint everything");
+     "Should repaint everything: " + paintRegion.toString());
 }
 
 // Check that we optimize scrolling in a reasonable way in the presence of a
@@ -111,25 +128,25 @@ function testFixedPosOverlay(blitRegion, paintRegion) {
   // The area of the fixed element should not be repainted or blitted at all
   var fixedElemRect = [0,50,100,95]
   ok(!blitRegion.intersectsRect(fixedElemRect),
-     "blit region should not intersect fixed-pos element area");
+     "blit region should not intersect fixed-pos element area: " + blitRegion.toString());
   ok(!paintRegion.intersectsRect(fixedElemRect),
-     "paint region should not intersect fixed-pos element area");
+     "paint region should not intersect fixed-pos element area: " + paintRegion.toString());
 
   // The area to the right of the fixed element that we can fill with blitting
   // existing content should not be repainted (but may be blitted, although
   // it doesn't all need to be blitted either, since most of it is blank)
   var noPaintRect = [100,0,200,180];
   ok(!paintRegion.intersectsRect(noPaintRect),
-     "paint region should not intersect blittable area");
+     "paint region should not intersect blittable area: " + paintRegion.toString());
 }
 
 // Check that scrolling an element with moving content in it does
 // the obvious thing, even if the element has a border.
 function testBorder(blitRegion, paintRegion) {
   ok(blitRegion.equalsRegion(new Region([[1,1,201,181]])),
-     "Should blit everything that was already visible");
+     "Should blit everything that was already visible: " + blitRegion.toString());
   ok(paintRegion.equalsRegion(new Region([[1,181,201,201]])),
-     "Should repaint area that was scrolled into view");
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
 }
 
 // Check that scrolling some content out of view updates the area that
@@ -137,7 +154,7 @@ function testBorder(blitRegion, paintRegion) {
 function testScrollOutOfView(blitRegion, paintRegion) {
   var all = blitRegion.unionRegion(paintRegion);
   ok(all.containsRect([0,0,200,10]),
-     "Should update everything that was previously visible but scrolled out of view");
+     "Should update everything that was previously visible but scrolled out of view: " + all.toString());
 }
 
 // Check that scrolling some content into view updates the area that
@@ -145,7 +162,58 @@ function testScrollOutOfView(blitRegion, paintRegion) {
 function testScrollIntoView(blitRegion, paintRegion) {
   var all = blitRegion.unionRegion(paintRegion);
   ok(all.containsRect([0,190,200,200]),
-     "Should update everything that was previously visible");
+     "Should update everything that was previously visible: " + all.toString());
+}
+
+// When we scroll an area which has a very small subpixel offset, we should
+// still be doing the obvious thing --- we shouldn't end up painting an extra
+// row of pixels anywhere
+function testSimpleScrollWithSubpixelOffset1(blitRegion, paintRegion) {
+  ok(blitRegion.equalsRegion(new Region([[0,0,200,180]])),
+     "Should blit everything that was already visible: " + blitRegion.toString());
+  // The next test is contains, not equals, since we repaint down to 200.2
+  // which is OK
+  ok(paintRegion.containsRegion(new Region([[0,180,200,200]])),
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
+  // Check that we're not repainting anything in the area that's blitted
+  ok(!paintRegion.intersectsRect([0,0,200,180]),
+     "Should not repaint area that was blitted: " + paintRegion.toString());
+}
+
+function testSimpleScrollWithSubpixelOffset2(blitRegion, paintRegion) {
+  ok(blitRegion.equalsRegion(new Region([[0,1,200,181]])),
+     "Should blit everything that was already visible: " + blitRegion.toString());
+  // The next test is contains, not equals, since we repaint down to 200.8
+  // which is OK
+  ok(paintRegion.containsRegion(new Region([[0,181,200,200]])),
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
+  // Check that we're not repainting anything in the area that's blitted
+  todo(!paintRegion.intersectsRect([0,0,200,180]),
+       "Should not repaint area that was blitted: " + paintRegion.toString());
+}
+
+function testSimpleScrollWithSubpixelOffset3(blitRegion, paintRegion) {
+  ok(blitRegion.equalsRegion(new Region([[0,0,180,200]])),
+     "Should blit everything that was already visible: " + blitRegion.toString());
+  // The next test is contains, not equals, since we repaint across to 200.2
+  // which is OK
+  ok(paintRegion.containsRegion(new Region([[180,0,200,200]])),
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
+  // Check that we're not repainting anything in the area that's blitted
+  ok(!paintRegion.intersectsRect([0,0,180,200]),
+     "Should not repaint area that was blitted: " + paintRegion.toString());
+}
+
+function testSimpleScrollWithSubpixelOffset4(blitRegion, paintRegion) {
+  ok(blitRegion.equalsRegion(new Region([[1,0,181,200]])),
+     "Should blit everything that was already visible: " + blitRegion.toString());
+  // The next test is contains, not equals, since we repaint down to 200.8
+  // which is OK
+  ok(paintRegion.containsRegion(new Region([[181,0,200,200]])),
+     "Should repaint area that was scrolled into view: " + paintRegion.toString());
+  // Check that we're not repainting anything in the area that's blitted
+  todo(!paintRegion.intersectsRect([0,0,180,200]),
+       "Should not repaint area that was blitted: " + paintRegion.toString());
 }
 
 function clientRectToRect(cr)
@@ -196,6 +264,7 @@ for (var i = 0; i < tests.length; ++i) {
   e.style.display = "none";
   // Make sure we don't remember a scroll position from history
   e.scrollTop = 0;
+  e.scrollLeft = 0;
 }
 
 function nextTest() {
@@ -207,7 +276,13 @@ function nextTest() {
 
   var e = tests[currentTest];
   e.style.display = "";
-  setTimeout(function() { e.scrollTop = 20; }, 0);
+  setTimeout(function() {
+    if (e.getAttribute("class") == "horizontal") {
+      e.scrollLeft = 20;
+    } else {
+      e.scrollTop = 20;
+    }
+  }, 0);
 }
 
 function runTests() {
diff --git a/view/src/nsScrollPortView.cpp b/view/src/nsScrollPortView.cpp
index 0788c91740aa..30e614fe2869 100644
--- a/view/src/nsScrollPortView.cpp
+++ b/view/src/nsScrollPortView.cpp
@@ -543,47 +543,38 @@ NS_IMETHODIMP nsScrollPortView::CanScroll(PRBool aHorizontal,
 
 /**
  * Given aBlitRegion in appunits, create and return an nsRegion in
- * device pixels that represents the device pixels that are wholly
- * contained in aBlitRegion. Whatever appunit area was removed in that
- * process is added to aRepaintRegion. An appunits version of the result
- * is placed in aAppunitsBlitRegion.
+ * device pixels that represents the device pixels whose centers are
+ * contained in aBlitRegion. Whatever appunit area was removed from
+ * aBlitRegion in that process is added to aRepaintRegion. An appunits
+ * version of the result is placed in aAppunitsBlitRegion.
+ * 
+ * We convert the blit region to pixels this way because in general
+ * frames touch the pixels whose centers are contained in the
+ * (possibly not pixel-aligned) frame bounds.
  */
-static nsRegion
-ConvertToInnerPixelRegion(const nsRegion& aBlitRegion,
-                          nscoord aAppUnitsPerPixel,
-                          nsRegion* aRepaintRegion,
-                          nsRegion* aAppunitsBlitRegion)
+static void
+ConvertBlitRegionToPixelRects(const nsRegion& aBlitRegion,
+                              nscoord aAppUnitsPerPixel,
+                              nsTArray<nsIntRect>* aPixelRects,
+                              nsRegion* aRepaintRegion,
+                              nsRegion* aAppunitsBlitRegion)
 {
-  // Basically we compute the inverse of aBlitRegion,
-  // expand each of its rectangles out to device pixel boundaries, then
-  // invert that.
-  nsIntRect boundingBoxPixels =
-    aBlitRegion.GetBounds().ToOutsidePixels(aAppUnitsPerPixel);
-  nsRect boundingBox = boundingBoxPixels.ToAppUnits(aAppUnitsPerPixel);
-  nsRegion outside;
-  outside.Sub(boundingBox, aBlitRegion);
-
-  nsRegion outsidePixels;
-  nsRegion outsideAppUnits;
   const nsRect* r;
-  for (nsRegionRectIterator iter(outside); (r = iter.Next());) {
-    nsIntRect pixRect = r->ToOutsidePixels(aAppUnitsPerPixel);
-    outsidePixels.Or(outsidePixels,
-                     nsRect(pixRect.x, pixRect.y, pixRect.width, pixRect.height));
-    outsideAppUnits.Or(outsideAppUnits,
-                       pixRect.ToAppUnits(aAppUnitsPerPixel));
+
+  aPixelRects->Clear();
+  aAppunitsBlitRegion->SetEmpty();
+  // The rectangles in aBlitRegion don't overlap so converting them via
+  // ToNearestPixels also produces a sequence of non-overlapping rectangles
+  for (nsRegionRectIterator iter(aBlitRegion); (r = iter.Next());) {
+    nsIntRect pixRect = r->ToNearestPixels(aAppUnitsPerPixel);
+    aPixelRects->AppendElement(pixRect);
+    aAppunitsBlitRegion->Or(*aAppunitsBlitRegion,
+                            pixRect.ToAppUnits(aAppUnitsPerPixel));
   }
 
   nsRegion repaint;
-  repaint.And(aBlitRegion, outsideAppUnits);
+  repaint.Sub(aBlitRegion, *aAppunitsBlitRegion);
   aRepaintRegion->Or(*aRepaintRegion, repaint);
-
-  nsRegion result;
-  result.Sub(nsRect(boundingBoxPixels.x, boundingBoxPixels.y,
-                    boundingBoxPixels.width, boundingBoxPixels.height),
-             outsidePixels);
-  aAppunitsBlitRegion->Sub(boundingBox, outsideAppUnits);
-  return result;
 }
 
 /**
@@ -619,25 +610,24 @@ FlipRect(const nsIntRect& aRect, nsIntPoint aPixDelta)
   return r;
 }
 
-// Extract the rectangles from aInnerPixRegion, and sort them into aRects
-// so that moving rectangle aRects[i] - aPixDelta to aRects[i] will not
-// cause the rectangle to overlap any rectangles that haven't moved yet. See
-// http://weblogs.mozillazine.org/roc/archives/2009/08/homework_answer.html
+// Sort aRects so that moving rectangle aRects[i] - aPixDelta to aRects[i]
+// will not cause the rectangle to overlap any rectangles that haven't
+// moved yet.
+// See http://weblogs.mozillazine.org/roc/archives/2009/08/homework_answer.html
 static void
-SortBlitRectsForCopy(const nsRegion& aInnerPixRegion,
-                     nsIntPoint aPixDelta,
-                     nsTArray<nsIntRect>* aResult)
+SortBlitRectsForCopy(nsIntPoint aPixDelta, nsTArray<nsIntRect>* aRects)
 {
   nsTArray<nsIntRect> rects;
 
-  const nsRect* r;
-  for (nsRegionRectIterator iter(aInnerPixRegion); (r = iter.Next());) {
+  for (PRUint32 i = 0; i < aRects->Length(); ++i) {
+    nsIntRect* r = &aRects->ElementAt(i);
     nsIntRect rect =
       FlipRect(nsIntRect(r->x, r->y, r->width, r->height), aPixDelta);
     rects.AppendElement(rect);
   }
   rects.Sort(RightEdgeComparator());
 
+  aRects->Clear();
   // This could probably be improved a bit for some worst-case scenarios.
   // But in common cases this should be very fast, and we shouldn't
   // make it more complex unless we really need to.
@@ -667,7 +657,7 @@ SortBlitRectsForCopy(const nsRegion& aInnerPixRegion,
 
     // Rectangle i has no rectangles to the right or below.
     // Flip it back before saving the result.
-    aResult->AppendElement(FlipRect(rects[i], aPixDelta));
+    aRects->AppendElement(FlipRect(rects[i], aPixDelta));
     rects.RemoveElementAt(i);
   }
 }
@@ -714,18 +704,17 @@ void nsScrollPortView::Scroll(nsView *aScrolledView, nsPoint aTwipsDelta,
       mViewManager->WillBitBlit(this, aTwipsDelta);
 
       // innerPixRegion is in device pixels
-      nsRegion innerBlitRegion;
-      nsRegion innerBlitPixRegion =
-        ConvertToInnerPixelRegion(blitRegion, aP2A, &repaintRegion,
-                                  &innerBlitRegion);
       nsTArray<nsIntRect> blitRects;
-      SortBlitRectsForCopy(innerBlitPixRegion, aPixDelta, &blitRects);
+      nsRegion blitRectsRegion;
+      ConvertBlitRegionToPixelRects(blitRegion, aP2A, &blitRects, &repaintRegion,
+                                    &blitRectsRegion);
+      SortBlitRectsForCopy(aPixDelta, &blitRects);
 
       nearestWidget->Scroll(aPixDelta, blitRects, aConfigurations);
       AdjustChildWidgets(aScrolledView, nearestWidgetOffset, aP2A, PR_TRUE);
       repaintRegion.MoveBy(-nearestWidgetOffset);
-      innerBlitRegion.MoveBy(-nearestWidgetOffset);
-      mViewManager->UpdateViewAfterScroll(this, innerBlitRegion, repaintRegion);
+      blitRectsRegion.MoveBy(-nearestWidgetOffset);
+      mViewManager->UpdateViewAfterScroll(this, blitRectsRegion, repaintRegion);
     }
   }
 }