Use SkPaint::getFontBounds() for text bounding boxes in pictures.

Now that SkTextBlobs have landed, this is a perf no-op, but it at least lets us eliminate a bunch of questionable hacks.

CQ_EXTRA_TRYBOTS=client.skia:Test-Win7-ShuttleA-HD2000-x86-Debug-Trybot,Test-Mac10.8-MacMini4.1-GeForce320M-x86_64-Debug-Trybot

BUG=skia:

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

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 86621f2170fc8d09afe48ac9274279945bdba8af..ba401005fa3e59590fe6f6c0bbc66af18fb8197c 100644 (file)
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -487,16 +487,13 @@ private:
     Bounds bounds(const DrawTextOnPath& op) const {
         SkRect dst = op.path.getBounds();
 
-        // Pad all sides by the maximum padding in any direction we'd normally apply.
+        // We don't know how the text will curve around the path, so
+        // pad all sides by the maximum padding in any direction we'd normally apply.
         SkRect pad = { 0, 0, 0, 0};
         AdjustTextForFontMetrics(&pad, op.paint);
-
-        // That maximum padding happens to always be the right pad today.
-        SkASSERT(pad.fLeft == -pad.fRight);
-        SkASSERT(pad.fTop  == -pad.fBottom);
-        SkASSERT(pad.fRight > pad.fBottom);
-        dst.outset(pad.fRight, pad.fRight);
-
+        SkScalar max = SkTMax(SkTMax(-pad.fLeft, pad.fRight),
+                              SkTMax(-pad.fTop, pad.fBottom));
+        dst.outset(max, max);
         return this->adjustAndMap(dst, &op.paint);
     }
 
@@ -511,25 +508,21 @@ private:
     }
 
     static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint) {
-#ifdef SK_DEBUG
-        SkRect correct = *rect;
-#endif
-        // crbug.com/373785 ~~> xPad = 4x yPad
-        // crbug.com/424824 ~~> bump yPad from 2x text size to 2.5x
-        const SkScalar yPad = 2.5f * paint.getTextSize(),
-                       xPad = 4.0f * yPad;
-        rect->outset(xPad, yPad);
+        // rect was built from only the text's origin points, so we need to
+        // outset it by the worst-case bounds of the font.
+        const SkRect bounds = paint.getFontBounds();
+        rect->fLeft   += bounds.fLeft;
+        rect->fRight  += bounds.fRight;
+        rect->fTop    += bounds.fTop;
+        rect->fBottom += bounds.fBottom;
 #ifdef SK_DEBUG
         SkPaint::FontMetrics metrics;
         paint.getFontMetrics(&metrics);
-        correct.fLeft   += metrics.fXMin;
-        correct.fTop    += metrics.fTop;
-        correct.fRight  += metrics.fXMax;
-        correct.fBottom += metrics.fBottom;
+        const SkRect correct = { metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom };
         // See skia:2862 for why we ignore small text sizes.
-        SkASSERTF(paint.getTextSize() < 0.001f || rect->contains(correct),
+        SkASSERTF(paint.getTextSize() < 0.001f || bounds.contains(correct),
                   "%f %f %f %f vs. %f %f %f %f\n",
-                  -xPad, -yPad, +xPad, +yPad,
+                   bounds.fLeft,  bounds.fTop, bounds.fRight,  bounds.fBottom,
                   metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom);
 #endif
     }

diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index e830af4b430e6582658ffcd6f0db2fdd7042bb05..fb5e9cf3dd5fe2e37c8e566605ad1379e60c83f1 100644 (file)
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -191,11 +191,11 @@ DEF_TEST(RecordDraw_TextBounds, r) {
     SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh);
     REPORTER_ASSERT(r, bbh.fEntries.count() == 2);
 
-    // We can make these next assertions confidently because SkRecordFillBounds
-    // builds its bounds by overestimating font metrics in a platform-independent way.
-    // If that changes, these tests will need to be more flexible.
-    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(0,  0, 140, 60)));
-    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 20, 180, 100)));
+    // Font metrics are somewhat platform dependent, so these assertions may need to be adjusted.
+    // But these particular numbers are left over from the days when we used to wildly overestimate
+    // font metrics, so these assertions should actually be pretty safe.
+    REPORTER_ASSERT(r, SkRect::MakeLTRB(0, 0,140, 60).contains(bbh.fEntries[0].bounds));
+    REPORTER_ASSERT(r, SkRect::MakeLTRB(0,20,180,100).contains(bbh.fEntries[1].bounds));
 }
 
 // Base test to ensure start/stop range is respected