Convert printing IPCs to Mojo

chrome/utility/printing_handler.cc

Index: chrome/utility/printing_handler.cc
diff --git a/chrome/utility/printing_handler.cc b/chrome/utility/printing_handler.cc
index 7dbae1a132df5b38ba8e615c28545bf39148fbd2..8afc0228a3fd36c45d6b77ac2dc4015cbd5b6e8e 100644
--- a/chrome/utility/printing_handler.cc
+++ b/chrome/utility/printing_handler.cc
@@ -10,10 +10,11 @@
 #include "base/files/file_util.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_paths.h"
-#include "chrome/common/chrome_utility_printing_messages.h"
 #include "chrome/utility/cloud_print/bitmap_image.h"
 #include "chrome/utility/cloud_print/pwg_encoder.h"
 #include "content/public/utility/utility_thread.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/system/platform_handle.h"
 #include "pdf/pdf.h"
 #include "printing/features/features.h"
 #include "printing/page_range.h"
@@ -33,104 +34,112 @@ namespace printing {
 
 namespace {
 
-bool Send(IPC::Message* message) {
-  return content::UtilityThread::Get()->Send(message);
-}
-
 void ReleaseProcessIfNeeded() {
   content::UtilityThread::Get()->ReleaseProcessIfNeeded();
 }
 
+static mojom::FontPreCaching* g_font_pre_caching;
+
 #if defined(OS_WIN)
 void PreCacheFontCharacters(const LOGFONT* logfont,
                             const wchar_t* text,
                             size_t text_length) {
-  Send(new ChromeUtilityHostMsg_PreCacheFontCharacters(
-      *logfont, base::string16(text, text_length)));
+  if (g_font_pre_caching) {
+    g_font_pre_caching->PreCacheFontCharacters(
+        *logfont, base::string16(text, text_length));
+  }
 }
 #endif
 
 }  // namespace
 
-PrintingHandler::PrintingHandler() {
+PrintingHandler::PrintingHandler(mojom::FontPreCachingPtr font_pre_caching)
+    : font_pre_caching_(std::move(font_pre_caching)) {
+  g_font_pre_caching = font_pre_caching_.get();
 #if defined(OS_WIN)
   chrome_pdf::SetPDFEnsureTypefaceCharactersAccessible(PreCacheFontCharacters);
 #endif
 }
 
-PrintingHandler::~PrintingHandler() {}
+PrintingHandler::~PrintingHandler() {
+  g_font_pre_caching = nullptr;

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

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())

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

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.

+}
 
 bool PrintingHandler::OnMessageReceived(const IPC::Message& message) {

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

Do we still need this? Technically, this is not a message handler any more, is
it?

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

Done.

-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(PrintingHandler, message)
-#if defined(OS_WIN)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_RenderPDFPagesToMetafiles,
-                        OnRenderPDFPagesToMetafile)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_RenderPDFPagesToMetafiles_GetPage,
-                        OnRenderPDFPagesToMetafileGetPage)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_RenderPDFPagesToMetafiles_Stop,
-                        OnRenderPDFPagesToMetafileStop)
-#endif  // OS_WIN
-#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_RenderPDFPagesToPWGRaster,
-                        OnRenderPDFPagesToPWGRaster)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_GetPrinterCapsAndDefaults,
-                        OnGetPrinterCapsAndDefaults)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_GetPrinterSemanticCapsAndDefaults,
-                        OnGetPrinterSemanticCapsAndDefaults)
-#endif  // ENABLE_PRINT_PREVIEW
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  return false;
 }
 
-#if defined(OS_WIN)
-void PrintingHandler::OnRenderPDFPagesToMetafile(
-    IPC::PlatformFileForTransit pdf_transit,
+void PrintingHandler::PrintingHandlerFactory::MakePrinting(
+    mojom::PrintingRequest request,
+    mojom::FontPreCachingPtr font_pre_caching) {
+  auto impl = base::MakeUnique<PrintingHandler>(std::move(font_pre_caching));
+  base::Closure error_handler = base::Bind(&PrintingHandler::OnConnectionClosed,
+                                           base::Unretained(impl.get()));
+  auto binding = mojo::MakeStrongBinding(std::move(impl), std::move(request));
+  binding->set_connection_error_handler(error_handler);
+}
+
+// static
+void PrintingHandler::Create(mojom::PrintingFactoryRequest request) {
+  mojo::MakeStrongBinding(base::MakeUnique<PrintingHandlerFactory>(),
+                          std::move(request));
+}
+
+void PrintingHandler::RenderPDFPagesToMetafiles(

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

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?

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

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.

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

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

+    base::File pdf_file,
     const PdfRenderSettings& settings,
-    bool print_text_with_gdi) {
+    bool print_text_with_gdi,
+    const RenderPDFPagesToMetafilesCallback& callback) {
+#if defined(OS_WIN)
   pdf_rendering_settings_ = settings;
   chrome_pdf::SetPDFUseGDIPrinting(print_text_with_gdi);
-  base::File pdf_file = IPC::PlatformFileForTransitToFile(pdf_transit);
   int page_count = LoadPDF(std::move(pdf_file));
-  Send(
-      new ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_PageCount(page_count));
+  callback.Run(page_count);
+#else
+  NOTREACHED();
+#endif
 }
 
-void PrintingHandler::OnRenderPDFPagesToMetafileGetPage(
+void PrintingHandler::RenderPDFPagesToMetafilesGetPage(
     int page_number,
-    IPC::PlatformFileForTransit output_file) {
-  base::File emf_file = IPC::PlatformFileForTransitToFile(output_file);
+    base::File emf_file,
+    const RenderPDFPagesToMetafilesGetPageCallback& callback) {
+#if defined(OS_WIN)
   float scale_factor = 1.0f;
   bool success =
       RenderPdfPageToMetafile(page_number, std::move(emf_file), &scale_factor);
-  Send(new ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_PageDone(
-      success, scale_factor));
+  callback.Run(success, scale_factor);
+#else
+  NOTREACHED();
+#endif
 }
 
-void PrintingHandler::OnRenderPDFPagesToMetafileStop() {
+void PrintingHandler::OnConnectionClosed() {
+#if defined(OS_WIN)
   ReleaseProcessIfNeeded();
+#else
+  NOTREACHED();
+#endif
 }
 
-#endif  // OS_WIN
-
-#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-void PrintingHandler::OnRenderPDFPagesToPWGRaster(
-    IPC::PlatformFileForTransit pdf_transit,
+void PrintingHandler::RenderPDFPagesToPWGRaster(
+    base::File pdf,
     const PdfRenderSettings& settings,
     const PwgRasterSettings& bitmap_settings,
-    IPC::PlatformFileForTransit bitmap_transit) {
-  base::File pdf = IPC::PlatformFileForTransitToFile(pdf_transit);
-  base::File bitmap = IPC::PlatformFileForTransitToFile(bitmap_transit);
+    base::File bitmap,
+    const RenderPDFPagesToPWGRasterCallback& callback) {
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   if (RenderPDFPagesToPWGRaster(std::move(pdf), settings, bitmap_settings,
                                 std::move(bitmap))) {
-    Send(new ChromeUtilityHostMsg_RenderPDFPagesToPWGRaster_Succeeded());
+    callback.Run(true);
   } else {
-    Send(new ChromeUtilityHostMsg_RenderPDFPagesToPWGRaster_Failed());
+    callback.Run(false);
   }
   ReleaseProcessIfNeeded();
-}
+#else
+  NOTREACHED();
 #endif  // ENABLE_PRINT_PREVIEW
+}
 
 #if defined(OS_WIN)
 int PrintingHandler::LoadPDF(base::File pdf_file) {
@@ -270,9 +279,12 @@ bool PrintingHandler::RenderPDFPagesToPWGRaster(
   }
   return true;
 }
+#endif  // ENABLE_PRINT_PREVIEW
 
-void PrintingHandler::OnGetPrinterCapsAndDefaults(
-    const std::string& printer_name) {
+void PrintingHandler::GetPrinterCapsAndDefaults(
+    const std::string& printer_name,
+    const GetPrinterCapsAndDefaultsCallback& callback) {
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   scoped_refptr<PrintBackend> print_backend =
       PrintBackend::CreateInstance(nullptr);
   PrinterCapsAndDefaults printer_info;
@@ -281,17 +293,20 @@ void PrintingHandler::OnGetPrinterCapsAndDefaults(
       print_backend->GetPrinterDriverInfo(printer_name));
 
   if (print_backend->GetPrinterCapsAndDefaults(printer_name, &printer_info)) {
-    Send(new ChromeUtilityHostMsg_GetPrinterCapsAndDefaults_Succeeded(
-        printer_name, printer_info));
+    callback.Run(printer_info);
   } else  {
-    Send(new ChromeUtilityHostMsg_GetPrinterCapsAndDefaults_Failed(
-        printer_name));
+    callback.Run(base::nullopt);
   }
   ReleaseProcessIfNeeded();
+#else
+  NOTREACHED();
+#endif  // ENABLE_PRINT_PREVIEW
 }
 
-void PrintingHandler::OnGetPrinterSemanticCapsAndDefaults(
-    const std::string& printer_name) {
+void PrintingHandler::GetPrinterSemanticCapsAndDefaults(

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

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.

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

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?

+    const std::string& printer_name,
+    const GetPrinterSemanticCapsAndDefaultsCallback& callback) {
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   scoped_refptr<PrintBackend> print_backend =
       PrintBackend::CreateInstance(nullptr);
   PrinterSemanticCapsAndDefaults printer_info;
@@ -301,14 +316,14 @@ void PrintingHandler::OnGetPrinterSemanticCapsAndDefaults(
 
   if (print_backend->GetPrinterSemanticCapsAndDefaults(printer_name,
                                                        &printer_info)) {
-    Send(new ChromeUtilityHostMsg_GetPrinterSemanticCapsAndDefaults_Succeeded(
-        printer_name, printer_info));
+    callback.Run(printer_info);
   } else {
-    Send(new ChromeUtilityHostMsg_GetPrinterSemanticCapsAndDefaults_Failed(
-        printer_name));
+    callback.Run(base::nullopt);
   }
   ReleaseProcessIfNeeded();
-}
+#else
+  NOTREACHED();
 #endif  // ENABLE_PRINT_PREVIEW
+}
 
 }  // namespace printing