Implicit SkTextBlob bounds

Implicit SkTextBlob bounds

Compute blob bounds when the client doesn't pass explicit run rects to
the builder. This allows us to remove a couple of internal workarounds.

R=reed@google.com,mtklein@google.com,bungeman@google.com

Committed: https://skia.googlesource.com/skia/+/19653d1d004610b4ba07ce563a5701164e120e45

[f(malita), 2014-10-16 16:46:40]

fmalita@chromium.org changed reviewers:
+ robertphillips@google.com

[bungeman-skia, 2014-10-16 16:58:14]

lgtm, though I'm not sure how much weight I give that here.

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

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:327: run->glyphCount() * sizeof(uint16_t),
So we're always GlyphID encoded in SkTextBlobs?

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:340: glyphBounds.offset(*glyphOffset,
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;
}

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:347: SkASSERT((void*)glyphOffset <=
SkTextBlob::RunRecord::Next(run));
Can we assert == here, or is there other data stored at the end?

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

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:327: run->glyphCount() * sizeof(uint16_t),
On 2014/10/16 17:03:59, mtklein wrote:
> So we're always GlyphID encoded in SkTextBlobs?

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

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

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:342: ? *(glyphOffset + 1) : 0);
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?

[robertphillips, 2014-10-16 17:13:44]

lgtm fwiw

[f(malita), 2014-10-16 17:21:57]

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:327: run->glyphCount() * sizeof(uint16_t),
On 2014/10/16 17:06:17, reed1 wrote:
> On 2014/10/16 17:03:59, mtklein wrote:
> > So we're always GlyphID encoded in SkTextBlobs?
> 
> agreed. If blobs require glyphs, then lets at least SkASSERT() that about the
> run's font/paint.

We already do, when allocating runs (allocInternal).

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:340: glyphBounds.offset(*glyphOffset,
On 2014/10/16 17:03:59, mtklein wrote:
> 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;
> }

Will do.

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:342: ? *(glyphOffset + 1) : 0);
On 2014/10/16 17:06:20, mtklein wrote:
> 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?

They're relative to the start, and measureText always computes glyphBounds
relative to (0,0).

> 
> Perhaps add a unit test?

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).

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:347: SkASSERT((void*)glyphOffset <=
SkTextBlob::RunRecord::Next(run));
On 2014/10/16 17:03:59, mtklein wrote:
> Can we assert == here, or is there other data stored at the end?

There's padding/alignment.

[mtklein, 2014-10-16 17:25:02]

lgtm

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

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:327: run->glyphCount() * sizeof(uint16_t),
On 2014/10/16 17:21:56, Florin Malita wrote:
> On 2014/10/16 17:06:17, reed1 wrote:
> > On 2014/10/16 17:03:59, mtklein wrote:
> > > So we're always GlyphID encoded in SkTextBlobs?
> > 
> > agreed. If blobs require glyphs, then lets at least SkASSERT() that about
the
> > run's font/paint.
> 
> We already do, when allocating runs (allocInternal).

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

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

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/654873003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:327: run->glyphCount() * sizeof(uint16_t),
On 2014/10/16 17:27:18, reed1 wrote:
> On 2014/10/16 17:21:56, Florin Malita wrote:
> > On 2014/10/16 17:06:17, reed1 wrote:
> > > On 2014/10/16 17:03:59, mtklein wrote:
> > > > So we're always GlyphID encoded in SkTextBlobs?
> > > 
> > > agreed. If blobs require glyphs, then lets at least SkASSERT() that about
> the
> > > run's font/paint.
> > 
> > We already do, when allocating runs (allocInternal).
> 
>  the value of an assert here is to document that *this* code is making that
> assumption.

Done.

[reed1, 2014-10-16 18:15:47]

lgtm

[f(malita), 2014-10-16 18:47:06]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-10-16 18:47:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654873003/80001

[commit-bot: I haz the power, 2014-10-16 18:53:40]

Committed patchset #5 (id:80001) as 19653d1d004610b4ba07ce563a5701164e120e45