Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780433002:
add print to pdf for headless (Closed)
DescriptionLet users to print a webpage into pdf through PrintToPDF devtools command
under headless chrome.
This patch includes:
1) adding a new print manager, i.e., HeadlessPrintManager to handle all the
printing related IPCs for headless.
2) plumbing through the pdf data from HeadlessPrintManager to
HeadlessDevToolsManagerDelegate to handle the PrintToPDF command.
3) adding a new option to PrintWebViewHelper on Mac to return all the printed
pages in the first PrintHostMsg_DidPrintPage message as a single file.
This makes it conform to Linux and Windows, which simplify the logic in
HeadlessPrintManager.
This patch guarantees that HeadlessPrintManager is the only IPC entry point.
In the future, if there are requirements to support the same devtools command
in regular chrome, we may consider a framework to ensure multiple print managers
to work properly under a content embedder.
BUG=603559
Review-Url: https://codereview.chromium.org/2780433002
Cr-Commit-Position: refs/heads/master@{#463105}
Committed: https://chromium.googlesource.com/chromium/src/+/0cbec8b353f24515cc77a67ee95addc4f7a714ab
Patch Set 1 #
Total comments: 54
Patch Set 2 : fix lint and style errors #
Total comments: 48
Patch Set 3 : fix style #Patch Set 4 : test mac #Patch Set 5 : add new option for mac #
Total comments: 2
Patch Set 6 : fix mac page setting #
Total comments: 24
Patch Set 7 : polish code #Patch Set 8 : fix typo #
Total comments: 6
Patch Set 9 : add more comment and error handle protection #
Total comments: 1
Patch Set 10 : improve comments as suggested #
Total comments: 2
Messages
Total messages: 69 (28 generated)
Description was changed from ========== add print to pdf for headless BUG=603559 ========== to ========== add print to pdf for headless As discussed with Lei, we agreed on adding a new print manager to headless. This won't cause multiple IPC handlers, since the new print manager is only installed by headless. And the printing people is willing to maintain this new print manager since there are already several different managers in chrome. In the future, we may find good ways to let content embedders to install multiple print managers, but it requires a lot of work. BUG=603559 ==========
jzfeng@chromium.org changed reviewers: + thestig@chromium.org
This is a proto type that only works on linux. PTAL.
Hi Lei, Could you take some time to read and give some high level comments? Thanks!
On 2017/03/28 23:01:09, jzfeng wrote: > Hi Lei, > > Could you take some time to read and give some high level comments? > > Thanks! Pong. It's in the queue.
This is much more straight-forward. My main concern is about getting browser / renderer separation right. The printing code looks similar to the Android WebView version so that's fine. We may need to improve it a bit more to work on Windows/Mac. https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", Is headless multi-process? Do you intend to separate browser from renderer? If yes, the team should discuss where to do that separation. Right now this makes it look like everything goes in 1 big build target. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/DEPS File headless/lib/browser/DEPS (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/DEPS#n... headless/lib/browser/DEPS:2: "+components/printing", You should restrict this to c/p/{browser, common}. headless/lib/browser should not have access to c/p/renderer. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:78: if (print_result == printing::HeadlessPrintManager::kPrintSuccess) nit: You need braces when the body is multi-line. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:141: return false; nit: add a blank line like after line 115. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:154: ((this)->*async_command_fn_ptr)(agent_host, id, params, callback); nit: Do you need a parens around "this" ? Same for line 130. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:31: CommandCallback callback) override; Pass by const-ref. You need to fix content/public/browser/devtools_manager_delegate.h (from r451947) as well. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:63: using AsyncCommandMemberFnPtr = void (HeadlessDevToolsManagerDelegate::*)( I asked on https://codereview.chromium.org/2119063002 why we have CommandMemberFnPtr instead of using just base::Callback. Let's see how that conversation proceeds and adjust this accordingly. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:30: int dpi = printing::kDefaultPdfDpi; No need for printing:: if you decide to keep this class in namespace printing. (Or do you want namespace headless?) https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:55: printing_rfh_(nullptr), Maybe implement a Reset() method and call it both here and in ReleaseJob() ? https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:87: return "Unknown PrintResult"; Probably needs a NOTREACHED(); https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:96: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()); auto result = base::MakeUnique<base::DictionaryValue>(); https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:105: return true; Shouldn't this return false? https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:176: new PdfMetafileSkia(PDF_SKIA_DOCUMENT_TYPE)); More base::MakeUnique. Basically try to avoid using "new" in new code if possible. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:25: enum PrintResult { nit: This and the callback defintion goes first, per style guide. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:41: bool GetPDFContents(content::RenderFrameHost* rfh, GetPDFCallback callback); nit: Pass callbacks by const-ref here too. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:48: friend class content::WebContentsUserData<HeadlessPrintManager>; nit: friends first in this section. https://codereview.chromium.org/2780433002/diff/1/headless/lib/headless_conte... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/headless_conte... headless/lib/headless_content_main_delegate.cc:276: renderer_client_.reset(new HeadlessContentRendererClient); nit: foo.reset(new Foo(bar)) -> foo = base::MakeUnique<Foo>(bar) https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_content_renderer_client.cc:22: } nit: blank line before, add // namespace headless comment. If you run "git cl lint" it should tell you these things. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_content_renderer_client.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_content_renderer_client.h:19: DISALLOW_COPY_AND_ASSIGN(HeadlessContentRendererClient); nit: This needs to be private. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_print_web_view_helper_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.cc:32: bool HeadlessPrintWebViewHelperDelegate::IsAskPrintSettingsEnabled() { For later - You may want to flip this to true, so you can feed in the settings from the user however you like. WebView did this recently in https://codereview.chromium.org/2770713003/ https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_print_web_view_helper_delegate.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.h:15: ~HeadlessPrintWebViewHelperDelegate() override; Provide a ctor. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.h:17: bool CancelPrerender(content::RenderFrame* render_frame) override; nit: Bundle the override methods together. e.g. class Foo : public Bar { public: Foo(); ~Foo() override; // Bar: BarMethod1(...) override; ... BarMethodN(...) override; NotOverrideMethod(...); ... };
jzfeng@chromium.org changed reviewers: + eseckler@chromium.org
Thanks for the review! I'm setting up my windows and mac machine for development. Eric, would you minding also take a look at this cl and the comments from Lei? I'm not sure about the browser / renderer separation issue. Thanks! https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", On 2017/03/29 at 01:38:03, Lei Zhang wrote: > Is headless multi-process? Do you intend to separate browser from renderer? If yes, the team should discuss where to do that separation. Right now this makes it look like everything goes in 1 big build target. As I remember, we discussed this issue a bit and agreed on putting them together. Add eseckler@ to explain more on this issue. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/DEPS File headless/lib/browser/DEPS (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/DEPS#n... headless/lib/browser/DEPS:2: "+components/printing", On 2017/03/29 at 01:38:03, Lei Zhang wrote: > You should restrict this to c/p/{browser, common}. headless/lib/browser should not have access to c/p/renderer. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:78: if (print_result == printing::HeadlessPrintManager::kPrintSuccess) On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: You need braces when the body is multi-line. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:141: return false; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: add a blank line like after line 115. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.cc:154: ((this)->*async_command_fn_ptr)(agent_host, id, params, callback); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: Do you need a parens around "this" ? Same for line 130. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:31: CommandCallback callback) override; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > Pass by const-ref. You need to fix content/public/browser/devtools_manager_delegate.h (from r451947) as well. Done. So whenever I want to pass a callback, it should be const-ref, right? https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:63: using AsyncCommandMemberFnPtr = void (HeadlessDevToolsManagerDelegate::*)( On 2017/03/29 at 01:38:03, Lei Zhang wrote: > I asked on https://codereview.chromium.org/2119063002 why we have CommandMemberFnPtr instead of using just base::Callback. Let's see how that conversation proceeds and adjust this accordingly. Sounds good. Maybe Eric can also answer this question. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:30: int dpi = printing::kDefaultPdfDpi; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > No need for printing:: if you decide to keep this class in namespace printing. (Or do you want namespace headless?) Done. I feel it is ok to be under printing like PrintViewManager. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:55: printing_rfh_(nullptr), On 2017/03/29 at 01:38:03, Lei Zhang wrote: > Maybe implement a Reset() method and call it both here and in ReleaseJob() ? Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:87: return "Unknown PrintResult"; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > Probably needs a NOTREACHED(); Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:96: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > auto result = base::MakeUnique<base::DictionaryValue>(); Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:105: return true; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > Shouldn't this return false? I was thinking something else. Changed to just return void. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:176: new PdfMetafileSkia(PDF_SKIA_DOCUMENT_TYPE)); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > More base::MakeUnique. Basically try to avoid using "new" in new code if possible. Cool. Also change some other places that use "new". https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 at 01:38:03, Lei Zhang wrote: > no "(c)" Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:25: enum PrintResult { On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: This and the callback defintion goes first, per style guide. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:41: bool GetPDFContents(content::RenderFrameHost* rfh, GetPDFCallback callback); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: Pass callbacks by const-ref here too. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:48: friend class content::WebContentsUserData<HeadlessPrintManager>; On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: friends first in this section. I see both content/public/browser/web_contents_user_data.h and /chrome/browser/printing/print_view_manager.h put friend after the explicit constructor. I feel a bit confused. https://codereview.chromium.org/2780433002/diff/1/headless/lib/headless_conte... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/headless_conte... headless/lib/headless_content_main_delegate.cc:276: renderer_client_.reset(new HeadlessContentRendererClient); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: foo.reset(new Foo(bar)) -> foo = base::MakeUnique<Foo>(bar) Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_content_renderer_client.cc:22: } On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: blank line before, add // namespace headless comment. If you run "git cl lint" it should tell you these things. This command is useful! Fixed all lint errors. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_content_renderer_client.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_content_renderer_client.h:19: DISALLOW_COPY_AND_ASSIGN(HeadlessContentRendererClient); On 2017/03/29 at 01:38:03, Lei Zhang wrote: > nit: This needs to be private. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_print_web_view_helper_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.cc:32: bool HeadlessPrintWebViewHelperDelegate::IsAskPrintSettingsEnabled() { On 2017/03/29 at 01:38:03, Lei Zhang wrote: > For later - You may want to flip this to true, so you can feed in the settings from the user however you like. WebView did this recently in https://codereview.chromium.org/2770713003/ This cl is helpful! The current plan is to let users feed the settings through devtools command instead of using a fixed one. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... File headless/lib/renderer/headless_print_web_view_helper_delegate.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.h:15: ~HeadlessPrintWebViewHelperDelegate() override; On 2017/03/29 at 01:38:04, Lei Zhang wrote: > Provide a ctor. Done. https://codereview.chromium.org/2780433002/diff/1/headless/lib/renderer/headl... headless/lib/renderer/headless_print_web_view_helper_delegate.h:17: bool CancelPrerender(content::RenderFrame* render_frame) override; On 2017/03/29 at 01:38:04, Lei Zhang wrote: > nit: Bundle the override methods together. e.g. > > class Foo : public Bar { > public: > Foo(); > ~Foo() override; > > // Bar: > BarMethod1(...) override; > ... > BarMethodN(...) override; > > NotOverrideMethod(...); > ... > }; Done.
https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", On 2017/03/29 03:50:12, jzfeng wrote: > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > Is headless multi-process? Do you intend to separate browser from renderer? If > yes, the team should discuss where to do that separation. Right now this makes > it look like everything goes in 1 big build target. > > As I remember, we discussed this issue a bit and agreed on putting them > together. Add eseckler@ to explain more on this issue. So is headless multi-process? If the answer is no, then this makes a bit more sense. If there's a discussion thread somewhere, I can take a look. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.h:48: friend class content::WebContentsUserData<HeadlessPrintManager>; On 2017/03/29 03:50:13, jzfeng wrote: > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > nit: friends first in this section. > > I see both content/public/browser/web_contents_user_data.h and > /chrome/browser/printing/print_view_manager.h put friend after the explicit > constructor. > I feel a bit confused. I guess it doesn't matter since https://google.github.io/styleguide/cppguide.html#Declaration_Order now doesn't say anything about friends. Previously, it just said they should be private. https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... File content/public/browser/devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... content/public/browser/devtools_manager_delegate.h:54: virtual bool HandleAsyncCommand(DevToolsAgentHost* agent_host, Add comment to explain what this does. https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... content/public/browser/devtools_manager_delegate.h:56: const CommandCallback& callback); Blank line after. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:363: OnFileOpened("", file_name, base::File::FILE_ERROR_FAILED); Pass in a std::string() instead of "". There's a few more in HeadlessPrintManager. See https://crrev.com/193040 for the reason. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:378: base::Base64Decode(data, &decoded_data); base::Base64Decode() can fail, BTW. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.h:74: void OnPDFCreated(std::unique_ptr<page::PrintToPDFResult> result); Can this and methods below be private? https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.h:53: CommandCallback callback); more const-ref https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:53: void HeadlessPrintManager::Reset() { Can you put these in the same order as they are declared in the header? https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:108: const GetPDFCallback& callback) { Add DCHECK(callback); https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:24: kPrintSuccess, Chromium C++ style diverges slightly from Google style here due to historic reasons. So these should be MACRO_STYLE. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:45: // this function. You may want to say |callback| will always get called when printing finishes. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:69: bool is_active_; You may want to just remove this and check if |callback_| is_null() instead. You'll need to call callback_.Reset() in Reset() for that to work. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_content_renderer_client.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_content_renderer_client.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_print_web_view_helper_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_print_web_view_helper_delegate.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c)
https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", On 2017/03/29 05:30:15, Lei Zhang wrote: > On 2017/03/29 03:50:12, jzfeng wrote: > > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > > Is headless multi-process? Do you intend to separate browser from renderer? > If > > yes, the team should discuss where to do that separation. Right now this makes > > it look like everything goes in 1 big build target. > > > > As I remember, we discussed this issue a bit and agreed on putting them > > together. Add eseckler@ to explain more on this issue. > > So is headless multi-process? If the answer is no, then this makes a bit more > sense. If there's a discussion thread somewhere, I can take a look. Headless is multi-process, but so far, we are linking browser+renderer together in one target, since we didn't have (m)any renderer deps. (Our build setup follows that of content_shell, which also links renderer+browser together into a single lib for simplicity.) While it'd be nice to separate things into two targets as we're growing, I don't think we need to do this now. jzfeng, feel free to look into this though if you feel adventurous :) https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:31: CommandCallback callback) override; On 2017/03/29 03:50:13, jzfeng wrote: > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > Pass by const-ref. You need to fix > content/public/browser/devtools_manager_delegate.h (from r451947) as well. > > Done. So whenever I want to pass a callback, it should be const-ref, right? Yeah, usually :) https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_devtools_manager_delegate.h:63: using AsyncCommandMemberFnPtr = void (HeadlessDevToolsManagerDelegate::*)( On 2017/03/29 03:50:13, jzfeng wrote: > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > I asked on https://codereview.chromium.org/2119063002 why we have > CommandMemberFnPtr instead of using just base::Callback. Let's see how that > conversation proceeds and adjust this accordingly. > > Sounds good. Maybe Eric can also answer this question. Yeah, sounds like you came to a conclusion in the discussion on the other patch. Feel free to stick with these function pointers for now, but cleaning up later sounds good. https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:30: int dpi = printing::kDefaultPdfDpi; On 2017/03/29 03:50:13, jzfeng wrote: > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > No need for printing:: if you decide to keep this class in namespace printing. > (Or do you want namespace headless?) > > Done. > I feel it is ok to be under printing like PrintViewManager. FWIW, either namespace is fine with me :) https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:340: if (!result) { For consistency, could you add the same check to OnScreenshotCaptured? :) https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:348: void HeadlessShell::WriteFile(const std::string& switch_string, nit: file_path_switch https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:350: const std::string& data) { nit: base64_data https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:399: << " was unsuccessful: " << net::ErrorToString(write_result); ups, I think we should be using File::ErrorToString here: https://cs.chromium.org/chromium/src/base/files/file.cc?type=cs&l=97 https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.h:74: void OnPDFCreated(std::unique_ptr<page::PrintToPDFResult> result); On 2017/03/29 05:30:15, Lei Zhang wrote: > Can this and methods below be private? If you move those to private (which I don't object to), could you also move other non-necessarily public methods above to private as well? :) https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:10: #include "base/base64.h" nit: unnecessary? https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:71: std::string HeadlessPrintManager::PrintResultToErrMsg(PrintResult result) { nit: PrintResultToString https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:192: data_ = std::string(buffer.data(), buffer.size()); This seems to override whatever is in data_. What happens if we receive more than one page (DidPrintPage message)? I'd expect that we need to somehow stitch together the pages? (Lei:) Or are we guaranteed that that we only get one DidPrintPage callback with all the pages? If so, can we ReleaseJob if we get more than one and remove the expecting_first_page_ stuff? https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:203: callback_.Run(result, ""); nit: std::string() https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:32: kDuplicatedPrintingError, nit: kSimultaneousPrintActive? https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_content_renderer_client.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. also here - no (c) :)
Thanks for the review! I will 1) keep working to make it work on windows and mac. 2) change to use base::Callback when https://codereview.chromium.org/2784733003/ has been submitted. 3) see how hard it is to separate renderer and browser. PTAL. https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", On 2017/03/29 at 11:21:19, Eric Seckler wrote: > On 2017/03/29 05:30:15, Lei Zhang wrote: > > On 2017/03/29 03:50:12, jzfeng wrote: > > > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > > > Is headless multi-process? Do you intend to separate browser from renderer? > > If > > > yes, the team should discuss where to do that separation. Right now this makes > > > it look like everything goes in 1 big build target. > > > > > > As I remember, we discussed this issue a bit and agreed on putting them > > > together. Add eseckler@ to explain more on this issue. > > > > So is headless multi-process? If the answer is no, then this makes a bit more > > sense. If there's a discussion thread somewhere, I can take a look. > > Headless is multi-process, but so far, we are linking browser+renderer together in one target, since we didn't have (m)any renderer deps. (Our build setup follows that of content_shell, which also links renderer+browser together into a single lib for simplicity.) > > While it'd be nice to separate things into two targets as we're growing, I don't think we need to do this now. jzfeng, feel free to look into this though if you feel adventurous :) OK. I can look into it a bit. If it is easy to do, I can add it into this patch. Otherwise, I prefer doing in a separate patch. https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... File content/public/browser/devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... content/public/browser/devtools_manager_delegate.h:54: virtual bool HandleAsyncCommand(DevToolsAgentHost* agent_host, On 2017/03/29 at 05:30:15, Lei Zhang wrote: > Add comment to explain what this does. Done. https://codereview.chromium.org/2780433002/diff/20001/content/public/browser/... content/public/browser/devtools_manager_delegate.h:56: const CommandCallback& callback); On 2017/03/29 at 05:30:15, Lei Zhang wrote: > Blank line after. Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:340: if (!result) { On 2017/03/29 at 11:21:19, Eric Seckler wrote: > For consistency, could you add the same check to OnScreenshotCaptured? :) Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:348: void HeadlessShell::WriteFile(const std::string& switch_string, On 2017/03/29 at 11:21:19, Eric Seckler wrote: > nit: file_path_switch Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:350: const std::string& data) { On 2017/03/29 at 11:21:19, Eric Seckler wrote: > nit: base64_data Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:363: OnFileOpened("", file_name, base::File::FILE_ERROR_FAILED); On 2017/03/29 at 05:30:15, Lei Zhang wrote: > Pass in a std::string() instead of "". There's a few more in HeadlessPrintManager. See https://crrev.com/193040 for the reason. Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:378: base::Base64Decode(data, &decoded_data); On 2017/03/29 at 05:30:15, Lei Zhang wrote: > base::Base64Decode() can fail, BTW. Added error handling logic. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.cc:399: << " was unsuccessful: " << net::ErrorToString(write_result); On 2017/03/29 at 11:21:19, Eric Seckler wrote: > ups, I think we should be using File::ErrorToString here: > https://cs.chromium.org/chromium/src/base/files/file.cc?type=cs&l=97 Done. https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... File headless/app/headless_shell.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/app/headless_s... headless/app/headless_shell.h:74: void OnPDFCreated(std::unique_ptr<page::PrintToPDFResult> result); On 2017/03/29 at 11:21:19, Eric Seckler wrote: > On 2017/03/29 05:30:15, Lei Zhang wrote: > > Can this and methods below be private? > > If you move those to private (which I don't object to), could you also move other non-necessarily public methods above to private as well? :) Sure. Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:10: #include "base/base64.h" On 2017/03/29 at 11:21:19, Eric Seckler wrote: > nit: unnecessary? Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.h:53: CommandCallback callback); On 2017/03/29 at 05:30:15, Lei Zhang wrote: > more const-ref Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:53: void HeadlessPrintManager::Reset() { On 2017/03/29 at 05:30:16, Lei Zhang wrote: > Can you put these in the same order as they are declared in the header? Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:71: std::string HeadlessPrintManager::PrintResultToErrMsg(PrintResult result) { On 2017/03/29 at 11:21:20, Eric Seckler wrote: > nit: PrintResultToString Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:108: const GetPDFCallback& callback) { On 2017/03/29 at 05:30:15, Lei Zhang wrote: > Add DCHECK(callback); Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:192: data_ = std::string(buffer.data(), buffer.size()); On 2017/03/29 at 11:21:19, Eric Seckler wrote: > This seems to override whatever is in data_. What happens if we receive more than one page (DidPrintPage message)? I'd expect that we need to somehow stitch together the pages? > > (Lei:) Or are we guaranteed that that we only get one DidPrintPage callback with all the pages? If so, can we ReleaseJob if we get more than one and remove the expecting_first_page_ stuff? You are right. This is a temporary solution. Looks like linux and windows only have one valid metafile. And mac requires to stitch the pages, though I don't know how at the moment. I will do some experiment on windows and mac. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:203: callback_.Run(result, ""); On 2017/03/29 at 11:21:19, Eric Seckler wrote: > nit: std::string() Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:24: kPrintSuccess, On 2017/03/29 at 05:30:16, Lei Zhang wrote: > Chromium C++ style diverges slightly from Google style here due to historic reasons. So these should be MACRO_STYLE. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:32: kDuplicatedPrintingError, On 2017/03/29 at 11:21:20, Eric Seckler wrote: > nit: kSimultaneousPrintActive? Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:45: // this function. On 2017/03/29 at 05:30:16, Lei Zhang wrote: > You may want to say |callback| will always get called when printing finishes. Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:69: bool is_active_; On 2017/03/29 at 05:30:16, Lei Zhang wrote: > You may want to just remove this and check if |callback_| is_null() instead. You'll need to call callback_.Reset() in Reset() for that to work. Cool! Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_content_renderer_client.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 at 11:21:20, Eric Seckler wrote: > also here - no (c) :) Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_content_renderer_client.h (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_content_renderer_client.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 at 05:30:16, Lei Zhang wrote: > no (c) Done. https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... File headless/lib/renderer/headless_print_web_view_helper_delegate.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/renderer/h... headless/lib/renderer/headless_print_web_view_helper_delegate.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 at 05:30:16, Lei Zhang wrote: > No (c) Done.
https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2780433002/diff/1/headless/BUILD.gn#newcode299 headless/BUILD.gn:299: "//components/printing/browser", On 2017/03/30 at 03:04:56, jzfeng wrote: > On 2017/03/29 at 11:21:19, Eric Seckler wrote: > > On 2017/03/29 05:30:15, Lei Zhang wrote: > > > On 2017/03/29 03:50:12, jzfeng wrote: > > > > On 2017/03/29 at 01:38:03, Lei Zhang wrote: > > > > > Is headless multi-process? Do you intend to separate browser from renderer? > > > If > > > > yes, the team should discuss where to do that separation. Right now this makes > > > > it look like everything goes in 1 big build target. > > > > > > > > As I remember, we discussed this issue a bit and agreed on putting them > > > > together. Add eseckler@ to explain more on this issue. > > > > > > So is headless multi-process? If the answer is no, then this makes a bit more > > > sense. If there's a discussion thread somewhere, I can take a look. > > > > Headless is multi-process, but so far, we are linking browser+renderer together in one target, since we didn't have (m)any renderer deps. (Our build setup follows that of content_shell, which also links renderer+browser together into a single lib for simplicity.) > > > > While it'd be nice to separate things into two targets as we're growing, I don't think we need to do this now. jzfeng, feel free to look into this though if you feel adventurous :) > > OK. I can look into it a bit. If it is easy to do, I can add it into this patch. Otherwise, I prefer doing in a separate patch. Add David, who knows more about headless on mac and windows. Looks like on windows, we need to separate them to make it compile.
If we added a runtime option for the renderer on Mac to select between: a) Single page PDF per PrintHostMsg_DidPrintPage (current Mac behavior) b) All pages PDF in the first PrintHostMsg_DidPrintPage (current Linux behavior) Would that make HeadlessPrintManager::OnDidPrintPage() cleaner? Otherwise, if you only had (a) as is, how to would you stitch all the PDFs together?
On 2017/03/30 06:06:17, Lei Zhang wrote: > If we added a runtime option for the renderer on Mac to select between: > > a) Single page PDF per PrintHostMsg_DidPrintPage (current Mac behavior) > b) All pages PDF in the first PrintHostMsg_DidPrintPage (current Linux behavior) > > Would that make HeadlessPrintManager::OnDidPrintPage() cleaner? Otherwise, if > you only had (a) as is, how to would you stitch all the PDFs together? Yeah, I think not having to deal with stitching pages together would simplify our OnDidPrintPage. If that option works on all platforms, let's use it :)
On 2017/03/30 06:33:42, Eric Seckler wrote: > Yeah, I think not having to deal with stitching pages together would simplify > our OnDidPrintPage. If that option works on all platforms, let's use it :) That option doesn't exist yet. If someone write it, I'll review it.
On 2017/03/30 at 06:37:43, thestig wrote: > On 2017/03/30 06:33:42, Eric Seckler wrote: > > Yeah, I think not having to deal with stitching pages together would simplify > > our OnDidPrintPage. If that option works on all platforms, let's use it :) > > That option doesn't exist yet. If someone write it, I'll review it. The runtime option sounds pretty good! Do you have a general idea on how to add it? Maybe through the print_web_view_helper_delegate?
On 2017/03/30 06:43:37, jzfeng wrote: > On 2017/03/30 at 06:37:43, thestig wrote: > > On 2017/03/30 06:33:42, Eric Seckler wrote: > > > Yeah, I think not having to deal with stitching pages together would > simplify > > > our OnDidPrintPage. If that option works on all platforms, let's use it :) > > > > That option doesn't exist yet. If someone write it, I'll review it. Ah, I didn't see the "if" :) but yeah, this option sgtm. Alternatively, with the OOPIF work, we would already be stitching things together somewhere right? Would it make sense to do this page-stitching in the same place? I'd be fine with disabling print-to-pdf for Mac for now if the OOPIF work would make this easier in the near future, too.
Added the option to let mac return all pages PDF in the first PrintHostMsg_DidPrintPage. I tested it on mac. The pdf file is generated successfully. The only issue is that the page layout seems to be different from linux for some reason. PTAL.
lgtm % comment below. Btw, please also reformat the patch description to be <72 chars per line - I'm afraid the tooling doesn't do that for you yet :( https://codereview.chromium.org/2780433002/diff/80001/headless/app/headless_s... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2780433002/diff/80001/headless/app/headless_s... headless/app/headless_shell.cc:385: return; Should also close the file in this case. Maybe call into OnFileWritten: OnFileWritten(file_name, decoded_data.size(), base::File::FILE_ERROR_FAILED, 0);
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== add print to pdf for headless As discussed with Lei, we agreed on adding a new print manager to headless. This won't cause multiple IPC handlers, since the new print manager is only installed by headless. And the printing people is willing to maintain this new print manager since there are already several different managers in chrome. In the future, we may find good ways to let content embedders to install multiple print managers, but it requires a lot of work. BUG=603559 ========== to ========== add print to pdf for headless As discussed with Lei, we agreed on adding a new print manager to headless. This won't cause multiple IPC handlers, since the new print manager is only installed by headless. And the printing people is willing to maintain this new print manager since there are already several different managers in chrome. In the future, we may find good ways to let content embedders to install multiple print managers, but it requires a lot of work. BUG=603559 ==========
Hi Lei, Would you mind take a look at the new changes, which added the option for mac? It now work the same way as linux under the new option. I would leave the browser/renderer split into future patches. https://codereview.chromium.org/2780433002/diff/80001/headless/app/headless_s... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2780433002/diff/80001/headless/app/headless_s... headless/app/headless_shell.cc:385: return; On 2017/04/03 at 10:34:34, Eric Seckler wrote: > Should also close the file in this case. Maybe call into OnFileWritten: > > OnFileWritten(file_name, decoded_data.size(), base::File::FILE_ERROR_FAILED, 0); Done.
Hi Lei, Do you have time to take a look at this patch? Thanks!
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/03 09:47:38, jzfeng wrote: > I tested it on mac. The pdf file is generated successfully. The only issue is > that the page layout seems to be different from linux for some reason. Given the same print settings, does headless Mac render to PDF differently from normal Mac Chrome? How about headless Linux vs normal Linux? If there's no difference on Linux, but there is a difference on Mac only, then I suspect either the print settings or the single page mode needs more work.
On 2017/04/06 at 07:38:38, thestig wrote: > On 2017/04/03 09:47:38, jzfeng wrote: > > I tested it on mac. The pdf file is generated successfully. The only issue is > > that the page layout seems to be different from linux for some reason. > > Given the same print settings, does headless Mac render to PDF differently from normal Mac Chrome? > > How about headless Linux vs normal Linux? If there's no difference on Linux, but there is a difference on Mac only, then I suspect either the print settings or the single page mode needs more work. Hi Lei, I have fixed the issue on mac. The only change is to use kPointsPerInch instead of kDefaultPdfDpi to compute the page size. It now looks good!
The CQ bit was unchecked by jzfeng@chromium.org
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jzfeng@chromium.org
On 2017/04/06 07:43:51, jzfeng wrote: > Hi Lei, I have fixed the issue on mac. The only change is to use kPointsPerInch > instead of kDefaultPdfDpi to compute the page size. > > It now looks good! This reminds me - could you add a HeadlessWebContentsTest that exercises the pdf printing upstream (in a follow-up patch would be fine)? :)
On 2017/04/06 07:38:38, Lei Zhang wrote: > On 2017/04/03 09:47:38, jzfeng wrote: > > I tested it on mac. The pdf file is generated successfully. The only issue is > > that the page layout seems to be different from linux for some reason. > > Given the same print settings, does headless Mac render to PDF differently from > normal Mac Chrome? > > How about headless Linux vs normal Linux? If there's no difference on Linux, but > there is a difference on Mac only, then I suspect either the print settings or > the single page mode needs more work. Or did patch set 6 already fix this?
On 2017/04/06 at 07:52:56, thestig wrote: > On 2017/04/06 07:38:38, Lei Zhang wrote: > > On 2017/04/03 09:47:38, jzfeng wrote: > > > I tested it on mac. The pdf file is generated successfully. The only issue is > > > that the page layout seems to be different from linux for some reason. > > > > Given the same print settings, does headless Mac render to PDF differently from > > normal Mac Chrome? > > > > How about headless Linux vs normal Linux? If there's no difference on Linux, but > > there is a difference on Mac only, then I suspect either the print settings or > > the single page mode needs more work. > > Or did patch set 6 already fix this? Yes, it is fixed :) On 2017/04/06 at 07:52:35, eseckler wrote: > On 2017/04/06 07:43:51, jzfeng wrote: > > Hi Lei, I have fixed the issue on mac. The only change is to use kPointsPerInch > > instead of kDefaultPdfDpi to compute the page size. > > > > It now looks good! > > This reminds me - could you add a HeadlessWebContentsTest that exercises the pdf printing upstream (in a follow-up patch would be fine)? :) Sure. Will do that in a follow up patch.
https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:192: data_ = std::string(buffer.data(), buffer.size()); On 2017/03/29 11:21:19, Eric Seckler wrote: > This seems to override whatever is in data_. What happens if we receive more > than one page (DidPrintPage message)? I'd expect that we need to somehow stitch > together the pages? > > (Lei:) Or are we guaranteed that that we only get one DidPrintPage callback with > all the pages? If so, can we ReleaseJob if we get more than one and remove the > expecting_first_page_ stuff? Under normal operations, you should get 1 per page. Only the 1st page has the metadata in patch set 5 and onwards. If a renderer is compromised, anything can happen. This is why we do some validation and we can potentially hit the UNEXPECTED_VALID_MEMORY_HANDLE case. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.h:113: virtual bool UseSingleMetafile(); Explain what this does. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.h:292: std::vector<int> printed_pages); Pass by const ref. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:53: gfx::Rect content_area_in_dpi; In principle, due to fancy CSS, this can be different per page. e.g. https://www.smashingmagazine.com/2015/01/designing-for-print-with-css/#left-a... Though I haven't actually tried this and I don't know how well we actually support it. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:31: int dpi = kPointsPerInch; Copy over the comment from GetDpi() to explain why we do this here. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:74: return "Invalid memory handle"; The ones above this DoNotHaveSpaces. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:104: if (!callback_.is_null()) { I think you can write: if (callback_) {, just like the line above. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:146: const PrintHostMsg_DidPrintPage_Params& params) { Add a check to defend against spurious messages: if (!callback_) return; https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:171: auto metafile = base::MakeUnique<PdfMetafileSkia>(PDF_SKIA_DOCUMENT_TYPE); You can merge this block of code with the if (metafile_must_be_valid) block above. Unlike the normal printing code, you don't need |metafile| to have this scoped. Neither does |shared_buf| once you do this. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:197: void HeadlessPrintManager::ReleaseJob(PrintResult result) { Add a check to defend against spurious messages here too. Otherwise, if ReleaseJob() gets triggered twice, the second call will crash while trying to run the callback.
Thanks for the review! PTAL. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.h:113: virtual bool UseSingleMetafile(); On 2017/04/06 at 08:08:42, Lei Zhang wrote: > Explain what this does. Done. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.h:292: std::vector<int> printed_pages); On 2017/04/06 at 08:08:42, Lei Zhang wrote: > Pass by const ref. Done. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:53: gfx::Rect content_area_in_dpi; On 2017/04/06 at 08:08:42, Lei Zhang wrote: > In principle, due to fancy CSS, this can be different per page. e.g. https://www.smashingmagazine.com/2015/01/designing-for-print-with-css/#left-a... > > Though I haven't actually tried this and I don't know how well we actually support it. I feel it is ok to leave as is? For the normal case, the size of printed_pages is always one, so the code won't change anything. These two thing can still be different for each page. For the headless case, the page size and content area is not used by the print manager. So I guess it won't matter. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:31: int dpi = kPointsPerInch; On 2017/04/06 at 08:08:42, Lei Zhang wrote: > Copy over the comment from GetDpi() to explain why we do this here. Done. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:74: return "Invalid memory handle"; On 2017/04/06 at 08:08:42, Lei Zhang wrote: > The ones above this DoNotHaveSpaces. Done. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:104: if (!callback_.is_null()) { On 2017/04/06 at 08:08:42, Lei Zhang wrote: > I think you can write: if (callback_) {, just like the line above. Done. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:146: const PrintHostMsg_DidPrintPage_Params& params) { On 2017/04/06 at 08:08:42, Lei Zhang wrote: > Add a check to defend against spurious messages: > > if (!callback_) > return; Use DCHECK instead, since callback_ should not be null. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:171: auto metafile = base::MakeUnique<PdfMetafileSkia>(PDF_SKIA_DOCUMENT_TYPE); On 2017/04/06 at 08:08:42, Lei Zhang wrote: > You can merge this block of code with the if (metafile_must_be_valid) block above. Unlike the normal printing code, you don't need |metafile| to have this scoped. Neither does |shared_buf| once you do this. Good catch! Done. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:197: void HeadlessPrintManager::ReleaseJob(PrintResult result) { On 2017/04/06 at 08:08:42, Lei Zhang wrote: > Add a check to defend against spurious messages here too. Otherwise, if ReleaseJob() gets triggered twice, the second call will crash while trying to run the callback. Done.
https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:146: const PrintHostMsg_DidPrintPage_Params& params) { On 2017/04/06 09:01:51, jzfeng wrote: > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > Add a check to defend against spurious messages: > > > > if (!callback_) > > return; > > Use DCHECK instead, since callback_ should not be null. I believe Lei meant that we shouldn't trust the renderer to send this message only when we expect it. Thus, callback_ might not be set. In such a case, we don't want to crash the browser process - maybe add a DLOG(ERROR) instead. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:197: void HeadlessPrintManager::ReleaseJob(PrintResult result) { On 2017/04/06 09:01:51, jzfeng wrote: > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > Add a check to defend against spurious messages here too. Otherwise, if > ReleaseJob() gets triggered twice, the second call will crash while trying to > run the callback. > > Done. Same here, DCHECK doesn't protect us from spurious messages :)
Getting close, so let's also talk about the commit message. It should make sense to some extent on its own, when viewed through git lot for example. Pretend you are another developer who knows nothing / very little about this CL or me or the other CL where we tried to do multiple IPC handlers. Then see if the CL description makes sense. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:53: gfx::Rect content_area_in_dpi; On 2017/04/06 09:01:51, jzfeng wrote: > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > In principle, due to fancy CSS, this can be different per page. e.g. > https://www.smashingmagazine.com/2015/01/designing-for-print-with-css/#left-a... > > > > Though I haven't actually tried this and I don't know how well we actually > support it. > > I feel it is ok to leave as is? > > For the normal case, the size of printed_pages is always one, so the code won't > change anything. These two thing can still be different for each page. > > For the headless case, the page size and content area is not used by the print > manager. So I guess it won't matter. That assumes nobody else will try to use UseSingleMetafile(). Since we can't always predict the future, and this is a subtle difference that most readers won't notice by just glancing at the code, can we leave a comment here to describe the potential bug? https://codereview.chromium.org/2780433002/diff/140001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2780433002/diff/140001/components/printing/re... components/printing/renderer/print_web_view_helper.h:115: // windows. It's good to capitalize Windows to distinguish it from windows. Might as well for 'linux' for consistency. https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:105: DCHECK(callback); DCHECK and then handling the DCHECK failure is not preferred. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:150: DCHECK(callback_); Yes, actually handle this. Don't fully trust renderers.
https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:105: DCHECK(callback); On 2017/04/06 10:23:00, Lei Zhang wrote: > DCHECK and then handling the DCHECK failure is not preferred. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... This is actually fine, I think, because the DCHECK is for the argument (callback) and the test below is for the instance variable (callback_).
https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/140001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:105: DCHECK(callback); On 2017/04/06 10:28:55, Eric Seckler wrote: > This is actually fine, I think, because the DCHECK is for the argument > (callback) and the test below is for the instance variable (callback_). *blink* *blink*
+weili: Does this help or hinder OOPIF printing?
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/06 21:09:37, Lei Zhang wrote: > +weili: Does this help or hinder OOPIF printing? The OOPIF printing design still holds, this should not affect it. However, to support OOPIF printing for headless Chrome, there needs more work such as being able to start/connect to a compositor service from headless print manager. I guess those will be future work then.
Description was changed from ========== add print to pdf for headless As discussed with Lei, we agreed on adding a new print manager to headless. This won't cause multiple IPC handlers, since the new print manager is only installed by headless. And the printing people is willing to maintain this new print manager since there are already several different managers in chrome. In the future, we may find good ways to let content embedders to install multiple print managers, but it requires a lot of work. BUG=603559 ========== to ========== Let users to print a webpage into pdf through PrintToPDF devtools command under headless chrome. This patch includes: 1) adding a new print manager, i.e., HeadlessPrintManager to handle all the printing related IPCs for headless. 2) plumbing through the pdf data from HeadlessPrintManager to HeadlessDevToolsManagerDelegate to handle the PrintToPDF command. 3) adding a new option to PrintWebViewHelper on Mac to return all the printed pages in the first PrintHostMsg_DidPrintPage message as a single file. This makes it conform to Linux and Windows, which simplify the logic in HeadlessPrintManager. This patch guarantees that HeadlessPrintManager is the only IPC entry point. In the future, if there are requirements to support the same devtools command in regular chrome, we may consider a framework to ensure multiple print managers to work properly under a content embedder. BUG=603559 ==========
Description was changed from ========== Let users to print a webpage into pdf through PrintToPDF devtools command under headless chrome. This patch includes: 1) adding a new print manager, i.e., HeadlessPrintManager to handle all the printing related IPCs for headless. 2) plumbing through the pdf data from HeadlessPrintManager to HeadlessDevToolsManagerDelegate to handle the PrintToPDF command. 3) adding a new option to PrintWebViewHelper on Mac to return all the printed pages in the first PrintHostMsg_DidPrintPage message as a single file. This makes it conform to Linux and Windows, which simplify the logic in HeadlessPrintManager. This patch guarantees that HeadlessPrintManager is the only IPC entry point. In the future, if there are requirements to support the same devtools command in regular chrome, we may consider a framework to ensure multiple print managers to work properly under a content embedder. BUG=603559 ========== to ========== Let users to print a webpage into pdf through PrintToPDF devtools command under headless chrome. This patch includes: 1) adding a new print manager, i.e., HeadlessPrintManager to handle all the printing related IPCs for headless. 2) plumbing through the pdf data from HeadlessPrintManager to HeadlessDevToolsManagerDelegate to handle the PrintToPDF command. 3) adding a new option to PrintWebViewHelper on Mac to return all the printed pages in the first PrintHostMsg_DidPrintPage message as a single file. This makes it conform to Linux and Windows, which simplify the logic in HeadlessPrintManager. This patch guarantees that HeadlessPrintManager is the only IPC entry point. In the future, if there are requirements to support the same devtools command in regular chrome, we may consider a framework to ensure multiple print managers to work properly under a content embedder. BUG=603559 ==========
Thanks for the review! I have rewrite the commit message. It should be easier to read now :) To weili: Would it be easy to call the compositor service from headless? Headless chrome is a separate embedder, so it can only depend on content and components. PTAL. https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:53: gfx::Rect content_area_in_dpi; On 2017/04/06 at 10:23:00, Lei Zhang wrote: > On 2017/04/06 09:01:51, jzfeng wrote: > > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > > In principle, due to fancy CSS, this can be different per page. e.g. > > https://www.smashingmagazine.com/2015/01/designing-for-print-with-css/#left-a... > > > > > > Though I haven't actually tried this and I don't know how well we actually > > support it. > > > > I feel it is ok to leave as is? > > > > For the normal case, the size of printed_pages is always one, so the code won't > > change anything. These two thing can still be different for each page. > > > > For the headless case, the page size and content area is not used by the print > > manager. So I guess it won't matter. > > That assumes nobody else will try to use UseSingleMetafile(). Since we can't always predict the future, and this is a subtle difference that most readers won't notice by just glancing at the code, can we leave a comment here to describe the potential bug? Done. Added this potential bug to the comment of the UseSingleMetafile method. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:146: const PrintHostMsg_DidPrintPage_Params& params) { On 2017/04/06 at 09:14:25, Eric Seckler wrote: > On 2017/04/06 09:01:51, jzfeng wrote: > > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > > Add a check to defend against spurious messages: > > > > > > if (!callback_) > > > return; > > > > Use DCHECK instead, since callback_ should not be null. > > I believe Lei meant that we shouldn't trust the renderer to send this message only when we expect it. Thus, callback_ might not be set. In such a case, we don't want to crash the browser process - maybe add a DLOG(ERROR) instead. Thanks for the explanation! Done. https://codereview.chromium.org/2780433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:197: void HeadlessPrintManager::ReleaseJob(PrintResult result) { On 2017/04/06 at 09:14:25, Eric Seckler wrote: > On 2017/04/06 09:01:51, jzfeng wrote: > > On 2017/04/06 at 08:08:42, Lei Zhang wrote: > > > Add a check to defend against spurious messages here too. Otherwise, if > > ReleaseJob() gets triggered twice, the second call will crash while trying to > > run the callback. > > > > Done. > > Same here, DCHECK doesn't protect us from spurious messages :) Done. https://codereview.chromium.org/2780433002/diff/140001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2780433002/diff/140001/components/printing/re... components/printing/renderer/print_web_view_helper.h:115: // windows. On 2017/04/06 at 10:23:00, Lei Zhang wrote: > It's good to capitalize Windows to distinguish it from windows. Might as well for 'linux' for consistency. Done.
lgtm https://codereview.chromium.org/2780433002/diff/160001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2780433002/diff/160001/components/printing/re... components/printing/renderer/print_web_view_helper.h:115: // NOTE: This option only returns one set of page size and content area, NOTE: PrintHostMsg_DidPrintPage messages for all pages contain the same page and content area, which ...
jzfeng@chromium.org changed reviewers: + pfeldman@chromium.org
jzfeng@chromium.org changed reviewers: + dpranke@chromium.org
On 2017/04/07 at 04:09:43, thestig wrote: > lgtm > > https://codereview.chromium.org/2780433002/diff/160001/components/printing/re... > File components/printing/renderer/print_web_view_helper.h (right): > > https://codereview.chromium.org/2780433002/diff/160001/components/printing/re... > components/printing/renderer/print_web_view_helper.h:115: // NOTE: This option only returns one set of page size and content area, > NOTE: PrintHostMsg_DidPrintPage messages for all pages contain the same page and content area, which ... Thanks for the review! Hi Pavel, could you help to check content/public/browser/devtools_manager_delegate.{h,cc}, since you are the owner? There are only minor changes. Hi Dirk, could you help to check build/args/headless.gn, since you are the owner? Just a one line change. Thanks!
//build lgtm.
content lgtm
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2780433002/#ps180001 (title: "improve comments as suggested")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491608139471780,
"parent_rev": "56d5720cf9a277a61d859d6829bf1a5bfb7d845c", "commit_rev":
"0cbec8b353f24515cc77a67ee95addc4f7a714ab"}
Message was sent while issue was closed.
Description was changed from ========== Let users to print a webpage into pdf through PrintToPDF devtools command under headless chrome. This patch includes: 1) adding a new print manager, i.e., HeadlessPrintManager to handle all the printing related IPCs for headless. 2) plumbing through the pdf data from HeadlessPrintManager to HeadlessDevToolsManagerDelegate to handle the PrintToPDF command. 3) adding a new option to PrintWebViewHelper on Mac to return all the printed pages in the first PrintHostMsg_DidPrintPage message as a single file. This makes it conform to Linux and Windows, which simplify the logic in HeadlessPrintManager. This patch guarantees that HeadlessPrintManager is the only IPC entry point. In the future, if there are requirements to support the same devtools command in regular chrome, we may consider a framework to ensure multiple print managers to work properly under a content embedder. BUG=603559 ========== to ========== Let users to print a webpage into pdf through PrintToPDF devtools command under headless chrome. This patch includes: 1) adding a new print manager, i.e., HeadlessPrintManager to handle all the printing related IPCs for headless. 2) plumbing through the pdf data from HeadlessPrintManager to HeadlessDevToolsManagerDelegate to handle the PrintToPDF command. 3) adding a new option to PrintWebViewHelper on Mac to return all the printed pages in the first PrintHostMsg_DidPrintPage message as a single file. This makes it conform to Linux and Windows, which simplify the logic in HeadlessPrintManager. This patch guarantees that HeadlessPrintManager is the only IPC entry point. In the future, if there are requirements to support the same devtools command in regular chrome, we may consider a framework to ensure multiple print managers to work properly under a content embedder. BUG=603559 Review-Url: https://codereview.chromium.org/2780433002 Cr-Commit-Position: refs/heads/master@{#463105} Committed: https://chromium.googlesource.com/chromium/src/+/0cbec8b353f24515cc77a67ee95a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/0cbec8b353f24515cc77a67ee95a...
Message was sent while issue was closed.
halcanary@google.com changed reviewers: + halcanary@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2780433002/diff/180001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/180001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:76: page_params.metafile_data_handle = base::SharedMemoryHandle(); What does line 76 do here? Why do pages after the first one send a different value of page_params.metafile_data_handle ?
Message was sent while issue was closed.
Also, why is there a patch set 11 when commit-bot landed patch set 10? https://codereview.chromium.org/2780433002/diff/180001/components/printing/re... File components/printing/renderer/print_web_view_helper_mac.mm (right): https://codereview.chromium.org/2780433002/diff/180001/components/printing/re... components/printing/renderer/print_web_view_helper_mac.mm:76: page_params.metafile_data_handle = base::SharedMemoryHandle(); On 2017/04/24 20:07:54, hal.canary wrote: > What does line 76 do here? > > Why do pages after the first one send a different value of > page_params.metafile_data_handle ? Only the first page contains real data. The rest are just dummy messages to satisfy the browser's expectations. See UseSingleMetafile() and related modifications.
Message was sent while issue was closed.
Patchset #11 (id:200001) has been deleted
Message was sent while issue was closed.
On 2017/04/24 at 21:30:05, thestig wrote: > Also, why is there a patch set 11 when commit-bot landed patch set 10? I kept using the same git branch and did a git cl upload, without noticing it would upload a new patch to this CL. Deleted patch 11. |
