Print Preview: Handling pending print to pdf requests.

chrome/browser/resources/print_preview/print_preview.js

 // 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.
 
 // require: cr/ui/print_preview_cloud.js
 
 var localStrings = new LocalStrings();
 
 // If useCloudPrint is true we attempt to connect to cloud print
 // and populate the list of printers with cloud print printers.
@@ skipping 55 matching lines @@
 // Object holding all the header footer related settings.
 var headerFooterSettings;
 
 // Object holding all the color related settings.
 var colorSettings;
 
 // True if the user has click 'Advanced...' in order to open the system print
 // dialog.
 var showingSystemDialog = false;
 
+// True when there is a pending request to print to pdf. It means that the user
+// has requested to save to pdf but the pdf file is not ready yet.
+var hasPendingPrintToPdfRequest = false;

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

This variable is not required. You can reuse  "hasPendingPrintDocumentRequest"
to achieve the same thing. Please correct me if I am wrong. 

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

I am using this variable so that I can keep displaying the "Printing to Pdf"
message, when the first page arrives (if necessary), otherwise the page would be
displayed.
It seems that you are right, but want to try some cases to make sure it is safe
to do this. Will address this along with my next batch of comments (addressing
also arv's comments).

[dpapad, 2011/09/08 03:29:20]

Done.

+
 // The range of options in the printer dropdown controlled by cloud print.
 var firstCloudPrintOptionPos = 0;
 var lastCloudPrintOptionPos = firstCloudPrintOptionPos;
 
 // Store the current previewUid.
 var currentPreviewUid = '';
 
 // True if we need to generate draft preview data.
 var generateDraftData = true;
 
@@ skipping 341 matching lines @@
 /**
  * Asks the browser to print the preview PDF based on current print
  * settings. If the preview is still loading, printPendingFile() will get
  * called once the preview loads.
  */
 function requestToPrintDocument() {
   hasPendingPrintDocumentRequest = !isPrintReadyMetafileReady;
   var printToPDF = getSelectedPrinterName() == PRINT_TO_PDF;
 
   if (hasPendingPrintDocumentRequest) {
     if (printToPDF) {

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

You need to disable the input controls as soon as the user clicks print button.
With this CL, you are disabling the controls only after the user selected the
file name.

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

See my next comment.

-      // TODO(thestig) disable controls here.
-     } else {
-       isTabHidden = true;
-       chrome.send('hidePreview');
-     }
+      sendPrintDocumentRequest();
+    } else {
+      isTabHidden = true;
+      chrome.send('hidePreview');
+    }
     return;
   }
 
   if (printToPDF) {
     sendPrintDocumentRequest();
   } else {
     window.setTimeout(function() { sendPrintDocumentRequest(); }, 1000);
   }
 }
 
@@ skipping 79 matching lines @@
 
 /**
  * Called from PrintPreviewUI::OnFileSelectionCancelled to notify the print
  * preview tab regarding the file selection cancel event.
  */
 function fileSelectionCancelled() {
   // TODO(thestig) re-enable controls here.
 }
 
 /**
+ * Called from PrintPreviewUI::OnFileSelectionCompleted to notify the print
+ * preview tab regarding the file selection completed event.
+ */
+function fileSelectionCompleted() {
+  // If the file selection is completed and the tab is not already closed it
+  // means that a pending print to pdf request exists.
+  disableInputElementsInSidebar();

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

Move the disableInput..() function call to line #442.

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

If I disable the controls at line 442, and then the user hits cancel in the file
dialog, I need to re-enable them (this also adds UI clutter/flickering). The
file dialog is modal so when it is shown the controls are not reachable (no
point in disabling them). I believe it is better to keep it as it is now. They
only need to be disabled after the user clicks "save" in the dialog and before
the tab is closed. Let me know if you have any objections.

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

I am not sure about the time it takes to show the "Save As.." dialog after the
print button is clicked. If that is instant, this is fine. If not, we need to
fix this. I dont want the user to change the settings after the print button is
clicked and before the file dialog is displayed. 

If the file selection is cancelled, we can re-enable the controls in 
fileSelectionCancelled() function. Why do you think the re-enable of controls
will cause a flickering? It should not be a problem.

[dpapad, 2011/09/08 00:11:51]

The main point of this CL is to make opening the file dialog instant and hiding
implementation details from the user. It is instant as far as my eye can tell.
Pls try locally and confirm.

When disabling the controls they become grayed out (and vice versa when
enabling) which in the past has been perceived as flickering from the UI leads
and from other users, so it is best to avoid it if not absolutely necessary.

+  hasPendingPrintToPdfRequest = true;
+  showCustomMessage(localStrings.getString('printingToPDFInProgress'));
+}
+
+/**
  * Set the default printer. If there is one, generate a print preview.
  * @param {string} printer Name of the default printer. Empty if none.
  * @param {string} cloudPrintData Cloud print related data to restore if
  *                 the default printer is a cloud printer.
  */
 function setDefaultPrinter(printer_name, cloudPrintData) {
   // Add a placeholder value so the printer list looks valid.
   addDestinationListOption('', '', true, true, true);
   if (printer_name) {
     defaultOrLastUsedPrinterName = printer_name;
@@ skipping 264 matching lines @@
   cloudprint.setColor(printerList[printerList.selectedIndex], color);
 }
 
 /**
  * Display an error message in the center of the preview area.
  * @param {string} errorMessage The error message to be displayed.
  */
 function displayErrorMessage(errorMessage) {
   $('print-button').disabled = true;
   $('overlay-layer').classList.remove('invisible');
-  $('dancing-dots-text').classList.add('hidden');
-  $('error-text').innerHTML = errorMessage;
-  $('error-text').classList.remove('hidden');
+  $('custom-message').innerHTML = errorMessage;

[arv (Not doing code reviews), 2011/09/07 21:53:52]

Add a var so you do not have to call $ multiple times

[arv (Not doing code reviews), 2011/09/07 21:53:52]

Do you really need innerHTML here? Are you escaping things correctly?

Maybe use textContent instead?

[dpapad, 2011/09/08 03:29:20]

Done.

[dpapad, 2011/09/08 03:29:20]

Done.

+  $('custom-message').classList.remove('hidden');

[arv (Not doing code reviews), 2011/09/07 21:53:52]

$('custom-message').hidden = false;

[dpapad, 2011/09/08 03:29:20]

Done.

+  $('custom-message-with-dots').innerHTML = '';

[arv (Not doing code reviews), 2011/09/07 21:53:52]

textContent = ''

[dpapad, 2011/09/08 03:29:20]

In this case innerHTML is needed since the contents have html tags (span).

+  $('custom-message-with-dots').classList.add('hidden');
   var pdfViewer = $('pdf-viewer');
   if (pdfViewer)
     $('mainview').removeChild(pdfViewer);
 
   if (isTabHidden)
     cancelPendingPrintRequest();
 }
 
 /**
  * Display an error message in the center of the preview area followed by a
@@ skipping 35 matching lines @@
 
 /**
  * Called when the PDF plugin loads its document.
  */
 function onPDFLoad() {
   if (previewModifiable) {
     setPluginPreviewPageCount();
   }
   $('pdf-viewer').fitToHeight();
   cr.dispatchSimpleEvent(document, 'PDFLoaded');
-  hideLoadingAnimation();
+  hideOverlayLayer();
 }
 
 function setPluginPreviewPageCount() {
   $('pdf-viewer').printPreviewPageCount(
       pageSettings.previouslySelectedPages.length);
 }
 
 /**
  * Update the page count and check the page range.
  * Called from PrintPreviewUI::OnDidGetPreviewPageCount().
@@ skipping 24 matching lines @@
  * @param {number} previewResponseId The preview request id that resulted in
  *     this response.
  */
 function reloadPreviewPages(previewUid, previewResponseId) {
   if (!isExpectedPreviewResponse(previewResponseId))
     return;
   hasPendingPreviewRequest = false;
   isPrintReadyMetafileReady = true;
 
   cr.dispatchSimpleEvent(document, 'updatePrintButton');
-  hideLoadingAnimation();
+  hideOverlayLayer();
   var pageSet = pageSettings.previouslySelectedPages;
   for (var i = 0; i < pageSet.length; i++)
     $('pdf-viewer').loadPreviewPage(getPageSrcURL(previewUid, pageSet[i]-1), i);
   // TODO(dpapad): handle pending print file requests.
 }
 
 /**
  * Notification that a print preview page has been rendered.
  * Check if the settings have changed and request a regeneration if needed.
  * Called from PrintPreviewUI::OnDidPreviewPage().
@@ skipping 161 matching lines @@
 <include src="print_preview_animations.js"/>
 <include src="print_preview_cloud.js"/>
 <include src="print_preview_utils.js"/>
 <include src="print_header.js"/>
 <include src="page_settings.js"/>
 <include src="copies_settings.js"/>
 <include src="header_footer_settings.js"/>
 <include src="layout_settings.js"/>
 <include src="color_settings.js"/>
 <include src="margin_settings.js"/>