Convert printing IPCs to Mojo

Convert printing IPCs to Mojo

BUG=577685

[tibell, 2016-11-07 06:25:55]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 06:26:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 06:33:55]

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

[commit-bot: I haz the power, 2016-11-07 06:33:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tibell, 2016-11-08 02:25:19]

Don't conditionally define param traits

[tibell, 2016-11-08 02:25:24]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 02:25:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 02:30:08]

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

[commit-bot: I haz the power, 2016-11-08 02:30:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tibell, 2016-11-08 03:05:12]

Move components/printing/public to components/printing/common

[tibell, 2016-11-08 03:05:16]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 03:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 03:24:39]

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

[commit-bot: I haz the power, 2016-11-08 03:24:40]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[tibell, 2016-11-08 03:31:08]

Remove unused Send function

[tibell, 2016-11-08 03:31:12]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 03:31:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 04:16:23]

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

[commit-bot: I haz the power, 2016-11-08 04:16:24]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tibell, 2016-11-09 03:36:05]

Update Windows-only code

[tibell, 2016-11-09 03:36:09]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 03:36:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 04:17:43]

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

[commit-bot: I haz the power, 2016-11-09 04:17:44]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tibell, 2016-11-09 05:52:56]

Fix Windows compile errors

[tibell, 2016-11-09 05:53:00]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 05:53:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 06:33:38]

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

[commit-bot: I haz the power, 2016-11-09 06:33:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-11-09 22:40:05]

Add missing namespace prefix

[tibell, 2016-11-09 22:40:10]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 22:40:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 23:23:50]

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

[commit-bot: I haz the power, 2016-11-09 23:23:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-12-11 23:51:45]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 23:51:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 23:53:49]

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

[commit-bot: I haz the power, 2016-12-11 23:53:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tibell, 2016-12-11 23:55:23]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 23:55:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 23:55:49]

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

[commit-bot: I haz the power, 2016-12-11 23:55:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2016-12-11 23:58:13]

Merge

[tibell, 2016-12-11 23:58:45]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 23:58:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 00:12:23]

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

[commit-bot: I haz the power, 2016-12-12 00:12:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tibell, 2016-12-12 00:12:36]

common_custom_types.mojom moved

[tibell, 2016-12-12 00:12:47]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 00:12:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-12 00:18:58]

Left-over merge

[tibell, 2016-12-12 00:19:06]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 00:19:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-12 00:35:18]

Add global font pre cacher needed for pdf callback

[tibell, 2016-12-12 00:35:24]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 00:35:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 01:30:12]

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

[commit-bot: I haz the power, 2016-12-12 01:30:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tibell, 2016-12-12 02:13:34]

Add missing win-only dep

[tibell, 2016-12-12 02:13:39]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 02:13:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-12 02:31:40]

Add more win-only deps

[tibell, 2016-12-12 02:31:46]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 02:31:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 03:52:15]

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

[commit-bot: I haz the power, 2016-12-12 03:52:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tibell, 2016-12-12 04:37:01]

git cl format

[tibell, 2016-12-12 04:37:07]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 04:37:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-12 04:47:19]

tibell@chromium.org changed reviewers:
+ gene@chromium.org, mbarbella@google.com, sammc@chromium.org

[tibell, 2016-12-12 04:47:20]

This moves printing IPCs to Mojo (we're converting all old IPCs to Mojo).

Sam, please review all Mojo related changes and changes to mojo/

Gene, please review changes to printing code (or let me know if someone else
would be more appropriate).

Martin, please check IPC changes for security.

[tibell, 2016-12-12 05:30:12]

Description was changed from

==========
Convert printing IPCs to Mojo

BUG=
==========

to

==========
Convert printing IPCs to Mojo
==========

[commit-bot: I haz the power, 2016-12-12 05:34:12]

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

[commit-bot: I haz the power, 2016-12-12 05:34:13]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-12-12 05:38:21]

tibell@chromium.org changed reviewers:
+ mbarbella@chromium.org
- mbarbella@google.com

[Sam McNally, 2016-12-12 06:09:20]

https://codereview.chromium.org/2477283002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/260001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:107: class
PdfToEmfUtilityProcessHostClient
Consider using UtilityProcessMojoClient to abstract utility process lifetime
management.

https://codereview.chromium.org/2477283002/diff/260001/chrome/service/service...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2477283002/diff/260001/chrome/service/service...
chrome/service/service_utility_process_host.h:119: printing::mojom::PrintingPtr
printing_;
Why is this protected?

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
File components/printing/common/printing.mojom (right):

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
components/printing/common/printing.mojom:41: handle input_pdf_file,
Use mojo.common.mojom.File for passing files around.

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
components/printing/common/printing.mojom:79: RenderPDFPagesToMetafilesStop();
Why not use the closing of the connection to indicate that it should stop?

https://codereview.chromium.org/2477283002/diff/260001/content/common/typemap...
File content/common/typemaps_win.gni (right):

https://codereview.chromium.org/2477283002/diff/260001/content/common/typemap...
content/common/typemaps_win.gni:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
Remove this file?

[tibell, 2016-12-13 23:37:46]

Address sammc's review comments

[tibell, 2016-12-13 23:37:50]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 23:38:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-13 23:41:55]

PTAL

https://codereview.chromium.org/2477283002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/260001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:107: class
PdfToEmfUtilityProcessHostClient
On 2016/12/12 06:09:20, Sam McNally wrote:
> Consider using UtilityProcessMojoClient to abstract utility process lifetime
> management.

Done.

https://codereview.chromium.org/2477283002/diff/260001/chrome/service/service...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2477283002/diff/260001/chrome/service/service...
chrome/service/service_utility_process_host.h:119: printing::mojom::PrintingPtr
printing_;
On 2016/12/12 06:09:20, Sam McNally wrote:
> Why is this protected?

Done.

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
File components/printing/common/printing.mojom (right):

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
components/printing/common/printing.mojom:41: handle input_pdf_file,
On 2016/12/12 06:09:20, Sam McNally wrote:
> Use mojo.common.mojom.File for passing files around.

Done.

https://codereview.chromium.org/2477283002/diff/260001/components/printing/co...
components/printing/common/printing.mojom:79: RenderPDFPagesToMetafilesStop();
On 2016/12/12 06:09:20, Sam McNally wrote:
> Why not use the closing of the connection to indicate that it should stop?

Done.

https://codereview.chromium.org/2477283002/diff/260001/content/common/typemap...
File content/common/typemaps_win.gni (right):

https://codereview.chromium.org/2477283002/diff/260001/content/common/typemap...
content/common/typemaps_win.gni:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/12/12 06:09:20, Sam McNally wrote:
> Remove this file?

Done.

Initially I thought of following the pattern in
chromium_bindings_configuration.gni but the extra layer of files seemed
unnecessary.

[commit-bot: I haz the power, 2016-12-14 00:29:52]

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

[commit-bot: I haz the power, 2016-12-14 00:29:53]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tibell, 2016-12-14 00:35:35]

Change callback arg

[tibell, 2016-12-14 00:35:39]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 00:36:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 01:30:20]

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

[commit-bot: I haz the power, 2016-12-14 01:30:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-12-14 02:45:31]

Keep PdfToEmfUtilityProcessHostClient ref-counted

[tibell, 2016-12-14 02:45:37]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 02:46:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:35:16]

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

[commit-bot: I haz the power, 2016-12-14 03:35:17]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-12-14 04:43:14]

Fix windows compile error

[tibell, 2016-12-14 04:43:23]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 04:43:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 05:49:58]

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

[commit-bot: I haz the power, 2016-12-14 05:49:59]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2016-12-14 17:28:28]

groby@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2016-12-14 17:28:29]

Thank you for tackling this!

Added a few quick drive-by comments.

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
chrome/service/service_utility_process_host.cc:319: 
Does this still need to be an IPC::Listener?

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
chrome/service/service_utility_process_host.cc:453: // We don't care about calls
to FontPreCaching so we pass a null pointer for
This comment seems out of date?

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
File chrome/utility/printing_handler.cc (right):

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:65: g_font_pre_caching = nullptr;
I have no idea what the threading implications around the font precaching being
called from pdfium - just to be sure, I'd suggest calling
chrome_pdf::SetPDFEnsureTypefaceCharactersAccessible(nullptr) first. 

(It likely should never be called once this DTOR is hit - if that's true, then
we should DCHECK in PreCacheFontCharacters instead of using the if())

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:68: bool
PrintingHandler::OnMessageReceived(const IPC::Message& message) {
Do we still need this? Technically, this is not a message handler any more, is
it?

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:88: void
PrintingHandler::RenderPDFPagesToMetafiles(
This seems kind of odd from a non-mojo-educated point of view.

We used to have a fairly strict stance from security folk that if a message
doesn't do anything for a given platform, it shouldn't have a handler in the
first place. (Hence the #ifdef OS_WIN)

Have we changed that stance with mojo?

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:306: void
PrintingHandler::GetPrinterSemanticCapsAndDefaults(
Same comment as above - I'd be much happier if the print preview used a separate
mojo interface that simply isn't present if print preview doesn't exist.

[groby-ooo-7-16, 2016-12-14 17:28:30]

groby@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2016-12-14 17:28:31]

Thank you for tackling this!

Added a few quick drive-by comments.

[Vitaly Buka (NO REVIEWS), 2016-12-14 21:28:54]

vitalybuka@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[tibell, 2016-12-14 23:11:39]

Address groby's review comments

[tibell, 2016-12-14 23:11:43]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[tibell, 2016-12-14 23:12:18]

tibell@chromium.org changed reviewers:
+ dcheng@chromium.org
- mbarbella@chromium.org

[tibell, 2016-12-14 23:12:19]

PTAL

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
chrome/service/service_utility_process_host.cc:319: 
On 2016/12/14 17:28:29, groby wrote:
> Does this still need to be an IPC::Listener?

Unfortunately OnMessageReceived is defined pure virtual (by inheritance) in
ChildProcessHostDelegate, which we inherit from since we use other methods from
that class, so we're forced to define it here.

https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
chrome/service/service_utility_process_host.cc:453: // We don't care about calls
to FontPreCaching so we pass a null pointer for
On 2016/12/14 17:28:29, groby wrote:
> This comment seems out of date?

printing::mojom::FontPreCachingPtr() does create a "null" interface pointer. I
have reworded the comment to make that more clear.

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
File chrome/utility/printing_handler.cc (right):

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:65: g_font_pre_caching = nullptr;
On 2016/12/14 17:28:29, groby wrote:
> I have no idea what the threading implications around the font precaching
being
> called from pdfium - just to be sure, I'd suggest calling
> chrome_pdf::SetPDFEnsureTypefaceCharactersAccessible(nullptr) first. 
> 
> (It likely should never be called once this DTOR is hit - if that's true, then
> we should DCHECK in PreCacheFontCharacters instead of using the if())

Addressed the first paragraph.

I don't know the code well enough (i.e. at all :)) to address the second
paragraph. Could we punt on adding the DCHECK to someone who understands this
code/chrome_pdf better? That would mean that we're keeping whatever the current
behavior is.

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:68: bool
PrintingHandler::OnMessageReceived(const IPC::Message& message) {
On 2016/12/14 17:28:29, groby wrote:
> Do we still need this? Technically, this is not a message handler any more, is
> it?

Done.

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:88: void
PrintingHandler::RenderPDFPagesToMetafiles(
On 2016/12/14 17:28:29, groby wrote:
> This seems kind of odd from a non-mojo-educated point of view.
> 
> We used to have a fairly strict stance from security folk that if a message
> doesn't do anything for a given platform, it shouldn't have a handler in the
> first place. (Hence the #ifdef OS_WIN)
> 
> Have we changed that stance with mojo?

I will bring this up during the Mojo meeting today to see if we can define a
policy.

In the mean time I've changed the NOTREACHED macros to mojo::ReportBadMessage,
which will kill this process and report the error to the caller if a bad message
is received.

Somewhat more philosophically:

Mojo doesn't support conditionally defining methods and I don't think we want it
to (at least not using the C preprocessor). We're trying to move to a service
architecture with coherent services and interfaces and conditional interfaces
can be confusing to use and it's hard to know what it means if the client and
service were compiled on different platforms.

It is possible to conditionally define interfaces (using build system ifs on the
mojom targets) so it is possible to split out OS-specific messages to a
different interface, but see previous paragraph.

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:306: void
PrintingHandler::GetPrinterSemanticCapsAndDefaults(
On 2016/12/14 17:28:29, groby wrote:
> Same comment as above - I'd be much happier if the print preview used a
separate
> mojo interface that simply isn't present if print preview doesn't exist.

If print preview does make sense as a separate interface (regardless of
OS-specific concerns) we should probably split it out. Would such an interface
make sense even if the rest of the printing functionality wasn't available?

[commit-bot: I haz the power, 2016-12-14 23:12:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-14 23:12:59]

tibell@chromium.org changed reviewers:
- gene@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:42:15]

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

[commit-bot: I haz the power, 2016-12-15 00:42:17]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-12-21 03:50:48]

Add TODO for conditionally-defined messages

[tibell, 2016-12-21 03:50:54]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[tibell, 2016-12-21 03:51:03]

On 2016/12/14 23:12:19, tibell wrote:
> PTAL
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
> File chrome/service/service_utility_process_host.cc (right):
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
> chrome/service/service_utility_process_host.cc:319: 
> On 2016/12/14 17:28:29, groby wrote:
> > Does this still need to be an IPC::Listener?
> 
> Unfortunately OnMessageReceived is defined pure virtual (by inheritance) in
> ChildProcessHostDelegate, which we inherit from since we use other methods
from
> that class, so we're forced to define it here.
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/service/service...
> chrome/service/service_utility_process_host.cc:453: // We don't care about
calls
> to FontPreCaching so we pass a null pointer for
> On 2016/12/14 17:28:29, groby wrote:
> > This comment seems out of date?
> 
> printing::mojom::FontPreCachingPtr() does create a "null" interface pointer. I
> have reworded the comment to make that more clear.
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
> File chrome/utility/printing_handler.cc (right):
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
> chrome/utility/printing_handler.cc:65: g_font_pre_caching = nullptr;
> On 2016/12/14 17:28:29, groby wrote:
> > I have no idea what the threading implications around the font precaching
> being
> > called from pdfium - just to be sure, I'd suggest calling
> > chrome_pdf::SetPDFEnsureTypefaceCharactersAccessible(nullptr) first. 
> > 
> > (It likely should never be called once this DTOR is hit - if that's true,
then
> > we should DCHECK in PreCacheFontCharacters instead of using the if())
> 
> Addressed the first paragraph.
> 
> I don't know the code well enough (i.e. at all :)) to address the second
> paragraph. Could we punt on adding the DCHECK to someone who understands this
> code/chrome_pdf better? That would mean that we're keeping whatever the
current
> behavior is.
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
> chrome/utility/printing_handler.cc:68: bool
> PrintingHandler::OnMessageReceived(const IPC::Message& message) {
> On 2016/12/14 17:28:29, groby wrote:
> > Do we still need this? Technically, this is not a message handler any more,
is
> > it?
> 
> Done.
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
> chrome/utility/printing_handler.cc:88: void
> PrintingHandler::RenderPDFPagesToMetafiles(
> On 2016/12/14 17:28:29, groby wrote:
> > This seems kind of odd from a non-mojo-educated point of view.
> > 
> > We used to have a fairly strict stance from security folk that if a message
> > doesn't do anything for a given platform, it shouldn't have a handler in the
> > first place. (Hence the #ifdef OS_WIN)
> > 
> > Have we changed that stance with mojo?
> 
> I will bring this up during the Mojo meeting today to see if we can define a
> policy.
> 
> In the mean time I've changed the NOTREACHED macros to mojo::ReportBadMessage,
> which will kill this process and report the error to the caller if a bad
message
> is received.
> 
> Somewhat more philosophically:
> 
> Mojo doesn't support conditionally defining methods and I don't think we want
it
> to (at least not using the C preprocessor). We're trying to move to a service
> architecture with coherent services and interfaces and conditional interfaces
> can be confusing to use and it's hard to know what it means if the client and
> service were compiled on different platforms.
> 
> It is possible to conditionally define interfaces (using build system ifs on
the
> mojom targets) so it is possible to split out OS-specific messages to a
> different interface, but see previous paragraph.
> 
>
https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
> chrome/utility/printing_handler.cc:306: void
> PrintingHandler::GetPrinterSemanticCapsAndDefaults(
> On 2016/12/14 17:28:29, groby wrote:
> > Same comment as above - I'd be much happier if the print preview used a
> separate
> > mojo interface that simply isn't present if print preview doesn't exist.
> 
> If print preview does make sense as a separate interface (regardless of
> OS-specific concerns) we should probably split it out. Would such an interface
> make sense even if the rest of the printing functionality wasn't available?

PTAL

Added a TODO while we wait for preprocessing support to be implemented in Mojo.
In the mean time I'd like to processed to get this CL submitted.

[commit-bot: I haz the power, 2016-12-21 03:51:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-12-21 03:52:11]

Description was changed from

==========
Convert printing IPCs to Mojo
==========

to

==========
Convert printing IPCs to Mojo

BUG=577685
==========

[tibell, 2016-12-21 03:52:31]

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
File chrome/utility/printing_handler.cc (right):

https://codereview.chromium.org/2477283002/diff/340001/chrome/utility/printin...
chrome/utility/printing_handler.cc:88: void
PrintingHandler::RenderPDFPagesToMetafiles(
On 2016/12/14 23:12:19, tibell wrote:
> On 2016/12/14 17:28:29, groby wrote:
> > This seems kind of odd from a non-mojo-educated point of view.
> > 
> > We used to have a fairly strict stance from security folk that if a message
> > doesn't do anything for a given platform, it shouldn't have a handler in the
> > first place. (Hence the #ifdef OS_WIN)
> > 
> > Have we changed that stance with mojo?
> 
> I will bring this up during the Mojo meeting today to see if we can define a
> policy.
> 
> In the mean time I've changed the NOTREACHED macros to mojo::ReportBadMessage,
> which will kill this process and report the error to the caller if a bad
message
> is received.
> 
> Somewhat more philosophically:
> 
> Mojo doesn't support conditionally defining methods and I don't think we want
it
> to (at least not using the C preprocessor). We're trying to move to a service
> architecture with coherent services and interfaces and conditional interfaces
> can be confusing to use and it's hard to know what it means if the client and
> service were compiled on different platforms.
> 
> It is possible to conditionally define interfaces (using build system ifs on
the
> mojom targets) so it is possible to split out OS-specific messages to a
> different interface, but see previous paragraph.
> 

We've now discussed this
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/BfvXCPBpcUs).
We will add technical support for preprocessing mojoms. Until that has been
implemented I will add a TODO and keep this as is (I took this pattern from
existing platform-specific Mojo code).

[tibell, 2016-12-21 03:56:11]

Merge

[tibell, 2016-12-21 03:56:15]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 03:56:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 04:14:01]

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

[commit-bot: I haz the power, 2016-12-21 04:14:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tibell, 2016-12-21 06:10:59]

Follow mojo::GetProxy rename

[tibell, 2016-12-21 06:11:04]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 06:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 07:13:26]

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

[commit-bot: I haz the power, 2016-12-21 07:13:28]

Dry run: This issue passed the CQ dry run.

[tibell, 2017-01-03 00:16:55]

Friendly ping to all reviewers. I still need your reviews. :)

[Sam McNally, 2017-01-05 06:23:15]

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:321: utility_client_.reset(
Prefer base::MakeUnique over std::unique_ptr::reset().

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:351:
start_callback_.Run(page_count);
base::ResetAndReturn(&start_callback_).Run(page_count);
from base/callback_helpers.h

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:406: void
PdfToEmfUtilityProcessHostClient::PreCacheFontCharacters(
Is the UI thread the right place to be doing this?

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:31: #include
"mojo/public/cpp/system/platform_handle.h"
Remove.

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:200:
utility_process_host->SetName(
I suspect this could also be simplified by using UtilityProcessMojoClient.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/DEPS
File chrome/service/DEPS (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/DEPS#ne...
chrome/service/DEPS:9: "+services/service_manager",
/public

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.cc:39: #include
"mojo/public/cpp/system/platform_handle.h"
Remove.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.h:156: printing::mojom::Printing*
printing();
Methods before fields, and GetPrinting() if you aren't going to inline it.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.h:157: printing::mojom::PrintingPtr
printing_;
Add a comment to use the accessor method instead.

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing.mojom (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Happy new year!

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing.mojom:69: // TODO(tibell): Use a null
|page_count| for failure.
I don't think primitive types are nullable.

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing_param_traits.h (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing_param_traits.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2014?

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing_param_traits_macros.h (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing_param_traits_macros.h:14: #include
"ipc/ipc_platform_file.h"
Remove?

[tibell, 2017-01-12 03:28:37]

Address sammc's review comments

[tibell, 2017-01-12 03:28:42]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 03:28:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2017-01-12 03:30:15]

PTAL

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:321: utility_client_.reset(
On 2017/01/05 06:23:15, Sam McNally wrote:
> Prefer base::MakeUnique over std::unique_ptr::reset().

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:351:
start_callback_.Run(page_count);
On 2017/01/05 06:23:15, Sam McNally wrote:
> base::ResetAndReturn(&start_callback_).Run(page_count);
> from base/callback_helpers.h

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:406: void
PdfToEmfUtilityProcessHostClient::PreCacheFontCharacters(
On 2017/01/05 06:23:15, Sam McNally wrote:
> Is the UI thread the right place to be doing this?

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:31: #include
"mojo/public/cpp/system/platform_handle.h"
On 2017/01/05 06:23:15, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:200:
utility_process_host->SetName(
On 2017/01/05 06:23:15, Sam McNally wrote:
> I suspect this could also be simplified by using UtilityProcessMojoClient.

Added TODO.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/DEPS
File chrome/service/DEPS (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/DEPS#ne...
chrome/service/DEPS:9: "+services/service_manager",
On 2017/01/05 06:23:15, Sam McNally wrote:
> /public

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.cc:39: #include
"mojo/public/cpp/system/platform_handle.h"
On 2017/01/05 06:23:15, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.h:156: printing::mojom::Printing*
printing();
On 2017/01/05 06:23:15, Sam McNally wrote:
> Methods before fields, and GetPrinting() if you aren't going to inline it.

Done.

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.h:157: printing::mojom::PrintingPtr
printing_;
On 2017/01/05 06:23:15, Sam McNally wrote:
> Add a comment to use the accessor method instead.

Done.

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing.mojom (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/01/05 06:23:15, Sam McNally wrote:
> Happy new year!

Done.

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing.mojom:69: // TODO(tibell): Use a null
|page_count| for failure.
On 2017/01/05 06:23:15, Sam McNally wrote:
> I don't think primitive types are nullable.

Done.

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing_param_traits.h (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing_param_traits.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2017/01/05 06:23:15, Sam McNally wrote:
> 2014?

Blast from the past!

(Done)

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
File components/printing/common/printing_param_traits_macros.h (right):

https://codereview.chromium.org/2477283002/diff/410001/components/printing/co...
components/printing/common/printing_param_traits_macros.h:14: #include
"ipc/ipc_platform_file.h"
On 2017/01/05 06:23:15, Sam McNally wrote:
> Remove?

Done.

[commit-bot: I haz the power, 2017-01-12 03:38:03]

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

[commit-bot: I haz the power, 2017-01-12 03:38:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Sam McNally, 2017-01-12 04:49:53]

lgtm

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2477283002/diff/410001/chrome/service/service...
chrome/service/service_utility_process_host.h:157: printing::mojom::PrintingPtr
printing_;
On 2017/01/12 03:30:15, tibell wrote:
> On 2017/01/05 06:23:15, Sam McNally wrote:
> > Add a comment to use the accessor method instead.
> 
> Done.

I don't see it.

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:299: class FontPreCachingImpl :
public mojom::FontPreCaching {
This class seems bit scattered.

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:196: // TODO: Use
UtilityProcessMojoClient to manage process lifetime instead.
TODO(...)

[tibell, 2017-01-12 06:01:52]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 06:02:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 06:49:38]

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

[commit-bot: I haz the power, 2017-01-12 06:49:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Vitaly Buka (NO REVIEWS), 2017-01-12 19:57:59]

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:66: return
pdf_file_.Duplicate();
does this guarantee that files will be deleted when both base::File are closed?

[Vitaly Buka (NO REVIEWS), 2017-01-12 19:58:12]

The CQ bit was checked by vitalybuka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 19:59:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Vitaly Buka (NO REVIEWS), 2017-01-12 19:59:36]

Can you please make win bots green?
I'll try to apply and run the patch.

[commit-bot: I haz the power, 2017-01-12 20:41:16]

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

[commit-bot: I haz the power, 2017-01-12 20:41:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tibell, 2017-01-12 23:39:03]

Fix Windows compilation errors

[tibell, 2017-01-12 23:39:08]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 23:40:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2017-01-13 00:22:45]

Address review comments

[tibell, 2017-01-13 00:22:49]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 00:25:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 01:22:37]

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

[commit-bot: I haz the power, 2017-01-13 01:22:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tibell, 2017-01-13 02:51:10]

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:299: class FontPreCachingImpl :
public mojom::FontPreCaching {
On 2017/01/12 04:49:53, Sam McNally wrote:
> This class seems bit scattered.

Done.

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:66: return
pdf_file_.Duplicate();
On 2017/01/12 19:57:59, Vitaly Buka wrote:
> does this guarantee that files will be deleted when both base::File are
closed?

Done.

https://codereview.chromium.org/2477283002/diff/430001/chrome/browser/printin...
chrome/browser/printing/pwg_raster_converter.cc:196: // TODO: Use
UtilityProcessMojoClient to manage process lifetime instead.
On 2017/01/12 04:49:53, Sam McNally wrote:
> TODO(...)

Done.

[tibell, 2017-01-13 03:18:56]

Fix more Windows compile errors

[tibell, 2017-01-13 03:19:01]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 03:19:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 05:41:40]

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

[commit-bot: I haz the power, 2017-01-13 05:41:41]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-01-17 18:25:14]

Adding rbpotter@ as an FYI (feel free to chime in, though!)

[leonhsl(Using Gerrit), 2017-01-18 03:35:27]

leon.han@intel.com changed reviewers:
+ leon.han@intel.com

[leonhsl(Using Gerrit), 2017-01-18 03:35:29]

https://codereview.chromium.org/2477283002/diff/490001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/490001/chrome/service/service...
chrome/service/service_utility_process_host.cc:451:
child_process_host_->GetRemoteInterfaces()->GetInterface(&printing_factory);
Hi, I came across here while checking some issue around service utility process,
would you please help to clarify my confusion: 
I found service utility process is launched without service manager connection,
it's launched via ChildProcessHostImpl but not BrowserChildProcessHostImpl, this
is different with normal utility process. That is to say, there has no interface
provider/registry pair established between ServiceUtilityProcessHost and
ServiceUtilityProcess, here seems child_process_host_->GetRemoteInterfaces()
would return nullptr?

Thanks!

[leonhsl(Using Gerrit), 2017-01-18 03:38:11]

https://codereview.chromium.org/2477283002/diff/490001/chrome/service/service...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2477283002/diff/490001/chrome/service/service...
chrome/service/service_utility_process_host.cc:451:
child_process_host_->GetRemoteInterfaces()->GetInterface(&printing_factory);
On 2017/01/18 03:35:29, leonhsl wrote:
> Hi, I came across here while checking some issue around service utility
process,
> would you please help to clarify my confusion: 
> I found service utility process is launched without service manager
connection,
> it's launched via ChildProcessHostImpl but not BrowserChildProcessHostImpl,
this
> is different with normal utility process. That is to say, there has no
interface
> provider/registry pair established between ServiceUtilityProcessHost and
> ServiceUtilityProcess, here seems child_process_host_->GetRemoteInterfaces()
> would return nullptr?
> 
> Thanks!

The issue I'm investigating is
https://bugs.chromium.org/p/chromium/issues/detail?id=680400, CL is
https://codereview.chromium.org/2629873002/, that bug's root cause is service
utility process has no service manager connection.

[Vitaly Buka (NO REVIEWS), 2017-01-19 03:18:22]

I patched this my checkout with this patch and can't print.
E.g. printing to Microsoft XPS writer.

[tibell, 2017-01-19 03:43:27]

On 2017/01/19 03:18:22, Vitaly Buka wrote:
> I patched this my checkout with this patch and can't print.
> E.g. printing to Microsoft XPS writer.

Could you please provide repro steps so I can better test this CL? Does testing
it actually require a printer (will corp network printers work)?

[skau, 2017-01-19 03:48:27]

Microsoft XPS means it's on Windows. No printer required. It's like
printing to PDF


On Wed, Jan 18, 2017, 7:43 PM <tibell@chromium.org> wrote:

> On 2017/01/19 03:18:22, Vitaly Buka wrote:
> > I patched this my checkout with this patch and can't print.
> > E.g. printing to Microsoft XPS writer.
>
> Could you please provide repro steps so I can better test this CL? Does
> testing
> it actually require a printer (will corp network printers work)?
>
> https://codereview.chromium.org/2477283002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Vitaly Buka (NO REVIEWS), 2017-01-19 04:01:23]

On 2017/01/19 03:48:27, skau wrote:
> Microsoft XPS means it's on Windows. No printer required. It's like
> printing to PDF
> 

Correct.
1. Run chrome.
2. ctrl+p
3. select "Microsoft XPS..."
4. print
5. now it should ask for output file

with patch: no file created
without patch: file is created

Similar way cloud print needs to be verified:
1. Run chrome.
2. goto chrome:devices
3. Share Microsoft XPS printer
4. ctrl+p
5. Now select Cloud Printer -> "Microsoft XPS..."
6. print
7. now it should ask for output file

[groby-ooo-7-16, 2017-02-15 01:39:30]

On 2017/01/19 04:01:23, Vitaly Buka wrote:
> On 2017/01/19 03:48:27, skau wrote:
> > Microsoft XPS means it's on Windows. No printer required. It's like
> > printing to PDF
> > 
> 
> Correct.
> 1. Run chrome.
> 2. ctrl+p
> 3. select "Microsoft XPS..."
> 4. print
> 5. now it should ask for output file
> 
> with patch: no file created
> without patch: file is created
> 
> Similar way cloud print needs to be verified:
> 1. Run chrome.
> 2. goto chrome:devices
> 3. Share Microsoft XPS printer
> 4. ctrl+p
> 5. Now select Cloud Printer -> "Microsoft XPS..."
> 6. print
> 7. now it should ask for output file

So, is this stalled out?

[dcheng, 2017-02-15 22:05:20]

On 2017/02/15 01:39:30, groby wrote:
> On 2017/01/19 04:01:23, Vitaly Buka wrote:
> > On 2017/01/19 03:48:27, skau wrote:
> > > Microsoft XPS means it's on Windows. No printer required. It's like
> > > printing to PDF
> > > 
> > 
> > Correct.
> > 1. Run chrome.
> > 2. ctrl+p
> > 3. select "Microsoft XPS..."
> > 4. print
> > 5. now it should ask for output file
> > 
> > with patch: no file created
> > without patch: file is created
> > 
> > Similar way cloud print needs to be verified:
> > 1. Run chrome.
> > 2. goto chrome:devices
> > 3. Share Microsoft XPS printer
> > 4. ctrl+p
> > 5. Now select Cloud Printer -> "Microsoft XPS..."
> > 6. print
> > 7. now it should ask for output file
> 
> So, is this stalled out?

On my side, I'm waiting for conditional compilation in mojoms (which I think the
Mojo team plans to implement, but I'm not sure what the status is)

[tibell, 2017-02-15 23:21:52]

On 2017/02/15 01:39:30, groby wrote:
> On 2017/01/19 04:01:23, Vitaly Buka wrote:
> > On 2017/01/19 03:48:27, skau wrote:
> > > Microsoft XPS means it's on Windows. No printer required. It's like
> > > printing to PDF
> > > 
> > 
> > Correct.
> > 1. Run chrome.
> > 2. ctrl+p
> > 3. select "Microsoft XPS..."
> > 4. print
> > 5. now it should ask for output file
> > 
> > with patch: no file created
> > without patch: file is created
> > 
> > Similar way cloud print needs to be verified:
> > 1. Run chrome.
> > 2. goto chrome:devices
> > 3. Share Microsoft XPS printer
> > 4. ctrl+p
> > 5. Now select Cloud Printer -> "Microsoft XPS..."
> > 6. print
> > 7. now it should ask for output file
> 
> So, is this stalled out?

This isn't on the top of my stack right now (I'm working on servicification) but
I'll get back to it. Testing on Windows manually is a bit more work (I have to
switch over to our shared Windows testing machine, patch the CL there, etc.)

[tibell, 2017-02-15 23:22:50]

On 2017/02/15 22:05:20, dcheng wrote:
> On 2017/02/15 01:39:30, groby wrote:
> > On 2017/01/19 04:01:23, Vitaly Buka wrote:
> > > On 2017/01/19 03:48:27, skau wrote:
> > > > Microsoft XPS means it's on Windows. No printer required. It's like
> > > > printing to PDF
> > > > 
> > > 
> > > Correct.
> > > 1. Run chrome.
> > > 2. ctrl+p
> > > 3. select "Microsoft XPS..."
> > > 4. print
> > > 5. now it should ask for output file
> > > 
> > > with patch: no file created
> > > without patch: file is created
> > > 
> > > Similar way cloud print needs to be verified:
> > > 1. Run chrome.
> > > 2. goto chrome:devices
> > > 3. Share Microsoft XPS printer
> > > 4. ctrl+p
> > > 5. Now select Cloud Printer -> "Microsoft XPS..."
> > > 6. print
> > > 7. now it should ask for output file
> > 
> > So, is this stalled out?
> 
> On my side, I'm waiting for conditional compilation in mojoms (which I think
the
> Mojo team plans to implement, but I'm not sure what the status is)

AFAIK no one is working or planning to work on this at the moment. We plan to go
with the existing approach (i.e. what I did in this CL) until someone implements
conditional compilation.

[chromium-reviews, 2017-02-15 23:25:51]

FYI, blundell@ expressed interest in helping with some bindings work.
Besides, he actively participated in our discussion of conditional
compilation. Maybe we could ask whether he has got free cycles?


On Wed, Feb 15, 2017 at 3:22 PM, <tibell@chromium.org> wrote:

> On 2017/02/15 22:05:20, dcheng wrote:
> > On 2017/02/15 01:39:30, groby wrote:
> > > On 2017/01/19 04:01:23, Vitaly Buka wrote:
> > > > On 2017/01/19 03:48:27, skau wrote:
> > > > > Microsoft XPS means it's on Windows. No printer required. It's like
> > > > > printing to PDF
> > > > >
> > > >
> > > > Correct.
> > > > 1. Run chrome.
> > > > 2. ctrl+p
> > > > 3. select "Microsoft XPS..."
> > > > 4. print
> > > > 5. now it should ask for output file
> > > >
> > > > with patch: no file created
> > > > without patch: file is created
> > > >
> > > > Similar way cloud print needs to be verified:
> > > > 1. Run chrome.
> > > > 2. goto chrome:devices
> > > > 3. Share Microsoft XPS printer
> > > > 4. ctrl+p
> > > > 5. Now select Cloud Printer -> "Microsoft XPS..."
> > > > 6. print
> > > > 7. now it should ask for output file
> > >
> > > So, is this stalled out?
> >
> > On my side, I'm waiting for conditional compilation in mojoms (which I
> think
> the
> > Mojo team plans to implement, but I'm not sure what the status is)
>
> AFAIK no one is working or planning to work on this at the moment. We plan
> to go
> with the existing approach (i.e. what I did in this CL) until someone
> implements
> conditional compilation.
>
> https://codereview.chromium.org/2477283002/
>



-- 
Best regards,
Yuzhu Shen.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2017-02-16 00:49:38]

On 2017/02/15 23:22:50, tibell wrote:
> On 2017/02/15 22:05:20, dcheng wrote:
> > On 2017/02/15 01:39:30, groby wrote:
> > > On 2017/01/19 04:01:23, Vitaly Buka wrote:
> > > > On 2017/01/19 03:48:27, skau wrote:
> > > > > Microsoft XPS means it's on Windows. No printer required. It's like
> > > > > printing to PDF
> > > > > 
> > > > 
> > > > Correct.
> > > > 1. Run chrome.
> > > > 2. ctrl+p
> > > > 3. select "Microsoft XPS..."
> > > > 4. print
> > > > 5. now it should ask for output file
> > > > 
> > > > with patch: no file created
> > > > without patch: file is created
> > > > 
> > > > Similar way cloud print needs to be verified:
> > > > 1. Run chrome.
> > > > 2. goto chrome:devices
> > > > 3. Share Microsoft XPS printer
> > > > 4. ctrl+p
> > > > 5. Now select Cloud Printer -> "Microsoft XPS..."
> > > > 6. print
> > > > 7. now it should ask for output file
> > > 
> > > So, is this stalled out?
> > 
> > On my side, I'm waiting for conditional compilation in mojoms (which I think
> the
> > Mojo team plans to implement, but I'm not sure what the status is)
> 
> AFAIK no one is working or planning to work on this at the moment. We plan to
go
> with the existing approach (i.e. what I did in this CL) until someone
implements
> conditional compilation.

That means I can stop feeling guilty about having this in my review list :)