Provide text mapping in text drawing for PDF backend.

Provide text mapping in text drawing for PDF backend.

This is the first CL in the series that changes drawText signatures in SkCanvas and SkDevice.

See https://groups.google.com/a/google.com/forum/#!topic/skia-dev/0JJHFnqvLkk for discussions.

BUG=chromium:104062
TEST=existing

[Steve VanDeBogart, 2012-01-13 22:31:19]

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h#newcode30
include/core/SkCanvas.h:30: typedef struct {
Is there a better header for this definition?  SkTypeface maybe?

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h#newcode700
include/core/SkCanvas.h:700: @param mapInfo Unicode mapping info, @see
SkDevice::drawText
indent "Unicode..." x4

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode75
include/core/SkDevice.h:75: kTextMapping_Capability = 0x4  //!< mask indicating
text mapping output
Put before all, and update all

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode226
include/core/SkDevice.h:226: *  When SkTextGlyphMapInfo presents, the |text| in
draw*Text*() calls
is present

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode230
include/core/SkDevice.h:230: *  responsible of calling text drawing API four
times in the order of
of calling -> for calling

http://codereview.appspot.com/5528100/diff/1/include/gpu/SkGpuDevice.h
File include/gpu/SkGpuDevice.h (right):

http://codereview.appspot.com/5528100/diff/1/include/gpu/SkGpuDevice.h#newcode95
include/gpu/SkGpuDevice.h:95: ) SK_OVERRIDE;
nit: not sure how Mike feels about it, but Chrome style is that ) and
SK_OVERRIDE should be on the same line as the last argument.

http://codereview.appspot.com/5528100/diff/1/obsolete/SkGLDevice.cpp
File obsolete/SkGLDevice.cpp (right):

http://codereview.appspot.com/5528100/diff/1/obsolete/SkGLDevice.cpp#newcode822
obsolete/SkGLDevice.cpp:822: const SkTextGlyphMapInfo* mapInfo) {
nit: omit (in .cpp files) parameter names that aren't used.

http://codereview.appspot.com/5528100/diff/1/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

http://codereview.appspot.com/5528100/diff/1/src/core/SkPictureRecord.h#newco...
src/core/SkPictureRecord.h:60: const SkScalar xpos[], SkScalar constY,
nit: remove extra whitespace.

http://codereview.appspot.com/5528100/diff/1/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/1/src/utils/SkDumpCanvas.cpp#newco...
src/utils/SkDumpCanvas.cpp:336: this->dump(kDrawText_Verb, &paint, "drawText(%s
[%d] %g %g)", str.c_str(),
should mapInfo get dumped too?

http://codereview.appspot.com/5528100/diff/1/src/utils/SkNWayCanvas.cpp
File src/utils/SkNWayCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/1/src/utils/SkNWayCanvas.cpp#newco...
src/utils/SkNWayCanvas.cpp:222: iter->drawText(text, byteLength, x, y, paint);
propagate mapInfo

[arthurhsu, 2012-01-14 00:11:30]

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h#newcode30
include/core/SkCanvas.h:30: typedef struct {
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> Is there a better header for this definition?  SkTypeface maybe?

Done.

http://codereview.appspot.com/5528100/diff/1/include/core/SkCanvas.h#newcode700
include/core/SkCanvas.h:700: @param mapInfo Unicode mapping info, @see
SkDevice::drawText
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> indent "Unicode..." x4

Done.

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode75
include/core/SkDevice.h:75: kTextMapping_Capability = 0x4  //!< mask indicating
text mapping output
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> Put before all, and update all

Done.

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode226
include/core/SkDevice.h:226: *  When SkTextGlyphMapInfo presents, the |text| in
draw*Text*() calls
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> is present

Done.

http://codereview.appspot.com/5528100/diff/1/include/core/SkDevice.h#newcode230
include/core/SkDevice.h:230: *  responsible of calling text drawing API four
times in the order of
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> of calling -> for calling

Done.

http://codereview.appspot.com/5528100/diff/1/include/gpu/SkGpuDevice.h
File include/gpu/SkGpuDevice.h (right):

http://codereview.appspot.com/5528100/diff/1/include/gpu/SkGpuDevice.h#newcode95
include/gpu/SkGpuDevice.h:95: ) SK_OVERRIDE;
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> nit: not sure how Mike feels about it, but Chrome style is that ) and
> SK_OVERRIDE should be on the same line as the last argument.

Will wait for Mike's input.

http://codereview.appspot.com/5528100/diff/1/obsolete/SkGLDevice.cpp
File obsolete/SkGLDevice.cpp (right):

http://codereview.appspot.com/5528100/diff/1/obsolete/SkGLDevice.cpp#newcode822
obsolete/SkGLDevice.cpp:822: const SkTextGlyphMapInfo* mapInfo) {
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> nit: omit (in .cpp files) parameter names that aren't used.

Done.

http://codereview.appspot.com/5528100/diff/1/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

http://codereview.appspot.com/5528100/diff/1/src/core/SkPictureRecord.h#newco...
src/core/SkPictureRecord.h:60: const SkScalar xpos[], SkScalar constY,
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> nit: remove extra whitespace.

Not seen in vi, probably Rietveld?

http://codereview.appspot.com/5528100/diff/1/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/1/src/utils/SkDumpCanvas.cpp#newco...
src/utils/SkDumpCanvas.cpp:336: this->dump(kDrawText_Verb, &paint, "drawText(%s
[%d] %g %g)", str.c_str(),
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> should mapInfo get dumped too?

We will, right now we don't know how to present it meaningfully yet.  I think it
would be better done in another CL.

http://codereview.appspot.com/5528100/diff/1/src/utils/SkNWayCanvas.cpp
File src/utils/SkNWayCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/1/src/utils/SkNWayCanvas.cpp#newco...
src/utils/SkNWayCanvas.cpp:222: iter->drawText(text, byteLength, x, y, paint);
On 2012/01/13 22:31:19, Steve VanDeBogart wrote:
> propagate mapInfo

Done.

[reed1, 2012-01-17 16:50:02]

Lets move this new struct into its own header, and just forward-declare it in
SkCanvas.h

Lets move the block-documentation in SkDevice.h into that new header.

The new struct will need read/write methods into SkReader32 and SkWriter32 to
work in SkPictureRecord.cpp (i.e. we must copy this data if present into the
picture, and extract it on playback).

What does the impl look like in SkPDFDevice.cpp? I think it is pretty important
to see some impl now so we can feel confident that this new param is in fact
defined in a useful way.

We should see what the corresponding CL for chrome looks like (for the new
parameters), since if this CL lands in Skia, it will block a DEPS roll until we
can update Chrome as well...

[Steve VanDeBogart, 2012-01-17 18:10:29]

On 2012/01/17 16:50:02, reed1 wrote:
> What does the impl look like in SkPDFDevice.cpp? I think it is pretty
important
> to see some impl now so we can feel confident that this new param is in fact
> defined in a useful way.

The actual change for SkPDFDevice is going to be non trivial, but a code sample
that shows how it could be used may be sufficient?
 
> We should see what the corresponding CL for chrome looks like (for the new
> parameters), since if this CL lands in Skia, it will block a DEPS roll until
we
> can update Chrome as well...

Seeing the Chrome side code is good to make sure this is the right interface,
but as an optional argument it won't block the roll.

[reed1, 2012-01-17 18:37:30]

On 2012/01/17 18:10:29, Steve VanDeBogart wrote:
> On 2012/01/17 16:50:02, reed1 wrote:
> > What does the impl look like in SkPDFDevice.cpp? I think it is pretty
> important
> > to see some impl now so we can feel confident that this new param is in fact
> > defined in a useful way.
> 
> The actual change for SkPDFDevice is going to be non trivial, but a code
sample
> that shows how it could be used may be sufficient?

That would help.

>  
> > We should see what the corresponding CL for chrome looks like (for the new
> > parameters), since if this CL lands in Skia, it will block a DEPS roll until
> we
> > can update Chrome as well...
> 
> Seeing the Chrome side code is good to make sure this is the right interface,
> but as an optional argument it won't block the roll.

I was thinking of any subclasses of SkCanvas or SkDevice that need to have their
method signatures updated.

[arthurhsu, 2012-01-24 02:45:07]

On 2012/01/17 18:37:30, reed1 wrote:
> On 2012/01/17 18:10:29, Steve VanDeBogart wrote:
> > On 2012/01/17 16:50:02, reed1 wrote:
> > > What does the impl look like in SkPDFDevice.cpp? I think it is pretty
> > important
> > > to see some impl now so we can feel confident that this new param is in
fact
> > > defined in a useful way.
> > 
> > The actual change for SkPDFDevice is going to be non trivial, but a code
> sample
> > that shows how it could be used may be sufficient?
> 
> That would help.
> 

As said they are not trivial, so I'll just describe. 
SkPDFDevice::fFontGlyphUsage will be changed to map of (font id, link list of
SkTextGlyphMapInfo).  When SkPDFDevice::drawText functions are called, the
passed in map info will be pushed into corresponding link list.
 
We will also remove SkAdvancedTypefaceMetrics::fGlyphToUnicode,
SkPDFDevice::fFontGlyphUsage.  SkPDFFont::populateToUnicodeTable and the
functions it use will be rewritten to use the data in new structures.  Reverse
lookup code (i.e. glyph to char lookup) in SkFontHost*.cpp will be removed since
we no longer use them.

The structure contains code points, glyph ids, and clusters, which is
essentially the same as Cairo.  We can then follow the same algorithm in Cairo
to generate corresponding cmap.

> >  
> > > We should see what the corresponding CL for chrome looks like (for the new
> > > parameters), since if this CL lands in Skia, it will block a DEPS roll
until
> > we
> > > can update Chrome as well...
> > 
> > Seeing the Chrome side code is good to make sure this is the right
interface,
> > but as an optional argument it won't block the roll.
> 
> I was thinking of any subclasses of SkCanvas or SkDevice that need to have
their
> method signatures updated.

I've done my search and found VectorPlatformDeviceEmf.  The patch is here:
http://codereview.chromium.org/9129010/

[reed1, 2012-01-24 16:09:43]

Lets remove the changes to any files in obsolete/

For playback, we can have a SkTextGlyphMapInfo struct point directly into the
reader's stream, rather than copying the data (as we do for the text itself
today). No need for any dynamic allocations.

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
File include/core/SkTextGlyphMapInfo.h (right):

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:12: #include "SkTDArray.h"
Why is SkWriter32.h included?

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:28: 
Is this really UnicharGlyphMapInfo?

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:29: struct SkTextGlyphMapInfo {
!? Are any of the draw calls going to modify this data? I had presumed not.

Lets not require the caller to organize their data using our arrays. ptrs and
len are much more general, and more friendly to the callers.

e.g.

struct SkTextGlyphMapInfo {
    const SkUnichar* fCodePoints;
    int fCodePointCount;
    const uint8_t* fClusterPairs;    // [2N + 0]==unichar count, [2N + 1]==glyph
count
    int fClusterPairCount;  // byte length of fClusterPairs == 2 *
fClusterPairCount
    bool fBackwardClusters;
};

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPicturePlayback.h#...
src/core/SkPicturePlayback.h:126: text->fHasMapInfo = fReader.readBool();
See comments on writing entire arrays in PictureRecord: we can read entire
arrays at once, rather than one read per element.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:307: 
Note how we use a different verb already depending on top/bottom. I think we
could try the same trick for w/ or w/o info, so we don't need the extra bool
(which will be 4 bytes) in the stream for every text call. Perhaps something for
a subsequent CL.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:574: #endif
Maybe better to write both counts up front, and their corresponding arrays of
data later. That way on playback we can read a fixed amount (2 counts or 2
counts + bool) and know how much RAM to allocate for everything else.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:584:
fWriter.writeInt(mapInfo->fCodePoints.count());
fWriter.write(mapInfo->fCodePoints, mapInfo->fCodePointCount *
sizeof(SkUnichar));

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:588: fWriter.writeInt(mapInfo->fCluster.count());
fWriter.writePad(mapInfo->fClusterPairs, mapInfo->fClusterPairCount * 2);

[arthurhsu, 2012-01-24 19:57:14]

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
File include/core/SkTextGlyphMapInfo.h (right):

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:12: #include "SkTDArray.h"
On 2012/01/24 16:09:44, reed1 wrote:
> Why is SkWriter32.h included?

Done.

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:28: 
On 2012/01/24 16:09:44, reed1 wrote:
> Is this really UnicharGlyphMapInfo?

Done.

http://codereview.appspot.com/5528100/diff/9001/include/core/SkTextGlyphMapIn...
include/core/SkTextGlyphMapInfo.h:29: struct SkTextGlyphMapInfo {
On 2012/01/24 16:09:44, reed1 wrote:
> !? Are any of the draw calls going to modify this data? I had presumed not.
> 
> Lets not require the caller to organize their data using our arrays. ptrs and
> len are much more general, and more friendly to the callers.
> 
> e.g.
> 
> struct SkTextGlyphMapInfo {
>     const SkUnichar* fCodePoints;
>     int fCodePointCount;
>     const uint8_t* fClusterPairs;    // [2N + 0]==unichar count, [2N +
1]==glyph
> count
>     int fClusterPairCount;  // byte length of fClusterPairs == 2 *
> fClusterPairCount
>     bool fBackwardClusters;
> };

Done.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPicturePlayback.h#...
src/core/SkPicturePlayback.h:126: text->fHasMapInfo = fReader.readBool();
On 2012/01/24 16:09:44, reed1 wrote:
> See comments on writing entire arrays in PictureRecord: we can read entire
> arrays at once, rather than one read per element.

Done.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:574: #endif
On 2012/01/24 16:09:44, reed1 wrote:
> Maybe better to write both counts up front, and their corresponding arrays of
> data later. That way on playback we can read a fixed amount (2 counts or 2
> counts + bool) and know how much RAM to allocate for everything else.

Done.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:584:
fWriter.writeInt(mapInfo->fCodePoints.count());
On 2012/01/24 16:09:44, reed1 wrote:
> fWriter.write(mapInfo->fCodePoints, mapInfo->fCodePointCount *
> sizeof(SkUnichar));

Done.

http://codereview.appspot.com/5528100/diff/9001/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:588: fWriter.writeInt(mapInfo->fCluster.count());
On 2012/01/24 16:09:44, reed1 wrote:
> fWriter.writePad(mapInfo->fClusterPairs, mapInfo->fClusterPairCount * 2);

Done.

[reed1, 2012-01-25 16:14:00]

Thanks for the changes. We definitely need to test this in pictures, as it will
not work as written (value to skip is wrong in playback, didn't set the count
fields in playback).

A gm seems appropriate here, since that will allow us to test with
raster/gpu/picture/pdf/xps

http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h#...
src/core/SkPicturePlayback.h:126: text->fHasMapInfo = fReader.readBool();
Need to set the count fields on mapinfo.
Need to scale the counts (as you did in PictureRecord) for the parameter to
skip()

[arthurhsu, 2012-01-25 20:54:43]

I don't know how to do gm.  I've added a test case to validate the correctness
of changes within picture playback.

http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h#...
src/core/SkPicturePlayback.h:126: text->fHasMapInfo = fReader.readBool();
On 2012/01/25 16:14:01, reed1 wrote:
> Need to set the count fields on mapinfo.
> Need to scale the counts (as you did in PictureRecord) for the parameter to
> skip()

Done.

[arthurhsu, 2012-01-25 21:07:46]

On 2012/01/25 20:54:43, arthurhsu wrote:
> I don't know how to do gm.  I've added a test case to validate the correctness
> of changes within picture playback.
> 
> http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h
> File src/core/SkPicturePlayback.h (right):
> 
>
http://codereview.appspot.com/5528100/diff/6002/src/core/SkPicturePlayback.h#...
> src/core/SkPicturePlayback.h:126: text->fHasMapInfo = fReader.readBool();
> On 2012/01/25 16:14:01, reed1 wrote:
> > Need to set the count fields on mapinfo.
> > Need to scale the counts (as you did in PictureRecord) for the parameter to
> > skip()
> 
> Done.

Please disregard this CL, I found some errors on my end.  Still working on it.

[arthurhsu, 2012-01-25 21:25:56]

Unit test bug addressed, please review, thanks.

[caryclark1, 2012-01-25 21:59:58]

http://codereview.appspot.com/5528100/diff/15004/gyp/core.gyp
File gyp/core.gyp (right):

http://codereview.appspot.com/5528100/diff/15004/gyp/core.gyp#newcode233
gyp/core.gyp:233: '../include/core/SkUnicharGlyphMapInfo.h',
to keep this alphabetized, move down a line. Note that the list is case
insensitive

http://codereview.appspot.com/5528100/diff/15004/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:103: SkDevice* createCompatibleDevice(SkBitmap::Config
config,
On such a large change, please consider not making formatting changes as part of
the checkin -- this will help a lot in understanding the actual changes you're
making.

http://codereview.appspot.com/5528100/diff/15004/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkDevice.h#newc...
include/core/SkDevice.h:73: kVector_Capability = 0x2,  //!< mask indicating a
vector representation
indent the comments on the two lines above to match the new line

http://codereview.appspot.com/5528100/diff/15004/include/core/SkUnicharGlyphM...
File include/core/SkUnicharGlyphMapInfo.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkUnicharGlyphM...
include/core/SkUnicharGlyphMapInfo.h:14: * always represents glyphs in visual
order.  Corresponding Unicode
s/represents/represent/

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h...
src/core/SkPicturePlayback.h:58: bool fHasMapInfo;
An alternative would be to replace fHasMapInfo with:
  SkUnicharGlyphMapInfo* fMapInfoPtr;
and set it to NULL if there is no fMapInfo. This would allow the playback code
to avoid testing the boolean.

http://codereview.appspot.com/5528100/diff/15004/src/device/xps/SkXPSDevice.cpp
File src/device/xps/SkXPSDevice.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:94: 
This file is a good example why it is worthwhile to separate formatting changes
from code changes.

http://codereview.appspot.com/5528100/diff/15004/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:795: const SkUnicharGlyphMapInfo* mapInfo) {
In other files, you omitted mapInfo from the parameter list if it wasn't used by
the function. Please do so here as well (and below)

http://codereview.appspot.com/5528100/diff/15004/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/src/utils/SkDumpCanvas.cpp#n...
src/utils/SkDumpCanvas.cpp:333: const SkUnicharGlyphMapInfo* mapInfo) {
omit mapInfo parameter name if it isn't used (here and below)

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cpp
File tests/PicturePlaybackTest.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:64: info.fClusterPairCount = 4;
use sizeof(clusterPairs) / 2

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:65: info.fCodePointCount = 5;
use sizeof(codePoints) / sizeof(codePoints[0])

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:72: rec.drawText(text, sizeof(uint16_t) * 5, 7, 9,
paint, &info);
use sizeof(text) instead

[reed1, 2012-01-25 22:17:24]

unittest == good

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h...
src/core/SkPicturePlayback.h:58: bool fHasMapInfo;
On 2012/01/25 21:59:58, caryclark1 wrote:
> An alternative would be to replace fHasMapInfo with:
>   SkUnicharGlyphMapInfo* fMapInfoPtr;
> and set it to NULL if there is no fMapInfo. This would allow the playback code
> to avoid testing the boolean.

+1

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cpp
File tests/PicturePlaybackTest.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:71: SkPaint paint;
SkIntToScalar(7), SkIntToScalar(9)

Need to promote coordinates to SkScalar, so this will make sense when compiled
in fixed-point.

[arthurhsu, 2012-01-25 23:20:36]

http://codereview.appspot.com/5528100/diff/15004/gyp/core.gyp
File gyp/core.gyp (right):

http://codereview.appspot.com/5528100/diff/15004/gyp/core.gyp#newcode233
gyp/core.gyp:233: '../include/core/SkUnicharGlyphMapInfo.h',
On 2012/01/25 21:59:58, caryclark1 wrote:
> to keep this alphabetized, move down a line. Note that the list is case
> insensitive

Done.

http://codereview.appspot.com/5528100/diff/15004/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:103: SkDevice* createCompatibleDevice(SkBitmap::Config
config,
On 2012/01/25 21:59:58, caryclark1 wrote:
> On such a large change, please consider not making formatting changes as part
of
> the checkin -- this will help a lot in understanding the actual changes you're
> making.

Will disable Chrome suggested automatic formatting next time.

http://codereview.appspot.com/5528100/diff/15004/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkDevice.h#newc...
include/core/SkDevice.h:73: kVector_Capability = 0x2,  //!< mask indicating a
vector representation
On 2012/01/25 21:59:58, caryclark1 wrote:
> indent the comments on the two lines above to match the new line

Done.

http://codereview.appspot.com/5528100/diff/15004/include/core/SkUnicharGlyphM...
File include/core/SkUnicharGlyphMapInfo.h (right):

http://codereview.appspot.com/5528100/diff/15004/include/core/SkUnicharGlyphM...
include/core/SkUnicharGlyphMapInfo.h:14: * always represents glyphs in visual
order.  Corresponding Unicode
On 2012/01/25 21:59:58, caryclark1 wrote:
> s/represents/represent/

Done.

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/5528100/diff/15004/src/core/SkPicturePlayback.h...
src/core/SkPicturePlayback.h:58: bool fHasMapInfo;
On 2012/01/25 21:59:58, caryclark1 wrote:
> An alternative would be to replace fHasMapInfo with:
>   SkUnicharGlyphMapInfo* fMapInfoPtr;
> and set it to NULL if there is no fMapInfo. This would allow the playback code
> to avoid testing the boolean.

Done.

http://codereview.appspot.com/5528100/diff/15004/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:795: const SkUnicharGlyphMapInfo* mapInfo) {
On 2012/01/25 21:59:58, caryclark1 wrote:
> In other files, you omitted mapInfo from the parameter list if it wasn't used
by
> the function. Please do so here as well (and below)

Done.

http://codereview.appspot.com/5528100/diff/15004/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/src/utils/SkDumpCanvas.cpp#n...
src/utils/SkDumpCanvas.cpp:333: const SkUnicharGlyphMapInfo* mapInfo) {
On 2012/01/25 21:59:58, caryclark1 wrote:
> omit mapInfo parameter name if it isn't used (here and below)

Done.

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cpp
File tests/PicturePlaybackTest.cpp (right):

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:64: info.fClusterPairCount = 4;
On 2012/01/25 21:59:58, caryclark1 wrote:
> use sizeof(clusterPairs) / 2

Done.

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:65: info.fCodePointCount = 5;
On 2012/01/25 21:59:58, caryclark1 wrote:
> use sizeof(codePoints) / sizeof(codePoints[0])

Done.

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:71: SkPaint paint;
On 2012/01/25 22:17:24, reed1 wrote:
> SkIntToScalar(7), SkIntToScalar(9)
> 
> Need to promote coordinates to SkScalar, so this will make sense when compiled
> in fixed-point.

Done.

http://codereview.appspot.com/5528100/diff/15004/tests/PicturePlaybackTest.cp...
tests/PicturePlaybackTest.cpp:72: rec.drawText(text, sizeof(uint16_t) * 5, 7, 9,
paint, &info);
On 2012/01/25 21:59:58, caryclark1 wrote:
> use sizeof(text) instead

Done.

[reed1, 2012-01-26 00:04:39]

We can/should avoid the dynamic allocation of the SkUnicharGlyphMapInfo. I think
cary's suggestion was more like the following:

TextContainer {
    ....
    SkUnicharGlyphMapInfo fInfoStorage;
    SkUnicharGlyphMapInfo* fInfo;
};

Then when we read the stream...

    if (has some info) {
        fInfoStorage.fThisOrThat = ...;
        fInfo = &fInfoStorage;
    } else {
        fInfo = NULL;
    }

[arthurhsu, 2012-01-30 19:58:50]

On 2012/01/26 00:04:39, reed1 wrote:
> We can/should avoid the dynamic allocation of the SkUnicharGlyphMapInfo. I
think
> cary's suggestion was more like the following:
> 
> TextContainer {
>     ....
>     SkUnicharGlyphMapInfo fInfoStorage;
>     SkUnicharGlyphMapInfo* fInfo;
> };
> 
> Then when we read the stream...
> 
>     if (has some info) {
>         fInfoStorage.fThisOrThat = ...;
>         fInfo = &fInfoStorage;
>     } else {
>         fInfo = NULL;
>     }

Done.

[reed1, 2012-01-30 21:06:35]

lgtm

in future, rebaselining in the middle of a CL makes it hard for reviews to see
what has changed between uploads (i.e. we see your incremental mods and we see
external changes).