add customized printing setting for headless

headless/lib/browser/headless_devtools_manager_delegate.cc

Index: headless/lib/browser/headless_devtools_manager_delegate.cc
diff --git a/headless/lib/browser/headless_devtools_manager_delegate.cc b/headless/lib/browser/headless_devtools_manager_delegate.cc
index 040b8207c9e89b1fae5ff68455756533ce756c14..814bf756d41daf87a22feeb1c2b4fb5ff755833c 100644
--- a/headless/lib/browser/headless_devtools_manager_delegate.cc
+++ b/headless/lib/browser/headless_devtools_manager_delegate.cc
@@ -7,6 +7,8 @@
 #include <string>
 #include <utility>
 
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_frontend_host.h"
@@ -73,6 +75,85 @@ std::unique_ptr<base::DictionaryValue> CreateInvalidParamResponse(
 }
 
 #if BUILDFLAG(ENABLE_BASIC_PRINTING)
+std::unique_ptr<base::DictionaryValue> ParsePrintSettings(

[Lei Zhang, 2017/04/20 08:35:58]

Now that you have written a parser, you probably want unit tests. Parsers love
testing.

[jzfeng, 2017/04/27 06:56:06]

Done. Added headless_printing_unittest.cc to do all the tests.

+    int command_id,
+    const base::DictionaryValue* params,
+    printing::HeadlessPrintSettings* settings) {
+  params->GetInteger("dpi", &settings->dpi);

[Eric Seckler, 2017/04/20 09:15:28]

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

[jzfeng, 2017/04/27 06:56:06]

Done.

+  params->GetBoolean("landscape", &settings->landscape);
+  params->GetBoolean("displayHeaderFooter", &settings->display_header_footer);
+  params->GetBoolean("printBackgrounds", &settings->should_print_backgrounds);
+  params->GetDouble("scale", &settings->scale);
+  if (settings->scale < 0)

[Lei Zhang, 2017/04/20 08:35:58]

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.

[jzfeng, 2017/04/27 06:56:06]

Done.

+    return CreateInvalidParamResponse(command_id, "scale");
+
+  std::string paper_type;
+  if (params->GetString("paperType", &paper_type)) {
+    if (paper_type == "letter")

[Lei Zhang, 2017/04/20 08:35:58]

Maybe use the same names as
https://developers.google.com/cloud-print/docs/cdd#cdd ? e.g. NA_LETTER, ISO_A4.

[jzfeng, 2017/04/27 06:56:05]

Done.

+      settings->paper_type = printing::HeadlessPrintSettings::LETTER;
+    else if (paper_type == "legal")
+      settings->paper_type = printing::HeadlessPrintSettings::LEGAL;
+    else if (paper_type == "A4")
+      settings->paper_type = printing::HeadlessPrintSettings::A4;
+    else if (paper_type == "A3")
+      settings->paper_type = printing::HeadlessPrintSettings::A3;
+    else
+      return CreateInvalidParamResponse(command_id, "paperType");
+  }
+
+  std::string margin_type;
+  if (params->GetString("marginType", &margin_type)) {
+    if (margin_type == "defaultMargin") {
+      settings->margin_type = printing::DEFAULT_MARGINS;
+    } else if (margin_type == "noMargin") {
+      settings->margin_type = printing::NO_MARGINS;
+    } else if (margin_type == "customMargin") {
+      settings->margin_type = printing::CUSTOM_MARGINS;
+      if (!params->GetDouble("marginTop", &settings->margin_top_in_inch))

[Lei Zhang, 2017/04/20 08:35:58]

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.

[jzfeng, 2017/04/27 06:56:06]

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.

+        return CreateInvalidParamResponse(command_id, "marginTop");
+      if (!params->GetDouble("marginBottom",
+                             &settings->margin_bottom_in_inch)) {
+        return CreateInvalidParamResponse(command_id, "marginBottom");
+      }
+      if (!params->GetDouble("marginLeft", &settings->margin_left_in_inch))
+        return CreateInvalidParamResponse(command_id, "marginLeft");
+      if (!params->GetDouble("marginRight", &settings->margin_right_in_inch)) {
+        return CreateInvalidParamResponse(command_id, "marginRight");
+      }
+    } else {

[Lei Zhang, 2017/04/20 08:35:58]

Did you leave out PRINTABLE_AREA_MARGINS on purpose?

[jzfeng, 2017/04/27 06:56:06]

Yeah. Now I further restrict the type to be DEFAULT_MARGINS and CUSTOM_MARGINS.

+      return CreateInvalidParamResponse(command_id, "marginType");
+    }
+  }
+
+  std::string page_ranges;
+  if (params->GetString("pageRanges", &page_ranges)) {

[Lei Zhang, 2017/04/20 09:15:05]

If you want to get fancy, see pageRangeTextToPageRanges() in
chrome/browser/resources/print_preview/print_preview_utils.js.

[jzfeng, 2017/04/27 06:56:06]

Re-implement the logic in headless_print_manager.cc. It does the same thing as
the one in the js file now.

+    for (const auto& range_string :
+         base::SplitStringPiece(page_ranges, ",", base::TRIM_WHITESPACE,
+                                base::SPLIT_WANT_NONEMPTY)) {
+      auto tokens = base::SplitStringPiece(
+          range_string, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+      if (tokens.size() > 2 || tokens.empty())
+        return CreateInvalidParamResponse(command_id, "pageRanges");
+      printing::PageRange range;
+      base::StringToInt(tokens[0], &range.from);

[Lei Zhang, 2017/04/20 08:35:58]

StringToInt() comes with a 16 line comment about its return value. Do you care
about "non-perfect" conversions?

[jzfeng, 2017/04/27 06:56:06]

Added unit test for it.

+      if (tokens.size() == 2)
+        base::StringToInt(tokens[1], &range.to);
+      else
+        range.to = range.from;
+      if (range.from < 1 || range.from > range.to)
+        return CreateInvalidParamResponse(command_id, "pageRanges");
+
+      // Page numbers are 1-based in the dictionary.
+      // Page numbers are 0-based for the print settings.
+      range.from--;
+      range.to--;
+      settings->page_ranges.push_back(range);
+    }
+  }
+
+  return nullptr;
+}
+
 void PDFCreated(
     const content::DevToolsManagerDelegate::CommandCallback& callback,
     int command_id,
@@ -189,14 +270,26 @@ void HeadlessDevToolsManagerDelegate::PrintToPDF(
     int command_id,
     const base::DictionaryValue* params,
     const CommandCallback& callback) {
+  DCHECK(callback);
+
 #if BUILDFLAG(ENABLE_BASIC_PRINTING)
+#if !BUILDFLAG(ENABLE_PRINT_PREVIEW)
+  LOG(WARNING)
+      << "Print preview is not enabled. Some print settings may not work.";

[Lei Zhang, 2017/04/20 08:35:58]

Which settings don't work? Is it just headers+footers? I'll give you
PrintWebViewHelper feedback based on this.

[jzfeng, 2017/04/27 06:56:06]

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.

[Lei Zhang, 2017/04/27 22:17:46]

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.

[Eric Seckler, 2017/04/28 08:13:06]

If this works, would we be able to avoid adding the locale paks, too? :)

[jzfeng, 2017/05/02 07:50:55]

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.

+#endif
   content::WebContents* web_contents = agent_host->GetWebContents();
   content::RenderFrameHost* rfh = web_contents->GetMainFrame();
 
+  printing::HeadlessPrintSettings settings;
+  auto response = ParsePrintSettings(command_id, params, &settings);

[Lei Zhang, 2017/04/20 08:35:58]

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.

[Lei Zhang, 2017/04/20 08:53:12]

See https://google.github.io/styleguide/cppguide.html#auto

[jzfeng, 2017/04/27 06:56:06]

Done.

+  if (response) {
+    callback.Run(std::move(response));
+    return;
+  }
   printing::HeadlessPrintManager::FromWebContents(web_contents)
-      ->GetPDFContents(rfh, base::Bind(&PDFCreated, callback, command_id));
+      ->GetPDFContents(rfh, settings,
+                       base::Bind(&PDFCreated, callback, command_id));
 #else
-  DCHECK(callback);
   callback.Run(CreateErrorResponse(command_id, kErrorServerError,
                                    "Printing is not enabled"));
 #endif