PrintPreview: Preview generation should not block print button.

chrome/browser/resources/print_preview.js

Index: chrome/browser/resources/print_preview.js
diff --git a/chrome/browser/resources/print_preview.js b/chrome/browser/resources/print_preview.js
index 7118615fcf1abd994fc2ab2ca60befd4356448ce..defa6e4f575214b1431db855113e545985558f07 100644
--- a/chrome/browser/resources/print_preview.js
+++ b/chrome/browser/resources/print_preview.js
@@ -30,6 +30,18 @@ const MANAGE_PRINTERS = 'Manage Printers';
 // State of the print preview settings.
 var printSettings = new PrintSettings();
 
+// True when a pending print preview request exists.
+var hasPendingPreviewRequest = false;
+
+// True when a pending print file request exists.
+var hasPendingPrintFileRequest = false;
+
+// True when a compatible plugin exists.
+var hasCompatiblePlugin = true;
+
+// True when initiator tab is closed.
+var isInitiatorTabClosed = false;
+
 /**
  * Window onload handler, sets up the page and starts print preview by getting
  * the printer list.
@@ -39,6 +51,8 @@ function onLoad() {
   $('cancel-button').addEventListener('click', handleCancelButtonClick);
 
   if (!checkCompatiblePluginExists()) {
+    hasCompatiblePlugin = false;
+    $('print-button').disabled = true;
     displayErrorMessage(localStrings.getString('noPlugin'), false);
     $('mainview').parentElement.removeChild($('dummy-viewer'));
     return;
@@ -46,17 +60,72 @@ function onLoad() {
   $('mainview').parentElement.removeChild($('dummy-viewer'));
 
   $('printer-list').disabled = true;
-  $('print-button').disabled = true;
+  $('print-button').onclick = printFile;
+
+  setDefaultHandlersForCopiesControls();
+  setDefaultHandlersForPagesControls();
   showLoadingAnimation();
   chrome.send('getDefaultPrinter');
 }
 
 /**
+ * Handles all pages checkbox click event.
+ */
+function handleAllPagesCheckbox() {
+  updatePrintButtonState();
+}
+
+/**
+ * Validates the individual pages text format.
+ */
+function validateIndividualPagesText() {
+  $('print-pages').checked = true;
+  validatePageRangesField();
+  updatePrintButtonState();
+}
+
+/**
+ * Handles the individual pages input event.
+ */
+function handleIndividualPagesInputEvent() {
+  $('print-pages').checked = true;
+  resetPageRangeFieldTimer();
+}
+
+/**
+ * Sets the default event handlers for pages controls.
+ */
+function setDefaultHandlersForPagesControls() {
+  var allPages = $('all-pages');
+  var printPages = $('print-pages');
+  var individualPages = $('individual-pages');
+  individualPages.onblur = null;
+  individualPages.onfocus = null;
+
+  if (!hasCompatiblePlugin || isInitiatorTabClosed) {
+    allPages.onclick = null;
+    printPages.onclick = null;
+    individualPages.oninput = null;
+  } else {
+    allPages.onclick = handleAllPagesCheckbox;
+    printPages.onclick = handleIndividualPagesCheckbox;
+    individualPages.oninput = validateIndividualPagesText;

[dpapad, 2011/06/07 20:47:14]

Shouldn't we validate the the page range field only when it loses focus or the
timeout expires? I think this will result in a lot of code being executed in
every single keystroke.

[kmadhusu, 2011/06/08 00:39:15]

Fixed. Changed the behavior to validate on onblur event.

+  }
+}
+
+/**
+ * Sets the default event handlers for copies controls.
+ */
+function setDefaultHandlersForCopiesControls() {
+  $('copies').oninput = copiesFieldChanged;
+  $('increment').onclick = function() { onCopiesButtonsClicked(1); };
+  $('decrement').onclick = function() { onCopiesButtonsClicked(-1); };
+}
+
+/**
  * Adds event listeners to the settings controls.
  */
 function addEventListeners() {
-  $('print-button').onclick = printFile;
-
   // Controls that require preview rendering.
   $('all-pages').onclick = onPageSelectionMayHaveChanged;
   $('print-pages').onclick = handleIndividualPagesCheckbox;
@@ -66,7 +135,7 @@ function addEventListeners() {
       onPageSelectionMayHaveChanged();
   };
   individualPages.onfocus = addTimerToPageRangeField;
-  individualPages.oninput = resetPageRangeFieldTimer;
+  individualPages.oninput = handleIndividualPagesInputEvent;
   $('landscape').onclick = onLayoutModeToggle;
   $('portrait').onclick = onLayoutModeToggle;
   $('printer-list').onchange = updateControlsWithSelectedPrinterCapabilities;
@@ -96,26 +165,19 @@ function addEventListeners() {
  * Removes event listeners from the settings controls.
  */
 function removeEventListeners() {
-  // Controls that require preview rendering.
-  $('print-button').disabled = true;
-  $('all-pages').onclick = null;
-  $('print-pages').onclick = null;
-  var individualPages = $('individual-pages');
-  individualPages.onblur = null;
-  individualPages.onfocus = null;
-  individualPages.oninput = null;
   clearTimeout(timerId);
+
+  // Controls that require preview rendering
+  setDefaultHandlersForPagesControls();
   $('landscape').onclick = null;
   $('portrait').onclick = null;
   $('printer-list').onchange = null;
 
   // Controls that dont require preview rendering.
-  $('copies').oninput = copiesFieldChanged;
   $('two-sided').onclick = null;
   $('color').onclick = null;
   $('bw').onclick = null;
-  $('increment').onclick = function() { onCopiesButtonsClicked(1); };
-  $('decrement').onclick = function() { onCopiesButtonsClicked(-1); };
+  setDefaultHandlersForCopiesControls();
 }
 
 /**
@@ -138,10 +200,12 @@ function showSystemDialog() {
  * @param {string} initiatorTabURL The URL of the initiator tab.
  */
 function onInitiatorTabClosed(initiatorTabURL) {
+  isInitiatorTabClosed = true;
   $('reopen-page').addEventListener('click', function() {
       window.location = initiatorTabURL;
   });
   displayErrorMessage(localStrings.getString('initiatorTabClosed'), true);
+  $('print-button').disabled = true;

[dpapad, 2011/06/07 20:47:14]

Should we move this line within displayErrorMessage? Is there a case when we
display an error but the button should be enabled?

[kmadhusu, 2011/06/08 00:39:15]

Fixed. (Removed line #208 and #55).

 }
 
 /**
@@ -326,6 +390,14 @@ function getSelectedPrinterName() {
  * Asks the browser to print the preview PDF based on current print settings.
  */
 function printFile() {
+  hasPendingPrintFileRequest = hasPendingPreviewRequest ? true : false;

[dpapad, 2011/06/07 20:47:14]

hasPendingPrintFileRequest = hasPendingPreviewRequest;

[kmadhusu, 2011/06/08 00:39:15]

Just for better readability of the code, I used the tri-state format.

+
+  if (hasPendingPrintFileRequest) {
+    if (getSelectedPrinterName() != PRINT_TO_PDF)
+      chrome.send('hidePreview');
+    return;
+  }
+
   if (getSelectedPrinterName() != PRINT_TO_PDF) {
     $('print-button').classList.add('loading');
     $('cancel-button').classList.add('loading');
@@ -333,14 +405,16 @@ function printFile() {
     removeEventListeners();
     window.setTimeout(function() { chrome.send('print', [getSettingsJSON()]); },
                       1000);
-  } else
+  } else {
     chrome.send('print', [getSettingsJSON()]);
+  }
 }
 
 /**
  * Asks the browser to generate a preview PDF based on current print settings.
  */
 function requestPrintPreview() {
+  hasPendingPreviewRequest = true;
   removeEventListeners();
   printSettings.save();
   showLoadingAnimation();
@@ -497,6 +571,8 @@ function updatePrintPreview(pageCount, jobTitle, modifiable, previewUid) {
 
   previewModifiable = modifiable;
 
+  hasPendingPreviewRequest = false;
+
   if (totalPageCount == -1)
     totalPageCount = pageCount;
 
@@ -526,6 +602,12 @@ function updatePrintPreview(pageCount, jobTitle, modifiable, previewUid) {
   document.title = localStrings.getStringF('printPreviewTitleFormat', jobTitle);
 
   createPDFPlugin(previewUid);
+
+  if (hasPendingPrintFileRequest) {
+    printFile();
+    return;
+  }
+
   updatePrintSummary();
   updatePrintButtonState();
   addEventListeners();
@@ -607,13 +689,9 @@ function copiesFieldChanged() {
 }
 
 /**
- * Executes whenever a blur event occurs on the 'individual-pages'
- * field or when the timer expires. It takes care of
- * 1) showing/hiding warnings/suggestions
- * 2) updating print button/summary
+ * Validates the page ranges text and updates the hint accordingly.
  */
-function pageRangesFieldChanged() {
-  var currentlySelectedPages = getSelectedPagesSet();
+function validatePageRangesField() {
   var individualPagesField = $('individual-pages');
   var individualPagesHint = $('individual-pages-hint');
   var validityLevel = getSelectedPagesValidityLevel();
@@ -630,6 +708,16 @@ function pageRangesFieldChanged() {
                                     'examplePageRangeText'));
     fadeInElement(individualPagesHint);
   }
+}
+
+/**
+ * Executes whenever a blur event occurs on the 'individual-pages'
+ * field or when the timer expires. It takes care of
+ * 1) showing/hiding warnings/suggestions
+ * 2) updating print button/summary
+ */
+function pageRangesFieldChanged() {
+  validatePageRangesField();
 
   resetPageRangeFieldTimer();
   updatePrintButtonState();
@@ -810,20 +898,33 @@ function getSelectedPagesValidityLevel() {
       continue;
 
     var match = part.match(/^([0-9]+)-([0-9]*)$/);
-    if (match && match[1]) {
-      var from = parseInt(match[1], 10);
-      var to = match[2] ? parseInt(match[2], 10) : totalPageCount;
-
-      if (from && to && from <= to)
+    if (totalPageCount != -1) {
+      if (match && match[1]) {
+        var from = parseInt(match[1], 10);
+        var to = match[2] ? parseInt(match[2], 10) : totalPageCount;
+
+        if (from && to && from <= to)
+          successfullyParsed += 1;
+        else
+          failedToParse += 1;
+      } else if (isInteger(part) && parseInt(part, 10) <= totalPageCount &&
+                 parseInt(part, 10) > 0) {
         successfullyParsed += 1;
-      else
+      } else {
         failedToParse += 1;
-
-    } else if (isInteger(part) && parseInt(part, 10) <= totalPageCount &&
-               parseInt(part, 10) > 0)
-      successfullyParsed += 1;
-    else
-      failedToParse += 1;
+      }
+    } else {
+      // totalPageCount is -1. Just validate the page range format.
+      if (match && isInteger(match[1])) {
+        if (match[2] && !isInteger(match[2]))
+          failedToParse += 1;
+        successfullyParsed += 1;
+      } else if (isInteger(part)) {

[dpapad, 2011/06/07 20:47:14]

Shouldn't we check also for parseInt(part, 10) > 0 here?

[kmadhusu, 2011/06/08 00:39:15]

Fixed.

+        successfullyParsed += 1;
+      } else {
+        failedToParse += 1;
+      }
+    }
   }
   if (successfullyParsed > 0 && failedToParse == 0)
     return 1;