change getBounds to return 0000 iff there are zero points

This is a contract change for SkPath::getBounds(), which formally was defined to return 0,0,0,0 for a 1-point path, regardless of the coordinates of that point. This seems wacky/inconsistent, and was causing other bugs (incorrect bounds) when this was unioned with other rects.

Does anyone remember why we defined it this way?

BUG=513799

Review URL: https://codereview.chromium.org/1261773002

3 files changed, 25 insertions, 14 deletions

diff --git a/include/core/SkPath.h b/include/core/SkPath.h
index fe89766558..dc50ae76c3 100644
--- a/include/core/SkPath.h
+++ b/include/core/SkPath.h
@@ -273,11 +273,12 @@ public:
     //! Swap contents of this and other. Guaranteed not to throw
     void swap(SkPath& other);
 
-    /** Returns the bounds of the path's points. If the path contains 0 or 1
-        points, the bounds is set to (0,0,0,0), and isEmpty() will return true.
-        Note: this bounds may be larger than the actual shape, since curves
-        do not extend as far as their control points. Additionally this bound
-        can contain trailing MoveTo points (cf. isRect).
+    /**
+     *  Returns the bounds of the path's points. If the path contains zero points/verbs, this
+     *  will return the "empty" rect [0, 0, 0, 0].
+     *  Note: this bounds may be larger than the actual shape, since curves
+     *  do not extend as far as their control points. Additionally this bound encompases all points,
+     *  even isolated moveTos either preceeding or following the last non-degenerate contour.
     */
     const SkRect& getBounds() const {
         return fPathRef->getBounds();
diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index ae8945de08..e7cc31cff4 100644
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -278,13 +278,7 @@ private:
 
     // Return true if the computed bounds are finite.
     static bool ComputePtBounds(SkRect* bounds, const SkPathRef& ref) {
-        int count = ref.countPoints();
-        if (count <= 1) {  // we ignore just 1 point (moveto)
-            bounds->setEmpty();
-            return count ? ref.points()->isFinite() : true;
-        } else {
-            return bounds->setBoundsCheck(ref.points(), count);
-        }
+        return bounds->setBoundsCheck(ref.points(), ref.countPoints());
     }
 
     // called, if dirty, by getBounds()
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 313d84a142..abe8b39590 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -568,6 +568,21 @@ static void test_clipped_cubic() {
     }
 }
 
+static void test_bounds_crbug_513799(skiatest::Reporter* reporter) {
+    SkPath path;
+
+    REPORTER_ASSERT(reporter, SkRect::MakeLTRB(0, 0, 0, 0) == path.getBounds());
+
+    path.moveTo(-5, -8);
+    REPORTER_ASSERT(reporter, SkRect::MakeLTRB(-5, -8, -5, -8) == path.getBounds());
+
+    path.addRect(SkRect::MakeLTRB(1, 2, 3, 4));
+    REPORTER_ASSERT(reporter, SkRect::MakeLTRB(-5, -8, 3, 4) == path.getBounds());
+
+    path.moveTo(1, 2);
+    REPORTER_ASSERT(reporter, SkRect::MakeLTRB(-5, -8, 3, 4) == path.getBounds());
+}
+
 // Inspired by http://ie.microsoft.com/testdrive/Performance/Chalkboard/
 // which triggered an assert, from a tricky cubic. This test replicates that
 // example, so we can ensure that we handle it (in SkEdge.cpp), and don't
@@ -2358,9 +2373,9 @@ static void test_zero_length_paths(skiatest::Reporter* reporter) {
         SkPath::kMove_Verb, SkPath::kCubic_Verb, SkPath::kClose_Verb, SkPath::kMove_Verb, SkPath::kCubic_Verb, SkPath::kClose_Verb
     };
     static const struct zeroPathTestData gZeroLengthTests[] = {
-        { "M 1 1", 1, {0, 0, 0, 0}, resultVerbs1, SK_ARRAY_COUNT(resultVerbs1) },
+        { "M 1 1", 1, {1, 1, 1, 1}, resultVerbs1, SK_ARRAY_COUNT(resultVerbs1) },
         { "M 1 1 M 2 1", 2, {SK_Scalar1, SK_Scalar1, 2*SK_Scalar1, SK_Scalar1}, resultVerbs2, SK_ARRAY_COUNT(resultVerbs2) },
-        { "M 1 1 z", 1, {0, 0, 0, 0}, resultVerbs3, SK_ARRAY_COUNT(resultVerbs3) },
+        { "M 1 1 z", 1, {1, 1, 1, 1}, resultVerbs3, SK_ARRAY_COUNT(resultVerbs3) },
         { "M 1 1 z M 2 1 z", 2, {SK_Scalar1, SK_Scalar1, 2*SK_Scalar1, SK_Scalar1}, resultVerbs4, SK_ARRAY_COUNT(resultVerbs4) },
         { "M 1 1 L 1 1", 2, {SK_Scalar1, SK_Scalar1, SK_Scalar1, SK_Scalar1}, resultVerbs5, SK_ARRAY_COUNT(resultVerbs5) },
         { "M 1 1 L 1 1 M 2 1 L 2 1", 4, {SK_Scalar1, SK_Scalar1, 2*SK_Scalar1, SK_Scalar1}, resultVerbs6, SK_ARRAY_COUNT(resultVerbs6) },
@@ -3848,4 +3863,5 @@ DEF_TEST(Paths, reporter) {
     test_path_crbugskia2820(reporter);
     test_skbug_3469(reporter);
     test_skbug_3239(reporter);
+    test_bounds_crbug_513799(reporter);
 }