Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2295373002:
documentation: update PDF design doc (Closed)
|
Created:
4 years, 3 months ago by hal.canary Modified:
4 years, 2 months ago Reviewers:
tomhudson CC:
reviews_skia.org Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptiondocumentation: update PDF design doc
NOTRY=true
Committed: https://skia.googlesource.com/skia/+/ca9654e4c6ff1a8a54d3ae1e277ba7e38a0fa00b
Patch Set 1 #
Total comments: 16
Patch Set 2 : bungeman #
Total comments: 6
Patch Set 3 : comments from tom #Patch Set 4 : more changes #Patch Set 5 : final #Messages
Total messages: 17 (11 generated)
Description was changed from ========== documentation: update PDF design doc BUG=skia: ========== to ========== documentation: update PDF design doc BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 NOTRY=true DOCS_PREVIEW= https://skia.org/?cl=2295373002 ==========
Description was changed from ========== documentation: update PDF design doc BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 NOTRY=true DOCS_PREVIEW= https://skia.org/?cl=2295373002 ========== to ========== documentation: update PDF design doc NOTRY=true DOCS_PREVIEW= https://skia.org/dev/design/pdftheory?cl=2295373002 ==========
halcanary@google.com changed reviewers: + bungeman@google.com, tomhudson@google.com
ptal
https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.md File site/dev/design/pdftheory.md (right): https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:65: objects is known, which requires assignment of object numbers. use Capitalize 'use'. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:529: show that almost all fonts are truetype. Can we add a ')' at the end of this line. The unbalanced parens are making me uneasy.
Description was changed from ========== documentation: update PDF design doc NOTRY=true DOCS_PREVIEW= https://skia.org/dev/design/pdftheory?cl=2295373002 ========== to ========== documentation: update PDF design doc NOTRY=true DOCS_PREVIEW= https://skia.org/dev/design/pdftheory?cl=2295373002 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 ==========
I have lots of concerns with this doc, we could spend forever editing, but your changes mostly LGTM; comments below can all be ignored. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.md File site/dev/design/pdftheory.md (right): https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:15: [Using Skia's PDF Backend](../../user/sample/pdf). Nit: This is a tautological statement that doesn't actually tell me what's at the link or why I'd want to follow it. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:17: Internally, Skia uses SkPDFDocument and SkPDFDevice to represent PDF Nit: you could shed the passive voice and be a bit more precise here. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:20: are subject to perpetual change.** Maybe the "make use of" sentence, reworded, belongs after this paragraph? https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:45: SkDevice can be set on an SkPDFCanvas and drawn to. Once drawing is ...can be set on an SkPDFCanvas. Once drawing to the canvas is complete, ... https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:46: complete, the device's content and resouces are added to the How is completion of drawing signaled via the API? https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:50: called to finish the PDF file. Do we need any more definition of what 'finish' entails? https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... File site/dev/design/pdftheory.md (right): https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:97: used as resources. SkPDFTypes.h defines most of the basic PDF objects Nit: object shouldn't be plural here. https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:102: both a direct and an indirect fashion, i.e. an array can have an int Nit: this isn't clear at all. Most of the rest of this paragraph isn't, either. https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:528: Mostly, only TTF fonts are directly supported. (User metrics Nit: mostly only?
Description was changed from ========== documentation: update PDF design doc NOTRY=true DOCS_PREVIEW= https://skia.org/dev/design/pdftheory?cl=2295373002 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 ========== to ========== documentation: update PDF design doc NOTRY=true ==========
halcanary@google.com changed reviewers: - bungeman@google.com
Description was changed from ========== documentation: update PDF design doc NOTRY=true ========== to ========== documentation: update PDF design doc NOTRY=true GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 DOCS_PREVIEW= https://skia.org/?cl=2295373002 ==========
Description was changed from ========== documentation: update PDF design doc NOTRY=true GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2295373002 DOCS_PREVIEW= https://skia.org/?cl=2295373002 ========== to ========== documentation: update PDF design doc NOTRY=true ==========
https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.md File site/dev/design/pdftheory.md (right): https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:15: [Using Skia's PDF Backend](../../user/sample/pdf). On 2016/09/01 19:11:58, tomhudson wrote: > Nit: This is a tautological statement that doesn't actually tell me what's at > the link or why I'd want to follow it. Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:17: Internally, Skia uses SkPDFDocument and SkPDFDevice to represent PDF On 2016/09/01 19:11:58, tomhudson wrote: > Nit: you could shed the passive voice and be a bit more precise here. Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:20: are subject to perpetual change.** On 2016/09/01 19:11:58, tomhudson wrote: > Maybe the "make use of" sentence, reworded, belongs after this paragraph? Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:45: SkDevice can be set on an SkPDFCanvas and drawn to. Once drawing is On 2016/09/01 19:11:58, tomhudson wrote: > ...can be set on an SkPDFCanvas. Once drawing to the canvas is complete, ... Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:46: complete, the device's content and resouces are added to the On 2016/09/01 19:11:58, tomhudson wrote: > How is completion of drawing signaled via the API? Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:50: called to finish the PDF file. On 2016/09/01 19:11:58, tomhudson wrote: > Do we need any more definition of what 'finish' entails? Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:65: objects is known, which requires assignment of object numbers. use On 2016/08/31 21:14:26, bungeman-skia wrote: > Capitalize 'use'. Done. https://codereview.chromium.org/2295373002/diff/1/site/dev/design/pdftheory.m... site/dev/design/pdftheory.md:529: show that almost all fonts are truetype. On 2016/08/31 21:14:26, bungeman-skia wrote: > Can we add a ')' at the end of this line. The unbalanced parens are making me > uneasy. Done. https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... File site/dev/design/pdftheory.md (right): https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:97: used as resources. SkPDFTypes.h defines most of the basic PDF objects On 2016/09/01 19:11:58, tomhudson wrote: > Nit: object shouldn't be plural here. Done. https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:102: both a direct and an indirect fashion, i.e. an array can have an int On 2016/09/01 19:11:58, tomhudson wrote: > Nit: this isn't clear at all. Most of the rest of this paragraph isn't, either. Acknowledged. https://codereview.chromium.org/2295373002/diff/20001/site/dev/design/pdftheo... site/dev/design/pdftheory.md:528: Mostly, only TTF fonts are directly supported. (User metrics On 2016/09/01 19:11:58, tomhudson wrote: > Nit: mostly only? Acknowledged.
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/2295373002/#ps80001 (title: "final")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== documentation: update PDF design doc NOTRY=true ========== to ========== documentation: update PDF design doc NOTRY=true Committed: https://skia.googlesource.com/skia/+/ca9654e4c6ff1a8a54d3ae1e277ba7e38a0fa00b ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/ca9654e4c6ff1a8a54d3ae1e277ba7e38a0fa00b |
