Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829973002:
add customized printing setting for headless (Closed)
|
Created:
3 years, 8 months ago by jzfeng Modified:
3 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, mac-reviews_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd customized printing setting for headless
1) Add parameters to printToPDF command, which let the user to specify printing
settings like paper size, margin size, etc.
2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed
print_preview_context_.total_page_count() to PrintHeaderAndFooter. However,
HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves
print_preview_context_ uninitialized. To solve the problem, add page_count as
an arg to these two methods.
3) Added unit test and browser test for print to pdf.
BUG=603559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2829973002
Cr-Commit-Position: refs/heads/master@{#470159}
Committed: https://chromium.googlesource.com/chromium/src/+/3d83dad2db679c3924ffe74599cfd48a03211778
Patch Set 1 #Patch Set 2 : nit change #
Total comments: 72
Patch Set 3 : adjust command interface and add unit tests #
Total comments: 6
Patch Set 4 : simplify parameters for paper size #Patch Set 5 : make scale and header+footer available for basic print #Patch Set 6 : add locale pak for browsertests #Patch Set 7 : fix typo #Patch Set 8 : add browser test for print to pdf #Patch Set 9 : remove unused includes #Patch Set 10 : use GetRawDataResource to avoid using locale pak #
Total comments: 4
Patch Set 11 : nit change #
Total comments: 4
Patch Set 12 : nit #
Total comments: 44
Patch Set 13 : polish code #
Total comments: 4
Patch Set 14 : nit #Patch Set 15 : use dep skia #Patch Set 16 : move content/public/common to public dep #Patch Set 17 : add skia as public_deps instead #
Total comments: 5
Messages
Total messages: 99 (49 generated)
Description was changed from ========== add customized printing setting for headless BUG=603559 ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 ==========
Description was changed from ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 ==========
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.
jzfeng@chromium.org changed reviewers: + eseckler@chromium.org, thestig@chromium.org
This patch adds the customized printing setting feature. PTAL. Thanks!
https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.h:80: void PrintToPDF(Maybe<int> dpi, Not sure how well a DPI setting will be supported. Have you tried it out? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:78: std::unique_ptr<base::DictionaryValue> ParsePrintSettings( Now that you have written a parser, you probably want unit tests. Parsers love testing. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:87: if (settings->scale < 0) In print preview, the valid range is [10, 200]. However, the constants are in chrome/browser/resources/print_preview/data/ticket_items/scaling.js so it's hard to use that here. You can add your own constants, and then update both files to cross-ref each other to say when one changes, the other must change as well. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:92: if (paper_type == "letter") Maybe use the same names as https://developers.google.com/cloud-print/docs/cdd#cdd ? e.g. NA_LETTER, ISO_A4. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:112: if (!params->GetDouble("marginTop", &settings->margin_top_in_inch)) Can you try setting this to 20 inches and see what happens? Or maybe top and bottom cross each other? I'm not sure if you need to do more value validation here, or if somewhere else already does it. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:123: } else { Did you leave out PRINTABLE_AREA_MARGINS on purpose? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:138: base::StringToInt(tokens[0], &range.from); StringToInt() comes with a 16 line comment about its return value. Do you care about "non-perfect" conversions? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:278: << "Print preview is not enabled. Some print settings may not work."; Which settings don't work? Is it just headers+footers? I'll give you PrintWebViewHelper feedback based on this. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:284: auto response = ParsePrintSettings(command_id, params, &settings); You may want to write out the type instead of using auto. Unlike: auto it = stl_map.find(); or auto foo = base::MakeUnique<Foo>(); ParsePrintSettings() is not well known to most readers and they have no idea what is being returned. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:148: print_settings.set_url( Do you need to set the page title as well? Sanitize the URL? (See PrintPreviewHandler::HandleGetPreview()) https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:194: print_params_->pages.back() > params.expected_pages_count) { Is the vector of pages sorted? If the range input is "1-3,9-10,4-6" does back() return 10 or 6? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:216: number_pages_ = print_params_->pages.size(); Why do we need to do this? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:267: print_params_.reset(nullptr); There's no need to pass nullptr to std::unique_ptr::reset(). https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:22: struct HeadlessPrintSettings { https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:50: double scale; Mention 1 = 100%? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:85: HeadlessPrintSettings settings, Don't pass by value. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_c... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_c... headless/lib/headless_content_main_delegate.cc:252: dir_module.AppendASCII(FILE_PATH_LITERAL("headless_locales")); AppendASCII and FILE_PATH_LITERAL should never be used together. The former always wants to take a regular string, and the latter is trying to convert the regular string into a wide string on Windows. I think the fact that the trybots are green means they are probably not even building this file. Which then brings up the question of why does this even have checks for defined(OS_WIN).
https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:284: auto response = ParsePrintSettings(command_id, params, &settings); On 2017/04/20 08:35:58, Lei Zhang wrote: > You may want to write out the type instead of using auto. Unlike: > > auto it = stl_map.find(); > > or > > auto foo = base::MakeUnique<Foo>(); > > ParsePrintSettings() is not well known to most readers and they have no idea > what is being returned. See https://google.github.io/styleguide/cppguide.html#auto
https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:129: if (params->GetString("pageRanges", &page_ranges)) { If you want to get fancy, see pageRangeTextToPageRanges() in chrome/browser/resources/print_preview/print_preview_utils.js.
https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:28: repack_locales("locale_pak") { I'm not very familiar with locales. Can you explain why we need this extra pak and can't just use the ui/strings paks we include in repack("pak") below? There, we also include ui/strings/app_locale_settings_en-US.pak and ui/strings/ui_strings_en-US.pak. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:82: params->GetInteger("dpi", &settings->dpi); nit: add a comment that we can safely ignore return values of these calls (even though fields are optional) b/c defaults are already set in |settings|. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:37: int HeadlessPrintSettings::GetDPI() { nit: this doesn't always return the DPI, so should probably have a different method name? https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:47: gfx::Size HeadlessPrintSettings::PaperSizeInPixel() { nit: PaperSizeInPixelsOrPoints https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:104: return "Page range exceed page count"; nit: exceeds https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:69: EXCEED_PAGE_LIMIT, nit: PAGE_COUNT_EXCEEDED? https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:434: {"name": "dpi", "type": "integer", "optional": true, "description": "DPI of the printed file."}, what's the default value? also, this doesn't seem to have any effect on MacOS (should it?), maybe that justifies a comment here :) https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:437: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background graphics. Defaults to false."}, nit: colors/graphics https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, Are these all the paper sizes we can support? Maybe it'd be more useful to pass the actual width/height (millimeters/inches) instead. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:440: {"name": "marginType", "type": "string", "optional": true, "enum": ["defaultMargin", "noMargin", "customMargin"], "description": "Margin type. Set to 'customMargin' to customize margin."}, I'm not sure we need this field. I'd propose we get rid of it and: - by default, marginTop/Bottom/Left/Right is set to "defaultMargin" values. - if the user doesn't want any margin, they can set marginTop/Bottom/Left/Right to 0. - if the user wants a custom margin, they can set marginTop/Bottom/Left/Right to a custom value.
+weili FYI since there's potential PrintWebViewHelper changes here. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, On 2017/04/20 09:15:28, Eric Seckler wrote: > Are these all the paper sizes we can support? Maybe it'd be more useful to pass > the actual width/height (millimeters/inches) instead. That's a product decision. If you go with only mm or inches, then some part of the world is going to be slightly unhappy. e.g. A4 is some odd number of inches, and ditto for Letter in mm.
eseckler@chromium.org changed reviewers: + pfeldman@chromium.org
On 2017/04/20 19:08:59, Lei Zhang wrote: > https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": > "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", > "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, > On 2017/04/20 09:15:28, Eric Seckler wrote: > > Are these all the paper sizes we can support? Maybe it'd be more useful to > pass > > the actual width/height (millimeters/inches) instead. > > That's a product decision. If you go with only mm or inches, then some part of > the world is going to be slightly unhappy. e.g. A4 is some odd number of inches, > and ditto for Letter in mm. True. +pfeldman since this is ultimately a devtools API question.
> True. +pfeldman since this is ultimately a devtools API question. Use the same terms you are going to use when passing this information to the printing subsystem. I would suspect everything to boil down to dpi, but I am not sure.
mb@infogr.am changed reviewers: + mb@infogr.am
https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, > That's a product decision. If you go with only mm or inches, then some part of > the world is going to be slightly unhappy. e.g. A4 is some odd number of inches, > and ditto for Letter in mm. Thank you very much for your efforts, I have no doubt that PDF printing customization is an essential and very welcome feature. There may be multiple use cases when a custom size is required. We at infogr.am (https://infogr.am) would be happy to migrate our document export to PDF from PhantomJS to headless chromium, however not being able to specify custom size for the output PDF (in inches, millimeters, microns or anything else) still would be a deal breaker for us.
Description was changed from ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for the review! PTAL. https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.h:80: void PrintToPDF(Maybe<int> dpi, On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Not sure how well a DPI setting will be supported. Have you tried it out? Sadly setting different dpi's yield the same pdf file. The reason is, the page size is computed from inch to dpi in headless print manager, and then in print_web_view_helper.cc, dpi is only used to convert the page size back to inch. Hence, I removed it and always use kPointsPerInch as dpi on all platforms. Please let me know if there are better options. https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:28: repack_locales("locale_pak") { On 2017/04/20 at 09:15:28, Eric Seckler wrote: > I'm not very familiar with locales. Can you explain why we need this extra pak and can't just use the ui/strings paks we include in repack("pak") below? There, we also include ui/strings/app_locale_settings_en-US.pak and ui/strings/ui_strings_en-US.pak. The locale pak is loaded by ResourceBundle::LoadLocaleResources in resource_bundle.cc. On Linux, the fallback app_locale is "en-US", and it tries to find the en-US.pak file. On Mac, it tries to find locale.pak file. Chrome generates this file, while headless shell doesn't. That's why we see "locale_file_path.empty() for locale" warning when running headless shell. If we want to reuse repack("pak") below, we need to change its output from headless_lib.pak to en-US.pak or locale.pak depend on the platform. This option doesn't looks so good to me. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:78: std::unique_ptr<base::DictionaryValue> ParsePrintSettings( On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Now that you have written a parser, you probably want unit tests. Parsers love testing. Done. Added headless_printing_unittest.cc to do all the tests. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:82: params->GetInteger("dpi", &settings->dpi); On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: add a comment that we can safely ignore return values of these calls (even though fields are optional) b/c defaults are already set in |settings|. Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:87: if (settings->scale < 0) On 2017/04/20 at 08:35:58, Lei Zhang wrote: > In print preview, the valid range is [10, 200]. However, the constants are in chrome/browser/resources/print_preview/data/ticket_items/scaling.js so it's hard to use that here. You can add your own constants, and then update both files to cross-ref each other to say when one changes, the other must change as well. Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:92: if (paper_type == "letter") On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Maybe use the same names as https://developers.google.com/cloud-print/docs/cdd#cdd ? e.g. NA_LETTER, ISO_A4. Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:112: if (!params->GetDouble("marginTop", &settings->margin_top_in_inch)) On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Can you try setting this to 20 inches and see what happens? Or maybe top and bottom cross each other? I'm not sure if you need to do more value validation here, or if somewhere else already does it. I tested it, this type of error has been handled by SetPrinterPrintableArea and PrintMsg_Print_Params_IsValid. In SetPrinterPrintableArea, it sets the printable_area to be empty if the margin is invalid. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:123: } else { On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Did you leave out PRINTABLE_AREA_MARGINS on purpose? Yeah. Now I further restrict the type to be DEFAULT_MARGINS and CUSTOM_MARGINS. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:129: if (params->GetString("pageRanges", &page_ranges)) { On 2017/04/20 at 09:15:05, Lei Zhang wrote: > If you want to get fancy, see pageRangeTextToPageRanges() in chrome/browser/resources/print_preview/print_preview_utils.js. Re-implement the logic in headless_print_manager.cc. It does the same thing as the one in the js file now. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:138: base::StringToInt(tokens[0], &range.from); On 2017/04/20 at 08:35:58, Lei Zhang wrote: > StringToInt() comes with a 16 line comment about its return value. Do you care about "non-perfect" conversions? Added unit test for it. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:278: << "Print preview is not enabled. Some print settings may not work."; On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Which settings don't work? Is it just headers+footers? I'll give you PrintWebViewHelper feedback based on this. headers+footers and scale don't work. In PrintWebViewHelper::PrintPageInternal, under #if BUILDFLAG(ENABLE_PRINT_PREVIEW), there is: 1) scale: css_scale_factor = params.scale_factor; 2) headers+footers: PrintHeaderAndFooter. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:284: auto response = ParsePrintSettings(command_id, params, &settings); On 2017/04/20 at 08:53:12, Lei Zhang wrote: > On 2017/04/20 08:35:58, Lei Zhang wrote: > > You may want to write out the type instead of using auto. Unlike: > > > > auto it = stl_map.find(); > > > > or > > > > auto foo = base::MakeUnique<Foo>(); > > > > ParsePrintSettings() is not well known to most readers and they have no idea > > what is being returned. > > See https://google.github.io/styleguide/cppguide.html#auto Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:37: int HeadlessPrintSettings::GetDPI() { On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: this doesn't always return the DPI, so should probably have a different method name? Removed this method since setting different dpi doesn't really work. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:47: gfx::Size HeadlessPrintSettings::PaperSizeInPixel() { On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: PaperSizeInPixelsOrPoints Change to PaperSizeInPoints. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:104: return "Page range exceed page count"; On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: exceeds Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:148: print_settings.set_url( On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Do you need to set the page title as well? Sanitize the URL? (See PrintPreviewHandler::HandleGetPreview()) Setting page title doesn't work in most case, since in print_web_view_helper.cc, there is options->SetString("title", title.empty() ? params.title : title); where title is usually not empty (actually I don't know when would it be empty), so params.title is not used. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:194: print_params_->pages.back() > params.expected_pages_count) { On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Is the vector of pages sorted? If the range input is "1-3,9-10,4-6" does back() return 10 or 6? Yes, it is sorted. PageRange::GetPages use a set to store the pages, and convert it into vector. The default order is ascending. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:216: number_pages_ = print_params_->pages.size(); On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Why do we need to do this? number_pages returned from the renderer is always the page_count of the file, not the actual number of printed pages. We need the actual number to check whether the job has finished in OnDidPrintPage. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:267: print_params_.reset(nullptr); On 2017/04/20 at 08:35:58, Lei Zhang wrote: > There's no need to pass nullptr to std::unique_ptr::reset(). Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:22: struct HeadlessPrintSettings { On 2017/04/20 at 08:35:58, Lei Zhang wrote: > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes Done. Changed struct to class. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:50: double scale; On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Mention 1 = 100%? Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:69: EXCEED_PAGE_LIMIT, On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: PAGE_COUNT_EXCEEDED? Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.h:85: HeadlessPrintSettings settings, On 2017/04/20 at 08:35:58, Lei Zhang wrote: > Don't pass by value. Done. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_c... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_c... headless/lib/headless_content_main_delegate.cc:252: dir_module.AppendASCII(FILE_PATH_LITERAL("headless_locales")); On 2017/04/20 at 08:35:58, Lei Zhang wrote: > AppendASCII and FILE_PATH_LITERAL should never be used together. The former always wants to take a regular string, and the latter is trying to convert the regular string into a wide string on Windows. > > I think the fact that the trybots are green means they are probably not even building this file. Which then brings up the question of why does this even have checks for defined(OS_WIN). Done. Headless chrome is about to be enabled on Windows . It should check for this file afterwards. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:434: {"name": "dpi", "type": "integer", "optional": true, "description": "DPI of the printed file."}, On 2017/04/20 at 09:15:28, Eric Seckler wrote: > what's the default value? also, this doesn't seem to have any effect on MacOS (should it?), maybe that justifies a comment here :) Removed this parameter, because it is not well supported yet. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:437: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background graphics. Defaults to false."}, On 2017/04/20 at 09:15:28, Eric Seckler wrote: > nit: colors/graphics Done. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, On 2017/04/26 at 08:10:00, martinsb wrote: > > That's a product decision. If you go with only mm or inches, then some part of > > the world is going to be slightly unhappy. e.g. A4 is some odd number of inches, > > and ditto for Letter in mm. > Thank you very much for your efforts, I have no doubt that PDF printing customization is an essential and very welcome feature. > There may be multiple use cases when a custom size is required. We at infogr.am (https://infogr.am) would be happy to migrate our document export to PDF from PhantomJS to headless chromium, however not being able to specify custom size for the output PDF (in inches, millimeters, microns or anything else) still would be a deal breaker for us. Add a new "CUSTOM" page type to let the user specify page width and height. Also add a "unit" parameter to the command to let the user specify using mm or inch. Hence, user can pick popular page types, such as letter. They can also specify an arbitrary page size as they wish. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:440: {"name": "marginType", "type": "string", "optional": true, "enum": ["defaultMargin", "noMargin", "customMargin"], "description": "Margin type. Set to 'customMargin' to customize margin."}, On 2017/04/20 at 09:15:28, Eric Seckler wrote: > I'm not sure we need this field. I'd propose we get rid of it and: > - by default, marginTop/Bottom/Left/Right is set to "defaultMargin" values. > - if the user doesn't want any margin, they can set marginTop/Bottom/Left/Right to 0. > - if the user wants a custom margin, they can set marginTop/Bottom/Left/Right to a custom value. Sounds good. Done.
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.
eseckler@chromium.org changed reviewers: + altimin@chromium.org, skyostil@chromium.org
Cool, thank you! Since I don't think we have any yet, could we also add browser tests that verify basic functionality of the DevTools command and (if feasible) pdf size / page numbers and alike? +altimin and +skyostil for comment in headless/BUILD.gn https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.h:80: void PrintToPDF(Maybe<int> dpi, On 2017/04/27 06:56:05, jzfeng wrote: > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > Not sure how well a DPI setting will be supported. Have you tried it out? > > Sadly setting different dpi's yield the same pdf file. > The reason is, the page size is computed from inch to dpi in headless print > manager, and then in print_web_view_helper.cc, dpi is only used to convert the > page size back to inch. > > Hence, I removed it and always use kPointsPerInch as dpi on all platforms. > Please let me know if there are better options. Lei: Would there be a way to specify a DPI setting that affects the resolution of images in the PDF etc? Or is that not supported by the printing stack? https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:28: repack_locales("locale_pak") { On 2017/04/27 06:56:05, jzfeng wrote: > On 2017/04/20 at 09:15:28, Eric Seckler wrote: > > I'm not very familiar with locales. Can you explain why we need this extra pak > and can't just use the ui/strings paks we include in repack("pak") below? There, > we also include ui/strings/app_locale_settings_en-US.pak and > ui/strings/ui_strings_en-US.pak. > > The locale pak is loaded by ResourceBundle::LoadLocaleResources in > resource_bundle.cc. > On Linux, the fallback app_locale is "en-US", and it tries to find the en-US.pak > file. > On Mac, it tries to find locale.pak file. > Chrome generates this file, while headless shell doesn't. That's why we see > "locale_file_path.empty() for locale" > warning when running headless shell. > > If we want to reuse repack("pak") below, we need to change its output from > headless_lib.pak to en-US.pak or locale.pak depend on the platform. This option > doesn't looks so good to me. I have two concerns about this: a) Would this have a significant binary size impact? b) Would this create problems for building with embedded resources (headless_use_embedded_resources)? Can we do the same for these locale paks as we do for headless_lib.pak, or is that difficult because of the way the ui/ code loads these locale paks? +skyostil for a) +altimin for b) https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, On 2017/04/27 06:56:07, jzfeng wrote: > On 2017/04/26 at 08:10:00, martinsb wrote: > > > That's a product decision. If you go with only mm or inches, then some part > of > > > the world is going to be slightly unhappy. e.g. A4 is some odd number of > inches, > > > and ditto for Letter in mm. > > Thank you very much for your efforts, I have no doubt that PDF printing > customization is an essential and very welcome feature. > > There may be multiple use cases when a custom size is required. We at > infogr.am (https://infogr.am) would be happy to migrate our document export to > PDF from PhantomJS to headless chromium, however not being able to specify > custom size for the output PDF (in inches, millimeters, microns or anything > else) still would be a deal breaker for us. > > Add a new "CUSTOM" page type to let the user specify page width and height. Also > add a "unit" parameter to the command to let the user specify using mm or inch. > Hence, user can pick popular page types, such as letter. They can also specify > an arbitrary page size as they wish. For my taste, this is more complicated than it needs to be. I'd be happier if we fixed this to one unit (inches, for argument's sake, since that's what the printing system seems to use). Users can then look up and specify page sizes in inches themselves. I don't think it's necessary for us to supply "common" page sizes ourselves - that just opens us up to people asking for more explicitly listed page sizes in the protocol. WDYT?
https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:28: repack_locales("locale_pak") { On 2017/04/27 08:57:00, Eric Seckler wrote: > On 2017/04/27 06:56:05, jzfeng wrote: > > On 2017/04/20 at 09:15:28, Eric Seckler wrote: > > > I'm not very familiar with locales. Can you explain why we need this extra > pak > > and can't just use the ui/strings paks we include in repack("pak") below? > There, > > we also include ui/strings/app_locale_settings_en-US.pak and > > ui/strings/ui_strings_en-US.pak. > > > > The locale pak is loaded by ResourceBundle::LoadLocaleResources in > > resource_bundle.cc. > > On Linux, the fallback app_locale is "en-US", and it tries to find the > en-US.pak > > file. > > On Mac, it tries to find locale.pak file. > > Chrome generates this file, while headless shell doesn't. That's why we see > > "locale_file_path.empty() for locale" > > warning when running headless shell. > > > > If we want to reuse repack("pak") below, we need to change its output from > > headless_lib.pak to en-US.pak or locale.pak depend on the platform. This > option > > doesn't looks so good to me. > > I have two concerns about this: > a) Would this have a significant binary size impact? > b) Would this create problems for building with embedded resources > (headless_use_embedded_resources)? Can we do the same for these locale paks as > we do for headless_lib.pak, or is that difficult because of the way the ui/ code > loads these locale paks? > > +skyostil for a) > +altimin for b) Yes, current approach does not support embedded resources. We should add the option to provide locale resources as embedded content. Something along these lines should work: - InitSharedInstanceWithLocale should be able to skip loading resources (a new setting in ResourceBundle::LoadResources?) - Add an option to load locale resources directly from a DataPack (we already can create a data pack from a buffer). ResourceBundle::ReloadLocaleResoucesFromDataPack? I'd recommend doing that in a separate CL and looping in sadrul@ and sky@ as ui/base owners. If you have any questions, I'd be happy to answer.
We only really care about binary size in the case when we're embedded into Chrome, and as far as I can tell in that case the resources are already there and the resource compiles should deal with the duplicates. https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:28: : paper_type(NA_LETTER), Looks like not all fields are being initialized (you could initialize them in the header too if you want).
https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.h:80: void PrintToPDF(Maybe<int> dpi, On 2017/04/27 08:56:59, Eric Seckler wrote: > On 2017/04/27 06:56:05, jzfeng wrote: > > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > > Not sure how well a DPI setting will be supported. Have you tried it out? > > > > Sadly setting different dpi's yield the same pdf file. > > The reason is, the page size is computed from inch to dpi in headless print > > manager, and then in print_web_view_helper.cc, dpi is only used to convert the > > page size back to inch. > > > > Hence, I removed it and always use kPointsPerInch as dpi on all platforms. > > Please let me know if there are better options. > > Lei: Would there be a way to specify a DPI setting that affects the resolution > of images in the PDF etc? Or is that not supported by the printing stack? Since we don't expose such a setting, I'd say it's certainly not well supported. If you really want to know, we can start at the bottom layer and ask the Skia folks if SkPDF supports a DPI setting. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:278: << "Print preview is not enabled. Some print settings may not work."; On 2017/04/27 06:56:06, jzfeng wrote: > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > Which settings don't work? Is it just headers+footers? I'll give you > PrintWebViewHelper feedback based on this. > > headers+footers and scale don't work. > > In PrintWebViewHelper::PrintPageInternal, under #if > BUILDFLAG(ENABLE_PRINT_PREVIEW), there is: > 1) scale: css_scale_factor = params.scale_factor; > 2) headers+footers: PrintHeaderAndFooter. I would suggest experimenting with making these features work for basic printing. Can you see if that's do able? It can probably be a separate CL. You shouldn't flip on the print preview build flag when you don't have the print preview feature. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:216: number_pages_ = print_params_->pages.size(); On 2017/04/27 06:56:06, jzfeng wrote: > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > Why do we need to do this? > > number_pages returned from the renderer is always the page_count of the file, > not the actual number of printed pages. We need the actual number to check > whether the job has finished in OnDidPrintPage. Is this the issue described in https://crbug.com/161576 ? I wonder how PrintManager deals with it. https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn#newco... headless/BUILD.gn:229: defines = [ "PRINTING_IMPLEMENTATION" ] This doesn't look right. This is not printing/. https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn#newco... headless/BUILD.gn:438: test("headless_printing_unittests") { I think you just want to add your test to headless_unittests. You shouldn't have to create a new binary, and configure buildbots to run the new binary, etc.
https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.h:80: void PrintToPDF(Maybe<int> dpi, On 2017/04/27 22:17:46, Lei Zhang wrote: > On 2017/04/27 08:56:59, Eric Seckler wrote: > > On 2017/04/27 06:56:05, jzfeng wrote: > > > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > > > Not sure how well a DPI setting will be supported. Have you tried it out? > > > > > > Sadly setting different dpi's yield the same pdf file. > > > The reason is, the page size is computed from inch to dpi in headless print > > > manager, and then in print_web_view_helper.cc, dpi is only used to convert > the > > > page size back to inch. > > > > > > Hence, I removed it and always use kPointsPerInch as dpi on all platforms. > > > Please let me know if there are better options. > > > > Lei: Would there be a way to specify a DPI setting that affects the resolution > > of images in the PDF etc? Or is that not supported by the printing stack? > > Since we don't expose such a setting, I'd say it's certainly not well supported. > If you really want to know, we can start at the bottom layer and ask the Skia > folks if SkPDF supports a DPI setting. Let's go without it for now then. I believe someone asked on the headless bug for an option to set DPI, so we might want to query Skia folks about this at some point. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:278: << "Print preview is not enabled. Some print settings may not work."; On 2017/04/27 22:17:46, Lei Zhang wrote: > On 2017/04/27 06:56:06, jzfeng wrote: > > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > > Which settings don't work? Is it just headers+footers? I'll give you > > PrintWebViewHelper feedback based on this. > > > > headers+footers and scale don't work. > > > > In PrintWebViewHelper::PrintPageInternal, under #if > > BUILDFLAG(ENABLE_PRINT_PREVIEW), there is: > > 1) scale: css_scale_factor = params.scale_factor; > > 2) headers+footers: PrintHeaderAndFooter. > > I would suggest experimenting with making these features work for basic > printing. Can you see if that's do able? It can probably be a separate CL. You > shouldn't flip on the print preview build flag when you don't have the print > preview feature. If this works, would we be able to avoid adding the locale paks, too? :)
> Let's go without it for now then. I believe someone asked on the headless bug for an option to set DPI, so we might want to query Skia folks about this at some point. I have not yet commented on the PDF print issues/CL, but I'm watching them closely. I hope to replace my current stack with chrome --headless PDF printing. Setting the DPI on the entire print job would be great, but more important (for my use case) is to ensure the DPI is independent of any operating system. So if it is hard coded for all print jobs to 72 or 96 dpi on all OSes, that's fine (for me). It just shouldn't change based on the underlying system or system settings.
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 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.
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.
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 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...
Description was changed from ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like dpi, paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Add locale pak for headless, since PrintWebViewHelper requires it for print preview. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Added unit test and browser test for print to pdf. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jzfeng@chromium.org changed reviewers: - mb@infogr.am
Thanks for the review! Change the code as commented and added browser test. PTAL. https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:28: repack_locales("locale_pak") { On 2017/04/27 at 15:16:34, altimin wrote: > On 2017/04/27 08:57:00, Eric Seckler wrote: > > On 2017/04/27 06:56:05, jzfeng wrote: > > > On 2017/04/20 at 09:15:28, Eric Seckler wrote: > > > > I'm not very familiar with locales. Can you explain why we need this extra > > pak > > > and can't just use the ui/strings paks we include in repack("pak") below? > > There, > > > we also include ui/strings/app_locale_settings_en-US.pak and > > > ui/strings/ui_strings_en-US.pak. > > > > > > The locale pak is loaded by ResourceBundle::LoadLocaleResources in > > > resource_bundle.cc. > > > On Linux, the fallback app_locale is "en-US", and it tries to find the > > en-US.pak > > > file. > > > On Mac, it tries to find locale.pak file. > > > Chrome generates this file, while headless shell doesn't. That's why we see > > > "locale_file_path.empty() for locale" > > > warning when running headless shell. > > > > > > If we want to reuse repack("pak") below, we need to change its output from > > > headless_lib.pak to en-US.pak or locale.pak depend on the platform. This > > option > > > doesn't looks so good to me. > > > > I have two concerns about this: > > a) Would this have a significant binary size impact? > > b) Would this create problems for building with embedded resources > > (headless_use_embedded_resources)? Can we do the same for these locale paks as > > we do for headless_lib.pak, or is that difficult because of the way the ui/ code > > loads these locale paks? > > > > +skyostil for a) > > +altimin for b) > > Yes, current approach does not support embedded resources. We should add the option to provide locale resources as embedded content. Something along these lines should work: > - InitSharedInstanceWithLocale should be able to skip loading resources (a new setting in ResourceBundle::LoadResources?) > - Add an option to load locale resources directly from a DataPack (we already can create a data pack from a buffer). ResourceBundle::ReloadLocaleResoucesFromDataPack? > > I'd recommend doing that in a separate CL and looping in sadrul@ and sky@ as ui/base owners. If you have any questions, I'd be happy to answer. Thanks for the help! Managed to find a way to avoid using the locale pak for now. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_devtools_manager_delegate.cc:278: << "Print preview is not enabled. Some print settings may not work."; On 2017/04/28 at 08:13:06, Eric Seckler wrote: > On 2017/04/27 22:17:46, Lei Zhang wrote: > > On 2017/04/27 06:56:06, jzfeng wrote: > > > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > > > Which settings don't work? Is it just headers+footers? I'll give you > > > PrintWebViewHelper feedback based on this. > > > > > > headers+footers and scale don't work. > > > > > > In PrintWebViewHelper::PrintPageInternal, under #if > > > BUILDFLAG(ENABLE_PRINT_PREVIEW), there is: > > > 1) scale: css_scale_factor = params.scale_factor; > > > 2) headers+footers: PrintHeaderAndFooter. > > > > I would suggest experimenting with making these features work for basic > > printing. Can you see if that's do able? It can probably be a separate CL. You > > shouldn't flip on the print preview build flag when you don't have the print > > preview feature. > > If this works, would we be able to avoid adding the locale paks, too? :) In PrintWebViewHelper, 1) I removed some of the BUILDFLAG(ENABLE_PRINT_PREVIEW) check so that headless chrome works fine with only basic printing enabled. 2) I changed the code to call GetRawDataResource instead of GetLocalizedString, which avoid adding the locale paks. All the tests looks fine. https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:216: number_pages_ = print_params_->pages.size(); On 2017/04/27 at 22:17:46, Lei Zhang wrote: > On 2017/04/27 06:56:06, jzfeng wrote: > > On 2017/04/20 at 08:35:58, Lei Zhang wrote: > > > Why do we need to do this? > > > > number_pages returned from the renderer is always the page_count of the file, > > not the actual number of printed pages. We need the actual number to check > > whether the job has finished in OnDidPrintPage. > > Is this the issue described in https://crbug.com/161576 ? I wonder how PrintManager deals with it. The bug you found is right. The reason why chrome is not affected by this is because, it doesn't use number_pages to check the completeness. As shown in PrintedDocument::IsComplete, it checks whether all the requested pages have the metafile instead. https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:439: {"name": "paperType", "type": "string", "optional": true, "enum": ["letter", "legal", "A4", "A3"], "description": "Paper type. Defaults to 'letter'."}, On 2017/04/27 at 08:57:00, Eric Seckler wrote: > On 2017/04/27 06:56:07, jzfeng wrote: > > On 2017/04/26 at 08:10:00, martinsb wrote: > > > > That's a product decision. If you go with only mm or inches, then some part > > of > > > > the world is going to be slightly unhappy. e.g. A4 is some odd number of > > inches, > > > > and ditto for Letter in mm. > > > Thank you very much for your efforts, I have no doubt that PDF printing > > customization is an essential and very welcome feature. > > > There may be multiple use cases when a custom size is required. We at > > infogr.am (https://infogr.am) would be happy to migrate our document export to > > PDF from PhantomJS to headless chromium, however not being able to specify > > custom size for the output PDF (in inches, millimeters, microns or anything > > else) still would be a deal breaker for us. > > > > Add a new "CUSTOM" page type to let the user specify page width and height. Also > > add a "unit" parameter to the command to let the user specify using mm or inch. > > Hence, user can pick popular page types, such as letter. They can also specify > > an arbitrary page size as they wish. > > For my taste, this is more complicated than it needs to be. I'd be happier if we fixed this to one unit (inches, for argument's sake, since that's what the printing system seems to use). Users can then look up and specify page sizes in inches themselves. I don't think it's necessary for us to supply "common" page sizes ourselves - that just opens us up to people asking for more explicitly listed page sizes in the protocol. WDYT? Sounds good. Done. https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn#newco... headless/BUILD.gn:229: defines = [ "PRINTING_IMPLEMENTATION" ] On 2017/04/27 at 22:17:46, Lei Zhang wrote: > This doesn't look right. This is not printing/. Done. For some reason, I added this for unittest. But now it is not needed. https://codereview.chromium.org/2829973002/diff/40001/headless/BUILD.gn#newco... headless/BUILD.gn:438: test("headless_printing_unittests") { On 2017/04/27 at 22:17:46, Lei Zhang wrote: > I think you just want to add your test to headless_unittests. You shouldn't have to create a new binary, and configure buildbots to run the new binary, etc. Done. https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:28: : paper_type(NA_LETTER), On 2017/04/27 at 17:54:55, Sami wrote: > Looks like not all fields are being initialized (you could initialize them in the header too if you want). Done. This part is moved into header with all the fields initialized now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
great work, lgtm! :) https://codereview.chromium.org/2829973002/diff/180001/components/resources/B... File components/resources/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/180001/components/resources/B... components/resources/BUILD.gn:35: "enable_print_preview_page=$enable_print_preview", this probably shouldn't be adding "_page" to the identifier https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:208: default: nit: make this a "case SUCCESS:" or alike and add a "default:" with NOTREACHED()
Thanks for the review! @Lei, would you minding taking a look at this CL? https://codereview.chromium.org/2829973002/diff/180001/components/resources/B... File components/resources/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/180001/components/resources/B... components/resources/BUILD.gn:35: "enable_print_preview_page=$enable_print_preview", On 2017/05/02 at 09:35:27, Eric Seckler wrote: > this probably shouldn't be adding "_page" to the identifier Good catch, I forget to remove this suffix. Done. https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.cc:208: default: On 2017/05/02 at 09:35:27, Eric Seckler wrote: > nit: make this a "case SUCCESS:" or alike and add a "default:" with NOTREACHED() Done.
On 2017/05/03 00:45:56, jzfeng wrote: > @Lei, would you minding taking a look at this CL? I'm a bit behind on code reviews. Hopefully tomorrow.
headless/ lgtm, thank you! https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:103: // we can safely ignore the return values of the following Get methods since nit: Start with a capital letter please. https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:126: // set default margin to 1.0cm = ~2/5 of an inch. Ditto.
Thanks for the review! @Lei, have you got sometime to review this CL? Thanks! https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:103: // we can safely ignore the return values of the following Get methods since On 2017/05/03 at 17:21:54, Sami wrote: > nit: Start with a capital letter please. Done. https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:126: // set default margin to 1.0cm = ~2/5 of an inch. On 2017/05/03 at 17:21:53, Sami wrote: > Ditto. Done.
On 2017/05/04 01:44:47, jzfeng wrote: > Thanks for the review! > > @Lei, have you got sometime to review this CL? Maybe tonight. :)
On 2017/05/04 at 01:51:55, thestig wrote: > On 2017/05/04 01:44:47, jzfeng wrote: > > Thanks for the review! > > > > @Lei, have you got sometime to review this CL? > > Maybe tonight. :) Cool. Thank you very much!
Looks good. Lots of nitty bits to polish. https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:94: #endif // BUILDFLAG(ENABLE_PRINT_PREVIEW) Drop the comment since this is now much closer to the #if. https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... components/printing/renderer/print_web_view_helper.h:352: // Given the |device| and |canvas| to draw on, prints the appropriate headers Uh, I noticed this is wrong. Would you mind correcting it since we are already touching this file? // Given the |canvas| to draw on, prints the appropriate headers and footers // to |canvas| using information from the remaining parameters. https://codereview.chromium.org/2829973002/diff/220001/components/resources/B... File components/resources/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/220001/components/resources/B... components/resources/BUILD.gn:36: "enable_basic_printing=$enable_basic_printing", Flip these two around. This first. https://codereview.chromium.org/2829973002/diff/220001/components/resources/p... File components/resources/printing_resources.grdp (right): https://codereview.chromium.org/2829973002/diff/220001/components/resources/p... components/resources/printing_resources.grdp:3: <if expr="enable_print_preview or enable_basic_printing"> Flip the order here too. https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:412: deps += [ "//content/public/common" ] Not obvious why headless_printing_unittest.cc needs this. Can you explain, or remove if it is not needed? https://codereview.chromium.org/2829973002/diff/220001/headless/lib/DEPS File headless/lib/DEPS (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/DEPS#newc... headless/lib/DEPS:4: "+printing", Alphabetical order. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:115: paper_height_in_inch = printing::kLetterHeightInch; One decl per line. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:74: static PageRangeStatus PageRangeTextToPages(std::string page_range_text, Can you pass in a base::StringPiece instead? https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:90: std::unique_ptr<PrintMsg_PrintPages_Params> PrintParams( Rename to GetPrintParamsFromSettings? https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:91: HeadlessPrintSettings settings); Pass by const ref. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_printing_unittest.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:76: params->SetString("pageRanges", "abcd"); Add a comment to explain why obvious bad values are not being rejected. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:151: std::vector<int> pages, expected_pages; 1 decl per line. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... File headless/lib/headless_web_contents_browsertest.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... headless/lib/headless_web_contents_browsertest.cc:217: EXPECT_LT(0U, base64.length()); Flip this around. Most people think of this as EXPECT_GT(base64.length(), 0); https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... headless/lib/headless_web_contents_browsertest.cc:227: double width_in_points, height_in_points; 1 decl per line. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:432: "description": "Print page as pdf.", BTW, write "PDF" because it expands out to Portable Document Format. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:436: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background colors. Defaults to false."}, Write "background graphics" to be more correct, and consistent with the phrase used in print preview. It's more than just colors. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:437: {"name": "scale", "type": "number", "optional": true, "description": "Scale of the pdf. Defaults to 1."}, The description is a bit confusing. Does this mean if scale = 2, then an A4 portrait page will be scaled up to A2? It's really the scale of the webpage rendering, or something like that. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:438: {"name": "paperWidth", "type": "number", "optional": true, "description": "Paper width in inch. Defaults to 8.5 inch."}, inches, ditto everywhere below. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:440: {"name": "marginTop", "type": "number", "optional": true, "description": "Top margin in inch. Defaults to 1cm = ~2/5 of an inch."}, Just write "Defaults to 1 cm. (~0.4 inches)" https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:444: {"name": "pageRanges", "type": "string", "optional": true, "description": "Paper ranges to print, e.g., '1-5, 8, 11-13'. Defaults to print all pages."} Defaults to the empty string, which means print all pages.
Thanks for the review! PTAL. https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:94: #endif // BUILDFLAG(ENABLE_PRINT_PREVIEW) On 2017/05/04 at 07:29:36, Lei Zhang wrote: > Drop the comment since this is now much closer to the #if. Done. https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2829973002/diff/220001/components/printing/re... components/printing/renderer/print_web_view_helper.h:352: // Given the |device| and |canvas| to draw on, prints the appropriate headers On 2017/05/04 at 07:29:36, Lei Zhang wrote: > Uh, I noticed this is wrong. Would you mind correcting it since we are already touching this file? > > // Given the |canvas| to draw on, prints the appropriate headers and footers > // to |canvas| using information from the remaining parameters. Done. https://codereview.chromium.org/2829973002/diff/220001/components/resources/B... File components/resources/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/220001/components/resources/B... components/resources/BUILD.gn:36: "enable_basic_printing=$enable_basic_printing", On 2017/05/04 at 07:29:36, Lei Zhang wrote: > Flip these two around. This first. Done. https://codereview.chromium.org/2829973002/diff/220001/components/resources/p... File components/resources/printing_resources.grdp (right): https://codereview.chromium.org/2829973002/diff/220001/components/resources/p... components/resources/printing_resources.grdp:3: <if expr="enable_print_preview or enable_basic_printing"> On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Flip the order here too. Done. https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:412: deps += [ "//content/public/common" ] On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Not obvious why headless_printing_unittest.cc needs this. Can you explain, or remove if it is not needed? There will compile error if it is removed. In file included from ../../headless/lib/browser/headless_printing_unittest.cc:7: In file included from ../../headless/lib/browser/headless_devtools_manager_delegate.h:19: In file included from ../../headless/lib/browser/headless_print_manager.h:12: In file included from ../../components/printing/browser/print_manager.h:10: In file included from ../../content/public/browser/web_contents_observer.h:22: In file included from ../../third_party/skia/include/core/SkColor.h:11: In file included from ../../third_party/skia/include/core/SkScalar.h:11: ../../third_party/skia/include/core/../private/SkFloatingPoint.h:13:10: fatal error: 'SkTypes.h' file not found #include "SkTypes.h" ^~~~~~~~~~~ 1 error generated. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/DEPS File headless/lib/DEPS (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/DEPS#newc... headless/lib/DEPS:4: "+printing", On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Alphabetical order. Done. I'm surprised this is not auto formatted. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_devtools_manager_delegate.cc:115: paper_height_in_inch = printing::kLetterHeightInch; On 2017/05/04 at 07:29:37, Lei Zhang wrote: > One decl per line. Done. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_print_manager.h (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:74: static PageRangeStatus PageRangeTextToPages(std::string page_range_text, On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Can you pass in a base::StringPiece instead? Done. When shall I use base::StringPiece? I see there are a lot of code using it, but to me it is the same as std::string so far. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:90: std::unique_ptr<PrintMsg_PrintPages_Params> PrintParams( On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Rename to GetPrintParamsFromSettings? Done. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_print_manager.h:91: HeadlessPrintSettings settings); On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Pass by const ref. Done. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_printing_unittest.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:76: params->SetString("pageRanges", "abcd"); On 2017/05/04 at 07:29:37, Lei Zhang wrote: > Add a comment to explain why obvious bad values are not being rejected. Done. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:151: std::vector<int> pages, expected_pages; On 2017/05/04 at 07:29:37, Lei Zhang wrote: > 1 decl per line. Done. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... File headless/lib/headless_web_contents_browsertest.cc (right): https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... headless/lib/headless_web_contents_browsertest.cc:217: EXPECT_LT(0U, base64.length()); On 2017/05/04 at 07:29:38, Lei Zhang wrote: > Flip this around. Most people think of this as EXPECT_GT(base64.length(), 0); Done. Changed the one in OnScreenshotCaptured as well. https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_... headless/lib/headless_web_contents_browsertest.cc:227: double width_in_points, height_in_points; On 2017/05/04 at 07:29:38, Lei Zhang wrote: > 1 decl per line. Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:432: "description": "Print page as pdf.", On 2017/05/04 at 07:29:38, Lei Zhang wrote: > BTW, write "PDF" because it expands out to Portable Document Format. Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:436: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background colors. Defaults to false."}, On 2017/05/04 at 07:29:38, Lei Zhang wrote: > Write "background graphics" to be more correct, and consistent with the phrase used in print preview. It's more than just colors. Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:437: {"name": "scale", "type": "number", "optional": true, "description": "Scale of the pdf. Defaults to 1."}, On 2017/05/04 at 07:29:38, Lei Zhang wrote: > The description is a bit confusing. Does this mean if scale = 2, then an A4 portrait page will be scaled up to A2? It's really the scale of the webpage rendering, or something like that. Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:438: {"name": "paperWidth", "type": "number", "optional": true, "description": "Paper width in inch. Defaults to 8.5 inch."}, On 2017/05/04 at 07:29:38, Lei Zhang wrote: > inches, ditto everywhere below. Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:440: {"name": "marginTop", "type": "number", "optional": true, "description": "Top margin in inch. Defaults to 1cm = ~2/5 of an inch."}, On 2017/05/04 at 07:29:38, Lei Zhang wrote: > Just write "Defaults to 1 cm. (~0.4 inches)" Done. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:444: {"name": "pageRanges", "type": "string", "optional": true, "description": "Paper ranges to print, e.g., '1-5, 8, 11-13'. Defaults to print all pages."} On 2017/05/04 at 07:29:38, Lei Zhang wrote: > Defaults to the empty string, which means print all pages. Done.
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Oh, looks like one of my CLs to reuse chrome::kChromeUIPrintURL broke the build here. :\
https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:412: deps += [ "//content/public/common" ] On 2017/05/05 02:41:33, jzfeng wrote: > On 2017/05/04 at 07:29:37, Lei Zhang wrote: > > Not obvious why headless_printing_unittest.cc needs this. Can you explain, or > remove if it is not needed? > > There will compile error if it is removed. I'm skeptical about this as the correct solution, but I don't have better advice at the moment. Let me look at the dependencies. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:438: {"name": "paperWidth", "type": "number", "optional": true, "description": "Paper width in inch. Defaults to 8.5 inch."}, On 2017/05/05 02:41:34, jzfeng wrote: > On 2017/05/04 at 07:29:38, Lei Zhang wrote: > > inches, ditto everywhere below. > > Done. Still need to write inches in a bunch of places. https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/h... File headless/lib/browser/headless_printing_unittest.cc (right): https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:81: // total page count is avaliable. See the PageRangeTextToPagesTest. Typo: available. https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:436: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background graphics. Defaults to false."}, printBackground
On 2017/05/05 03:44:10, Lei Zhang wrote: > Oh, looks like one of my CLs to reuse chrome::kChromeUIPrintURL broke the build > here. :\ Put https://codereview.chromium.org/2857743005/ in CQ to fix that.
Thanks for the review and fix! PTAL. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:438: {"name": "paperWidth", "type": "number", "optional": true, "description": "Paper width in inch. Defaults to 8.5 inch."}, On 2017/05/05 03:53:00, Lei Zhang wrote: > On 2017/05/05 02:41:34, jzfeng wrote: > > On 2017/05/04 at 07:29:38, Lei Zhang wrote: > > > inches, ditto everywhere below. > > > > Done. > > Still need to write inches in a bunch of places. Do you mean I should also use inches in "in inch"? I thought I only need to change the ones after a number. Done now. https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/h... File headless/lib/browser/headless_printing_unittest.cc (right): https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/h... headless/lib/browser/headless_printing_unittest.cc:81: // total page count is avaliable. See the PageRangeTextToPagesTest. On 2017/05/05 03:53:00, Lei Zhang wrote: > Typo: available. Done. https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:436: {"name": "printBackgrounds", "type": "boolean", "optional": true, "description": "Print background graphics. Defaults to false."}, On 2017/05/05 03:53:00, Lei Zhang wrote: > printBackground Done.
Still looking into why the skia public_deps entry from content/ isn't propagating. https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:438: {"name": "paperWidth", "type": "number", "optional": true, "description": "Paper width in inch. Defaults to 8.5 inch."}, On 2017/05/05 04:28:10, jzfeng wrote: > On 2017/05/05 03:53:00, Lei Zhang wrote: > > On 2017/05/05 02:41:34, jzfeng wrote: > > > On 2017/05/04 at 07:29:38, Lei Zhang wrote: > > > > inches, ditto everywhere below. > > > > > > Done. > > > > Still need to write inches in a bunch of places. > > Do you mean I should also use inches in "in inch"? I thought I only need to > change the ones after a number. > Done now. Yes, as in Q: "What is the unit of measure?" A: "We measure the paper width in inches."
I changed to use the //skia dep, which seems to work.
On 2017/05/05 04:41:32, jzfeng wrote: > I changed to use the //skia dep, which seems to work. Move "//content/public/common" in headless_lib to public_deps can also solve the problem. Maybe this way is better?
On 2017/05/05 04:54:20, jzfeng wrote: > On 2017/05/05 04:41:32, jzfeng wrote: > > I changed to use the //skia dep, which seems to work. > > Move "//content/public/common" in headless_lib to public_deps can also solve the > problem. > > Maybe this way is better? I'm not sure why you chose content/public/common. If you look at the error you pasted into https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn#newc... - it doesn't have anything to do with common. I think what you really need to expose via public_deps is //skia, because it's the Skia headers that are exposed indirectly via headless/lib/browser/headless_print_manager.h.
Otherwise, lgtm
On 2017/05/05 05:17:12, Lei Zhang wrote: > On 2017/05/05 04:54:20, jzfeng wrote: > > On 2017/05/05 04:41:32, jzfeng wrote: > > > I changed to use the //skia dep, which seems to work. > > > > Move "//content/public/common" in headless_lib to public_deps can also solve > the > > problem. > > > > Maybe this way is better? > > I'm not sure why you chose content/public/common. If you look at the error you > pasted into > https://codereview.chromium.org/2829973002/diff/220001/headless/BUILD.gn#newc... > - it doesn't have anything to do with common. > > I think what you really need to expose via public_deps is //skia, because it's > the Skia headers that are exposed indirectly via > headless/lib/browser/headless_print_manager.h. Sounds good. Done. Thanks!
jzfeng@chromium.org changed reviewers: + caitkp@chromium.org
Description was changed from ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Added unit test and browser test for print to pdf. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Added unit test and browser test for print to pdf. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jzfeng@chromium.org changed reviewers: + blundell@chromium.org - caitkp@chromium.org
@blundell: could you take a look at the change under components/resources? @pfeldman: could you take a look at the change in browser_protocol.json? Thanks!
//components top-level lgtm
https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... headless/BUILD.gn:322: public_deps += [ "//skia" ] This is probably fine if it's needed to fix a build error, but I'll just note that embedders of headless shouldn't depend on skia to avoid build breaks due to API churn.
weili@chromium.org changed reviewers: + weili@chromium.org
https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... headless/BUILD.gn:322: public_deps += [ "//skia" ] drive-by: Should this be a deps instead? I don't see why headless lib needs to expose skia? btw, is it better to merge sections at line307, this one and line352 all together?
https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... headless/BUILD.gn:322: public_deps += [ "//skia" ] On 2017/05/05 17:18:28, Wei Li wrote: > drive-by: Should this be a deps instead? I don't see why headless lib needs to > expose skia? It's not a deps entry because it is no the headless_lib target that needs it. It's any dependents that #includes headless/lib/browser/headless_print_manager.h that has problems. As an example, components/constrained_window does the same thing, even though the directory does not have a single skia #include.
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/05/05 21:39:28, Lei Zhang wrote: > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn > File headless/BUILD.gn (right): > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... > headless/BUILD.gn:322: public_deps += [ "//skia" ] > On 2017/05/05 17:18:28, Wei Li wrote: > > drive-by: Should this be a deps instead? I don't see why headless lib needs to > > expose skia? > > It's not a deps entry because it is no the headless_lib target that needs it. > It's any dependents that #includes headless/lib/browser/headless_print_manager.h > that has problems. > > As an example, components/constrained_window does the same thing, even though > the directory does not have a single skia #include. got it, upstream probably doesn't have to expose skia types. The current solution lgtm.
devtools protocol lgtm
The CQ bit was checked by jzfeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, skyostil@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2829973002/#ps320001 (title: "add skia as public_deps instead")
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": 320001, "attempt_start_ts": 1494290364664310,
"parent_rev": "86d8eaf62f5dfd8c4d3d9d542d6c4a859ca595fe", "commit_rev":
"3d83dad2db679c3924ffe74599cfd48a03211778"}
Message was sent while issue was closed.
Description was changed from ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Added unit test and browser test for print to pdf. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add customized printing setting for headless 1) Add parameters to printToPDF command, which let the user to specify printing settings like paper size, margin size, etc. 2) PrintWebViewHelper::PrintPageInternal and PrintWebViewHelper::RenderPage feed print_preview_context_.total_page_count() to PrintHeaderAndFooter. However, HeadlessPrintManager issues PrintMsg_PrintPages IPC message, which leaves print_preview_context_ uninitialized. To solve the problem, add page_count as an arg to these two methods. 3) Added unit test and browser test for print to pdf. BUG=603559 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2829973002 Cr-Commit-Position: refs/heads/master@{#470159} Committed: https://chromium.googlesource.com/chromium/src/+/3d83dad2db679c3924ffe74599cf... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/3d83dad2db679c3924ffe74599cf...
Message was sent while issue was closed.
https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... headless/BUILD.gn:322: public_deps += [ "//skia" ] On 2017/05/05 21:39:28, Lei Zhang wrote: > On 2017/05/05 17:18:28, Wei Li wrote: > > drive-by: Should this be a deps instead? I don't see why headless lib needs to > > expose skia? > > It's not a deps entry because it is no the headless_lib target that needs it. > It's any dependents that #includes headless/lib/browser/headless_print_manager.h > that has problems. > > As an example, components/constrained_window does the same thing, even though > the directory does not have a single skia #include. I've been following this discussion, and I still don't really understand it. If the target doesn't need to directly depend on or expose //skia, then it also shouldn't need to have //skia as a public dependency. If the issue is that this target is exposing its dependents to some intermediate target that then exposes //skia, I believe that the following structure should have the same effect and is cleaner from an encapsulation POV: - the intermediate target lists //skia as a public dep - this target lists the intermediate target as a public dep Am I missing something here?
Message was sent while issue was closed.
https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... headless/BUILD.gn:322: public_deps += [ "//skia" ] On 2017/05/09 15:34:04, blundell wrote: > On 2017/05/05 21:39:28, Lei Zhang wrote: > > On 2017/05/05 17:18:28, Wei Li wrote: > > > drive-by: Should this be a deps instead? I don't see why headless lib needs > to > > > expose skia? > > > > It's not a deps entry because it is no the headless_lib target that needs it. > > It's any dependents that #includes > headless/lib/browser/headless_print_manager.h > > that has problems. > > > > As an example, components/constrained_window does the same thing, even though > > the directory does not have a single skia #include. > > I've been following this discussion, and I still don't really understand it. If > the target doesn't need to directly depend on or expose //skia, then it also > shouldn't need to have //skia as a public dependency. If the issue is that this > target is exposing its dependents to some intermediate target that then exposes > //skia, I believe that the following structure should have the same effect and > is cleaner from an encapsulation POV: > - the intermediate target lists //skia as a public dep > - this target lists the intermediate target as a public dep > > Am I missing something here? headless_lib need to expose //skia to make the target depend on it to build. Among the current deps of headless_lib, several expose //skia. They are: content/public/app:both content/public/browser content/public/child:child content/public/common ui/base ui/display ui/events/devices So it wouldn't make much sense to expose one of them from headless_lib.
Message was sent while issue was closed.
On 2017/05/10 06:55:09, jzfeng wrote: > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn > File headless/BUILD.gn (right): > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... > headless/BUILD.gn:322: public_deps += [ "//skia" ] > On 2017/05/09 15:34:04, blundell wrote: > > On 2017/05/05 21:39:28, Lei Zhang wrote: > > > On 2017/05/05 17:18:28, Wei Li wrote: > > > > drive-by: Should this be a deps instead? I don't see why headless lib > needs > > to > > > > expose skia? > > > > > > It's not a deps entry because it is no the headless_lib target that needs > it. > > > It's any dependents that #includes > > headless/lib/browser/headless_print_manager.h > > > that has problems. > > > > > > As an example, components/constrained_window does the same thing, even > though > > > the directory does not have a single skia #include. > > > > I've been following this discussion, and I still don't really understand it. > If > > the target doesn't need to directly depend on or expose //skia, then it also > > shouldn't need to have //skia as a public dependency. If the issue is that > this > > target is exposing its dependents to some intermediate target that then > exposes > > //skia, I believe that the following structure should have the same effect and > > is cleaner from an encapsulation POV: > > - the intermediate target lists //skia as a public dep > > - this target lists the intermediate target as a public dep > > > > Am I missing something here? > > headless_lib need to expose //skia to make the target depend on it to build. Can you detail exactly why this is the case? > Among the current deps of headless_lib, several expose //skia. They are: > content/public/app:both > content/public/browser > content/public/child:child > content/public/common > ui/base > ui/display > ui/events/devices > So it wouldn't make much sense to expose one of them from headless_lib.
Message was sent while issue was closed.
On 2017/05/10 09:35:10, blundell wrote: > On 2017/05/10 06:55:09, jzfeng wrote: > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn > > File headless/BUILD.gn (right): > > > > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... > > headless/BUILD.gn:322: public_deps += [ "//skia" ] > > On 2017/05/09 15:34:04, blundell wrote: > > > On 2017/05/05 21:39:28, Lei Zhang wrote: > > > > On 2017/05/05 17:18:28, Wei Li wrote: > > > > > drive-by: Should this be a deps instead? I don't see why headless lib > > needs > > > to > > > > > expose skia? > > > > > > > > It's not a deps entry because it is no the headless_lib target that needs > > it. > > > > It's any dependents that #includes > > > headless/lib/browser/headless_print_manager.h > > > > that has problems. > > > > > > > > As an example, components/constrained_window does the same thing, even > > though > > > > the directory does not have a single skia #include. > > > > > > I've been following this discussion, and I still don't really understand it. > > If > > > the target doesn't need to directly depend on or expose //skia, then it also > > > shouldn't need to have //skia as a public dependency. If the issue is that > > this > > > target is exposing its dependents to some intermediate target that then > > exposes > > > //skia, I believe that the following structure should have the same effect > and > > > is cleaner from an encapsulation POV: > > > - the intermediate target lists //skia as a public dep > > > - this target lists the intermediate target as a public dep > > > > > > Am I missing something here? > > > > headless_lib need to expose //skia to make the target depend on it to build. > > Can you detail exactly why this is the case? I added a unit test which need to include headless_print_manager.h. Without //skia, it doesn't build. The error message is: in file included from ../../headless/lib/browser/headless_printing_unittest.cc:7: In file included from ../../headless/lib/browser/headless_devtools_manager_delegate.h:19: In file included from ../../headless/lib/browser/headless_print_manager.h:12: In file included from ../../components/printing/browser/print_manager.h:10: In file included from ../../content/public/browser/web_contents_observer.h:22: In file included from ../../third_party/skia/include/core/SkColor.h:11: In file included from ../../third_party/skia/include/core/SkScalar.h:11: ../../third_party/skia/include/core/../private/SkFloatingPoint.h:13:10: fatal error: 'SkTypes.h' file not found #include "SkTypes.h" ^~~~~~~~~~~ 1 error generated.
Message was sent while issue was closed.
On 2017/05/10 09:56:44, jzfeng (OOO) wrote: > On 2017/05/10 09:35:10, blundell wrote: > > On 2017/05/10 06:55:09, jzfeng wrote: > > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn > > > File headless/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2829973002/diff/320001/headless/BUILD.gn#newc... > > > headless/BUILD.gn:322: public_deps += [ "//skia" ] > > > On 2017/05/09 15:34:04, blundell wrote: > > > > On 2017/05/05 21:39:28, Lei Zhang wrote: > > > > > On 2017/05/05 17:18:28, Wei Li wrote: > > > > > > drive-by: Should this be a deps instead? I don't see why headless lib > > > needs > > > > to > > > > > > expose skia? > > > > > > > > > > It's not a deps entry because it is no the headless_lib target that > needs > > > it. > > > > > It's any dependents that #includes > > > > headless/lib/browser/headless_print_manager.h > > > > > that has problems. > > > > > > > > > > As an example, components/constrained_window does the same thing, even > > > though > > > > > the directory does not have a single skia #include. > > > > > > > > I've been following this discussion, and I still don't really understand > it. > > > If > > > > the target doesn't need to directly depend on or expose //skia, then it > also > > > > shouldn't need to have //skia as a public dependency. If the issue is that > > > this > > > > target is exposing its dependents to some intermediate target that then > > > exposes > > > > //skia, I believe that the following structure should have the same effect > > and > > > > is cleaner from an encapsulation POV: > > > > - the intermediate target lists //skia as a public dep > > > > - this target lists the intermediate target as a public dep > > > > > > > > Am I missing something here? > > > > > > headless_lib need to expose //skia to make the target depend on it to build. > > > > Can you detail exactly why this is the case? > > I added a unit test which need to include headless_print_manager.h. > > Without //skia, it doesn't build. > The error message is: > in file included from > ../../headless/lib/browser/headless_printing_unittest.cc:7: > In file included from > ../../headless/lib/browser/headless_devtools_manager_delegate.h:19: > In file included from > ../../headless/lib/browser/headless_print_manager.h:12: > In file included from > ../../components/printing/browser/print_manager.h:10: > In file included from > ../../content/public/browser/web_contents_observer.h:22: > In file included from ../../third_party/skia/include/core/SkColor.h:11: > In file included from ../../third_party/skia/include/core/SkScalar.h:11: > ../../third_party/skia/include/core/../private/SkFloatingPoint.h:13:10: > fatal error: 'SkTypes.h' file not found > #include "SkTypes.h" > ^~~~~~~~~~~ > 1 error generated. Great, thanks! Based on that inclusion chain, I think that the below CL is a cleaner approach, for reasons that I outline in the CL description: https://codereview.chromium.org/2871123003/ |
