Implicit SkTextBlob bounds

src/core/SkTextBlob.cpp

Index: src/core/SkTextBlob.cpp
diff --git a/src/core/SkTextBlob.cpp b/src/core/SkTextBlob.cpp
index b815c263a8ddfcf659bc868d2b52d187cb946f10..4f1df970982f75c619cbb212b2bb47c326e4843f 100644
--- a/src/core/SkTextBlob.cpp
+++ b/src/core/SkTextBlob.cpp
@@ -318,7 +318,38 @@ void SkTextBlobBuilder::updateDeferredBounds() {
         return;
     }
 
-    // FIXME: measure the current run & union bounds
+    SkASSERT(fLastRun >= sizeof(SkTextBlob));
+    SkTextBlob::RunRecord* run = reinterpret_cast<SkTextBlob::RunRecord*>(fStorage.get() +
+                                                                          fLastRun);
+    SkRect runBounds = SkRect::MakeEmpty();
+    if (SkTextBlob::kDefault_Positioning == run->positioning()) {
+        run->font().measureText(run->glyphBuffer(),
+                                run->glyphCount() * sizeof(uint16_t),

[mtklein, 2014/10/16 17:03:59]

So we're always GlyphID encoded in SkTextBlobs?

[reed1, 2014/10/16 17:06:17]

agreed. If blobs require glyphs, then lets at least SkASSERT() that about the
run's font/paint.

[f(malita), 2014/10/16 17:21:56]

We already do, when allocating runs (allocInternal).

[reed1, 2014/10/16 17:27:18]

 the value of an assert here is to document that *this* code is making that
assumption.

[f(malita), 2014/10/16 17:52:43]

Done.

+                                &runBounds);
+    } else {
+        SkTextBlob::GlyphPositioning positioning = run->positioning();
+        SkASSERT(SkTextBlob::kFull_Positioning == positioning
+                 || SkTextBlob::kHorizontal_Positioning == positioning);
+
+        SkScalar* glyphOffset = run->posBuffer();
+        for (unsigned i = 0; i < run->glyphCount(); ++i) {
+            SkRect glyphBounds;
+            run->font().measureText(run->glyphBuffer() + i,
+                                    sizeof(uint16_t),
+                                    &glyphBounds);
+            glyphBounds.offset(*glyphOffset,

[mtklein, 2014/10/16 17:03:59]

Can't hurt to comment this up a little, or perhaps move the conditional outside
the call to offset?

if (SkTextBlob::kFullPositioning == positioning) {
  glyphBounds.offset(glyphOffset[0], glyphOffset[1]);  // x,y,x,y,x,y...
  glyphOffset += 2;
} else {  SkASSERT(SkTextBlob::kHorizontal_Positioning == positioning);
  glyphBounds.offset(glyphOffset[0], 0);  // x,x,x,x,x,..., y is common to all,
handled by runBounds.offset(run->offset()) later.
  glyphOffset += 1;
}

[f(malita), 2014/10/16 17:21:56]

Will do.

+                               (SkTextBlob::kFull_Positioning == positioning)
+                               ? *(glyphOffset + 1) : 0);

[mtklein, 2014/10/16 17:06:20]

Wait a sec, are the offsets relative to each other, or relative to the start? 
If they're relative to the start, aren't we going to go way over?

Perhaps add a unit test?

[f(malita), 2014/10/16 17:21:56]

They're relative to the start, and measureText always computes glyphBounds
relative to (0,0).
A rigorous unit test is kind of tricky as there are very few assumption we can
make about where actual glyphs draw/where their bounds are. Probably doable with
some padding & common sense assumptions, will look into it. In the meantime,
note that the textblob GM does provide coverage: it draws a rect for the blob
bounds (only correct/visible after this CL).

+            runBounds.join(glyphBounds);
+            glyphOffset += SkTextBlob::ScalarsPerGlyph(positioning);
+        }
+
+        SkASSERT((void*)glyphOffset <= SkTextBlob::RunRecord::Next(run));

[mtklein, 2014/10/16 17:03:59]

Can we assert == here, or is there other data stored at the end?

[f(malita), 2014/10/16 17:21:56]

There's padding/alignment.

+    }
+
+    runBounds.offset(run->offset());
+
+    fBounds.join(runBounds);
     fDeferredBounds = false;
 }