Refactor win printing workflow.

chrome/renderer/print_web_view_helper_win.cc

Index: chrome/renderer/print_web_view_helper_win.cc
diff --git a/chrome/renderer/print_web_view_helper_win.cc b/chrome/renderer/print_web_view_helper_win.cc
index 286e801610071d418fa2ce693ec073432377ca4a..2e90ce5e8dfce585f1216aef4137dfb3ee175007 100644
--- a/chrome/renderer/print_web_view_helper_win.cc
+++ b/chrome/renderer/print_web_view_helper_win.cc
@@ -1,10 +1,11 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/print_web_view_helper.h"
 
 #include "base/logging.h"
+#include "base/process_util.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/render_messages_params.h"
 #include "chrome/renderer/render_view.h"
@@ -66,12 +67,11 @@ void PrintWebViewHelper::PrintPage(const ViewMsg_PrintPage_Params& params,
                                    const gfx::Size& canvas_size,
                                    WebFrame* frame) {
   // Generate a memory-based metafile. It will use the current screen's DPI.
-  printing::NativeMetafile metafile;
-
-  metafile.CreateDc(NULL, NULL);
-  HDC hdc = metafile.hdc();
-  DCHECK(hdc);
-  skia::PlatformDevice::InitializeDC(hdc);
+  // Each metafile contains a single page.
+  scoped_ptr<printing::NativeMetafile> metafile(new printing::NativeMetafile());
+  metafile->CreateDc(NULL, NULL);
+  DCHECK(metafile->hdc());
+  skia::PlatformDevice::InitializeDC(metafile->hdc());
 
   int page_number = params.page_number;
 
@@ -88,10 +88,45 @@ void PrintWebViewHelper::PrintPage(const ViewMsg_PrintPage_Params& params,
   // Since WebKit extends the page width depending on the magical |scale_factor|
   // we make sure the canvas covers the worst case scenario (x2.0 currently).
   // PrintContext will then set the correct clipping region.
-  int width = static_cast<int>(content_width_in_points *
-                               params.params.max_shrink);
-  int height = static_cast<int>(content_height_in_points *
-                                params.params.max_shrink);
+  gfx::Size page_size(
+      static_cast<int>(content_width_in_points * params.params.max_shrink),
+      static_cast<int>(content_height_in_points * params.params.max_shrink));
+
+  // Render page for printing.
+  RenderPage(page_size, &scale_factor, page_number, frame, &metafile,
+      params.params.supports_alpha_blend);
+
+  // Close the device context to retrieve the compiled metafile.
+  if (!metafile->CloseDc())
+    NOTREACHED();
+
+  ViewHostMsg_DidPrintPage_Params page_params;
+  page_params.data_size = metafile->GetDataSize();
+  page_params.metafile_data_handle = base::SharedMemory::NULLHandle();

[kmadhusu, 2011/01/24 16:23:22]

Removed this code because metafile_data_handle is already initialized to
INVALID_HANDLE_VALUE.

+  page_params.page_number = page_number;
+  page_params.document_cookie = params.params.document_cookie;
+  page_params.actual_shrink = scale_factor;
+  page_params.page_size = params.params.page_size;
+  page_params.content_area = gfx::Rect(params.params.margin_left,
+      params.params.margin_top, params.params.printable_size.width(),
+      params.params.printable_size.height());
+  page_params.has_visible_overlays = frame->isPageBoxVisible(page_number);
+
+  if (CopyMetafileDataToSharedMem(metafile,
+          &(page_params.metafile_data_handle))) {
+    Send(new ViewHostMsg_DidPrintPage(routing_id(), page_params));
+  }
+}
+
+void PrintWebViewHelper::RenderPage(
+    const gfx::Size& page_size, float* scale_factor, int page_number,
+    WebFrame* frame, scoped_ptr<printing::NativeMetafile>* metafile,
+    bool supports_alpha_blend) {
+  HDC hdc = (*metafile)->hdc();
+  DCHECK(hdc);
+
+  int width = page_size.width();
+  int height = page_size.height();
 #if 0
   // TODO(maruel): This code is kept for testing until the 100% GDI drawing
   // code is stable. maruels use this code's output as a reference when the
@@ -101,12 +136,12 @@ void PrintWebViewHelper::PrintPage(const ViewMsg_PrintPage_Params& params,
   skia::PlatformCanvas canvas(width, height, true);
   canvas.drawARGB(255, 255, 255, 255, SkXfermode::kSrc_Mode);
   float webkit_scale_factor = frame->printPage(page_number, &canvas);
-  if (scale_factor <= 0 || webkit_scale_factor <= 0) {
-    NOTREACHED() << "Printing page " << page_number << " failed.";
+  if (*scale_factor <= 0 || webkit_scale_factor <= 0) {
+    NOTREACHED() << "Printing page " << page_number << "failed.";

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

nit: "failed" -> " failed."

[kmadhusu, 2011/01/24 16:23:22]

Done.

   } else {
-    // Update the dpi adjustment with the "page |scale_factor|" calculated
-    // in webkit.
-    scale_factor /= webkit_scale_factor;
+    // Update the dpi adjustment with the "page |scale_factor|" calculated in
+    // webkit.
+    *scale_factor /= webkit_scale_factor;
   }
 
   // Create a BMP v4 header that we can serialize.
@@ -129,26 +164,23 @@ void PrintWebViewHelper::PrintPage(const ViewMsg_PrintPage_Params& params,
   // 100% GDI based.
   skia::VectorCanvas canvas(hdc, width, height);
   float webkit_scale_factor = frame->printPage(page_number, &canvas);
-  if (scale_factor <= 0 || webkit_scale_factor <= 0) {
-    NOTREACHED() << "Printing page " << page_number << " failed.";
+  if (*scale_factor <= 0 || webkit_scale_factor <= 0) {
+    NOTREACHED() << "Printing page " << page_number << "failed";

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

And here

[kmadhusu, 2011/01/24 16:23:22]

Done.

   } else {
-    // Update the dpi adjustment with the "page scale_factor" calculated
-    // in webkit.
-    scale_factor /= webkit_scale_factor;
+    // Update the dpi adjustment with the "page |scale_factor|" calculated in
+    // webkit.
+    *scale_factor /= webkit_scale_factor;
   }
 #endif
 
-  // Done printing. Close the device context to retrieve the compiled metafile.
-  if (!metafile.CloseDc()) {
-    NOTREACHED() << "metafile failed";
-  }
-  printing::NativeMetafile* mf = &metafile;
-  printing::NativeMetafile metafile2;
-
   skia::VectorPlatformDevice* platform_device =
-    static_cast<skia::VectorPlatformDevice*>(canvas.getDevice());
-  if (platform_device->alpha_blend_used() &&
-      !params.params.supports_alpha_blend) {
+      static_cast<skia::VectorPlatformDevice*>(canvas.getDevice());
+  if (platform_device->alpha_blend_used() && !supports_alpha_blend) {
+    // Close the device context to retrieve the compiled metafile.
+    if (!(*metafile)->CloseDc())
+      NOTREACHED();
+
+    printing::NativeMetafile* metafile2(new printing::NativeMetafile());

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

scoped_ptr<>

[kmadhusu, 2011/01/24 16:23:22]

Done.

     // Page used alpha blend, but printer doesn't support it.  Rewrite the
     // metafile and flatten out the transparency.
     HDC bitmap_dc = CreateCompatibleDC(GetDC(NULL));
@@ -170,72 +202,57 @@ void PrintWebViewHelper::PrintPage(const ViewMsg_PrintPage_Params& params,
     HBRUSH whiteBrush = static_cast<HBRUSH>(GetStockObject(WHITE_BRUSH));
     FillRect(bitmap_dc, &rect, whiteBrush);
 
-    metafile2.CreateDc(NULL, NULL);
-    HDC hdc = metafile2.hdc();
+    metafile2->CreateDc(NULL, NULL);
+    HDC hdc = metafile2->hdc();
     DCHECK(hdc);
     skia::PlatformDevice::InitializeDC(hdc);
 
-    RECT metafile_bounds = metafile.GetBounds().ToRECT();
+    RECT metafile_bounds = (*metafile)->GetBounds().ToRECT();
     // Process the old metafile, placing all non-AlphaBlend calls into the
     // new metafile, and copying the results of all the AlphaBlend calls
     // from the bitmap DC.
     EnumEnhMetaFile(hdc,
-                    metafile.emf(),
+                    (*metafile)->emf(),
                     EnhMetaFileProc,
                     &bitmap_dc,
                     &metafile_bounds);
 
     SelectObject(bitmap_dc, old_bitmap);
-
-    if (!metafile2.CloseDc()) {
-      NOTREACHED() << "metafile failed";
-    }
-    mf = &metafile2;
+    (*metafile)->CloseEmf();
+    metafile->reset(metafile2);

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

to go with the scoped_ptr<>:
metafile->reset(metafile2->release());

[kmadhusu, 2011/01/24 16:23:22]

Done.

   }
+}
 
-  // Get the size of the compiled metafile.
-  uint32 buf_size = mf->GetDataSize();
-  DCHECK_GT(buf_size, 128u);
-  ViewHostMsg_DidPrintPage_Params page_params;
-  page_params.data_size = 0;
-  page_params.metafile_data_handle = NULL;
-  page_params.page_number = page_number;
-  page_params.document_cookie = params.params.document_cookie;
-  page_params.actual_shrink = scale_factor;
-  page_params.page_size = params.params.page_size;
-  page_params.content_area = gfx::Rect(params.params.margin_left,
-      params.params.margin_top, params.params.printable_size.width(),
-      params.params.printable_size.height());
-  page_params.has_visible_overlays = frame->isPageBoxVisible(page_number);
-  base::SharedMemory shared_buf;
-
+bool PrintWebViewHelper::CopyMetafileDataToSharedMem(
+    scoped_ptr<printing::NativeMetafile>& metafile,
+    base::SharedMemoryHandle* shared_mem_handle) {
+  uint32 buf_size = metafile->GetDataSize();
   // http://msdn2.microsoft.com/en-us/library/ms535522.aspx
   // Windows 2000/XP: When a page in a spooled file exceeds approximately 350
   // MB, it can fail to print and not send an error message.
-  if (buf_size < 350*1024*1024) {
-    // Allocate a shared memory buffer to hold the generated metafile data.
-    if (shared_buf.CreateAndMapAnonymous(buf_size)) {
-      // Copy the bits into shared memory.
-      if (mf->GetData(shared_buf.memory(), buf_size)) {
-        page_params.metafile_data_handle = shared_buf.handle();
-        page_params.data_size = buf_size;
-      } else {
-        NOTREACHED() << "GetData() failed";
-      }
-      shared_buf.Unmap();
-    } else {
-      NOTREACHED() << "Buffer allocation failed";
-    }
-  } else {
+  if (buf_size > 350*1024*1024) {
     NOTREACHED() << "Buffer too large: " << buf_size;
+    metafile->CloseEmf();
+    return false;

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

The old code Sends the DuplicateSection call even if it is too big.  Is it a
no-op without metafile_data_handle or something?

[kmadhusu, 2011/01/24 16:23:22]

metafile_data_handle is initialized to "INVALID_HANDLE_VALUE". Even though the
|buf_size| is too big, we resend the "INVALID_HANDLE_VALUE" to browser process
which handles this case.

   }
-  mf->CloseEmf();
-  if (Send(new ViewHostMsg_DuplicateSection(
-      routing_id(),
-      page_params.metafile_data_handle,
-      &page_params.metafile_data_handle))) {
-    if (!is_preview_) {
-      Send(new ViewHostMsg_DidPrintPage(routing_id(), page_params));
-    }
+
+  base::SharedMemory shared_buf;
+  // Allocate a shared memory buffer to hold the generated metafile data.
+  if (!shared_buf.CreateAndMapAnonymous(buf_size)) {
+    NOTREACHED() << "Buffer allocation failed";
+    return false;

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

You're missing the CloseEmf() here.  This comment and the previous makes me
think that maybe the old control flow structure was for the best.

[kmadhusu, 2011/01/24 16:23:22]

Reverted to the old control flow.

   }
+
+  // Copy the bits into shared memory.
+  if (metafile->GetData(shared_buf.memory(), buf_size)) {
+    shared_buf.GiveToProcess(base::GetCurrentProcessHandle(),

[vandebo (ex-Chrome), 2011/01/21 21:53:28]

Will this cause the shared mem handle to leak?  You could pass in the the
shared_buf instead of creating it in this method.

[kmadhusu, 2011/01/24 16:23:22]

No. I confirmed this with brettw@chromium.org. 

+                             shared_mem_handle);
+  } else {
+    NOTREACHED() << "GetData() failed";
+  }
+  shared_buf.Unmap();
+  metafile->CloseEmf();
+
+  return (Send(new ViewHostMsg_DuplicateSection(routing_id(),
+               *shared_mem_handle, shared_mem_handle)));
 }