Issue 4650060: Update Catalog object to have the capability of placing substitutes. (Closed)
|
Created:
13 years, 6 months ago by arthurhsu Modified:
13 years, 6 months ago CC:
skia-review_googlegroups.com, reed1 Base URL:
http://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionUpdate Catalog object to have the capability of placing substitutes.
Separate from CR 4633050.
Note for Steve: I'm not a committer, so if you need this fast, please help commit if LGTM'ed.
BUG=none
TEST=none
Patch Set 1 #Patch Set 2 : Update per comments #
Total comments: 41
Patch Set 3 : Update per code review #
Total comments: 42
Patch Set 4 : Update per code review #
Total comments: 36
Patch Set 5 : Update per code review #
Total comments: 16
Patch Set 6 : Update per code review #Patch Set 7 : Update per code review #
Total comments: 4
Patch Set 8 : Update per code review #
MessagesTotal messages: 14
http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:118: SkTDArray<struct SubstituteMapping> fObjectMap; Lets make fObjectMap include Sub or Substitute in its name. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:54: virtual void emit(SkWStream* stream, SkPDFCatalog* catalog, bool indirect); not virtual. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:62: virtual size_t getOutputSize(SkPDFCatalog* catalog, bool indirect); We need to play the same game with getOutputSize and getResources. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:78: void emitIndirectObject(SkWStream* stream, SkPDFCatalog* catalog); I don't think these should be protected. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:151: // Sanity check: is the original already in substitute list? put this in an #ifdef SK_DEBUG block http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:159: for (int i = 0; i < fObjectMap.count(); ++i) { I don't think we want this to happen.... We've already added the resources of the original substitute object to the catalog. Make this also SkASSSERT and put it in the SK_DEBUG block? http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:161: fObjectMap[i].fSubstituteObject = substitute; Do we need to make fObjectMap reference the substitute objects? For regular objects it must be in a resource list somewhere, but for substitute objects, this may be the only reference. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:169: // Add descendant objects in substitute to catalog. Clever - this means we don't have to make getResources understand substitute objects. However, there may be resources shared among substitute objects, so we need to unique-ify things. I've been meaning for awhile to make addObject do the uniquification instead of doing it in SkPDFDocument addResourcesToCatalog. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:169: // Add descendant objects in substitute to catalog. Actually, this isn't clever enough to work. SkPDFDocument::emitPDF is what calls setFileOffset() and emit(). With this, there's no way to get file offsets for substitute objects or for them to get emitted. Unless I'm missing something.... http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:171: newMapping.fSubstituteObject->getResources(&resourceList); This is another complication - getResources returns references to the resources, so we need to unref them here. We probably don't need to keep our own reference to them though because the substitute object has one... http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:173: addObject(resourceList[i], onFirstPage); Can't we get this value from the original object? http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:177: off_t SkPDFCatalog::updateSubstituteOffset(off_t fileOffset) { This doesn't seem to be referenced. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:177: off_t SkPDFCatalog::updateSubstituteOffset(off_t fileOffset) { I see what your intention is with these, even if you haven't done it yet. Maybe this can be made to work, though it will be different from the rest of the system. For linearization, we need to emit the objects from the first page at a specific point in the stream, and the rest of the objects at a different point. I guess we could have a first page flag on this method and call it twice. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:195: void SkPDFCatalog::emitSubstituteDescendant(SkWStream *stream) { nor this.
Sign in to reply to this message.
I just did a quick glance at this. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:90: Nit: Extra newline here. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:110: SkPDFObject* fOriginalObject; Nit: Appending Object to these class members seems gratuitous. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Based on catalog data, emit the proper object. Perhaps someone can explain this comment to me in person. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:69: if (fObjectMap[i].fSubstituteObject == obj) { // yes it is Nit: This comment seems unneeded. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:77: SkPDFObject* obj = originalObj; There doesn't seem to be a logic change here. Accidentally left this in?
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Based on catalog data, emit the proper object. On 2011/06/30 03:40:41, Chris Guillory wrote: > Perhaps someone can explain this comment to me in person. Indeed, this comment can be improved... "Consult the catalog and emit the substitute object if one has been set or emit this object."
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:90: On 2011/06/30 03:40:41, Chris Guillory wrote: > Nit: Extra newline here. Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:110: SkPDFObject* fOriginalObject; On 2011/06/30 03:40:41, Chris Guillory wrote: > Nit: Appending Object to these class members seems gratuitous. Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:118: SkTDArray<struct SubstituteMapping> fObjectMap; On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > Lets make fObjectMap include Sub or Substitute in its name. Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Based on catalog data, emit the proper object. On 2011/06/30 16:00:25, Steve VanDeBogart wrote: > On 2011/06/30 03:40:41, Chris Guillory wrote: > > Perhaps someone can explain this comment to me in person. > > Indeed, this comment can be improved... > "Consult the catalog and emit the substitute object if one has been set or emit > this object." Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Based on catalog data, emit the proper object. On 2011/06/30 03:40:41, Chris Guillory wrote: > Perhaps someone can explain this comment to me in person. Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:54: virtual void emit(SkWStream* stream, SkPDFCatalog* catalog, bool indirect); On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > not virtual. Done. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:62: virtual size_t getOutputSize(SkPDFCatalog* catalog, bool indirect); On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > We need to play the same game with getOutputSize and getResources. My idea is to deal the substitute objects and their resources as meta objects in catalog. As a result, we don't need to push down to object level. Instead, the output size and emit are taken care of in catalog and document. http://codereview.appspot.com/4650060/diff/2001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:78: void emitIndirectObject(SkWStream* stream, SkPDFCatalog* catalog); On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > I don't think these should be protected. Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:69: if (fObjectMap[i].fSubstituteObject == obj) { // yes it is On 2011/06/30 03:40:41, Chris Guillory wrote: > Nit: This comment seems unneeded. Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:77: SkPDFObject* obj = originalObj; On 2011/06/30 03:40:41, Chris Guillory wrote: > There doesn't seem to be a logic change here. Accidentally left this in? Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:151: // Sanity check: is the original already in substitute list? On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > put this in an #ifdef SK_DEBUG block Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:159: for (int i = 0; i < fObjectMap.count(); ++i) { On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > I don't think we want this to happen.... We've already added the resources of > the original substitute object to the catalog. Make this also SkASSSERT and put > it in the SK_DEBUG block? Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:161: fObjectMap[i].fSubstituteObject = substitute; On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > Do we need to make fObjectMap reference the substitute objects? For regular > objects it must be in a resource list somewhere, but for substitute objects, > this may be the only reference. Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:169: // Add descendant objects in substitute to catalog. On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > Clever - this means we don't have to make getResources understand substitute > objects. However, there may be resources shared among substitute objects, so we > need to unique-ify things. > I've been meaning for awhile to make addObject do the uniquification instead of > doing it in SkPDFDocument addResourcesToCatalog. Resource objects of substitute objects are added to catalog. The general idea is to treat the substitute object and its resources as a meta object. The meta object is stored in catalog, and emit/getsize when the document says so. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:169: // Add descendant objects in substitute to catalog. On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > Actually, this isn't clever enough to work. SkPDFDocument::emitPDF is what > calls setFileOffset() and emit(). With this, there's no way to get file offsets > for substitute objects or for them to get emitted. Unless I'm missing > something.... Actually, I'm missing something. Fixed. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:171: newMapping.fSubstituteObject->getResources(&resourceList); On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > This is another complication - getResources returns references to the resources, > so we need to unref them here. We probably don't need to keep our own reference > to them though because the substitute object has one... Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:173: addObject(resourceList[i], onFirstPage); On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > Can't we get this value from the original object? Nope. Take font as example. Original object is just a skeleton, and the substitute is a type0 font with bunch of descendants (type 1 cid, font info, ...). They are in the resource list of the substitute object only. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:177: off_t SkPDFCatalog::updateSubstituteOffset(off_t fileOffset) { On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > I see what your intention is with these, even if you haven't done it yet. Maybe > this can be made to work, though it will be different from the rest of the > system. For linearization, we need to emit the objects from the first page at a > specific point in the stream, and the rest of the objects at a different point. > I guess we could have a first page flag on this method and call it twice. Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:177: off_t SkPDFCatalog::updateSubstituteOffset(off_t fileOffset) { On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > This doesn't seem to be referenced. Done. http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:195: void SkPDFCatalog::emitSubstituteDescendant(SkWStream *stream) { On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > nor this. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/2001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:173: addObject(resourceList[i], onFirstPage); On 2011/06/30 23:32:40, arthurhsu wrote: > On 2011/06/30 01:34:50, Steve VanDeBogart wrote: > > Can't we get this value from the original object? > > Nope. Take font as example. Original object is just a skeleton, and the > substitute is a type0 font with bunch of descendants (type 1 cid, font info, > ...). They are in the resource list of the substitute object only. Sorry, I meant get the value of onFirstPage from the catalog entry for the original object. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:73: /** Give the substitute object (i.e. subsetted font object) if found. Find and return any substitute object set for the passed object. If there is none, return the passed object. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:76: SkPDFObject* findRealObject(SkPDFObject* object); getSubstituteObject http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:78: /** Set substitute object for given object. nit: ...for the passed object... http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:83: /** Update file offset of substitute objects and their descendents. Set file offsets for the resources of substitute objects. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:84: */ Explain @param. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:85: off_t getSubstituteResourcesOffset(off_t fileOffset, bool firstPage); setSubstitiuteResourcesOffsets and update comment. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:87: /** Emit descendant objects for substitutes Emit the resources of substitute objects. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:112: SkRefPtr<SkPDFObject> fSubstitute; The destructor of SubstituteMapping won't ever be called, so make this a plain pointer. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Consult the catalog and emit the substitute object if one has been set Emit this object unless the catalog has a substitute object, in which case emit that. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:63: /** Return the size (number of bytes) of this object in the final output Why move this? http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:84: size_t getIndirectOutputSize(SkPDFCatalog* catalog); This should stay public. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:152: #if defined (SK_DEBUG) nit: no space, defined(SK_DEBUG) http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:172: resourceList.safeUnrefAll(); Should we just keep two local resource lists? one for first page sub obj resources and one for non-first page sub obj resources? getResources() can be kind of expensive. Plus the two functions below don't unique-ify the resources, which we could do by sorting the list and removing duplicates. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:185: int objIndex = assignObjNum(resourceList[j]) - 1; Why can't we let setFileOffset do this? http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:190: setFileOffset(resourceList[j], offset + offsetSum + fileOffset); This can be much simpler: fileOffset += setFileOffset(resourceList[j], fileOffset); http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:105: fileOffset += Move this up to line 100. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:135: fCatalog.emitSubstituteResources(stream, true); move up to line 130 http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (left): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFFont.cpp#oldcode432 src/pdf/SkPDFFont.cpp:432: #ifdef SK_DEBUG This is part of the font change. http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp#n... tests/PDFPrimitivesTest.cpp:34: } We need a test in here for sub objects and their resources.
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:73: /** Give the substitute object (i.e. subsetted font object) if found. On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Find and return any substitute object set for the passed object. If there is > none, return the passed object. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:76: SkPDFObject* findRealObject(SkPDFObject* object); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > getSubstituteObject Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:78: /** Set substitute object for given object. On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > nit: ...for the passed object... Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:83: /** Update file offset of substitute objects and their descendents. On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Set file offsets for the resources of substitute objects. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:84: */ On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Explain @param. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:85: off_t getSubstituteResourcesOffset(off_t fileOffset, bool firstPage); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > setSubstitiuteResourcesOffsets and update comment. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:87: /** Emit descendant objects for substitutes On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Emit the resources of substitute objects. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:112: SkRefPtr<SkPDFObject> fSubstitute; On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > The destructor of SubstituteMapping won't ever be called, so make this a plain > pointer. Sure. This is done so to provide a ref of substitute object per previous comments. In catalog's dtor an SkSafeUnref() is called to release the ref. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:51: /** Consult the catalog and emit the substitute object if one has been set On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Emit this object unless the catalog has a substitute object, in which case emit > that. Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:63: /** Return the size (number of bytes) of this object in the final output On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Why move this? Done. http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFTypes.h#newc... include/pdf/SkPDFTypes.h:84: size_t getIndirectOutputSize(SkPDFCatalog* catalog); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > This should stay public. Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:152: #if defined (SK_DEBUG) On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > nit: no space, defined(SK_DEBUG) Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:172: resourceList.safeUnrefAll(); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Should we just keep two local resource lists? one for first page sub obj > resources and one for non-first page sub obj resources? getResources() can be > kind of expensive. Plus the two functions below don't unique-ify the resources, > which we could do by sorting the list and removing duplicates. Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:185: int objIndex = assignObjNum(resourceList[j]) - 1; On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Why can't we let setFileOffset do this? Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFCatalog.cpp#newc... src/pdf/SkPDFCatalog.cpp:190: setFileOffset(resourceList[j], offset + offsetSum + fileOffset); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > This can be much simpler: > fileOffset += setFileOffset(resourceList[j], fileOffset); Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:105: fileOffset += On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > Move this up to line 100. Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:135: fCatalog.emitSubstituteResources(stream, true); On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > move up to line 130 Done. http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (left): http://codereview.appspot.com/4650060/diff/9001/src/pdf/SkPDFFont.cpp#oldcode432 src/pdf/SkPDFFont.cpp:432: #ifdef SK_DEBUG On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > This is part of the font change. Done. http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp#n... tests/PDFPrimitivesTest.cpp:34: } On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > We need a test in here for sub objects and their resources. Will be done when font subsetting is checked in. Can't do in this CL.
Sign in to reply to this message.
Getting close... http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:112: SkRefPtr<SkPDFObject> fSubstitute; On 2011/07/01 03:31:53, arthurhsu wrote: > On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > > The destructor of SubstituteMapping won't ever be called, so make this a plain > > pointer. > > Sure. This is done so to provide a ref of substitute object per previous > comments. In catalog's dtor an SkSafeUnref() is called to release the ref. Because SkRefPtr won't work as expected here, we should use a raw pointer. Using a SkRefPtr would mislead people into thinking it works as usual here. http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp#n... tests/PDFPrimitivesTest.cpp:34: } On 2011/07/01 03:31:53, arthurhsu wrote: > On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > > We need a test in here for sub objects and their resources. > > Will be done when font subsetting is checked in. Can't do in this CL. An integration test later can be useful, but that isn't a substitute for a unit test. Please add a unit test for basic functionality. It shouldn't take too long. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:80: void setSubstitute(SkPDFObject* original, SkPDFObject* substitute, nit: swap the order of get/set http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:81: bool onFirstPage); can't we get onFirstPage from catalog's knowledge of original? http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:86: */ document the return value http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:87: off_t setSubstituteResourcesOffset(off_t fileOffset, bool firstPage); Make this method name plural. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h#new... include/pdf/SkPDFTypes.h:63: /** Return the size (number of bytes) of this object in the final output nit: looks like you moved this definition for no reason. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h#new... include/pdf/SkPDFTypes.h:84: size_t getIndirectOutputSize(SkPDFCatalog* catalog); I don't think this should be protected. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { With these changes, addObject should check if an object is already in the catalog. Even if it is, we may have to update the onFirstPage bit. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:50: if (fCatalog[objIndex].fFileOffset) { I don't think we should make this change. Unless I'm missing something, encountering an already set size means the object is in the catalog multiple times. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:164: } nit: should we have another sanity check that original is already in the catalog? http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:174: int count = targetList->count(); nit: count -> existingSize http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:189: offsetSum += (*targetList)[i]->getOutputSize(this, true); setFileOffset already calls getOutputSize (which may be expensive). (Plus you're setting the offset to the end of the object instead of the start of the object by calling setFileOffset first.) ofsetSum += setFileOffset((*targetList)[i], offsetSum); http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:100: // Add the size of resources of substitute objects used in page 1. nit: s/in/on/ http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:102: fCatalog.setSubstituteResourcesOffset(fileOffset, true); this will fit on one line. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:120: fCatalog.setSubstituteResourcesOffset(fileOffset, false); As will this.
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/9001/include/pdf/SkPDFCatalog.h#ne... include/pdf/SkPDFCatalog.h:112: SkRefPtr<SkPDFObject> fSubstitute; On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > On 2011/07/01 03:31:53, arthurhsu wrote: > > On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > > > The destructor of SubstituteMapping won't ever be called, so make this a > plain > > > pointer. > > > > Sure. This is done so to provide a ref of substitute object per previous > > comments. In catalog's dtor an SkSafeUnref() is called to release the ref. > > Because SkRefPtr won't work as expected here, we should use a raw pointer. > Using a SkRefPtr would mislead people into thinking it works as usual here. Done. http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/9001/tests/PDFPrimitivesTest.cpp#n... tests/PDFPrimitivesTest.cpp:34: } On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > On 2011/07/01 03:31:53, arthurhsu wrote: > > On 2011/07/01 00:32:16, Steve VanDeBogart wrote: > > > We need a test in here for sub objects and their resources. > > > > Will be done when font subsetting is checked in. Can't do in this CL. > > An integration test later can be useful, but that isn't a substitute for a unit > test. Please add a unit test for basic functionality. It shouldn't take too > long. Done. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:80: void setSubstitute(SkPDFObject* original, SkPDFObject* substitute, On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > nit: swap the order of get/set Done. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:81: bool onFirstPage); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > can't we get onFirstPage from catalog's knowledge of original? Then we need to do a search in Rec array, which seems redundant since the caller knows it. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:86: */ On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > document the return value Done. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:87: off_t setSubstituteResourcesOffset(off_t fileOffset, bool firstPage); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > Make this method name plural. Done. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h File include/pdf/SkPDFTypes.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h#new... include/pdf/SkPDFTypes.h:63: /** Return the size (number of bytes) of this object in the final output On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > nit: looks like you moved this definition for no reason. Done. http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFTypes.h#new... include/pdf/SkPDFTypes.h:84: size_t getIndirectOutputSize(SkPDFCatalog* catalog); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > I don't think this should be protected. Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > With these changes, addObject should check if an object is already in the > catalog. Even if it is, we may have to update the onFirstPage bit. We don't need to update the first page bit. If an object is added to catalog, it shall not be added again no matter what. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:50: if (fCatalog[objIndex].fFileOffset) { On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > I don't think we should make this change. Unless I'm missing something, > encountering an already set size means the object is in the catalog multiple > times. Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:164: } On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > nit: should we have another sanity check that original is already in the > catalog? Nope, fonts are saved in global array, not the one in Catalog. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:174: int count = targetList->count(); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > nit: count -> existingSize Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:189: offsetSum += (*targetList)[i]->getOutputSize(this, true); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > setFileOffset already calls getOutputSize (which may be expensive). (Plus > you're setting the offset to the end of the object instead of the start of the > object by calling setFileOffset first.) > > ofsetSum += setFileOffset((*targetList)[i], offsetSum); Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:100: // Add the size of resources of substitute objects used in page 1. On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > nit: s/in/on/ Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:102: fCatalog.setSubstituteResourcesOffset(fileOffset, true); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > this will fit on one line. Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:120: fCatalog.setSubstituteResourcesOffset(fileOffset, false); On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > As will this. Done.
Sign in to reply to this message.
Thanks for the email explanation. I'd need an in person explanation in order to provide more value to the CR if that's desired. Here are some additional nits and style comments. http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:75: void setSubstitute(SkPDFObject* original, SkPDFObject* substitute, Nit: Is there any reason why 'original' cannot or should not be named simply 'object' here and in the other places? http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:120: SkTDArray<struct SubstituteMapping> fSubObjectMap; Nit: I don't believe struct is needed for the type in this template. http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:120: SkTDArray<struct SubstituteMapping> fSubObjectMap; Optional: SubObject -> Substitute. Seems a bit better to me. Optional. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:139: SkPDFObject* SkPDFCatalog::getSubstituteObject(SkPDFObject* object) { Optional: Reorder function definitions. getSubstituteObject comes after setSubstitute in the header. http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp#... tests/PDFPrimitivesTest.cpp:39: SkPDFCatalog dummy; Nit/Optional: Can we use another variable name? Perhaps just 'catalog' is another choice isn't available.
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:81: bool onFirstPage); On 2011/07/01 21:27:22, arthurhsu wrote: > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > can't we get onFirstPage from catalog's knowledge of original? > > Then we need to do a search in Rec array, which seems redundant since the caller > knows it. Is that true? I don't think it's true for the font subsetting case. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { On 2011/07/01 21:27:22, arthurhsu wrote: > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > With these changes, addObject should check if an object is already in the > > catalog. Even if it is, we may have to update the onFirstPage bit. > > We don't need to update the first page bit. If an object is added to catalog, > it shall not be added again no matter what. Except, we may be violating that by adding substitute object resources. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:164: } On 2011/07/01 21:27:22, arthurhsu wrote: > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > nit: should we have another sanity check that original is already in the > > catalog? > Nope, fonts are saved in global array, not the one in Catalog. I don't understand your reply. I'm saying that we shouldn't set a substitute for an object that isn't in the catalog, but that can be a debug only check. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:139: SkPDFObject* SkPDFCatalog::getSubstituteObject(SkPDFObject* object) { On 2011/07/01 22:52:49, Chris Guillory wrote: > Optional: Reorder function definitions. getSubstituteObject comes after > setSubstitute in the header. Yes, please do this. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:178: off_t offsetSum = fileOffset; nit: move to just before for loop. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:190: SkTDArray<SkPDFObject*>* targetList = nit: pull this into a helper function. http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp#... tests/PDFPrimitivesTest.cpp:141: REPORTER_ASSERT(reporter, stream_equals(buffer, 0, expectedResult, Great! Can we also test that substitute object resources work as intended?
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/12001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:81: bool onFirstPage); On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > On 2011/07/01 21:27:22, arthurhsu wrote: > > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > > can't we get onFirstPage from catalog's knowledge of original? > > > > Then we need to do a search in Rec array, which seems redundant since the > caller > > knows it. > > Is that true? I don't think it's true for the font subsetting case. Done. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > On 2011/07/01 21:27:22, arthurhsu wrote: > > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > > With these changes, addObject should check if an object is already in the > > > catalog. Even if it is, we may have to update the onFirstPage bit. > > > > We don't need to update the first page bit. If an object is added to catalog, > > it shall not be added again no matter what. > > Except, we may be violating that by adding substitute object resources. In the SkASSERT, findObjectIndex(obj) will do the check. If the original is there, it won't return -1 and will trigger the assert. If the obj is a substitute, the new findObjectIndex(obj) will return the assigned number of the original object that it substitutes, which won't be -1 and will trigger the assert, too. Once the onFirstPage bit is set, it won't change in Rec array. This is from previous design and this CL does not change the behavior. http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:164: } On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > On 2011/07/01 21:27:22, arthurhsu wrote: > > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > > nit: should we have another sanity check that original is already in the > > > catalog? > > Nope, fonts are saved in global array, not the one in Catalog. > > I don't understand your reply. I'm saying that we shouldn't set a substitute > for an object that isn't in the catalog, but that can be a debug only check. > Done. http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:120: SkTDArray<struct SubstituteMapping> fSubObjectMap; On 2011/07/01 22:52:49, Chris Guillory wrote: > Nit: I don't believe struct is needed for the type in this template. Done. http://codereview.appspot.com/4650060/diff/15001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:120: SkTDArray<struct SubstituteMapping> fSubObjectMap; On 2011/07/01 22:52:49, Chris Guillory wrote: > Optional: SubObject -> Substitute. Seems a bit better to me. Optional. Done. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:139: SkPDFObject* SkPDFCatalog::getSubstituteObject(SkPDFObject* object) { On 2011/07/01 22:52:49, Chris Guillory wrote: > Optional: Reorder function definitions. getSubstituteObject comes after > setSubstitute in the header. Done. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:139: SkPDFObject* SkPDFCatalog::getSubstituteObject(SkPDFObject* object) { On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > On 2011/07/01 22:52:49, Chris Guillory wrote: > > Optional: Reorder function definitions. getSubstituteObject comes after > > setSubstitute in the header. > > Yes, please do this. Done. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:178: off_t offsetSum = fileOffset; On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > nit: move to just before for loop. Done. http://codereview.appspot.com/4650060/diff/15001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:190: SkTDArray<SkPDFObject*>* targetList = On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > nit: pull this into a helper function. Done. http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/15001/tests/PDFPrimitivesTest.cpp#... tests/PDFPrimitivesTest.cpp:141: REPORTER_ASSERT(reporter, stream_equals(buffer, 0, expectedResult, On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > Great! Can we also test that substitute object resources work as intended? Done.
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { On 2011/07/06 20:11:59, arthurhsu wrote: > On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > > On 2011/07/01 21:27:22, arthurhsu wrote: > > > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > > > With these changes, addObject should check if an object is already in the > > > > catalog. Even if it is, we may have to update the onFirstPage bit. > > > > > > We don't need to update the first page bit. If an object is added to > catalog, > > > it shall not be added again no matter what. > > > > Except, we may be violating that by adding substitute object resources. > > In the SkASSERT, findObjectIndex(obj) will do the check. If the original is > there, it won't return -1 and will trigger the assert. If the obj is a > substitute, the new findObjectIndex(obj) will return the assigned number of the > original object that it substitutes, which won't be -1 and will trigger the > assert, too. > > Once the onFirstPage bit is set, it won't change in Rec array. This is from > previous design and this CL does not change the behavior. Yes, if substitute object resources share a resources, the assert will fire, but we don't want to assert, we want the right thing to happen... http://codereview.appspot.com/4650060/diff/21001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/21001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:94: SkTDArray<SkPDFObject*>* getSubstituteList(bool firstPage); nit: order is types before fields before methods. http://codereview.appspot.com/4650060/diff/21001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/21001/tests/PDFPrimitivesTest.cpp#... tests/PDFPrimitivesTest.cpp:57: SkPDFCatalog dummy; nit: summy -> catalog
Sign in to reply to this message.
http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4650060/diff/12001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:36: SkPDFObject* SkPDFCatalog::addObject(SkPDFObject* obj, bool onFirstPage) { On 2011/07/06 22:21:22, Steve VanDeBogart wrote: > On 2011/07/06 20:11:59, arthurhsu wrote: > > On 2011/07/02 01:12:38, Steve VanDeBogart wrote: > > > On 2011/07/01 21:27:22, arthurhsu wrote: > > > > On 2011/07/01 18:52:47, Steve VanDeBogart wrote: > > > > > With these changes, addObject should check if an object is already in > the > > > > > catalog. Even if it is, we may have to update the onFirstPage bit. > > > > > > > > We don't need to update the first page bit. If an object is added to > > catalog, > > > > it shall not be added again no matter what. > > > > > > Except, we may be violating that by adding substitute object resources. > > > > In the SkASSERT, findObjectIndex(obj) will do the check. If the original is > > there, it won't return -1 and will trigger the assert. If the obj is a > > substitute, the new findObjectIndex(obj) will return the assigned number of > the > > original object that it substitutes, which won't be -1 and will trigger the > > assert, too. > > > > Once the onFirstPage bit is set, it won't change in Rec array. This is from > > previous design and this CL does not change the behavior. > > Yes, if substitute object resources share a resources, the assert will fire, but > we don't want to assert, we want the right thing to happen... Done. http://codereview.appspot.com/4650060/diff/21001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4650060/diff/21001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:94: SkTDArray<SkPDFObject*>* getSubstituteList(bool firstPage); On 2011/07/06 22:21:22, Steve VanDeBogart wrote: > nit: order is types before fields before methods. Done. http://codereview.appspot.com/4650060/diff/21001/tests/PDFPrimitivesTest.cpp File tests/PDFPrimitivesTest.cpp (right): http://codereview.appspot.com/4650060/diff/21001/tests/PDFPrimitivesTest.cpp#... tests/PDFPrimitivesTest.cpp:57: SkPDFCatalog dummy; On 2011/07/06 22:21:22, Steve VanDeBogart wrote: > nit: summy -> catalog Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
