Print Preview: Fixing behavior of event listeners.

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 35ec000338f2f92da3c38462ea19c040f2c210a9..6d7effdbe5bd7c5f144fe6564871bfa3af9d895d 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -36,6 +36,9 @@ 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;
 
@@ -80,6 +83,8 @@ const maxCloudPrinters = 10;
  */
 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();
@@ -105,31 +110,13 @@ function onLoad() {
   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() {
@@ -311,6 +298,7 @@ function getSettings() {
        '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');
@@ -333,7 +321,15 @@ function generatePreviewRequestID() {
  * @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;
 }
 
 /**
@@ -381,7 +377,6 @@ function requestToPrintDocument() {
   if (printToPDF) {
     sendPrintDocumentRequest();
   } else {
-    removeEventListeners();
     window.setTimeout(function() { sendPrintDocumentRequest(); }, 1000);
   }
 }
@@ -425,13 +420,13 @@ function sendPrintDocumentRequest() {
  */
 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)]);
 }
 
@@ -731,7 +726,6 @@ function displayErrorMessage(errorMessage) {
   $('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);
@@ -774,7 +768,6 @@ function printPreviewFailed() {
 function onPDFLoad() {
   if (previewModifiable) {
     setPluginPreviewPageCount();
-    cr.dispatchSimpleEvent(document, 'updateSummary');
   }
   $('pdf-viewer').fitToHeight();
   cr.dispatchSimpleEvent(document, 'PDFLoaded');
@@ -792,25 +785,33 @@ function setPluginPreviewPageCount() {
  * @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++)
@@ -823,8 +824,15 @@ function reloadPreviewPages(previewUid, previewResponseId) {
  * 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;
@@ -853,6 +861,7 @@ function onDidPreviewPage(pageNumber, previewUid) {
 function updatePrintPreview(jobTitle,
                             previewUid,
                             previewResponseId) {
+  console.log("updatePrintPreview" + previewResponseId);
   if (!isExpectedPreviewResponse(previewResponseId))
     return;
   hasPendingPreviewRequest = false;
@@ -867,9 +876,7 @@ function updatePrintPreview(jobTitle,
     createPDFPlugin(previewUid);
   }
 
-  cr.dispatchSimpleEvent(document, 'updateSummary');
   cr.dispatchSimpleEvent(document, 'updatePrintButton');
-  addEventListeners();
 
   if (hasPendingPrintDocumentRequest)
     requestToPrintPendingDocument();