PrintPreview: Preview generation should not block print button.

chrome/browser/resources/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.
 
 var localStrings = new LocalStrings();
 
 // The total page count of the previewed document regardless of which pages the
 // user has selected.
 var totalPageCount = -1;
 
@@ skipping 15 matching lines @@
 // Destination list special value constants.
 const PRINT_TO_PDF = 'Print To PDF';
 const MANAGE_PRINTERS = 'Manage Printers';
 
 // State of the print preview settings.
 var printSettings = new PrintSettings();
 
 // The name of the default or last used printer.
 var defaultOrLastUsedPrinterName = '';
 
+// True when a pending print preview request exists.
+var hasPendingPreviewRequest = false;
+
+// True when a pending print file request exists.
+var hasPendingPrintFileRequest = false;
+
+// True when preview tab has some error.
+var hasError = false;

[kmadhusu, 2011/06/09 20:15:09]

Removed hasCompatiblePlugin and isInitiatorTabClosed variables and replaced them
with hasError variable to handle all preview error cases (Print Preview failed,
Initiator tab closed, no plugin case).

+
 /**
  * Window onload handler, sets up the page and starts print preview by getting
  * the printer list.
  */
 function onLoad() {
   $('system-dialog-link').addEventListener('click', showSystemDialog);
   $('cancel-button').addEventListener('click', handleCancelButtonClick);
 
   if (!checkCompatiblePluginExists()) {
     displayErrorMessage(localStrings.getString('noPlugin'), false);
     $('mainview').parentElement.removeChild($('dummy-viewer'));
     return;
   }
   $('mainview').parentElement.removeChild($('dummy-viewer'));
 
   $('printer-list').disabled = true;
-  $('print-button').disabled = true;
+  $('print-button').onclick = printFile;
+
+  setDefaultHandlersForPagesAndCopiesControls();
   showLoadingAnimation();
   chrome.send('getDefaultPrinter');
 }
 
 /**
+ * Handles the individual pages input event.
+ */
+function handleIndividualPagesInputEvent() {
+  $('print-pages').checked = true;
+  resetPageRangeFieldTimer();
+}
+
+/**
+ * Handles the individual pages blur event.
+ */
+function onPageRangesFieldBlur() {
+  $('print-pages').checked = true;

[dpapad, 2011/06/09 20:32:38]

Since this line exists also in handleIndividualPagesInputEvent() the checkbox
will be checked already (if the user typed anything). If the user just focused
the textfield without typing and then unfocus we should not change the checkbox
state, right?

Imagine a use case where there in no mouse and the user wants all pages selected
and landscape.

[kmadhusu, 2011/06/09 21:27:11]

(just repeating our in-person conversation): This code works as expected in the
specified test case. 

+  validatePageRangesField();
+  updatePrintButtonState();
+}
+
+/**
+ * Sets the default event handlers for pages and copies controls.
+ */
+function setDefaultHandlersForPagesAndCopiesControls() {
+  var allPages = $('all-pages');
+  var printPages = $('print-pages');
+  var individualPages = $('individual-pages');
+
+  allPages.onclick = null;
+  printPages.onclick = null;
+  individualPages.oninput = null;
+  individualPages.onfocus = null;
+  individualPages.onblur = null;
+
+  if (!hasError) {
+    allPages.onclick = updatePrintButtonState;
+    printPages.onclick = handleIndividualPagesCheckbox;
+    individualPages.onblur = onPageRangesFieldBlur;
+  }
+
+  $('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;
   var individualPages = $('individual-pages');
   individualPages.onblur = function() {
       clearTimeout(timerId);
       onPageSelectionMayHaveChanged();
   };
   individualPages.onfocus = addTimerToPageRangeField;
-  individualPages.oninput = resetPageRangeFieldTimer;
+  individualPages.oninput = handleIndividualPagesInputEvent;
   $('landscape').onclick = onLayoutModeToggle;
   $('portrait').onclick = onLayoutModeToggle;
   $('printer-list').onchange = updateControlsWithSelectedPrinterCapabilities;
 
   // Controls that dont require preview rendering.
   $('copies').oninput = function() {
     copiesFieldChanged();
     updatePrintButtonState();
     updatePrintSummary();
   };
   $('two-sided').onclick = handleTwoSidedClick;
   $('color').onclick = function() { setColor(true); };
   $('bw').onclick = function() { setColor(false); };
   $('increment').onclick = function() {
     onCopiesButtonsClicked(1);
     updatePrintButtonState();
     updatePrintSummary();
   };
   $('decrement').onclick = function() {
     onCopiesButtonsClicked(-1);
     updatePrintButtonState();
     updatePrintSummary();
   };
 }
 
 /**
  * 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);
+  setDefaultHandlersForPagesAndCopiesControls();
+
+  // Controls that require preview rendering
   $('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); };
 }
 
 /**
  * Asks the browser to close the preview tab.
  */
 function handleCancelButtonClick() {
   chrome.send('closePrintPreviewTab');
 }
 
 /**
@@ skipping 190 matching lines @@
   var deviceName = '';
   if (selectedPrinter >= 0)
     deviceName = printerList.options[selectedPrinter].value;
   return deviceName;
 }
 
 /**
  * Asks the browser to print the preview PDF based on current print settings.
  */
 function printFile() {
+  hasPendingPrintFileRequest = hasPendingPreviewRequest ? true : false;

[dpapad, 2011/06/09 20:32:38]

I agree with thestig on this, 
"hasPendingPrintFileRequest = hasPendingPreviewRequest;" is readable enough.
Moreover the ternary operator is a two-step operation (compare first then
execute the corresponding branch).

[kmadhusu, 2011/06/09 21:27:11]

Done.

+
+  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');
     $('print-summary').innerHTML = localStrings.getString('printing');
     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();
   chrome.send('getPreview', [getSettingsJSON()]);
 }
 
 /**
  * Set the default printer. If there is one, generate a print preview.
  * @param {string} printer Name of the default printer. Empty if none.
  */
@@ skipping 73 matching lines @@
   var pdfViewer = $('pdf-viewer');
   if (!pdfViewer) {
     return;
   }
   pdfViewer.grayscale(!color);
 }
 
 /**
  * Display an error message in the center of the preview area.
  * @param {string} errorMessage The error message to be displayed.
- * @param {boolean} showButton Indivates whether the "Reopen the page" button
+ * @param {boolean} showButton Indicates whether the "Reopen the page" button
  * should be displayed.
  */
 function displayErrorMessage(errorMessage, showButton) {
+  hasError = true;
+  $('print-button').disabled = true;
   $('overlay-layer').classList.remove('invisible');
   $('dancing-dots-text').classList.add('hidden');
   $('error-text').innerHTML = errorMessage;
   $('error-text').classList.remove('hidden');
   if (showButton)
     $('reopen-page').classList.remove('hidden');
   else
     $('reopen-page').classList.add('hidden');
 
   removeEventListeners();
@@ skipping 35 matching lines @@
  * @param {string} jobTitle The print job title.
  * @param {boolean} modifiable If the preview is modifiable.
  * @param {string} previewUid Preview unique identifier.
  */
 function updatePrintPreview(pageCount, jobTitle, modifiable, previewUid) {
   var tempPrintSettings = new PrintSettings();
   tempPrintSettings.save();
 
   previewModifiable = modifiable;
 
+  hasPendingPreviewRequest = false;
+
   if (totalPageCount == -1)
     totalPageCount = pageCount;
 
   if (previouslySelectedPages.length == 0)
     for (var i = 0; i < totalPageCount; i++)
       previouslySelectedPages.push(i+1);
 
   if (printSettings.deviceName != tempPrintSettings.deviceName) {
     updateControlsWithSelectedPrinterCapabilities();
     return;
   } else if (printSettings.isLandscape != tempPrintSettings.isLandscape) {
     setDefaultValuesAndRegeneratePreview();
     return;
   } else if (isSelectedPagesValid()) {
     var currentlySelectedPages = getSelectedPagesSet();
     if (!areArraysEqual(previouslySelectedPages, currentlySelectedPages)) {
       previouslySelectedPages = currentlySelectedPages;
       requestPrintPreview();
       return;
     }
   }
 
   if (!isSelectedPagesValid())
     pageRangesFieldChanged();
 
   // Update the current tab title.
   document.title = localStrings.getStringF('printPreviewTitleFormat', jobTitle);
 
   createPDFPlugin(previewUid);
+

[dpapad, 2011/06/09 20:32:38]

nit: Remove blank line.

[kmadhusu, 2011/06/09 21:27:11]

Done.

   updatePrintSummary();
   updatePrintButtonState();
   addEventListeners();
+
+  if (hasPendingPrintFileRequest)
+    printFile();
 }
 
 /**
  * Create the PDF plugin or reload the existing one.
  * @param {string} previewUid Preview unique identifier.
  */
 function createPDFPlugin(previewUid) {
   // Enable the print button.
   if (!$('printer-list').disabled) {
     $('print-button').disabled = false;
@@ skipping 58 matching lines @@
 /**
  * Listener function that executes whenever a change occurs in the 'copies'
  * field.
  */
 function copiesFieldChanged() {
   updateCopiesButtonsState();
   $('collate-option').hidden = getCopies() <= 1;
 }
 
 /**
- * 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');
 
   if (isSelectedPagesValid()) {
     individualPagesField.classList.remove('invalid');
     fadeOutElement(individualPagesHint);
   } else {
     individualPagesField.classList.add('invalid');
     individualPagesHint.classList.remove('suggestion');
     individualPagesHint.innerHTML =
         localStrings.getStringF('pageRangeInstruction',
                                 localStrings.getString(
                                     '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();
   updatePrintSummary();
 }
 
 /**
  * Updates the state of the increment/decrement buttons based on the current
  * 'copies' value.
  */
@@ skipping 158 matching lines @@
   var successfullyParsed = 0;
   var parts = pageText.split(/,/);
 
   for (var i = 0; i < parts.length; ++i) {
     var part = parts[i].replace(/\s*/g, '');
     if (part.length == 0)
       continue;
 
     var match = part.match(/^([0-9]+)-([0-9]*)$/);
     if (match && isValidNonZeroPositiveInteger(match[1])) {
+      if (!match[2] && totalPageCount == -1) {
+        successfullyParsed += 1;
+        continue;
+      }
       var from = parseInt(match[1], 10);
       var to = match[2] ? parseInt(match[2], 10) : totalPageCount;
 
       if (!to || from > to)
         return false;
-    } else if (!isValidNonZeroPositiveInteger(part) ||
-               !(parseInt(part, 10) <= totalPageCount)) {
+    } else if (!isValidNonZeroPositiveInteger(part) || (totalPageCount != -1 &&
+               !(parseInt(part, 10) <= totalPageCount))) {
       return false;
     }
     successfullyParsed += 1;
   }
   return successfullyParsed > 0
 }
 
 /**
  * Returns true if |value| is a valid non zero positive integer.
  * @param {string} value The string to be tested.
@@ skipping 129 matching lines @@
   this.isLandscape = '';
 }
 
 /**
  * Takes a snapshot of the print settings.
  */
 PrintSettings.prototype.save = function() {
   this.deviceName = getSelectedPrinterName();
   this.isLandscape = isLandscape();
 }