Print Preview: Handling pending print to pdf requests.

chrome/browser/resources/print_preview/print_preview.js

Index: chrome/browser/resources/print_preview/print_preview.js
diff --git a/chrome/browser/resources/print_preview/print_preview.js b/chrome/browser/resources/print_preview/print_preview.js
index b73f6b91197ddca018ddcbf220b551e5e3962d6f..c977f096039280b5198da40f71b3ac75c90b5b15 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -73,6 +73,10 @@ var colorSettings;
 // 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;
@@ -435,11 +439,11 @@ function requestToPrintDocument() {
 
   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;
   }
 
@@ -539,6 +543,18 @@ function fileSelectionCancelled() {
 }
 
 /**
+ * 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
@@ -823,9 +839,10 @@ function setColor(color) {
 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);
@@ -881,7 +898,7 @@ function onPDFLoad() {
   }
   $('pdf-viewer').fitToHeight();
   cr.dispatchSimpleEvent(document, 'PDFLoaded');
-  hideLoadingAnimation();
+  hideOverlayLayer();
 }
 
 function setPluginPreviewPageCount() {
@@ -926,7 +943,7 @@ function reloadPreviewPages(previewUid, previewResponseId) {
   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);