Improvements in --debug-print switch implementation.

Improvements in --debug-print switch implementation.

* Renamed print_settings_initializer.* into print_settings_conversion.*
* Dump on FILE thread.
* Added PrintSettingsToJobSettingsDebug to dump PrintSettings.
* Added PrintedDocument::CreateDebugDumpPath for consistent dump file naming.
* Dump PDFs received from renderer process.
* Moved GetCustomMarginsFromJobSettings into print_settings_conversion.cc.
* Moved Parsing PageRanges into print_settings_conversion.*

BUG=379961
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276716

[Noam Samuel, 2014-06-11 18:35:23]

lgtm

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:257: }
Does this only dump PDFS on Windows? Maybe it makes sense to dump PDFs on all
platforms?

(Feel free to ignore this comment if it's too much work/not needed)

[Lei Zhang, 2014-06-11 20:03:06]

lgtm

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:256:
base::Bind(&WriteBytesOnFileThread, debug_dump_path, bytes));
You can probably Bind() to &base::WriteFile with IgnoreResult() directly.

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn#newco...
printing/BUILD.gn:52: "print_settings_conversion.cc",
also sort in ascii order

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn#newco...
printing/BUILD.gn:56: "print_settings_initializer.cc",
remove these two files here too

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
printing/printed_document.cc:206: filename =
base::TimeFormatShortDateAndTime(now);
Might be clear to StringPrintf() this.

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
File printing/printed_document.h (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
printing/printed_document.h:49: // requests to have this page be rendered and
returns false.
update comment

https://codereview.chromium.org/329683002/diff/160001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printing.gyp#n...
printing/printing.gyp:66: 'print_settings_conversion.cc',
nit: in ascii '_' comes after '.'

[Vitaly Buka (NO REVIEWS), 2014-06-11 21:32:58]

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:256:
base::Bind(&WriteBytesOnFileThread, debug_dump_path, bytes));
On 2014/06/11 20:03:05, Lei Zhang wrote:
> You can probably Bind() to &base::WriteFile with IgnoreResult() directly.

Moved writing into document.

https://codereview.chromium.org/329683002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:257: }
On 2014/06/11 18:35:23, Noam Samuel wrote:
> Does this only dump PDFS on Windows? Maybe it makes sense to dump PDFs on all
> platforms?
> 
> (Feel free to ignore this comment if it's too much work/not needed)

Other platforms dumped pdfs inside of SetPage above.
Code is moved to GetPage, because it called on FILE thread.

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn#newco...
printing/BUILD.gn:52: "print_settings_conversion.cc",
On 2014/06/11 20:03:05, Lei Zhang wrote:
> also sort in ascii order

Done.

https://codereview.chromium.org/329683002/diff/160001/printing/BUILD.gn#newco...
printing/BUILD.gn:56: "print_settings_initializer.cc",
On 2014/06/11 20:03:05, Lei Zhang wrote:
> remove these two files here too

Done.

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
printing/printed_document.cc:206: filename =
base::TimeFormatShortDateAndTime(now);
On 2014/06/11 20:03:06, Lei Zhang wrote:
> Might be clear to StringPrintf() this.

You mean document name?
We have not StringPrintf for string16 on Android, and document name is string16

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
File printing/printed_document.h (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printed_docume...
printing/printed_document.h:49: // requests to have this page be rendered and
returns false.
On 2014/06/11 20:03:06, Lei Zhang wrote:
> update comment

Done.

https://codereview.chromium.org/329683002/diff/160001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/329683002/diff/160001/printing/printing.gyp#n...
printing/printing.gyp:66: 'print_settings_conversion.cc',
On 2014/06/11 20:03:06, Lei Zhang wrote:
> nit: in ascii '_' comes after '.'

Done.

[Vitaly Buka (NO REVIEWS), 2014-06-11 21:33:44]

Moved more dumping code into printed_document.cc.
Please take a look.

[Lei Zhang, 2014-06-11 21:43:42]

still lgtm

https://codereview.chromium.org/329683002/diff/200001/chrome/browser/printing...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/329683002/diff/200001/chrome/browser/printing...
chrome/browser/printing/print_job.cc:77:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
Can you use BrowserThread::GetBlockingPool() instead?

https://codereview.chromium.org/329683002/diff/200001/printing/printed_docume...
File printing/printed_document.h (right):

https://codereview.chromium.org/329683002/diff/200001/printing/printed_docume...
printing/printed_document.h:107: // Dump data on FILE thread if debug dumps
enabled.
- Update the comment if you use GetBlockingPool().
- Mention whether |extension| requires a '.' or not. FilePath::Extension()
returns ".foo" FWIW.

[Vitaly Buka (NO REVIEWS), 2014-06-11 22:22:58]

https://codereview.chromium.org/329683002/diff/200001/chrome/browser/printing...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/329683002/diff/200001/chrome/browser/printing...
chrome/browser/printing/print_job.cc:77:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
On 2014/06/11 21:43:42, Lei Zhang wrote:
> Can you use BrowserThread::GetBlockingPool() instead?

Done.

https://codereview.chromium.org/329683002/diff/200001/printing/printed_docume...
File printing/printed_document.h (right):

https://codereview.chromium.org/329683002/diff/200001/printing/printed_docume...
printing/printed_document.h:107: // Dump data on FILE thread if debug dumps
enabled.
On 2014/06/11 21:43:42, Lei Zhang wrote:
> - Update the comment if you use GetBlockingPool().
> - Mention whether |extension| requires a '.' or not. FilePath::Extension()
> returns ".foo" FWIW.

Done.

[Vitaly Buka (NO REVIEWS), 2014-06-11 22:23:05]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-11 22:23:19]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-11 22:26:38]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:27:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/240001

[commit-bot: I haz the power, 2014-06-11 23:02:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15722)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18855)

[commit-bot: I haz the power, 2014-06-11 23:04:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 23:04:42]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15757)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18889)

[Vitaly Buka (NO REVIEWS), 2014-06-12 00:15:24]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:17:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/260001

[Vitaly Buka (NO REVIEWS), 2014-06-12 00:44:35]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 00:45:55]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 00:46:01]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 00:50:44]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:51:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/260001

[Vitaly Buka (NO REVIEWS), 2014-06-12 07:14:28]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 08:10:56]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-12 08:13:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/320001

[Vitaly Buka (NO REVIEWS), 2014-06-12 09:30:44]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 09:54:51]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-12 09:56:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/380001

[Vitaly Buka (NO REVIEWS), 2014-06-12 16:50:21]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-06-12 16:50:28]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:51:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/329683002/380001

[commit-bot: I haz the power, 2014-06-12 16:57:05]

Change committed as 276716