|
|
|
Created:
13 years, 12 months ago by arthurhsu Modified:
13 years, 11 months ago CC:
skia-review_googlegroups.com, bungeman Base URL:
http://skia.googlecode.com/svn/trunk Visibility:
Public. |
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
Patch Set 1 #
Total comments: 20
Patch Set 2 : Update per code review #Patch Set 3 : Update per code review #
Total comments: 15
Patch Set 4 : Update per code review #
Total comments: 2
Patch Set 5 : Update per code review #Patch Set 6 : Fix unit test bugs #
Total comments: 24
Patch Set 7 : Update per code review #Patch Set 8 : Update per code review #Patch Set 9 : Update per code review #
Total messages: 19
|
arthurhsu
|
13 years, 12 months ago (2012年01月13日 22:17:16 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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.
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...
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.
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.
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/
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);
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.
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()
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.
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.
Unit test bug addressed, please review, thanks.
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
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.
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.
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;
}
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.
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).