Print Preview: Handling pending print to pdf requests.

chrome/browser/ui/webui/print_preview_handler.cc

Index: chrome/browser/ui/webui/print_preview_handler.cc
diff --git a/chrome/browser/ui/webui/print_preview_handler.cc b/chrome/browser/ui/webui/print_preview_handler.cc
index 9d233c40dcb29cf77d3368c869440c0f054c971b..2127acdee85348140706a6f965434494672f8113 100644
--- a/chrome/browser/ui/webui/print_preview_handler.cc
+++ b/chrome/browser/ui/webui/print_preview_handler.cc
@@ -18,7 +18,6 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/utf_string_conversions.h"
-#include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/printing/background_printing_manager.h"
@@ -608,26 +607,7 @@ void PrintPreviewHandler::HandlePrint(const ListValue* args) {
     args->GetString(1, &print_ticket);
     SendCloudPrintJob(*settings, print_ticket);
   } else if (print_to_pdf) {
-    ReportUserActionHistogram(PRINT_TO_PDF);
-    UMA_HISTOGRAM_COUNTS("PrintPreview.PageCount.PrintToPDF",
-                         GetPageCountFromSettingsDictionary(*settings));
-
-    // Pre-populating select file dialog with print job title.
-    string16 print_job_title_utf16 =
-        preview_tab_wrapper()->print_view_manager()->RenderSourceName();
-
-#if defined(OS_WIN)
-    FilePath::StringType print_job_title(print_job_title_utf16);
-#elif defined(OS_POSIX)
-    FilePath::StringType print_job_title = UTF16ToUTF8(print_job_title_utf16);
-#endif
-
-    file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_');
-    FilePath default_filename(print_job_title);
-    default_filename =
-        default_filename.ReplaceExtension(FILE_PATH_LITERAL("pdf"));
-
-    SelectFile(default_filename);
+    HandlePrintToPdf(settings.get());
   } else {
     ReportPrintSettingsStats(*settings);
     ReportUserActionHistogram(PRINT_TO_PRINTER);
@@ -649,6 +629,35 @@ void PrintPreviewHandler::HandlePrint(const ListValue* args) {
   }
 }
 
+void PrintPreviewHandler::HandlePrintToPdf(const DictionaryValue* settings) {

[vandebo (ex-Chrome), 2011/09/07 21:38:19]

Will this method be called exactly once?  If not, the UMA stuff needs to go into
one of the if conditions so that it only gets called once.

[dpapad, 2011/09/07 23:42:39]

Good catch. This function will be called twice in the case of pending print to
pdf requests. Only the second condition is guaranteed to be called once. Moved
UMA code there.

+  ReportUserActionHistogram(PRINT_TO_PDF);
+  UMA_HISTOGRAM_COUNTS("PrintPreview.PageCount.PrintToPDF",
+                       GetPageCountFromSettingsDictionary(*settings));
+
+  if (pending_print_to_pdf_path_.get()) {
+    // User has already selected a path, no need to show the dialog again.
+    PostPrintToPdfTask(*pending_print_to_pdf_path_.get());

[vandebo (ex-Chrome), 2011/09/07 21:38:19]

Can you explain how we would get to this point, I don't see an additional call
in the JS code.

[dpapad, 2011/09/07 23:42:39]

In chrome/browser/resources/print_preview/print_header.js::onPrintButtonClicked
requestToPrintDocument() is being called. Then, in
print_preview.js::updatePrintPreview requestToPrintDocument() is called for the
2nd time (if there is a pending print request).

+  } else if (!select_file_dialog_.get() || !select_file_dialog_->IsRunning(
+        platform_util::GetTopLevel(preview_tab()->GetNativeView()))) {
+    // Pre-populating select file dialog with print job title.
+    string16 print_job_title_utf16 =
+        preview_tab_wrapper()->print_view_manager()->RenderSourceName();
+
+#if defined(OS_WIN)
+    FilePath::StringType print_job_title(print_job_title_utf16);
+#elif defined(OS_POSIX)
+    FilePath::StringType print_job_title = UTF16ToUTF8(print_job_title_utf16);
+#endif
+
+    file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_');
+    FilePath default_filename(print_job_title);
+    default_filename =
+        default_filename.ReplaceExtension(FILE_PATH_LITERAL("pdf"));
+
+    SelectFile(default_filename);
+  }
+}
+
 void PrintPreviewHandler::HandleHidePreview(const ListValue* /*args*/) {
   HidePreviewTab();
 }
@@ -960,24 +969,30 @@ void PrintPreviewHandler::ShowSystemDialog() {
 
 void PrintPreviewHandler::FileSelected(const FilePath& path,
                                        int index, void* params) {
+  // Updating last_saved_path_ to the newly selected folder.
+  *last_saved_path_ = path.DirName();

[kmadhusu, 2011/09/07 20:42:57]

I think you need the save the path name only after we complete the save task. In
this function, the user has just selected that path. Preview workflow can fail
before generating the complete preview document. 

[dpapad, 2011/09/07 23:42:39]

At this point the user has demonstrated that he wants to save to |path|. If the
preview fails he will most probably reopen the page and try to save to the same
path, why have him selecting it again?

[kmadhusu, 2011/09/08 00:04:33]

ok.. leave the code as it is.

+
   PrintPreviewUI* print_preview_ui = static_cast<PrintPreviewUI*>(web_ui_);
+  print_preview_ui->OnFileSelectionCompleted();

[kmadhusu, 2011/09/07 20:42:57]

You can remove the changes in print_preview_ui.h and .cc and call
web_ui_->CallJavascript......(); from here.

[dpapad, 2011/09/07 23:42:39]

Done.

   scoped_refptr<RefCountedBytes> data;
   print_preview_ui->GetPrintPreviewDataForIndex(
       printing::COMPLETE_PREVIEW_DOCUMENT_INDEX, &data);
   if (!data.get()) {
-    NOTREACHED();
-    return;
+    pending_print_to_pdf_path_.reset(new FilePath(path));

[kmadhusu, 2011/09/07 20:42:57]

How about renaming pending_print_to_pdf_path_ => print_to_pdf_path_ and to store
the user selected path irrespective of whether the data exists or not. By doing
that you need not pass "path" parameter in PostPrintToPdfTask()?

[vandebo (ex-Chrome), 2011/09/07 21:38:19]

Then you could also free print_to_pdf_path_ in PostPrintToPdfTask as well.

[dpapad, 2011/09/07 23:42:39]

Done.

[dpapad, 2011/09/07 23:42:39]

Done. print_to_pdf_path_ is a scoped_ptr already. Is there a benefit from
releasing the pointer manually?

+  } else {
+    PostPrintToPdfTask(path);
   }
+}
 
+void PrintPreviewHandler::PostPrintToPdfTask(const FilePath& path) {
+  PrintPreviewUI* print_preview_ui = static_cast<PrintPreviewUI*>(web_ui_);
+  scoped_refptr<RefCountedBytes> data;
+  print_preview_ui->GetPrintPreviewDataForIndex(
+      printing::COMPLETE_PREVIEW_DOCUMENT_INDEX, &data);

[kmadhusu, 2011/09/07 20:42:57]

Add a dcheck to validate data.

[dpapad, 2011/09/07 23:42:39]

Done.

   printing::PreviewMetafile* metafile = new printing::PreviewMetafile;
   metafile->InitFromData(static_cast<const void*>(data->front()), data->size());
-
-  // Updating last_saved_path_ to the newly selected folder.
-  *last_saved_path_ = path.DirName();
-
   PrintToPdfTask* task = new PrintToPdfTask(metafile, path);
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, task);
-
   ActivateInitiatorTabAndClosePreviewTab();
 }