Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1397333003:
PDF Printing: embed browser user agent string in PDF metadata. (Closed)
|
Created:
5 years, 2 months ago by hal.canary Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPDF Printing: embed browser user agent string in PDF metadata.
Committed: https://crrev.com/73b63fd9d4501782f0033ba72f21a33e43fe290c
Cr-Commit-Position: refs/heads/master@{#358186}
Patch Set 1 #
Total comments: 1
Patch Set 2 : 2015-11-05 (Thursday) 15:50:39 EST #
Total comments: 12
Patch Set 3 : 2015-11-05 (Thursday) 17:13:38 EST #
Total comments: 4
Patch Set 4 : 2015-11-05 (Thursday) 17:34:22 EST #
Messages
Total messages: 29 (6 generated)
halcanary@google.com changed reviewers: + thestig@chromium.org, vitalybuka@chromium.org
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/1397333003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397333003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
PTAL. I think this will be worth it to make triaging printing issues easier.
https://codereview.chromium.org/1397333003/diff/1/printing/DEPS File printing/DEPS (right): https://codereview.chromium.org/1397333003/diff/1/printing/DEPS#newcode3 printing/DEPS:3: "+chrome/common", Sorry, but this is a layering violation. Instead, have a global in printing/, provide a SetAgent() function, and call that from chrome/.
Description was changed from ========== PDF Prining: embed browser user agent string in PDF metadata. ========== to ========== PDF Printing: embed browser user agent string in PDF metadata. ==========
On 2015/11/05 16:41:40, Lei Zhang wrote: > Instead, have a global in printing/, provide a SetAgent() function, and call > that from chrome/. That is, printing::SetAgent(). I also edited the CL desc to fix a typo.
On 2015/11/05 at 16:44:08, thestig wrote: > On 2015/11/05 16:41:40, Lei Zhang wrote: > > Instead, have a global in printing/, provide a SetAgent() function, and call > > that from chrome/. > > That is, printing::SetAgent(). I also edited the CL desc to fix a typo. where in chrome/ should I call printing::SetAgent()?
On 2015/11/05 17:28:57, Hal Canary wrote: > where in chrome/ should I call printing::SetAgent()? chrome/browser/browser_process_impl.cc, where we create the PrintJobManager seem good to me. You may want to default to Chromium if printing::SetAgent() never gets called. e.g. in other browsers based on Chromium but does not use chrome/.
lgtm
On 2015/11/05 18:55:16, Vitaly Buka wrote: > lgtm Sorry lgtmed before reading conversation, please continue with Lei.
On 2015/11/05 at 17:42:03, thestig wrote: > On 2015/11/05 17:28:57, Hal Canary wrote: > > where in chrome/ should I call printing::SetAgent()? > > chrome/browser/browser_process_impl.cc, where we create the PrintJobManager seem good to me. You may want to default to Chromium if printing::SetAgent() never gets called. e.g. in other browsers based on Chromium but does not use chrome/. That doesn't seem to work. Is it in a different process?
On 2015/11/05 20:00:08, Hal Canary wrote: > On 2015/11/05 at 17:42:03, thestig wrote: > > On 2015/11/05 17:28:57, Hal Canary wrote: > > > where in chrome/ should I call printing::SetAgent()? > > > > chrome/browser/browser_process_impl.cc, where we create the PrintJobManager > seem good to me. You may want to default to Chromium if printing::SetAgent() > never gets called. e.g. in other browsers based on Chromium but does not use > chrome/. > > That doesn't seem to work. Is it in a different process? What platform did you try?
On 2015/11/05 at 20:19:43, thestig wrote: > On 2015/11/05 20:00:08, Hal Canary wrote: > > On 2015/11/05 at 17:42:03, thestig wrote: > > > On 2015/11/05 17:28:57, Hal Canary wrote: > > > > where in chrome/ should I call printing::SetAgent()? > > > > > > chrome/browser/browser_process_impl.cc, where we create the PrintJobManager > > seem good to me. You may want to default to Chromium if printing::SetAgent() > > never gets called. e.g. in other browsers based on Chromium but does not use > > chrome/. > > > > That doesn't seem to work. Is it in a different process? > > What platform did you try? I'm on linux.
On 2015/11/05 20:22:42, Hal Canary wrote: > I'm on linux. Oh, this gets called in a renderer process. Let me find a better place to call it. In the meanwhile, do you care to write a test to verify this works?
On 2015/11/05 20:27:02, Lei Zhang wrote: > Oh, this gets called in a renderer process. Let me find a better place to call > it. Try the ChromeContentRendererClient ctor.
On 2015/11/05 at 20:27:02, thestig wrote: > On 2015/11/05 20:22:42, Hal Canary wrote: > > I'm on linux. > > Oh, this gets called in a renderer process. Let me find a better place to call it. > > In the meanwhile, do you care to write a test to verify this works? I am testing PDF output manually, until we develop a better way of testing.
On 2015/11/05 at 16:41:40, thestig wrote: > https://codereview.chromium.org/1397333003/diff/1/printing/DEPS > File printing/DEPS (right): > > https://codereview.chromium.org/1397333003/diff/1/printing/DEPS#newcode3 > printing/DEPS:3: "+chrome/common", > Sorry, but this is a layering violation. > > Instead, have a global in printing/, provide a SetAgent() function, and call that from chrome/. Please take a look at patch set 2.
https://codereview.chromium.org/1397333003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1397333003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:18: #include "chrome/common/chrome_content_client.h" You can move both new #includes into the ENABLE_PRINTING section down near line 129. https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:167: const std::string& user_agent = printing::GetAgent(); no need for printing:: https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:168: if (user_agent.size() == 0) { Most foo.size() == 0 checks can be more easily written as foo.empty() How about: info.emplace_back( SkString("Creator"), user_agent.empty() ? SkString("Chromium") : SkString(user_agent.c_str(), user_agent.size())); https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings.cc File printing/print_settings.cc (right): https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.cc:14: std::string g_user_agent; I think this creates a static initializer. You probably want to wrap it in a LazyInstance. https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.cc:16: g_user_agent = user_agent; 2 space indents, not 4. https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings.h File printing/print_settings.h (right): https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.h:31: PRINTING_EXPORT void SetAgent(const std::string&); include the parameter name
https://codereview.chromium.org/1397333003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1397333003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:18: #include "chrome/common/chrome_content_client.h" On 2015/11/05 at 21:37:23, Lei Zhang wrote: > You can move both new #includes into the ENABLE_PRINTING section down near line 129. done https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:167: const std::string& user_agent = printing::GetAgent(); On 2015/11/05 at 21:37:23, Lei Zhang wrote: > no need for printing:: done https://codereview.chromium.org/1397333003/diff/20001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:168: if (user_agent.size() == 0) { On 2015/11/05 at 21:37:23, Lei Zhang wrote: > Most foo.size() == 0 checks can be more easily written as foo.empty() > > How about: > > info.emplace_back( > SkString("Creator"), > user_agent.empty() ? SkString("Chromium") > : SkString(user_agent.c_str(), user_agent.size())); done https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings.cc File printing/print_settings.cc (right): https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.cc:14: std::string g_user_agent; On 2015/11/05 at 21:37:23, Lei Zhang wrote: > I think this creates a static initializer. You probably want to wrap it in a LazyInstance. done https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.cc:16: g_user_agent = user_agent; On 2015/11/05 at 21:37:23, Lei Zhang wrote: > 2 space indents, not 4. done https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings.h File printing/print_settings.h (right): https://codereview.chromium.org/1397333003/diff/20001/printing/print_settings... printing/print_settings.h:31: PRINTING_EXPORT void SetAgent(const std::string&); On 2015/11/05 at 21:37:23, Lei Zhang wrote: > include the parameter name done
lgtm with some nits. https://codereview.chromium.org/1397333003/diff/40001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/1397333003/diff/40001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:170: : SkString(user_agent.c_str(), user_agent.size())); Can we do this in some way as suggested by the style guide? https://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/1397333003/diff/40001/printing/print_settings.cc File printing/print_settings.cc (right): https://codereview.chromium.org/1397333003/diff/40001/printing/print_settings... printing/print_settings.cc:15: base::LazyInstance<std::string> g_user_agent; Add a blank line after.
The CQ bit was checked by halcanary@google.com
https://codereview.chromium.org/1397333003/diff/40001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/1397333003/diff/40001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:170: : SkString(user_agent.c_str(), user_agent.size())); On 2015/11/05 at 22:24:09, Lei Zhang wrote: > Can we do this in some way as suggested by the style guide? https://www.chromium.org/developers/coding-style#Code_Formatting done https://codereview.chromium.org/1397333003/diff/40001/printing/print_settings.cc File printing/print_settings.cc (right): https://codereview.chromium.org/1397333003/diff/40001/printing/print_settings... printing/print_settings.cc:15: base::LazyInstance<std::string> g_user_agent; On 2015/11/05 at 22:24:09, Lei Zhang wrote: > Add a blank line after. done
The patchset sent to the CQ was uploaded after l-g-t-m from vitalybuka@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1397333003/#ps60001 (title: "2015-11-05 (Thursday) 17:34:22 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397333003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397333003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/73b63fd9d4501782f0033ba72f21a33e43fe290c Cr-Commit-Position: refs/heads/master@{#358186} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
