SkTextBlob

SkTextBlob

Initial implementation.

R=reed@google.com,jbroman@chromium.org,mtklein@google.com,bungeman@google.com,robertphillips@google.com

Committed: https://skia.googlesource.com/skia/+/00d5c2c6523321d25b32905ff4822f083a4173ee

[f(malita), 2014-08-13 21:10:46]

Sharing mostly for API review.

The implementation is incomplete, although it does work in immediate mode.
SkPictureRecord serialization in particular is horrific, but maybe in the brave
new SkRecord world we don't care?

[mtklein, 2014-08-14 14:00:42]

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:29: static const SkTextChunk* Create(const uint16_t*
glyphs, size_t count, const SkScalar* pos,
Doesn't this need another SkScalar for the shared Y?

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:57: mutable SkRect fBounds;
We create TextChunks all at once right?  If so, let's proactively compute the
bounds?  Lazy things like this are really annoying to think about when dealing
with threads.

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:58: mutable bool   fBoundsDirty   : 1;
I'd drop the bit packing for now?  It's particularly hard to think about what it
means to have mutable bits and immutable bits sharing the same byte when
considering thread safety.  I think at best we'll get yelled at by TSAN. :)

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

https://codereview.chromium.org/473633002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:68: init(glyphs, count, bounds, (const SkScalar*)pos);
Can't hurt to label this explicitly as reinterpret_cast.  Don't want that sort
of thing to sneak by the reader.

[f(malita), 2014-08-14 14:29:26]

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:29: static const SkTextChunk* Create(const uint16_t*
glyphs, size_t count, const SkScalar* pos,
On 2014/08/14 14:00:42, mtklein wrote:
> Doesn't this need another SkScalar for the shared Y?

I'm hoping most blobs will be relocatable (at least for simple text) such that
we don't have to rebuild them when the positioning attributes change.

To encourage that thinking, the current plan is to only store relative offsets
(=> shared Y == 0), and use an 'offset' param to position the blob at draw time.

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:57: mutable SkRect fBounds;
On 2014/08/14 14:00:42, mtklein wrote:
> We create TextChunks all at once right?  If so, let's proactively compute the
> bounds?  Lazy things like this are really annoying to think about when dealing
> with threads.

It's not completely clear whether we *always* need bounds - I think the BBH
recorder uses heuristics for example and not sure if Blink computes them
unconditionally (at least not at layout time). Will need to look into it some
more to figure out the exact uses.

If we don't always compute them today, I'm somewhat worried about overhead --
although I'm inclined to heed your advice and revisit if it turns out to be a
problem. MikeR, WDYT?

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:58: mutable bool   fBoundsDirty   : 1;
On 2014/08/14 14:00:42, mtklein wrote:
> I'd drop the bit packing for now?  It's particularly hard to think about what
it
> means to have mutable bits and immutable bits sharing the same byte when
> considering thread safety.  I think at best we'll get yelled at by TSAN. :)

Done.

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

https://codereview.chromium.org/473633002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:68: init(glyphs, count, bounds, (const SkScalar*)pos);
On 2014/08/14 14:00:42, mtklein wrote:
> Can't hurt to label this explicitly as reinterpret_cast.  Don't want that sort
> of thing to sneak by the reader.

Done.

[mtklein, 2014-08-14 14:38:45]

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:57: mutable SkRect fBounds;
> If we don't always compute them today, I'm somewhat worried about overhead --
> although I'm inclined to heed your advice and revisit if it turns out to be a
> problem. MikeR, WDYT?

I'm going to try have pictures start querying the bounds of text all the time. 
Those heuristics we use today are... dicey.

[jbroman, 2014-08-14 14:46:02]

couple minor questions

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:116: const SkTextBlob* build();
nit: If SkTextBlobs are always immutable, why the "const" modifier here? I fear
this will just make us say "const SkTextBlob" everywhere and never simply
"SkTextBlob". Unsure.

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:120: SkTDArray<const SkTextChunk*>  fChunks;
Random thought: pros/cons of having this approach vs just having an
SkTArray<SkTextChunk> and having the caller construct them on the stack (passing
in a const SkTextChunk&, or giving the constructor args to addChunk and having
it be constructed in place)?

There's something to be said about memory locality, but I'm not sure how the
tradeoffs play out, especially while this holds a full SkPaint.

[reed1, 2014-08-14 15:55:49]

https://codereview.chromium.org/473633002/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/473633002/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:960: void drawTextBlob(const SkTextBlob* blob, const
SkPoint& offset, const SkPaint& paint);
For consistency (and because I think having to cons-up a struct just for 2
numbers of overkill) I think x,y is better than a point.

https://codereview.chromium.org/473633002/diff/60001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/60001/include/core/SkTextBlob....
include/core/SkTextBlob.h:99: void addChunk(const SkTextChunk* chunk);
Hmmm, I was imagining an alternate adder style:

uint16_t* addRun(int count, const SkPaint&, const SkRect* bounds = NULL);

This would return writable memory (only valid until the next call to addChunk)
so that the caller need not do their own allocation, only to have us also alloc
and memcpy.

[f(malita), 2014-08-14 16:13:36]

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:57: mutable SkRect fBounds;
On 2014/08/14 14:38:45, mtklein wrote:
> I'm going to try have pictures start querying the bounds of text all the time.

> Those heuristics we use today are... dicey.

Awesome, that settles it :)

I think it also makes sense to store the bounds per blob, not per chunk. Done.

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:116: const SkTextBlob* build();
On 2014/08/14 14:46:02, jbroman wrote:
> nit: If SkTextBlobs are always immutable, why the "const" modifier here? I
fear
> this will just make us say "const SkTextBlob" everywhere and never simply
> "SkTextBlob". Unsure.

In general, I would actually find that desirable: forcing clients to be
const-explicit => more info for the compiler to take advantage of. Whether it
buys us much for naturally immutable objects, I'm not sure. I guess we can relax
the factory methods and see if anyone complains.

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:120: SkTDArray<const SkTextChunk*>  fChunks;
On 2014/08/14 14:46:02, jbroman wrote:
> Random thought: pros/cons of having this approach vs just having an
> SkTArray<SkTextChunk> and having the caller construct them on the stack
(passing
> in a const SkTextChunk&, or giving the constructor args to addChunk and having
> it be constructed in place)?

The reason it turned out this way is trying to avoid extra chunk copying and
making sure we don't overallocate the array (due to dynamic growth/internal
reserve). It's possible there are more elegant solutions.


> this holds a full SkPaint.

Hopefully not for long :)

[mtklein, 2014-08-14 16:22:23]

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:116: const SkTextBlob* build();
I'm in (perhaps, am?) the the-more-const-the-better camp.  We have a couple
classes that are immutable but not passed as const (SkTypeface, SkXfermode) in
Skia, and I always have to think twice about whether or not the thing is going
to be changed by some function call I pass it to.  Even if it's technically
meaningless, const* is a nice readability signal.

Returning const SkTextBlob* does also leave us a bit more flexibility to mutate
SkTextBlobs internally before they escape the builder API.

[f(malita), 2014-08-14 16:27:01]

https://codereview.chromium.org/473633002/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/473633002/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:960: void drawTextBlob(const SkTextBlob* blob, const
SkPoint& offset, const SkPaint& paint);
On 2014/08/14 15:55:49, reed1 wrote:
> For consistency (and because I think having to cons-up a struct just for 2
> numbers of overkill) I think x,y is better than a point.

SGTM, will do.

https://codereview.chromium.org/473633002/diff/60001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/60001/include/core/SkTextBlob....
include/core/SkTextBlob.h:99: void addChunk(const SkTextChunk* chunk);
On 2014/08/14 15:55:49, reed1 wrote:
> Hmmm, I was imagining an alternate adder style:
> 
> uint16_t* addRun(int count, const SkPaint&, const SkRect* bounds = NULL);
> 
> This would return writable memory (only valid until the next call to addChunk)
> so that the caller need not do their own allocation, only to have us also
alloc
> and memcpy. 

I like this idea, and there's no reason not to have multiple adder variants.

A couple of wrinkles:

* this lowers the immutability guarantee some: clients could potentially hold on
to these pointers and mess with constructed chunks

* for positioned glyphs, we need to return two preallocated buffers -- the api
for it makes me cringe a little

Let me play with this and see how it turns out.

[f(malita), 2014-08-18 19:05:46]

On 2014/08/14 16:27:01, Florin Malita wrote:
>
https://codereview.chromium.org/473633002/diff/60001/include/core/SkTextBlob....
> include/core/SkTextBlob.h:99: void addChunk(const SkTextChunk* chunk);
> On 2014/08/14 15:55:49, reed1 wrote:
> > Hmmm, I was imagining an alternate adder style:
> > 
> > uint16_t* addRun(int count, const SkPaint&, const SkRect* bounds = NULL);
> > 
> > This would return writable memory (only valid until the next call to
addChunk)
> > so that the caller need not do their own allocation, only to have us also
> alloc
> > and memcpy. 
> 
> I like this idea, and there's no reason not to have multiple adder variants.
> 
> A couple of wrinkles:
> 
> * this lowers the immutability guarantee some: clients could potentially hold
on
> to these pointers and mess with constructed chunks
> 
> * for positioned glyphs, we need to return two preallocated buffers -- the api
> for it makes me cringe a little
> 
> Let me play with this and see how it turns out.

Updated API, now with builder-managed chunk storage -- PTAL.

Main differences:

* Chunks (now "runs") are no longer managed by clients, but instead allocated
internally by the builder.
* Runs share consolidated glyph and pos buffers, reducing allocator churn and
fragmentation.
* Runs use 32-bit indices into the consolidated buffers instead of pointers =>
mem savings on 64bit platforms.
* Runs are constructed using specialized builder types, coercing clients into
using the correct positioning API

Use example:

  SkPaint somePaint;
  uint16_t someGlyphs[100];
  SkScalar someOffsets[100];

  SkTextBlobBuilder blobBuilder();

  // Add a default-position run, in one swoop
  SkTextBlobBuilder::RunBuilder<SkTextBlob::kDefault_Positioning> runBuilder =
    blobBuilder.startRun<SkTextBlob::kDefault_Positioning>(somePaint, 100);
  runBuilder.addGlyphs(someGlyphs, 100);

  // Add a horizontal run, one glyph at a time
  SkTextBlobBuilder::RunBuilder<SkTextBlob::kHorizontal_Positioning>
hPosRunBuilder =
    blobBuilder.startRun<SkTextBlob::kHorizontal_Positioning>(somePaint, 100);
  for (unsigned i = 0; i < 100; ++i) {
    hPosRunBuilder.addGlyph(someGlyphs[i], someOffsets[i]);
  }

  // Now freeze the blob and draw it
  SkAutoTUnref<const SkTextBlob> blob(blobBuilder.build());
  canvas->drawTextBlob(blob, 100, 100, somePaint);



(the overall plumbing is much more shallow for the last patch set - I figured
there's not reason wasting time on it before nailing the API details)

[robertphillips, 2014-08-19 15:06:48]

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:68: private:
// This is ... ?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:91: Helper class for constructing SkTextBlobs.
// Expected use is:
//     SkTextBlobBuilder tbb;
//     for (textruns) {
//        RunBuilderBase rbb = tbb.startRun(...);
//        rbb.addGlyph(...);
//     }
//     SkAutoTUnref<SkTextBlob> result(tbb.build());
?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:94: private:
// This class ... 
specifically what sort of API is this class provided (e.g., the addGlyph(s)
variants)
?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:128: const SkRect* bounds = NULL) {
this->

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:160: 
// The Y-value will be supplied in the using SkCanvas::drawTextBlob call ?
// All the x values are relative to the x value passed to drawTextBlob ?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:169: 
// All the supplied positions are relative to the x,y supplied to the
drawTextBlob call ?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:175: void addGlyph(uint16_t glyph, SkScalar x,
SkScalar y);
const SkPoint&

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:178: size_t count);
pox ?

[jbroman, 2014-08-19 17:39:16]

a few comments/questions; only looked at SkTextBlob.h

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:42: const SkPaint*  font;
nit: Why not "const SkPaint&"? Is this a sign that this can be null?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:66: const RunInfo runInfo(unsigned runIndex) const;
Returning a const struct is meaningless (it's the same as returning a non-const
one). Unless Skia does this elsewhere, it seems clearer to return
"SkTextBlob::RunInfo".

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:81: const uint16_t*      fGlyphBuffer;
Any reason not to use SkAutoTMalloc or something similar here instead of writing
a destructor? If this is idiomatic Skia, disregard. (Here and below.)

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:86: mutable uint32_t     fUniqueID;
Why "mutable"? Making this lazy kind of scares me (what if two threads try to
get the unique ID at once?); why not just populate it on construction?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
unsigned glyphCount = 0,
Why document the glyphCount and bounds arguments, but not font?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
unsigned glyphCount = 0,
What do we get by making this a two-step process (start run, then supply glyph
positioning) as opposed to simply exposing addRun methods that take all the
needed information?

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:166: void addGlyph(uint16_t glyph, SkScalar x);
Would we ever want to add a singular glyph? If it's intended that users always
supply whole runs, it seems easier not to expand the API prematurely.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:179: void addGlyphs(const uint16_t glyphs[], const
SkPoint pox[], size_t count);
What does this method do, given this is the "full positioning" case?

[jbroman, 2014-08-19 17:41:38]

On 2014/08/19 17:39:16, jbroman wrote:
> a few comments/questions; only looked at SkTextBlob.h
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob.h
> File include/core/SkTextBlob.h (right):
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:42: const SkPaint*  font;
> nit: Why not "const SkPaint&"? Is this a sign that this can be null?
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:66: const RunInfo runInfo(unsigned runIndex) const;
> Returning a const struct is meaningless (it's the same as returning a
non-const
> one). Unless Skia does this elsewhere, it seems clearer to return
> "SkTextBlob::RunInfo".
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:81: const uint16_t*      fGlyphBuffer;
> Any reason not to use SkAutoTMalloc or something similar here instead of
writing
> a destructor? If this is idiomatic Skia, disregard. (Here and below.)
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:86: mutable uint32_t     fUniqueID;
> Why "mutable"? Making this lazy kind of scares me (what if two threads try to
> get the unique ID at once?); why not just populate it on construction?
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
> unsigned glyphCount = 0,
> Why document the glyphCount and bounds arguments, but not font?
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
> unsigned glyphCount = 0,
> What do we get by making this a two-step process (start run, then supply glyph
> positioning) as opposed to simply exposing addRun methods that take all the
> needed information?
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:166: void addGlyph(uint16_t glyph, SkScalar x);
> Would we ever want to add a singular glyph? If it's intended that users always
> supply whole runs, it seems easier not to expand the API prematurely.
> 
>
https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
> include/core/SkTextBlob.h:179: void addGlyphs(const uint16_t glyphs[], const
> SkPoint pox[], size_t count);
> What does this method do, given this is the "full positioning" case?

I realize now some of these questions are partially answered by the above review
comments, which I missed.
That said, I do think the header should be a little more self-explanatory in
this regard.

[f(malita), 2014-08-20 01:21:58]

Updated per review comments + simplified alloc-based API.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:42: const SkPaint*  font;
On 2014/08/19 17:39:16, jbroman wrote:
> nit: Why not "const SkPaint&"? Is this a sign that this can be null?

Good point -- done.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:66: const RunInfo runInfo(unsigned runIndex) const;
On 2014/08/19 17:39:16, jbroman wrote:
> Returning a const struct is meaningless (it's the same as returning a
non-const
> one). Unless Skia does this elsewhere, it seems clearer to return
> "SkTextBlob::RunInfo".

Removed from the API.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:68: private:
On 2014/08/19 15:06:47, robertphillips wrote:
> // This is ... ?

Done.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:81: const uint16_t*      fGlyphBuffer;
On 2014/08/19 17:39:16, jbroman wrote:
> Any reason not to use SkAutoTMalloc or something similar here instead of
writing
> a destructor? If this is idiomatic Skia, disregard. (Here and below.)

Done.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:86: mutable uint32_t     fUniqueID;
On 2014/08/19 17:39:16, jbroman wrote:
> Why "mutable"? Making this lazy kind of scares me (what if two threads try to
> get the unique ID at once?); why not just populate it on construction?

(this is the same pattern used by other Skia ID-ed classes)

The ID generation logic needs to be thread-safe (the impl uses atomic_inc) =>
we're trying to avoid the synchronization overhead for objects which never use
their ID => we defer the actual initialization until someone asks for it.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:94: private:
On 2014/08/19 15:06:47, robertphillips wrote:
> // This class ... 
> specifically what sort of API is this class provided (e.g., the addGlyph(s)
> variants)
> ?

Removed.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
unsigned glyphCount = 0,
On 2014/08/19 17:39:15, jbroman wrote:
> What do we get by making this a two-step process (start run, then supply glyph
> positioning) as opposed to simply exposing addRun methods that take all the
> needed information?

The latter forces clients to format data in a Skia-friendly manner, possibly
introducing extra copies. For example, given a Blink GlyphBuffer holding
advances we need to allocate another buffer, convert advances -> positions and
then pass that to addRun.

Using builder managed storage we could avoid allocating the intermediate buffer
and feed glyphs+positions directly into the builder, one at a time, as the
conversion progresses.

But after discussing this with Mike, I think we'll go with the simpler/more
direct solution for now. We can add advance-aware builder methods or convert
Blink to store positions if needed, later.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:127: RunBuilder<POS> startRun(const SkPaint& font,
unsigned glyphCount = 0,
On 2014/08/19 17:39:16, jbroman wrote:
> Why document the glyphCount and bounds arguments, but not font?

Done.

https://codereview.chromium.org/473633002/diff/140001/include/core/SkTextBlob...
include/core/SkTextBlob.h:128: const SkRect* bounds = NULL) {
On 2014/08/19 15:06:47, robertphillips wrote:
> this->

Done.

[reed1, 2014-08-20 01:29:42]

lgtm modulo naming nits

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:124: uint16_t* allocRun(const SkPaint& font, size_t
count, SkPoint offset,
nit: we usually use x,y instead of a point for a parameter (see drawText,
drawBitmap, drawTextBlob)
nit: counts are ints, not size_t when they are not explicitly byte_lengths

nit: I suggest explicit names for each.

e.g.
    allocRunDefaultAdvances (or allocRun)
    allocRunPosH
    allocRunPos

[robertphillips, 2014-08-20 14:52:18]

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:14: #include "SkTDArray.h"
Do we still need SkTemplates.h ?

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:74: const SkAutoTMalloc<SkScalar>       fPosBuffer;
Can this be an SkTDArray ?

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:78: mutable uint32_t                    fUniqueID;
INHERITED ?

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:159: private:
allocInternal ?

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

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:171: SkScalar yOffset, const SkRect* bounds) {
this->allocInternal ?

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:174: 
this-> ?

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:181: 
this-> ?

[jbroman, 2014-08-20 15:10:32]

This looks good to me.

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

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:135: fPosBuffer.append(count * positioning);
nit: It's not immediately obvious why this works (because the enum integer
mapping permits this). Why not a little inline function to isolate this logic,
or at least a comment?

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:186: const SkTextBlob* leBlob;
nit: what does "le blob" mean? Why not "blob"?

[f(malita), 2014-08-20 15:47:48]

Updated per review.

Also changed the allocRun* return value to be a const ref to an internal
RunBuffer.

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:14: #include "SkTDArray.h"
On 2014/08/20 14:52:18, robertphillips wrote:
> Do we still need SkTemplates.h ?

Done.

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:74: const SkAutoTMalloc<SkScalar>       fPosBuffer;
On 2014/08/20 14:52:18, robertphillips wrote:
> Can this be an SkTDArray ?

It could, but technically the count and reserve fields are redundant for a
constructed blob and storing raw pointers saves us four ints per blob.

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:78: mutable uint32_t                    fUniqueID;
On 2014/08/20 14:52:17, robertphillips wrote:
> INHERITED ?

Done.

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:124: uint16_t* allocRun(const SkPaint& font, size_t
count, SkPoint offset,
On 2014/08/20 01:29:41, reed1 wrote:
> nit: we usually use x,y instead of a point for a parameter (see drawText,
> drawBitmap, drawTextBlob)
> nit: counts are ints, not size_t when they are not explicitly byte_lengths
> 
> nit: I suggest explicit names for each.
> 
> e.g.
>     allocRunDefaultAdvances (or allocRun)
>     allocRunPosH
>     allocRunPos

Done.

https://codereview.chromium.org/473633002/diff/160001/include/core/SkTextBlob...
include/core/SkTextBlob.h:159: private:
On 2014/08/20 14:52:17, robertphillips wrote:
> allocInternal ?

Done.

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

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:135: fPosBuffer.append(count * positioning);
On 2014/08/20 15:10:32, jbroman wrote:
> nit: It's not immediately obvious why this works (because the enum integer
> mapping permits this). Why not a little inline function to isolate this logic,
> or at least a comment?

Done.

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:171: SkScalar yOffset, const SkRect* bounds) {
On 2014/08/20 14:52:18, robertphillips wrote:
> this->allocInternal ?

Done.

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:174: 
On 2014/08/20 14:52:18, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:181: 
On 2014/08/20 14:52:18, robertphillips wrote:
> this-> ?

Done.

https://codereview.chromium.org/473633002/diff/160001/src/core/SkTextBlob.cpp...
src/core/SkTextBlob.cpp:186: const SkTextBlob* leBlob;
On 2014/08/20 15:10:32, jbroman wrote:
> nit: what does "le blob" mean? Why not "blob"?

Done.

[f(malita), 2014-08-21 15:34:31]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:35:21]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/473633002/220001

[f(malita), 2014-08-21 15:40:38]

The CQ bit was unchecked by fmalita@chromium.org

[f(malita), 2014-08-21 15:44:46]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:45:33]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/473633002/240001

[commit-bot: I haz the power, 2014-08-21 15:53:31]

Committed patchset #13 (240001) as 00d5c2c6523321d25b32905ff4822f083a4173ee