Revert of Use SkTypeface::getBounds() in bounding-box calculations. (patchset #5 id:80001 of https://codereview.chromium.org/680363003/)

Reason for revert:
Mac mini asserting

Original issue's description:
> Use SkTypeface::getBounds() in bounding-box calculations.
>
> This should produce tighter conservative bounding boxes for text than the
> approximation code it replaces.
>
> Recording performance is neutral on my desktop.  Playback performance
> improves by up to 15% on text heavy pages, e.g.
>
>   desk_pokemonwiki.skp_1 3.24ms -> 2.83ms  0.87x
>         desk_baidu.skp_1 1.91ms -> 1.58ms  0.83x
>
> Committed: https://skia.googlesource.com/skia/+/bf8dc343df4fbdcb8af546eb68b640e011a33489
>
> CQ_EXTRA_TRYBOTS=client.skia:Test-Win7-ShuttleA-HD2000-x86-Debug-Trybot
>
> Committed: https://skia.googlesource.com/skia/+/c51add674dfb89b988a7fbc05f41838c203f9dcd

TBR=reed@google.com,mtklein@chromium.org
NOTREECHECKS=true
NOTRY=true

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

2 files changed, 28 insertions, 40 deletions

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 6dcb9b8a49..5981245c01 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -7,7 +7,6 @@
 
 #include "SkRecordDraw.h"
 #include "SkPatchUtils.h"
-#include "SkTypeface.h"
 
 void SkRecordDraw(const SkRecord& record,
                   SkCanvas* canvas,
@@ -445,13 +444,15 @@ private:
     Bounds bounds(const DrawTextOnPath& op) const {
         SkRect dst = op.path.getBounds();
 
-        // We don't know how the text will curve aroudn the path, so
-        // pad all sides by the maximum padding in any direction we'd normally apply.
+        // Pad all sides by the maximum padding in any direction we'd normally apply.
         SkRect pad = { 0, 0, 0, 0};
         AdjustTextForFontMetrics(&pad, op.paint);
-        SkScalar max = SkTMax(SkTMax(-pad.fLeft, pad.fRight),
-                              SkTMax(-pad.fTop, pad.fBottom));
-        dst.outset(max, max);
+
+        // 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);
 
         return this->adjustAndMap(dst, &op.paint);
     }
@@ -463,39 +464,26 @@ private:
     }
 
     static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint) {
-        // rect was built from only the text's origin points, so we need
-        // to outset it by the worst-case bounds of the typeface.
-        SkAutoTUnref<SkTypeface> au;
-        SkTypeface* tf = paint.getTypeface();
-        if (!tf) {
-            au.reset(SkTypeface::RefDefault());
-            tf = au.get();
-        }
-        const SkScalar size = paint.getTextSize();
-        const SkRect tb = tf->getBounds();
-        rect->fLeft   += size * tb.fLeft;
-        rect->fRight  += size * tb.fRight;
-        rect->fTop    += size * tb.fTop;
-        rect->fBottom += size * tb.fBottom;
+#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);
 #ifdef SK_DEBUG
         SkPaint::FontMetrics metrics;
         paint.getFontMetrics(&metrics);
-        // Our numbers are so close to correct now that we're sometimes _slightly_ under.  Add
-        // a little fudge factor here to make sure the assert just catches really bad failures.
-        const SkScalar kFudge = 0.0625f;
-        SkRect fudged = {
-            size * (tb.fLeft   - kFudge),
-            size * (tb.fTop    - kFudge),
-            size * (tb.fRight  + kFudge),
-            size * (tb.fBottom + kFudge),
-        };
-        SkRect correct = { metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom };
+        correct.fLeft   += metrics.fXMin;
+        correct.fTop    += metrics.fTop;
+        correct.fRight  += metrics.fXMax;
+        correct.fBottom += metrics.fBottom;
         // See skia:2862 for why we ignore small text sizes.
-        SkASSERTF(paint.getTextSize() < 0.001f || fudged.contains(correct),
-                  "%f %f %f %f vs. %f %f %f %f, text size %f\n",
-                  size*tb.fLeft, size*tb.fTop, size*tb.fRight, size*tb.fBottom,
-                  metrics.fXMin, metrics.fTop, metrics.fXMax,  metrics.fBottom,
-                  paint.getTextSize());
+        SkASSERTF(paint.getTextSize() < 0.001f || rect->contains(correct),
+                  "%f %f %f %f vs. %f %f %f %f\n",
+                  -xPad, -yPad, +xPad, +yPad,
+                  metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom);
 #endif
     }
 
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 2476c03984..6105c00f7f 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -166,11 +166,11 @@ DEF_TEST(RecordDraw_TextBounds, r) {
     SkRecordFillBounds(record, &bbh);
     REPORTER_ASSERT(r, bbh.fEntries.count() == 2);
 
-    // Font metrics are somewhat platform dependent.  These assertions may need to be adjusted.
-    // However, these particular numbers are left over from the days when we used to wildly
-    // overestimate font metrics, so these assertions should be pretty conservative.
-    REPORTER_ASSERT(r, SkRect::MakeLTRB(-110, 0, 140, 60) .contains(bbh.fEntries[0].bounds));
-    REPORTER_ASSERT(r, SkRect::MakeLTRB(-80, 20, 180, 100).contains(bbh.fEntries[1].bounds));
+    // 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(-110,  0, 140, 60)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(-80, 20, 180, 100)));
 }
 
 // Base test to ensure start/stop range is respected