Creation of GrBatchAtlas and Distancefieldpathrenderer batch

Creation of GrBatchAtlas and Distancefieldpathrenderer batch

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5bf99f1ca8f30287803b594d06c60a7b6796ad45

[joshualitt, 2015-03-05 21:01:23]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, jvanverth@google.com, robertphillips@google.com

[joshualitt, 2015-03-05 21:01:46]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com

[joshualitt, 2015-03-05 21:01:46]

Go tough on me guys, lets get this stuff right so it works well for text.

[bsalomon, 2015-03-05 22:11:09]

need to read this in a lot more depth, but some early comments.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:58: if (fAtlas) {
delete is legal on NULL.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:529:
fGeoData.push_back_n(that->geoData()->count(), that->geoData()->begin());
seems like this could lead to unfortunate combinations.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:545: const SkScalar kAntiAliasPad =
1.0f;
does this require c++11? (do we allow anything else?)

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:574: GrTexture* texture =
gpu->createTexture(desc, true, NULL, 0);
not using the cache?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:3: * Copyright 2010 Google Inc.
copyrights on new files are all 2010

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:89: class GrPlotUpdater {
It really seems like this should be a generic uploader with a GrPlotUpdater
subclass.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:127: void uploadPlotsToTexture();
SkDEBUGCODE()?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/gl/builders/GrGL...
File src/gpu/gl/builders/GrGLVertexShaderBuilder.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/gl/builders/GrGL...
src/gpu/gl/builders/GrGLVertexShaderBuilder.h:12: #include
"../../GrGeometryProcessor.h"
is the ../../ needed?

[bsalomon, 2015-03-05 22:20:49]

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:22: // The backing GrTexture for a set of GrAtlases is
broken into a spatial grid of GrPlots. When
for a GrAtlas, not a set of atlases, right?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:23: // a GrAtlas needs space on the texture, it requests
a GrPlot. Each GrAtlas can claim one
atlases claim plots or have plots?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:26: // GrAtlas can request a new GrPlot via
GrAtlas::addToAtlas().
Maybe in here state somewhere that plots accumulate new data but we never
replace data in a plot, we spill to the next plot.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:28: // If all GrPlots are allocated, the replacement
strategy is up to the client. The drawToken is
Document that the unit of replacement is the plot? Or is that not a requirement?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:32: class GrBatchPlot : public SkRefCnt {
Do plots need to be exposed to client of GrBatchAtlas or can this be defined in
the GrBatchAtlas.cpp

I also wonder what about this is really "batch"? It seems pretty reusable
outside of the context of GrBatchTarget.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:38: // This returns a plot ID unique to each plot in a
given GrAtlas. They are
we usually use genID to mean an ID that changes when the object is modifed, and
uniqueID to mean an unchanging ID that refers to an object.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:41: int generation() const { return fGeneration; }
comment about how this changes when content of the plot changes?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:47: BatchToken lastUploadToken() const { return
fLastUpload; }
These should be documented, esp lastRefToken

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:132: return fPlotArray[index]->generation() ==
this->getGenerationFromID(id);
is this right? I would think that we'd be using id() not generation() here, but
probably just don't understand.

[Jvsquare, 2015-03-06 15:45:35]

jvanverth@chromium.org changed reviewers:
+ jvanverth@chromium.org

[Jvsquare, 2015-03-06 15:45:36]

A few quick comments:

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:241: // get LRU, queue up an upload.  However, if our
plot was referenced in this draw, then we can't
I don't see where we are queuing up an upload here. Is that further down?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:22: // The backing GrTexture for a set of GrAtlases is
broken into a spatial grid of GrPlots. When
On 2015/03/05 22:20:49, bsalomon wrote:
> for a GrAtlas, not a set of atlases, right?

This comment block is from GrAtlas.h, so any changes here should be made there
as well. That said, there's a lot of duplicated code between GrBatchAtlas and
GrAtlas -- is it possible to subclass GrAtlas?

[bsalomon, 2015-03-06 16:34:59]

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:529:
fGeoData.push_back_n(that->geoData()->count(), that->geoData()->begin());
On 2015/03/05 22:11:08, bsalomon wrote:
> seems like this could lead to unfortunate combinations.

Retracted... realized this doesn't lead to caching the paths together in the
atlas.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:24: #define FONT_CACHE_STATS 0
ATLAS_STAST?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:120:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("skia.gpu"), "GrPlot::uploadToTexture");
update string?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:126: fGpu->writeTexturePixels(fTexture,
I think we really need a constrained API for the updater objects. Maybe the
updater get the data to be uploaded from the plot and use a constrained API on
the batch target or a batch target helper?

class TextureUploader {
public:
   bool writePixels(texture, ...) { return fGpu->writePixels(...); }
private:
   GrGpu* fGpu;
};

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:203: // can just piggyback
I understand this but might not in a few months...

"""
If our most recent upload has already occurred then we have to insert a new
upload. Otherwise, we already have a scheduled upload that hasn't yet ocurred.
This new update will piggy back on that previously scheduled update.
"""
?

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:265: batchTarget->update(GrPlotUpdater(newPlot));
@Jvsquare: I think it is here.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:71: int fPlotWidth;
Do these really need plot in the name? The object type is plot.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:75: GrGpu* fGpu;
Does this really need the gpu object, should it be able to make all the API
calls on GPU? Does it need to be persistent.

[joshualitt, 2015-03-06 22:09:11]

Patchset #7 (id:120001) has been deleted

[joshualitt, 2015-03-09 19:19:24]

Patchset #7 (id:140001) has been deleted

[joshualitt, 2015-03-09 19:35:14]

Okay, I think I got most of the feedback. Please take a look.

The only perf regression in any of the skps which really draw a bunch of paths
is chalkboard@1.0, due to the spilling issue(across linux, nexus5, and nexus10).
 As per discussion, I'll defer trying to create a more optimal plot management
strategy solely for this one skp in favor of redesigning the atlas down the
road.

[bsalomon, 2015-03-09 19:40:10]

On 2015/03/09 19:35:14, joshualitt wrote:
> Okay, I think I got most of the feedback. Please take a look.

Will take a look. In general, it would help to use the inline comment system
since it makes it easier for reviewers to track changes that are related to
comments.

> 
> The only perf regression in any of the skps which really draw a bunch of paths
> is mailto:chalkboard@1.0, due to the spilling issue(across linux, nexus5, and
nexus10).
>  As per discussion, I'll defer trying to create a more optimal plot management
> strategy solely for this one skp in favor of redesigning the atlas down the
> road.

[joshualitt, 2015-03-09 19:45:15]

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:58: if (fAtlas) {
On 2015/03/05 22:11:08, bsalomon wrote:
> delete is legal on NULL.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:545: const SkScalar kAntiAliasPad =
1.0f;
On 2015/03/05 22:11:08, bsalomon wrote:
> does this require c++11? (do we allow anything else?)

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrAADistanceFiel...
src/gpu/GrAADistanceFieldPathRenderer.cpp:574: GrTexture* texture =
gpu->createTexture(desc, true, NULL, 0);
On 2015/03/05 22:11:08, bsalomon wrote:
> not using the cache?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:24: #define FONT_CACHE_STATS 0
On 2015/03/06 16:34:59, bsalomon wrote:
> ATLAS_STAST?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:120:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("skia.gpu"), "GrPlot::uploadToTexture");
On 2015/03/06 16:34:59, bsalomon wrote:
> update string?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:126: fGpu->writeTexturePixels(fTexture,
On 2015/03/06 16:34:59, bsalomon wrote:
> I think we really need a constrained API for the updater objects. Maybe the
> updater get the data to be uploaded from the plot and use a constrained API on
> the batch target or a batch target helper?
> 
> class TextureUploader {
> public:
>    bool writePixels(texture, ...) { return fGpu->writePixels(...); }
> private:
>    GrGpu* fGpu;
> };

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:203: // can just piggyback
On 2015/03/06 16:34:59, bsalomon wrote:
> I understand this but might not in a few months...
> 
> """
> If our most recent upload has already occurred then we have to insert a new
> upload. Otherwise, we already have a scheduled upload that hasn't yet ocurred.
> This new update will piggy back on that previously scheduled update.
> """
> ?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.cpp...
src/gpu/GrBatchAtlas.cpp:241: // get LRU, queue up an upload.  However, if our
plot was referenced in this draw, then we can't
On 2015/03/06 15:45:36, Jvsquare wrote:
> I don't see where we are queuing up an upload here. Is that further down?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:3: * Copyright 2010 Google Inc.
On 2015/03/05 22:11:08, bsalomon wrote:
> copyrights on new files are all 2010

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:28: // If all GrPlots are allocated, the replacement
strategy is up to the client. The drawToken is
On 2015/03/05 22:20:49, bsalomon wrote:
> Document that the unit of replacement is the plot? Or is that not a
requirement?

I rewrote this entire comment in the cpp file

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:32: class GrBatchPlot : public SkRefCnt {
On 2015/03/05 22:20:49, bsalomon wrote:
> Do plots need to be exposed to client of GrBatchAtlas or can this be defined
in
> the GrBatchAtlas.cpp
> 
> I also wonder what about this is really "batch"? It seems pretty reusable
> outside of the context of GrBatchTarget.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:38: // This returns a plot ID unique to each plot in a
given GrAtlas. They are
On 2015/03/05 22:20:49, bsalomon wrote:
> we usually use genID to mean an ID that changes when the object is modifed,
and
> uniqueID to mean an unchanging ID that refers to an object.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:41: int generation() const { return fGeneration; }
On 2015/03/05 22:20:49, bsalomon wrote:
> comment about how this changes when content of the plot changes?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:47: BatchToken lastUploadToken() const { return
fLastUpload; }
On 2015/03/05 22:20:49, bsalomon wrote:
> These should be documented, esp lastRefToken 

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:71: int fPlotWidth;
On 2015/03/06 16:34:59, bsalomon wrote:
> Do these really need plot in the name? The object type is plot.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:75: GrGpu* fGpu;
On 2015/03/06 16:34:59, bsalomon wrote:
> Does this really need the gpu object, should it be able to make all the API
> calls on GPU? Does it need to be persistent.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:89: class GrPlotUpdater {
On 2015/03/05 22:11:08, bsalomon wrote:
> It really seems like this should be a generic uploader with a GrPlotUpdater
> subclass.

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:127: void uploadPlotsToTexture();
On 2015/03/05 22:11:08, bsalomon wrote:
> SkDEBUGCODE()?

Acknowledged.

https://codereview.chromium.org/975303005/diff/80001/src/gpu/GrBatchAtlas.h#n...
src/gpu/GrBatchAtlas.h:132: return fPlotArray[index]->generation() ==
this->getGenerationFromID(id);
On 2015/03/05 22:20:49, bsalomon wrote:
> is this right? I would think that we'd be using id() not generation() here,
but
> probably just don't understand.

this is correct, I have renamed id to index to make things clearer(ie, if the
index and generation are the same, its the same plot)

https://codereview.chromium.org/975303005/diff/80001/src/gpu/gl/builders/GrGL...
File src/gpu/gl/builders/GrGLVertexShaderBuilder.h (right):

https://codereview.chromium.org/975303005/diff/80001/src/gpu/gl/builders/GrGL...
src/gpu/gl/builders/GrGLVertexShaderBuilder.h:12: #include
"../../GrGeometryProcessor.h"
On 2015/03/05 22:11:09, bsalomon wrote:
> is the ../../ needed?

Acknowledged.

[joshualitt, 2015-03-10 17:39:17]

Updated to include some offline feedback from Brian

[bsalomon, 2015-03-11 13:34:13]

Some comments I typed two days ago but I forgot to publish, may be stale.

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:32: class GrBatchPlot : public SkRefCnt {
Doesn't need the Gr prefix when in a cpp

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:47: bool addSubImage(int width, int height, const void*
image, SkIPoint16* loc)  {
should this take a rowbytes? It seems like we're assuming the data passed in
shares its rowbytes with the plot. Is that always true?

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:345: int index = this->getIndexFromID(id);
this is much clearer.

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h
File src/gpu/GrBatchTarget.h (right):

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h...
src/gpu/GrBatchTarget.h:18: class GrTextureUploader {
Nest this and uploader in BatchTarget? They seem only intended to be used WRT
BatchTarget.

[joshualitt, 2015-03-11 16:03:27]

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:32: class GrBatchPlot : public SkRefCnt {
On 2015/03/11 13:34:12, bsalomon wrote:
> Doesn't need the Gr prefix when in a cpp

Acknowledged.

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:47: bool addSubImage(int width, int height, const void*
image, SkIPoint16* loc)  {
On 2015/03/11 13:34:12, bsalomon wrote:
> should this take a rowbytes? It seems like we're assuming the data passed in
> shares its rowbytes with the plot. Is that always true?

might want to check I did this one right

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h
File src/gpu/GrBatchTarget.h (right):

https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h...
src/gpu/GrBatchTarget.h:18: class GrTextureUploader {
On 2015/03/11 13:34:12, bsalomon wrote:
> Nest this and uploader in BatchTarget? They seem only intended to be used WRT
> BatchTarget.

Acknowledged.

[joshualitt, 2015-03-12 15:01:09]

On 2015/03/11 16:03:27, joshualitt wrote:
> https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cpp
> File src/gpu/GrBatchAtlas.cpp (right):
> 
>
https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
> src/gpu/GrBatchAtlas.cpp:32: class GrBatchPlot : public SkRefCnt {
> On 2015/03/11 13:34:12, bsalomon wrote:
> > Doesn't need the Gr prefix when in a cpp
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchAtlas.cp...
> src/gpu/GrBatchAtlas.cpp:47: bool addSubImage(int width, int height, const
void*
> image, SkIPoint16* loc)  {
> On 2015/03/11 13:34:12, bsalomon wrote:
> > should this take a rowbytes? It seems like we're assuming the data passed in
> > shares its rowbytes with the plot. Is that always true?
> 
> might want to check I did this one right
> 
> https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h
> File src/gpu/GrBatchTarget.h (right):
> 
>
https://codereview.chromium.org/975303005/diff/200001/src/gpu/GrBatchTarget.h...
> src/gpu/GrBatchTarget.h:18: class GrTextureUploader {
> On 2015/03/11 13:34:12, bsalomon wrote:
> > Nest this and uploader in BatchTarget? They seem only intended to be used
WRT
> > BatchTarget.
> 
> Acknowledged.

Jim or Rob, any other thoughts?  I think I've worked through most of Brian's
comments.

[bsalomon, 2015-03-12 15:20:25]

lgtm, one nit

https://codereview.chromium.org/975303005/diff/280001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.h (right):

https://codereview.chromium.org/975303005/diff/280001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.h:73: static inline void
HandleEviction(GrBatchAtlas::AtlasID, void*);
seems like this isn't really an inline since we take its address.

[joshualitt, 2015-03-13 13:48:38]

Robert, or Jim, any other feedback?  I'd really like to land this ASAP so it has
time to bake before the branch.

[jvanverth1, 2015-03-13 18:31:09]

lgtm

[joshualitt, 2015-03-13 18:41:18]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-03-13 18:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/975303005/#ps300001
(title: "nit")

[commit-bot: I haz the power, 2015-03-13 18:41:27]

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

[commit-bot: I haz the power, 2015-03-13 18:47:48]

Committed patchset #15 (id:300001) as
https://skia.googlesource.com/skia/+/5bf99f1ca8f30287803b594d06c60a7b6796ad45