Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1802033003:
SkPDF: SkPDFDocument reorganized so that some objects can be serialized early. (Closed)
|
Created:
4 years, 9 months ago by hal.canary Modified:
4 years, 9 months ago CC:
reviews_skia.org, djsollen Base URL:
https://skia.googlesource.com/skia.git@SkPdfDumpMethod Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: SkPDFDocument reorganized so that some objects can be serialized early.
No change in output.
BUG=skia:5087
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1802033003
Committed: https://skia.googlesource.com/skia/+/50e82e61766d22da5238905916a8abc3e6664060
Patch Set 1 #
Total comments: 28
Patch Set 2 : 2016-03-21 (Monday) 14:46:29 EDT #Patch Set 3 : 2016-03-21 (Monday) 15:39:58 EDT #Patch Set 4 : rebase #Patch Set 5 : 2016-03-21 (Monday) 16:31:34 EDT #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== SkPDF: SkPDFDocument reorganized so that some objects can be serialized early. No change in output. BUG=skia:5087 ========== to ========== SkPDF: SkPDFDocument reorganized so that some objects can be serialized early. No change in output. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + tomhudson@google.com
This CL depends on https://crrev.com/1790023003 Please review that first.
djsollen@google.com changed reviewers: + djsollen@google.com
https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h File src/pdf/SkPDFDocument.h (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:19: // Logically part of SkPDFDocument (like SkPDFCanon), but but what?
https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:40: const auto& objects = fObjNumMap.objects(); Still trying to stop worrying and love the auto; it seems to me that this is landing on both the pro and con side of https://google.github.io/styleguide/cppguide.html#auto. If fObjNumMap.objects() were transparently typed that might be OK, but having to call get() is a surprise that tells me that perhaps this is a more complicated than we'd intuit from just seeing a bare auto? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:43: int32_t index = fNextToBeSerialized + 1; // Skip object 0. I can see equivalent code in the previous version (lines 234-235), but I'd like to know *why* you aren't writing object 0. Is PDF's array 1-based? Am I missing a note about this somewhere else in our codebase? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:62: // Include the zeroth object in the count. Yeah, confusing after we previously skipped object zero. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:63: int32_t objCount = SkToS32(fOffsets.count() + 1); Switched from int64_t to int32_t? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:74: trailerDict.insertObjRef("Root", docCatalog); In the old code we explicitly ref'd this here? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:257: // subclasses must call close() in their destructors subclasses of SkDocument, or subclasses of SkPDFDocument? (If the latter, won't this automatically be invoked as subclass destructors fall through to superclass destructors? The SkDocument header comment says that doesn't work, because SkDocument::close() isn't virtual, but I don't see why that's necessarily true?) https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:261: void SkPDFDocument::serialize(const sk_sp<SkPDFObject>& object) { Name reads like it's going to serialize a single object, but implementation reads like it's going to serialize the entire document, including the new object? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:268: SkASSERT(!fCanvas.get()); // end page was called. Nit: This comment unclear. Reading through the code further it becomes understandable, but I'd either expand so it can be grasped completely here, or remove. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:292: renew(&fObjectSerializer); Should we be resetting fCanvas? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:357: // Build font subsetting info before proceeding. Nit: unnecessary comment? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:367: // } Nit: Delete before committing? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h File src/pdf/SkPDFDocument.h (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:12: You also need to #include SkPDFTypes.h here to get SkPDFObjNumMap et al!? https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:36: class SkPDFDocument : public SkDocument { This is an implementation detail of SkDocument, buried in src/. Is there really *nothing* that a developer needs to know to understand the relationship between the two? (From my reading the code the answer may very well be yes, but I want explicit agreement before landing without doxygen.)
https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:40: const auto& objects = fObjNumMap.objects(); On 2016/03/21 15:55:11, tomhudson wrote: > Still trying to stop worrying and love the auto; it seems to me that this is > landing on both the pro and con side of > https://google.github.io/styleguide/cppguide.html#auto. If fObjNumMap.objects() > were transparently typed that might be OK, but having to call get() is a > surprise that tells me that perhaps this is a more complicated than we'd intuit > from just seeing a bare auto? Done. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:43: int32_t index = fNextToBeSerialized + 1; // Skip object 0. On 2016/03/21 15:55:11, tomhudson wrote: > I can see equivalent code in the previous version (lines 234-235), but I'd like > to know *why* you aren't writing object 0. Is PDF's array 1-based? Am I missing > a note about this somewhere else in our codebase? This is a PDF thing. Object zero is special. "The first entry in the table (object number 0) is always free and has a generation number of 65,535; it is the head of the linked list of free objects." https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:62: // Include the zeroth object in the count. On 2016/03/21 15:55:12, tomhudson wrote: > Yeah, confusing after we previously skipped object zero. Acknowledged. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:63: int32_t objCount = SkToS32(fOffsets.count() + 1); On 2016/03/21 15:55:12, tomhudson wrote: > Switched from int64_t to int32_t? For whatever reason, `writeDecAsText` takes an int32_t. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:74: trailerDict.insertObjRef("Root", docCatalog); On 2016/03/21 15:55:12, tomhudson wrote: > In the old code we explicitly ref'd this here? we switched to using sk_sp<T> no need. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:257: // subclasses must call close() in their destructors On 2016/03/21 15:55:11, tomhudson wrote: > subclasses of SkDocument, or subclasses of SkPDFDocument? > (If the latter, won't this automatically be invoked as subclass destructors fall > through to superclass destructors? The SkDocument header comment says that > doesn't work, because SkDocument::close() isn't virtual, but I don't see why > that's necessarily true?) // subclasses of SkDocument https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:261: void SkPDFDocument::serialize(const sk_sp<SkPDFObject>& object) { On 2016/03/21 15:55:11, tomhudson wrote: > Name reads like it's going to serialize a single object, but implementation > reads like it's going to serialize the entire document, including the new > object? new comments: /** Serialize the object, as well as any other objects it indirectly refers to. If any any other objects have been added to the SkPDFObjNumMap without serializing them, they will be serialized as well. It might go without saying that objects should not be changed after calling serialize, since those changes will be too late. The same goes for changes to the SkPDFSubstituteMap that effect the object or its dependencies. */ https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:268: SkASSERT(!fCanvas.get()); // end page was called. On 2016/03/21 15:55:12, tomhudson wrote: > Nit: This comment unclear. > Reading through the code further it becomes understandable, but I'd either > expand so it can be grasped completely here, or remove. Done. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:292: renew(&fObjectSerializer); On 2016/03/21 15:55:11, tomhudson wrote: > Should we be resetting fCanvas? Done. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:357: // Build font subsetting info before proceeding. On 2016/03/21 15:55:12, tomhudson wrote: > Nit: unnecessary comment? // Build font subsetting info before calling addObjectRecursively() https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.cpp#n... src/pdf/SkPDFDocument.cpp:367: // } On 2016/03/21 15:55:11, tomhudson wrote: > Nit: Delete before committing? Done. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h File src/pdf/SkPDFDocument.h (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:12: On 2016/03/21 15:55:12, tomhudson wrote: > You also need to #include SkPDFTypes.h here to get SkPDFObjNumMap et al!? Done. https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:19: // Logically part of SkPDFDocument (like SkPDFCanon), but On 2016/03/16 12:41:49, djsollen wrote: > but what? Done.
https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h File src/pdf/SkPDFDocument.h (right): https://codereview.chromium.org/1802033003/diff/1/src/pdf/SkPDFDocument.h#new... src/pdf/SkPDFDocument.h:36: class SkPDFDocument : public SkDocument { On 2016/03/21 15:55:12, tomhudson wrote: > This is an implementation detail of SkDocument, buried in src/. Is there really > *nothing* that a developer needs to know to understand the relationship between > the two? > > (From my reading the code the answer may very well be yes, but I want explicit > agreement before landing without doxygen.) /** Concrete implementation of SkDocument that creates PDF files. This class does not produced linearized or optimized PDFs; instead it it attempts to use a minimum amount of RAM. */
Patchset #3 (id:40001) has been deleted
lgtm
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802033003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802033003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com Link to the patchset: https://codereview.chromium.org/1802033003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802033003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802033003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com Link to the patchset: https://codereview.chromium.org/1802033003/#ps100001 (title: "2016-03-21 (Monday) 16:31:34 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802033003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802033003/100001
Message was sent while issue was closed.
Description was changed from ========== SkPDF: SkPDFDocument reorganized so that some objects can be serialized early. No change in output. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: SkPDFDocument reorganized so that some objects can be serialized early. No change in output. BUG=skia:5087 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/50e82e61766d22da5238905916a8abc3e6664060 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://skia.googlesource.com/skia/+/50e82e61766d22da5238905916a8abc3e6664060 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
