Print Preview: Fixing behavior of event listeners.

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 18 matching lines @@
 
 // The name of the default or last used printer.
 var defaultOrLastUsedPrinterName = '';
 
 // True when a pending print preview request exists.
 var hasPendingPreviewRequest = false;
 
 // The ID of the last preview request.
 var lastPreviewRequestID = -1;
 
+// The ID of the initial preview request.
+var initialPreviewRequestID = -1;
+
 // True when a pending print file request exists.
 var hasPendingPrintDocumentRequest = false;
 
 // True when preview tab is hidden.
 var isTabHidden = false;
 
 // Object holding all the pages related settings.
 var pageSettings;
 
 // Object holding all the copies related settings.
@@ skipping 24 matching lines @@
 
 // The maximum number of cloud printers to allow in the dropdown.
 const maxCloudPrinters = 10;
 
 /**
  * Window onload handler, sets up the page and starts print preview by getting
  * the printer list.
  */
 function onLoad() {
   cr.enablePlatformSpecificCSSRules();
+  initialPreviewRequestID = getRandomIntegerWithinRange(0, 32000);

[kmadhusu, 2011/08/08 22:20:45]

Use constants for range min & max values.

[dpapad, 2011/08/08 23:41:43]

Done.

+  lastPreviewRequestID = initialPreviewRequestID;
 
   if (!checkCompatiblePluginExists()) {
     disableInputElementsInSidebar();
     displayErrorMessageWithButton(localStrings.getString('noPlugin'),
                                   localStrings.getString('launchNativeDialog'),
                                   launchNativePrintDialog);
     $('mainview').parentElement.removeChild($('dummy-viewer'));
     return;
   }
 
   $('system-dialog-link').addEventListener('click', onSystemDialogLinkClicked);
   $('mainview').parentElement.removeChild($('dummy-viewer'));
 
   $('printer-list').disabled = true;
 
   printHeader = print_preview.PrintHeader.getInstance();
   pageSettings = print_preview.PageSettings.getInstance();
   copiesSettings = print_preview.CopiesSettings.getInstance();
   layoutSettings = print_preview.LayoutSettings.getInstance();
   colorSettings = print_preview.ColorSettings.getInstance();
   printHeader.addEventListeners();
   pageSettings.addEventListeners();
   copiesSettings.addEventListeners();
   layoutSettings.addEventListeners();
   colorSettings.addEventListeners();
+  $('printer-list').onchange = updateControlsWithSelectedPrinterCapabilities;
 
   showLoadingAnimation();
   chrome.send('getDefaultPrinter');
 }
 
 /**
- * Adds event listeners to the settings controls.
- */
-function addEventListeners() {
-  // Controls that require preview rendering.
-  $('printer-list').onchange = updateControlsWithSelectedPrinterCapabilities;
-}
-
-/**
- * Removes event listeners from the settings controls.
- */
-function removeEventListeners() {
-  if (pageSettings)
-    clearTimeout(pageSettings.timerId_);
-
-  // Controls that require preview rendering
-  $('printer-list').onchange = null;
-}
-
-/**
  * Disables the input elements in the sidebar.
  */
 function disableInputElementsInSidebar() {
   var els = $('sidebar').querySelectorAll('input, button, select');
   for (var i = 0; i < els.length; i++)
     els[i].disabled = true;
 }
 
 /**
  * Disables the controls in the sidebar, shows the throbber and instructs the
@@ skipping 161 matching lines @@
   var settings =
       {'deviceName': deviceName,
        'pageRange': pageSettings.selectedPageRanges,
        'printAll': pageSettings.allPagesRadioButton.checked,
        'duplex': copiesSettings.duplexMode,
        'copies': copiesSettings.numberOfCopies,
        'collate': copiesSettings.isCollated(),
        'landscape': layoutSettings.isLandscape(),
        'color': colorSettings.isColor(),
        'printToPDF': printToPDF,
+       'isFirstRequst' : false,

[kmadhusu, 2011/08/08 22:20:45]

Typo "isFirstRequst" -> "isFirstRequest"

[dpapad, 2011/08/08 23:41:43]

Done. Good catch.

        'requestID': 0};
 
   var printerList = $('printer-list');
   var selectedPrinter = printerList.selectedIndex;
   if (cloudprint.isCloudPrint(printerList.options[selectedPrinter])) {
     settings['cloudPrintID'] =
         printerList.options[selectedPrinter].value;
   }
   return settings;
 }
 
 /**
  * @return {number} The next unused preview request id.
  */
 function generatePreviewRequestID() {
   return ++lastPreviewRequestID;
 }
 
 /**
  * @return {boolean} True iff a preview has been requested.
  */
 function hasRequestedPreview() {
-  return lastPreviewRequestID > -1;
+  return lastPreviewRequestID != initialPreviewRequestID;
 }
 
 /**
+ * @return {boolean} True iff this is the |lastPreviewRequestID| corresponds

[kmadhusu, 2011/08/08 22:20:45]

Typo "iff" => "if"

[dpapad, 2011/08/08 23:41:43]

Done. It is not a typo though, iff meand "if and only if"
(http://en.wikipedia.org/wiki/If_and_only_if).

+ *     to the initial preview request.
+ */
+function isFirstPreviewRequest() {
+  return lastPreviewRequestID == initialPreviewRequestID + 1;
+}
+
+/**
  * Checks if |previewResponseId| matches |lastPreviewRequestId|. Used to ignore
  * obsolete preview data responses.
  * @param {number} previewResponseId The id to check.
  * @return {boolean} True if previewResponseId reffers to the expected response.
  */
 function isExpectedPreviewResponse(previewResponseId) {
   return lastPreviewRequestID == previewResponseId;
 }
 
 /**
@@ skipping 24 matching lines @@
      } else {
        isTabHidden = true;
        chrome.send('hidePreview');
      }
     return;
   }
 
   if (printToPDF) {
     sendPrintDocumentRequest();
   } else {
-    removeEventListeners();
     window.setTimeout(function() { sendPrintDocumentRequest(); }, 1000);
   }
 }
 
 /**
  * Asks the browser to print the pending preview PDF that just finished
  * loading.
  */
 function requestToPrintPendingDocument() {
   hasPendingPrintDocumentRequest = false;
@@ skipping 23 matching lines @@
                                   cloudprint.getData(printer)]);
   chrome.send('print', [JSON.stringify(getSettings()),
                         cloudprint.getPrintTicketJSON(printer)]);
 }
 
 /**
  * Asks the browser to generate a preview PDF based on current print settings.
  */
 function requestPrintPreview() {
   hasPendingPreviewRequest = true;
-  removeEventListeners();
   printSettings.save();
   if (!isTabHidden)
     showLoadingAnimation();
 
   var settings = getSettings();
   settings.requestID = generatePreviewRequestID();
+  settings.isFirstRequest = isFirstPreviewRequest();

[kmadhusu, 2011/08/08 22:20:45]

Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in line
#301 & #302?

[dpapad, 2011/08/08 23:41:43]

No. Doing so would affect the Ids every time you call getSettings() making it
impossible to call it (even just for debugging). Just imagine adding a
console.log(getSettings()), it would break the code.

[kmadhusu, 2011/08/09 22:30:40]

For debugging purpose you can always call console.log(settings). Why do you need
to call console.log(getSettings()) if you very well know that the requestID and
isFirstRequest of the settings is always '0' and 'false'?

[dpapad, 2011/08/10 00:17:10]

Done, even though I am not sure that this makes the code better. Here is why:

1) There is no need for a "settings" local var (so you cant just print it) as
you suggest. 
2) A function getXXXX() should not have any side effects (violates the principle
of least surprise). Also now a specific note has to be made in the documentation
to prevent any misuse. (Maybe renaming the function to something like
generateSettingsTicket would solve that).
3) Now even when sending a request for printing and not for previewing these
fields are populated (and ignored by the backend).

Pls respond if you still prefer to keep it as suggested.

[Lei Zhang, 2011/08/10 03:26:07]

I would be surprised if I am new to the code and used getSettings() only to find
it actually changes the state. If you were to stick with patchset 14, I'd rename
this to something like getSettingsWithNextRequestID().

[dpapad, 2011/08/10 15:47:09]

Done. I restored getSettings() to have no side effects and added a function
getSettingsWithRequestID() that adds the side effects. When requesting to print
(or when debugging) getSettings() is called, when requesting a preview
getSettingsWithRequestID() should be called.

   chrome.send('getPreview', [JSON.stringify(settings)]);
 }
 
 /**
  * Called from PrintPreviewUI::OnFileSelectionCancelled to notify the print
  * preview tab regarding the file selection cancel event.
  */
 function fileSelectionCancelled() {
   // TODO(thestig) re-enable controls here.
 }
@@ skipping 279 matching lines @@
 /**
  * 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');
-  removeEventListeners();
   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 22 matching lines @@
 function printPreviewFailed() {
   displayErrorMessage(localStrings.getString('previewFailed'));
 }
 
 /**
  * Called when the PDF plugin loads its document.
  */
 function onPDFLoad() {
   if (previewModifiable) {
     setPluginPreviewPageCount();
-    cr.dispatchSimpleEvent(document, 'updateSummary');
   }
   $('pdf-viewer').fitToHeight();
   cr.dispatchSimpleEvent(document, 'PDFLoaded');
   hideLoadingAnimation();
 }
 
 function setPluginPreviewPageCount() {
   $('pdf-viewer').printPreviewPageCount(
       pageSettings.previouslySelectedPages.length);
 }
 
 /**
  * Update the page count and check the page range.
  * Called from PrintPreviewUI::OnDidGetPreviewPageCount().
  * @param {number} pageCount The number of pages.
  * @param {boolean} isModifiable Indicates whether the previewed document can be
  *     modified.
+ * @param {number} previewResponseId The preview request id that resulted in
+ *     this response.
  */
-function onDidGetPreviewPageCount(pageCount, isModifiable) {
+function onDidGetPreviewPageCount(pageCount, isModifiable, previewResponseId) {
+  console.log("onDidGetPreviewPageCount " + previewResponseId);

[kmadhusu, 2011/08/08 22:20:45]

Do you still need these debug statements?

[dpapad, 2011/08/08 23:41:43]

Done.

+  if (!isExpectedPreviewResponse(previewResponseId))
+    return;
   pageSettings.updateState(pageCount);
   previewModifiable = isModifiable;
+  cr.dispatchSimpleEvent(document, 'updateSummary');
 }
 
 /**
  * Called when no pipelining previewed pages.
+ * @param {string} previewUid Preview unique identifier.
+ * @param {number} previewResponseId The preview request id that resulted in
+ *     this response.
  */
 function reloadPreviewPages(previewUid, previewResponseId) {
+  console.log("reloadPreviewPages " + previewResponseId);
   if (!isExpectedPreviewResponse(previewResponseId))
     return;
   hasPendingPreviewRequest = false;
 
   if (checkIfSettingsChangedAndRegeneratePreview())
     return;
-  cr.dispatchSimpleEvent(document, 'updateSummary');
   cr.dispatchSimpleEvent(document, 'updatePrintButton');
-  addEventListeners();
   hideLoadingAnimation();
   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().
  * @param {number} pageNumber The page number, 0-based.
+ * @param {string} previewUid Preview unique identifier.
+ * @param {number} previewResponseId The preview request id that resulted in
+ *     this response.
  */
-function onDidPreviewPage(pageNumber, previewUid) {
+function onDidPreviewPage(pageNumber, previewUid, previewResponseId) {
+  console.log("onDidPreviewPage " + previewResponseId + " " + pageNumber);
+  if (!isExpectedPreviewResponse(previewResponseId))
+    return;
+
   // Refactor
   if (!previewModifiable)
     return;
 
   var pageIndex = pageSettings.previouslySelectedPages.indexOf(pageNumber + 1);
 
   if (checkIfSettingsChangedAndRegeneratePreview())
     return;
   if (pageIndex == 0)
     createPDFPlugin(previewUid);
 
   $('pdf-viewer').loadPreviewPage(
       getPageSrcURL(previewUid, pageNumber), pageIndex);
 }
 
 /**
  * Update the print preview when new preview data is available.
  * Create the PDF plugin as needed.
  * Called from PrintPreviewUI::PreviewDataIsAvailable().
  * @param {string} jobTitle The print job title.
  * @param {boolean} modifiable If the preview is modifiable.
  * @param {string} previewUid Preview unique identifier.
  * @param {number} previewResponseId The preview request id that resulted in
  *     this response.
  */
 function updatePrintPreview(jobTitle,
                             previewUid,
                             previewResponseId) {
+  console.log("updatePrintPreview" + previewResponseId);
   if (!isExpectedPreviewResponse(previewResponseId))
     return;
   hasPendingPreviewRequest = false;
 
   if (checkIfSettingsChangedAndRegeneratePreview())
     return;
 
   document.title = localStrings.getStringF('printPreviewTitleFormat', jobTitle);
 
   if (!previewModifiable) {
     // If the preview is not modifiable the plugin has not been created yet.
     createPDFPlugin(previewUid);
   }
 
-  cr.dispatchSimpleEvent(document, 'updateSummary');
   cr.dispatchSimpleEvent(document, 'updatePrintButton');
-  addEventListeners();
 
   if (hasPendingPrintDocumentRequest)
     requestToPrintPendingDocument();
 }
 
 /**
  * Check if any print settings changed and regenerate the preview if needed.
  * @return {boolean} true if a new preview is required.
  */
 function checkIfSettingsChangedAndRegeneratePreview() {
@@ skipping 76 matching lines @@
 }
 
 /**
  * Takes a snapshot of the print settings.
  */
 PrintSettings.prototype.save = function() {
   this.deviceName = getSelectedPrinterName();
   this.isLandscape = layoutSettings.isLandscape();
 }