Printer dropdown cleanup

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 47884de18fb06b1be74233f4341e7aead36c3c57..b031d987cb29369ee3e87d2e4ad340b470ff1d71 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -56,9 +56,22 @@ var layoutSettings;
 var showingSystemDialog = false;
 
 // The range of options in the printer dropdown controlled by cloud print.
-var firstCloudPrintOptionPos = 0
+var firstCloudPrintOptionPos = 0;
 var lastCloudPrintOptionPos = firstCloudPrintOptionPos;
 
+
+//TODO(abodenha@chromium.org) A lot of cloud print specific logic has

[dpapad, 2011/07/19 22:22:58]

Nit: space after //.

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

+// made its way into this file.  Refactor to create a cleaner boundary
+// between print preview and GCP code.
+
+// A dictionary of cloud printers that have been added to the printer
+// dropdown.
+var addedCloudPrinters = {};
+
+// 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.
@@ -193,6 +206,9 @@ function updateControlsWithSelectedPrinterCapabilities() {
     printerList.selectedIndex = lastSelectedPrinterIndex;
     chrome.send(selectedValue);
     skip_refresh = true;
+  } else if (selectedValue == MORE_PRINTERS) {
+    onSystemDialogLinkClicked();
+    skip_refresh = true;
   } else if (selectedValue == PRINT_TO_PDF) {
     updateWithPrinterCapabilities({
         'disableColorOption': true,
@@ -340,6 +356,14 @@ function generatePreviewRequestID() {
 }
 
 /**
+ * @return {boolean} True iff a preview has been requested.
+ */
+function hasRequestedPreview() {
+  return lastPreviewRequestID > -1;
+}
+
+

[dpapad, 2011/07/19 22:22:58]

Nit: 1 blank line between functions.

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

+/**
  * Returns the name of the selected printer or the empty string if no
  * printer is selected.
  * @return {string} The name of the currently selected printer.
@@ -446,8 +470,6 @@ function fileSelectionCancelled() {
  *                 the default printer is a cloud printer.
  */
 function setDefaultPrinter(printer_name, cloudPrintData) {
-  // Add a placeholder value so the printer list looks valid.
-  addDestinationListOption('', '', true, true, true);
   if (printer_name) {
     defaultOrLastUsedPrinterName = printer_name;
     if (cloudPrintData) {
@@ -456,7 +478,11 @@ function setDefaultPrinter(printer_name, cloudPrintData) {
                                    addCloudPrinters,
                                    doUpdateCloudPrinterCapabilities);
     } else {
-      $('printer-list')[0].value = defaultOrLastUsedPrinterName;
+      addDestinationListOption('',
+                               defaultOrLastUsedPrinterName,
+                               true,
+                               true,
+                               true);
       updateControlsWithSelectedPrinterCapabilities();
     }
   }
@@ -469,20 +495,12 @@ function setDefaultPrinter(printer_name, cloudPrintData) {
  * @param {Array} printers Array of printer info objects.
  */
 function setPrinters(printers) {
-  var printerList = $('printer-list');
-  // If there exists a dummy printer value, then setDefaultPrinter() already
-  // requested a preview, so no need to do it again.
-  var needPreview = (printerList[0].value == '');
   for (var i = 0; i < printers.length; ++i) {
     var isDefault = (printers[i].deviceName == defaultOrLastUsedPrinterName);
     addDestinationListOption(printers[i].printerName, printers[i].deviceName,
                              isDefault, false, false);
   }
 
-  if (!cloudprint.isCloudPrint(printerList[0]))
-    // Remove the dummy printer added in setDefaultPrinter().
-    printerList.remove(0);
-
   if (printers.length != 0)
     addDestinationListOption('', '', false, true, true);
 
@@ -500,14 +518,17 @@ function setPrinters(printers) {
         MANAGE_LOCAL_PRINTERS, false, false, false);
   }
   if (useCloudPrint) {
+    // Fetch recent printers.
     cloudprint.fetchPrinters(addCloudPrinters, false);
+    // Fetch the full printer list.
+    cloudprint.fetchPrinters(addCloudPrinters, true);
     addDestinationListOption(localStrings.getString('manageCloudPrinters'),
         MANAGE_CLOUD_PRINTERS, false, false, false);
   }
 
-  printerList.disabled = false;
+  $('printer-list').disabled = false;
 
-  if (needPreview)
+  if (!hasRequestedPreview())
     updateControlsWithSelectedPrinterCapabilities();
 }
 
@@ -518,7 +539,7 @@ function setPrinters(printers) {
  * @param {boolean} isDefault is true if the option needs to be selected.
  * @param {boolean} isDisabled is true if the option needs to be disabled.
  * @param {boolean} isSeparator is true if the option is a visual separator and
- *                  needs to be disabled.
+ *     needs to be disabled.
  * @return {Object} The created option.
  */
 function createDestinationListOption(optionText, optionValue, isDefault,
@@ -556,7 +577,7 @@ function addDestinationListOption(optionText, optionValue, isDefault,
 /**
  * Adds an option to the printer destination list at a specified position.
  * @param {Integer} position The index in the printer-list wher the option

[dpapad, 2011/07/19 22:22:58]

Nit: @param {number}, there is no type Integer in JS.

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

-                             should be created.
+       should be created.
  * @param {String} optionText specifies the option text content.

[dpapad, 2011/07/19 22:22:58]

Nit optional: Just for consistency (within this file and across WebUI pages) we
should go with either "string" or "String". It seems that most of the existing
comments use "string", but both cases occur.

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

  * @param {String} optionValue specifies the option value.
  * @param {boolean} isDefault is true if the option needs to be selected.
@@ -581,16 +602,32 @@ function addDestinationListOptionAtPosition(position,
 }
 
 /**
- * Removes the list of cloud printers from the printer pull down.
+ * Test if a particular cloud printer has already been added to the
+ * printer dropdown.
+ * @param {String} id A unique value to track this printer.
+ * @return {boolean} Returns true if this id has previously
+ *     been passed to trackCloudPrinterAdded.
  */
-function clearCloudPrinters() {
-  var printerList = $('printer-list');
-  while (firstCloudPrintOptionPos < lastCloudPrintOptionPos) {
-    lastCloudPrintOptionPos--;
-    printerList.remove(lastCloudPrintOptionPos);
+function cloudPrinterAlreadyAdded(id) {
+  return (addedCloudPrinters[id]);
+}
+
+/**
+ * Record that a cloud printer will added to the printer dropdown.
+ * @param {String} id A unique value to track this printer.
+ * @return {boolean} Returns false if there adding this printer

[dpapad, 2011/07/19 22:22:58]

Change to "Returns false if adding this printer".

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

+ *     would exceed maxCloudPrinters.

[dpapad, 2011/07/19 22:22:58]

Nit: s/maxCloudPrinters/|maxCloudPrinters|

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

+ */
+function trackCloudPrinterAdded(id) {
+  if (Object.keys(addedCloudPrinters).length < maxCloudPrinters) {
+    addedCloudPrinters[id] = true;
+    return true;
+  } else {
+    return false;
   }
 }
 
+
 /**
  * Add cloud printers to the list drop down.
  * Called from the cloudprint object on receipt of printer information from the
@@ -598,18 +635,21 @@ function clearCloudPrinters() {
  * @param {Array} printers Array of printer info objects.
  * @return {Object} The currently selected printer.
  */
-function addCloudPrinters(printers, showMorePrintersOption) {
-  // TODO(abodenha@chromium.org) Avoid removing printers from the list.
-  // Instead search for duplicates and don't add printers that already exist
-  // in the list.
-  clearCloudPrinters();
-  addDestinationListOptionAtPosition(
-      lastCloudPrintOptionPos++,
-      localStrings.getString('cloudPrinters'),
-      '',
-      false,
-      true,
-      false);
+function addCloudPrinters(printers) {
+  var isFirstPass = false;
+  var showMorePrintersOption = false;
+
+  if (firstCloudPrintOptionPos == lastCloudPrintOptionPos) {
+    isFirstPass = true;
+    var option = addDestinationListOptionAtPosition(
+        lastCloudPrintOptionPos++,
+        localStrings.getString('cloudPrinters'),
+        'Label',
+        false,
+        true,
+        false);
+    cloudprint.setCloudPrint(option, null, null);
+  }
   if (printers != null) {
     if (printers.length == 0) {
       addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
@@ -619,17 +659,23 @@ function addCloudPrinters(printers, showMorePrintersOption) {
           false,
           false);
     } else {
-      for (printer in printers) {
-        var option = addDestinationListOptionAtPosition(
-            lastCloudPrintOptionPos++,
-            printers[printer]['name'],
-            printers[printer]['id'],
-            printers[printer]['name'] == defaultOrLastUsedPrinterName,
-            false,
-            false);
-        cloudprint.setCloudPrint(option,
-                                 printers[printer]['name'],
-                                 printers[printer]['id']);
+      for (var printer = 0; printer < printers.length; printer++) {

[dpapad, 2011/07/19 22:22:58]

Nit optional: Using a counter like "i" seems a bit more intuitive.

[Albert Bodenhamer, 2011/07/19 22:43:48]

Done.

+        if (!cloudPrinterAlreadyAdded(printers[printer]['id'])) {
+          var option = addDestinationListOptionAtPosition(
+              lastCloudPrintOptionPos++,
+              printers[printer]['name'],
+              printers[printer]['id'],
+              printers[printer]['name'] == defaultOrLastUsedPrinterName,
+              false,
+              false);
+          cloudprint.setCloudPrint(option,
+                                   printers[printer]['name'],
+                                   printers[printer]['id']);
+          if (!trackCloudPrinterAdded(printers[printer]['id'])) {
+            showMorePrintersOption = true;
+            break;
+          }
+        }
       }
       if (showMorePrintersOption) {
         addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
@@ -644,25 +690,30 @@ function addCloudPrinters(printers, showMorePrintersOption) {
       }
     }
   } else {
-    addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                       localStrings.getString('signIn'),
-                                       SIGN_IN,
+    if (!cloudPrinterAlreadyAdded(SIGN_IN)) {
+      addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
+                                         localStrings.getString('signIn'),
+                                         SIGN_IN,
+                                         false,
+                                         false,
+                                         false);
+      trackCloudPrinterAdded(SIGN_IN);
+    }
+  }
+  if (isFirstPass) {
+    addDestinationListOptionAtPosition(lastCloudPrintOptionPos,
+                                       '',
+                                       '',
                                        false,
+                                       true,
+                                       true);
+    addDestinationListOptionAtPosition(lastCloudPrintOptionPos + 1,
+                                       localStrings.getString('localPrinters'),
+                                       '',
                                        false,
+                                       true,
                                        false);
   }
-  addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                     '',
-                                     '',
-                                     false,
-                                     true,
-                                     true);
-  addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                     localStrings.getString('localPrinters'),
-                                     '',
-                                     false,
-                                     true,
-                                     false);
   var printerList = $('printer-list')
   var selectedPrinter = printerList.selectedIndex;
   if (selectedPrinter < 0)