Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2074583003:
SkPDF: allow overriding Producer metadata (Closed)
|
Created:
4 years, 6 months ago by hal.canary Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: allow overriding Producer metadata
I recommend not using this functionality.
Also, some documentation.
BUG=skia:5436
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003
Committed: https://skia.googlesource.com/skia/+/9f4b332f59a67658106d9aaedc8e75a93e04481d
Patch Set 1 #Patch Set 2 : 2016-06-16 (Thursday) 11:13:35 EDT #
Total comments: 9
Patch Set 3 : comments #Patch Set 4 : 2016-06-30 (Thursday) 10:42:59 EDT #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. ========== to ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003 ==========
halcanary@google.com changed reviewers: + hcm@google.com, reed@google.com, tomhudson@chromium.org
PTAL
PTAL
Description was changed from ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003 ========== to ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. BUG=skia:5436 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003 ==========
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/2074583003/1
tomhudson@google.com changed reviewers: + tomhudson@google.com
LGTM modulo ongoing discussions of better tag names. AdditionalProducer ConcurringProducer SupportingProducer ContributingProducer AssistantProducer ProductionLibrary SupportLibrary AlsoProducedBy
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/16 14:42:19, tomhudson wrote: > LGTM modulo ongoing discussions of better tag names. > > AdditionalProducer > ConcurringProducer > SupportingProducer > ContributingProducer > AssistantProducer > ProductionLibrary > SupportLibrary > AlsoProducedBy Functionality looks perfect for what we're trying to enable, LGTM. As discussed, I would like to pursue registration of the developer prefix with a tag name that works for all parties, SupportingProducer seems like a good option that keeps keyword Producer in the string.
ExectutiveProducer
Patchset #2 (id:20001) has been deleted
I favor ProductionLibrary. We can always change it later.
On 2016/06/16 15:14:45, Hal Canary wrote: > I favor ProductionLibrary. We can always change it later. SGTM
https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument.h File include/core/SkDocument.h (right): https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:56: /** Keywords associated with the document. */ How are the words delineated? https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:58: /** If the document was converted to PDF from another format, nit: 1. our comments have the closing */ on its own line 2. properly our block comments should have a * at the beginning of each line (not strictly enforced yet) https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:60: original document from which it was converted. */ If Creator means a tool/product, should we consider changing the name of the field to more closely match that? https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:62: /** Leave fProducer empty to get the default, correct value. */ Lets first document what fProduce means, and then have a comment that says it can be left blank to get the default.
https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument.h File include/core/SkDocument.h (right): https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:56: /** Keywords associated with the document. */ On 2016/06/16 18:43:49, reed1 wrote: > How are the words delineated? This is unspecified by the standard. https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:58: /** If the document was converted to PDF from another format, On 2016/06/16 18:43:49, reed1 wrote: > nit: > > 1. our comments have the closing */ on its own line > 2. properly our block comments should have a * at the beginning of each line > (not strictly enforced yet) Done. https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:60: original document from which it was converted. */ On 2016/06/16 18:43:49, reed1 wrote: > If Creator means a tool/product, should we consider changing the name of the > field to more closely match that? I would rather match what the standard says. https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:62: /** Leave fProducer empty to get the default, correct value. */ On 2016/06/16 18:43:49, reed1 wrote: > Lets first document what fProduce means, and then have a comment that says it > can be left blank to get the default. Done.
https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument.h File include/core/SkDocument.h (right): https://codereview.chromium.org/2074583003/diff/40001/include/core/SkDocument... include/core/SkDocument.h:56: /** Keywords associated with the document. */ On 2016/06/30 14:13:03, Hal Canary wrote: > On 2016/06/16 18:43:49, reed1 wrote: > > How are the words delineated? > > This is unspecified by the standard. New verbage: /* * Keywords associated with the document. Commas may be * used to delineate keywords within the string. */
lgtm
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hcm@google.com, tomhudson@google.com Link to the patchset: https://codereview.chromium.org/2074583003/#ps80001 (title: "2016-06-30 (Thursday) 10:42:59 EDT")
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 ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. BUG=skia:5436 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003 ========== to ========== SkPDF: allow overriding Producer metadata I recommend not using this functionality. Also, some documentation. BUG=skia:5436 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2074583003 Committed: https://skia.googlesource.com/skia/+/9f4b332f59a67658106d9aaedc8e75a93e04481d ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://skia.googlesource.com/skia/+/9f4b332f59a67658106d9aaedc8e75a93e04481d
Message was sent while issue was closed.
CQ bit was unchecked. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
