add customized printing setting for headless (issue 2829973002 by jzfeng@chromium.org)

[jzf...@chromium.org]

Reviewers: Lei Zhang, Eric Seckler
CL: https://codereview.chromium.org/2829973002/

Message:
This patch adds the customized printing setting feature.

PTAL.

Thanks!

Description:
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

Affected files (+355, -58 lines):
M build/args/headless.gn
M components/printing/renderer/print_web_view_helper.h
M components/printing/renderer/print_web_view_helper.cc
M components/printing/renderer/print_web_view_helper_linux.cc
M components/printing/renderer/print_web_view_helper_mac.mm
M components/printing/renderer/print_web_view_helper_pdf_win.cc
M content/browser/devtools/protocol/page_handler.h
M content/browser/devtools/protocol/page_handler.cc
M headless/BUILD.gn
M headless/app/headless_shell.cc
M headless/lib/browser/headless_devtools_manager_delegate.cc
M headless/lib/browser/headless_print_manager.h
M headless/lib/browser/headless_print_manager.cc
M headless/lib/headless_content_main_delegate.cc
M headless/lib/renderer/headless_print_web_view_helper_delegate.cc
M third_party/WebKit/Source/core/inspector/browser_protocol.json

[the...@chromium.org]

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h#newcode80
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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode78
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/headless_devtools_manager_delegate.cc#newcode87
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/headless_devtools_manager_delegate.cc#newcode92
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/headless_devtools_manager_delegate.cc#newcode112
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/headless_devtools_manager_delegate.cc#newcode123
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/headless_devtools_manager_delegate.cc#newcode138
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/headless_devtools_manager_delegate.cc#newcode278
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/headless_devtools_manager_delegate.cc#newcode284
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode148
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/headless_print_manager.cc#newcode194
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/headless_print_manager.cc#newcode216
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/headless_print_manager.cc#newcode267
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/headless_print_manager.h
File headless/lib/browser/headless_print_manager.h (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.h#newcode22
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/headless_print_manager.h#newcode50
headless/lib/browser/headless_print_manager.h:50: double scale;
Mention 1 = 100%?

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.h#newcode85
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_content_main_delegate.cc
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_content_main_delegate.cc#newcode252
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/

[the...@chromium.org]

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode284
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/

[the...@chromium.org]

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode129
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/

[esec...@chromium.org]

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#newcode28
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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode82
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode37
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/headless_print_manager.cc#newcode47
headless/lib/browser/headless_print_manager.cc:47: gfx::Size
HeadlessPrintSettings::PaperSizeInPixel() {
nit: PaperSizeInPixelsOrPoints

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode104
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/headless_print_manager.h
File headless/lib/browser/headless_print_manager.h (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.h#newcode69
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode434
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/Source/core/inspector/browser_protocol.json#newcode437
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/Source/core/inspector/browser_protocol.json#newcode439
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/Source/core/inspector/browser_protocol.json#newcode440
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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

+weili FYI since there's potential PrintWebViewHelper changes here.

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode439
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.

https://codereview.chromium.org/2829973002/

[esec...@chromium.org]

On 2017/04/20 19:08:59, Lei Zhang wrote:
>
https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode439
> 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.

https://codereview.chromium.org/2829973002/

[pfel...@chromium.org]

> 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.

https://codereview.chromium.org/2829973002/

[m...@infogr.am]

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode439
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.

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Thanks for the review!

PTAL.

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h#newcode80
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#newcode28
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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode78
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/headless_devtools_manager_delegate.cc#newcode82
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/headless_devtools_manager_delegate.cc#newcode87
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/headless_devtools_manager_delegate.cc#newcode92
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/headless_devtools_manager_delegate.cc#newcode112
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/headless_devtools_manager_delegate.cc#newcode123
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/headless_devtools_manager_delegate.cc#newcode129
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/headless_devtools_manager_delegate.cc#newcode138
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/headless_devtools_manager_delegate.cc#newcode278
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/headless_devtools_manager_delegate.cc#newcode284
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode37
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/headless_print_manager.cc#newcode47
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/headless_print_manager.cc#newcode104
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/headless_print_manager.cc#newcode148
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/headless_print_manager.cc#newcode194
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/headless_print_manager.cc#newcode216
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/headless_print_manager.cc#newcode267
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/headless_print_manager.h
File headless/lib/browser/headless_print_manager.h (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.h#newcode22
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/headless_print_manager.h#newcode50
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/headless_print_manager.h#newcode69
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/headless_print_manager.h#newcode85
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_content_main_delegate.cc
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/headless_content_main_delegate.cc#newcode252
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode434
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/Source/core/inspector/browser_protocol.json#newcode437
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/Source/core/inspector/browser_protocol.json#newcode439
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/Source/core/inspector/browser_protocol.json#newcode440
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.

https://codereview.chromium.org/2829973002/

[esec...@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/devtools/protocol/page_handler.h
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h#newcode80
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#newcode28
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode439
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'."},

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/

[alt...@chromium.org]

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#newcode28
headless/BUILD.gn:28: repack_locales("locale_pak") {

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.

https://codereview.chromium.org/2829973002/

[skyostil@chromium.org via codereview.chromium.org]

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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/headless_print_manager.cc#newcode28
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/

[the...@chromium.org]

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h#newcode80
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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode278
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode216
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#newcode229
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#newcode438
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/

[esec...@chromium.org]

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/2829973002/diff/20001/content/browser/devtools/protocol/page_handler.h#newcode80
content/browser/devtools/protocol/page_handler.h:80: void
PrintToPDF(Maybe<int> dpi,

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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode278
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? :)

https://codereview.chromium.org/2829973002/

[kard...@gmail.com]

> 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.


https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

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#newcode28
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:

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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode278
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/20001/headless/lib/browser/headless_print_manager.cc#newcode216
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode439
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'."},

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#newcode229
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#newcode438
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/40001/headless/lib/browser/headless_print_manager.cc#newcode28
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.

https://codereview.chromium.org/2829973002/

[esec...@chromium.org]

great work, lgtm! :)


https://codereview.chromium.org/2829973002/diff/180001/components/resources/BUILD.gn
File components/resources/BUILD.gn (right):

https://codereview.chromium.org/2829973002/diff/180001/components/resources/BUILD.gn#newcode35
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/headless_print_manager.cc#newcode208
headless/lib/browser/headless_print_manager.cc:208: default:
nit: make this a "case SUCCESS:" or alike and add a "default:" with
NOTREACHED()

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Thanks for the review!

@Lei, would you minding taking a look at this CL?

https://codereview.chromium.org/2829973002/diff/180001/components/resources/BUILD.gn
File components/resources/BUILD.gn (right):

https://codereview.chromium.org/2829973002/diff/180001/components/resources/BUILD.gn#newcode35
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/headless_print_manager.cc
File headless/lib/browser/headless_print_manager.cc (right):

https://codereview.chromium.org/2829973002/diff/180001/headless/lib/browser/headless_print_manager.cc#newcode208
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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

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.

https://codereview.chromium.org/2829973002/

[skyostil@chromium.org via codereview.chromium.org]

headless/ lgtm, thank you!


https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode103
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/headless_devtools_manager_delegate.cc#newcode126
headless/lib/browser/headless_devtools_manager_delegate.cc:126: // set
default margin to 1.0cm = ~2/5 of an inch.
Ditto.

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Thanks for the review!

@Lei, have you got sometime to review this CL?

Thanks!

https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/200001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode103
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/headless_devtools_manager_delegate.cc#newcode126
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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

On 2017/05/04 01:44:47, jzfeng wrote:
> Thanks for the review!
>
> @Lei, have you got sometime to review this CL?

Maybe tonight. :)

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

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!

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

Looks good. Lots of nitty bits to polish.


https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.cc
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.cc#newcode94
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/renderer/print_web_view_helper.h
File components/printing/renderer/print_web_view_helper.h (right):

https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.h#newcode352
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/BUILD.gn
File components/resources/BUILD.gn (right):

https://codereview.chromium.org/2829973002/diff/220001/components/resources/BUILD.gn#newcode36
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/printing_resources.grdp
File components/resources/printing_resources.grdp (right):

https://codereview.chromium.org/2829973002/diff/220001/components/resources/printing_resources.grdp#newcode3
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#newcode412
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#newcode4
headless/lib/DEPS:4: "+printing",
Alphabetical order.

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode115
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/headless_print_manager.h
File headless/lib/browser/headless_print_manager.h (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_print_manager.h#newcode74
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/headless_print_manager.h#newcode90
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/headless_print_manager.h#newcode91
headless/lib/browser/headless_print_manager.h:91: HeadlessPrintSettings
settings);
Pass by const ref.

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_printing_unittest.cc
File headless/lib/browser/headless_printing_unittest.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_printing_unittest.cc#newcode76
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/headless_printing_unittest.cc#newcode151
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_web_contents_browsertest.cc
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_web_contents_browsertest.cc#newcode217
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_web_contents_browsertest.cc#newcode227
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode432
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/Source/core/inspector/browser_protocol.json#newcode436
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/Source/core/inspector/browser_protocol.json#newcode437
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/Source/core/inspector/browser_protocol.json#newcode438
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/Source/core/inspector/browser_protocol.json#newcode440
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/Source/core/inspector/browser_protocol.json#newcode444
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.

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Thanks for the review!

PTAL.

https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.cc
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.cc#newcode94
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/renderer/print_web_view_helper.h
File components/printing/renderer/print_web_view_helper.h (right):

https://codereview.chromium.org/2829973002/diff/220001/components/printing/renderer/print_web_view_helper.h#newcode352
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/BUILD.gn
File components/resources/BUILD.gn (right):

https://codereview.chromium.org/2829973002/diff/220001/components/resources/BUILD.gn#newcode36
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/printing_resources.grdp
File components/resources/printing_resources.grdp (right):

https://codereview.chromium.org/2829973002/diff/220001/components/resources/printing_resources.grdp#newcode3
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#newcode412
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#newcode4
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/headless_devtools_manager_delegate.cc
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_devtools_manager_delegate.cc#newcode115
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/headless_print_manager.h
File headless/lib/browser/headless_print_manager.h (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_print_manager.h#newcode74
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/headless_print_manager.h#newcode90
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/headless_print_manager.h#newcode91
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/headless_printing_unittest.cc
File headless/lib/browser/headless_printing_unittest.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/browser/headless_printing_unittest.cc#newcode76
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/headless_printing_unittest.cc#newcode151
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_web_contents_browsertest.cc
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2829973002/diff/220001/headless/lib/headless_web_contents_browsertest.cc#newcode217
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_web_contents_browsertest.cc#newcode227
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode432
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/Source/core/inspector/browser_protocol.json#newcode436
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/Source/core/inspector/browser_protocol.json#newcode437
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/Source/core/inspector/browser_protocol.json#newcode438
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/Source/core/inspector/browser_protocol.json#newcode440
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/Source/core/inspector/browser_protocol.json#newcode444
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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

Oh, looks like one of my CLs to reuse chrome::kChromeUIPrintURL broke the build
here. :\

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

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#newcode412
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode438
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/headless_printing_unittest.cc
File headless/lib/browser/headless_printing_unittest.cc (right):

https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/headless_printing_unittest.cc#newcode81
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode436

third_party/WebKit/Source/core/inspector/browser_protocol.json:436:
{"name": "printBackgrounds", "type": "boolean", "optional": true,

"description": "Print background graphics. Defaults to false."},

printBackground

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

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.

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Thanks for the review and fix!

PTAL.

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode438
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/headless_printing_unittest.cc
File headless/lib/browser/headless_printing_unittest.cc (right):

https://codereview.chromium.org/2829973002/diff/240001/headless/lib/browser/headless_printing_unittest.cc#newcode81
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/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/240001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode436
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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

Still looking into why the skia public_deps entry from content/ isn't
propagating.

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2829973002/diff/220001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode438
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."

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

I changed to use the //skia dep, which seems to work.

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

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?

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

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#newcode412
- 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.

https://codereview.chromium.org/2829973002/

[the...@chromium.org]

Otherwise, lgtm

https://codereview.chromium.org/2829973002/

[jzf...@chromium.org]

Sounds good. Done.

Thanks!

https://codereview.chromium.org/2829973002/

[jzf...@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!

https://codereview.chromium.org/2829973002/

[blun...@chromium.org]

//components top-level lgtm

https://codereview.chromium.org/2829973002/

[skyostil@chromium.org via codereview.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#newcode322
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.

https://codereview.chromium.org/2829973002/

[we...@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#newcode322
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/

[the...@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#newcode322
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.

https://codereview.chromium.org/2829973002/

[we...@chromium.org]

got it, upstream probably doesn't have to expose skia types. The current
solution lgtm.

https://codereview.chromium.org/2829973002/

[pfel...@chromium.org]

devtools protocol lgtm

https://codereview.chromium.org/2829973002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2829973002/320001

https://codereview.chromium.org/2829973002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/3d83dad2db679c3924ffe74599cfd48a03211778

https://codereview.chromium.org/2829973002/

[blun...@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#newcode322
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?

https://codereview.chromium.org/2829973002/

[jzf...@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#newcode322
headless/BUILD.gn:322: public_deps += [ "//skia" ]

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.

https://codereview.chromium.org/2829973002/

[blun...@chromium.org]

Can you detail exactly why this is the case?

[jzf...@chromium.org]

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

[blun...@chromium.org]

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/

https://codereview.chromium.org/2829973002/