son of path is rect bug

This variation tricks SkPath::isRect by exploiting
that the implementation resets the point pointer to
process the close verb, and using the reset pointer
to walk over a series of points that don't move.

In addition to fixing this, rename variables to
make the line creation more obvious, since left,
right, and friends, are not the left and right.

R=robertphillips@google.com

Bug: 824145,skia:7792
Change-Id: If8ebbc3eedd270652670d6e111a5bc02e61f0eec
Reviewed-on: https://skia-review.googlesource.com/121122
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Cary Clark <caryclark@skia.org>

3 files changed, 35 insertions, 14 deletions

diff --git a/gm/pathfill.cpp b/gm/pathfill.cpp
index 18b30c9b0b..74cafcb1c4 100644
--- a/gm/pathfill.cpp
+++ b/gm/pathfill.cpp
@@ -509,4 +509,19 @@ DEF_SIMPLE_GM(bug7792, canvas, 600, 600) {
     path.lineTo(75, 10);
     path.close();
     canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 19
+    canvas->translate(200, 0);
+    path.reset();
+    path.moveTo(75, 75);
+    path.lineTo(75, 75);
+    path.lineTo(75, 75);
+    path.lineTo(75, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.close();
+    path.moveTo(10, 10);
+    path.lineTo(30, 10);
+    path.lineTo(10, 30);
+    canvas->drawPath(path, p);
 }
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 62a3cd6c55..beb46eb7ee 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -446,15 +446,16 @@ FIXME: If the API passes fill-only, return true if the filled stroke
 static int rect_make_dir(SkScalar dx, SkScalar dy) {
     return ((0 != dx) << 0) | ((dx > 0 || dy > 0) << 1);
 }
+
 bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** ptsPtr,
         bool* isClosed, Direction* direction, SkRect* rect) const {
     int corners = 0;
-    SkPoint previous;  // used to construct line from previous point
+    SkPoint lineStart;  // used to construct line from previous point
     const SkPoint* firstPt = nullptr; // first point in the rect (last of first moves)
     const SkPoint* lastPt = nullptr;  // last point in the rect (last of lines or first if closed)
     const SkPoint* pts = *ptsPtr;
     const SkPoint* savePts = nullptr; // used to allow caller to iterate through a pair of rects
-    previous.set(0, 0);
+    lineStart.set(0, 0);
     int firstDirection = 0;
     int lastDirection = 0;
     int nextDirection = 0;
@@ -469,30 +470,26 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
         switch (verb) {
             case kClose_Verb:
                 savePts = pts;
-                pts = firstPt;
                 autoClose = true;
                 insertClose = false;
                 accumulatingRect = false;
             case kLine_Verb: {
-                SkScalar left = previous.fX;
-                SkScalar top = previous.fY;
-                SkScalar right = pts->fX;
-                SkScalar bottom = pts->fY;
                 if (accumulatingRect) {
                     lastPt = pts;
                 }
-                ++pts;
-                if (left != right && top != bottom) {
+                SkPoint lineEnd = kClose_Verb == verb ? *firstPt : *pts++;
+                SkVector lineDelta = lineEnd - lineStart;
+                if (lineDelta.fX && lineDelta.fY) {
                     return false; // diagonal
                 }
                 addedLine = true;
-                if (left == right && top == bottom) {
+                if (lineStart == lineEnd) {
                     break; // single point on side OK
                 }
-                nextDirection = rect_make_dir(right - left, bottom - top);
+                nextDirection = rect_make_dir(lineDelta.fX, lineDelta.fY);
                 if (0 == corners) {
                     firstDirection = nextDirection;
-                    previous = pts[-1];
+                    lineStart = lineEnd;
                     corners = 1;
                     closedOrMoved = false;
                     break;
@@ -509,7 +506,7 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
                         return false; // too many direction changes
                     }
                 }
-                previous = pts[-1];
+                lineStart = lineEnd;
                 if (lastDirection == nextDirection) {
                     break; // colinear segment
                 }
@@ -539,7 +536,7 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
                 } else {
                     accumulatingRect = false;
                 }
-                previous = *pts++;
+                lineStart = *pts++;
                 closedOrMoved = true;
                 break;
             default:
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 4d250283cf..b96b93ccfe 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -4977,4 +4977,13 @@ DEF_TEST(Path_isRect, reporter) {
     SkPoint points17[] = { {75, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 10} };
     path = makePath(points17, SK_ARRAY_COUNT(points17), true);
     REPORTER_ASSERT(reporter, !path.isRect(&rect, nullptr, nullptr));
+    // isolated from skbug.com/7792 comment 19
+    SkPath::Verb verbs19[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+                               SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+                               SkPath::kLine_Verb, SkPath::kClose_Verb, SkPath::kMove_Verb,
+                               SkPath::kLine_Verb, SkPath::kLine_Verb };
+    SkPoint points19[] = { {75, 75}, {75, 75}, {75, 75}, {75, 75}, {150, 75}, {150, 150},
+                           {75, 150}, {10, 10}, {30, 10}, {10, 30} };
+    path = makePath2(points19, verbs19, SK_ARRAY_COUNT(verbs19));
+    REPORTER_ASSERT(reporter, !path.isRect(&rect, nullptr, nullptr));
 }