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 a compatible plugin exists.
+var hasCompatiblePlugin = true;

[Lei Zhang, 2011/06/09 09:54:55]

Probably safer to start with this set to false and modify line 57 accordingly.
Sorry to curb your optimism.

[kmadhusu, 2011/06/09 18:11:44]

Done.

+
+// 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.
  */
 function onLoad() {
   $('system-dialog-link').addEventListener('click', showSystemDialog);
   $('cancel-button').addEventListener('click', handleCancelButtonClick);
 
   if (!checkCompatiblePluginExists()) {
+    hasCompatiblePlugin = false;
     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;
+
+  setDefaultHandlersForCopiesControls();
+  setDefaultHandlersForPagesControls();
   showLoadingAnimation();
   chrome.send('getDefaultPrinter');
 }
 
 /**
+ * Handles all pages checkbox click event.
+ */
+function handleAllPagesCheckbox() {

[dpapad, 2011/06/09 16:33:35]

Could you remove this function and also change line 112 to
"allPages.onclick = updatePrintButtonState;" so that we can keep the number of
functions to a minimum?

[kmadhusu, 2011/06/09 18:11:44]

Done.

+  updatePrintButtonState();
+}
+
+/**
+ * Validates the individual pages text format.
+ */
+function validateIndividualPagesText() {
+  $('print-pages').checked = true;

[dpapad, 2011/06/09 16:33:35]

Why is this necessary? If the print-pages checkbox is not checked this function
should not be called at all.

[kmadhusu, 2011/06/09 18:11:44]

It is required. 

Test case: When you navigate to the text box using the tab key and enter some
data in the text box, we need to select the print-pages checkbox.

+  validatePageRangesField();
+  updatePrintButtonState();
+}
+
+/**
+ * Handles the individual pages input event.
+ */
+function handleIndividualPagesInputEvent() {

[Lei Zhang, 2011/06/09 09:54:55]

nit: can you put this next to handleAllPagesCheckbox()?

[kmadhusu, 2011/06/09 18:11:44]

Done.

+  $('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');
+
+  allPages.onclick = null;
+  printPages.onclick = null;
+  individualPages.oninput = null;
+  individualPages.onfocus = null;
+  individualPages.onblur = null;
+
+  if (hasCompatiblePlugin && !isInitiatorTabClosed) {
+    allPages.onclick = handleAllPagesCheckbox;
+    printPages.onclick = handleIndividualPagesCheckbox;
+    individualPages.onblur = validateIndividualPagesText;
+  }
+}
+
+/**
+ * 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;
   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);
+
+  // 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();

[Lei Zhang, 2011/06/09 09:54:55]

Does this need to be here for can it be at line 174? Can
setDefaultHandlersForPagesControls() and setDefaultHandlersForCopiesControls()
be combined since you call them both at approximately the same time?

[kmadhusu, 2011/06/09 18:11:44]

Done.

 }
 
 /**
  * Asks the browser to close the preview tab.
  */
 function handleCancelButtonClick() {
   chrome.send('closePrintPreviewTab');
 }
 
 /**
  * Asks the browser to show the native print dialog for printing.
  */
 function showSystemDialog() {
   chrome.send('showSystemDialog');
 }
 
 /**
  * Disables the controls which need the initiator tab to generate preview
  * data. This function is called when the initiator tab is closed.
  * @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);
 }
 
 /**
  * Gets the selected printer capabilities and updates the controls accordingly.
  */
 function updateControlsWithSelectedPrinterCapabilities() {
@@ skipping 168 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;

[Lei Zhang, 2011/06/09 09:54:55]

Uhh, isn't this the same as:
hasPendingPrintFileRequest = hasPendingPreviewRequest;

???

[kmadhusu, 2011/06/09 18:11:44]

Just for better readability I used tri-state format. 

[Lei Zhang, 2011/06/09 21:03:27]

I don't understand how that enhances readability. Consider:

bool x = true;
...
bool y = x ? true : false;

vs

bool x = true;
...
bool y = x;

+
+  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) {
+  $('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);
+
   updatePrintSummary();
   updatePrintButtonState();
   addEventListeners();
+
+  if (hasPendingPrintFileRequest) {
+    printFile();
+    return;

[dpapad, 2011/06/09 16:33:35]

Remove return since it is not needed here.

[kmadhusu, 2011/06/09 18:11:44]

Done.

+  }
 }
 
 /**
  * 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();
 }