Printer dropdown cleanup

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 38 matching lines @@
 var copiesSettings;
 
 // Object holding all the layout related settings.
 var layoutSettings;
 
 // True if the user has click 'Advanced...' in order to open the system print
 // dialog.
 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.
  */
 function onLoad() {
   cr.enablePlatformSpecificCSSRules();
 
   $('cancel-button').addEventListener('click', handleCancelButtonClick);
 
   if (!checkCompatiblePluginExists()) {
@@ skipping 114 matching lines @@
   if (cloudprint.isCloudPrint(printerList.options[selectedIndex])) {
     updateWithCloudPrinterCapabilities();
     skip_refresh = true;
   } else if (selectedValue == SIGN_IN ||
              selectedValue == MANAGE_CLOUD_PRINTERS ||
              selectedValue == MANAGE_LOCAL_PRINTERS ||
              selectedValue == ADD_CLOUD_PRINTER) {
     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,
         'setColorAsDefault': true,
         'setDuplexAsDefault': false,
         'disableCopiesOption': true});
   } else {
     // This message will call back to 'updateWithPrinterCapabilities'
     // function.
     chrome.send('getPrinterCapabilities', [selectedValue]);
@@ skipping 127 matching lines @@
 }
 
 /**
  * @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;
+}
+
+

[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.
  */
 function getSelectedPrinterName() {
   var printerList = $('printer-list')
   var selectedPrinter = printerList.selectedIndex;
   if (selectedPrinter < 0)
     return '';
   return printerList.options[selectedPrinter].value;
@@ skipping 86 matching lines @@
   // TODO(thestig) re-enable controls here.
 }
 
 /**
  * Set the default printer. If there is one, generate a print preview.
  * @param {string} printer Name of the default printer. Empty if none.
  * @param {string} cloudPrintData Cloud print related data to restore if
  *                 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) {
       cloudprint.setDefaultPrinter(printer_name,
                                    cloudPrintData,
                                    addCloudPrinters,
                                    doUpdateCloudPrinterCapabilities);
     } else {
-      $('printer-list')[0].value = defaultOrLastUsedPrinterName;
+      addDestinationListOption('',
+                               defaultOrLastUsedPrinterName,
+                               true,
+                               true,
+                               true);
       updateControlsWithSelectedPrinterCapabilities();
     }
   }
   chrome.send('getPrinters');
 }
 
 /**
  * Fill the printer list drop down.
  * Called from PrintPreviewHandler::SendPrinterList().
  * @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);
 
   // Adding option for saving PDF to disk.
   addDestinationListOption(localStrings.getString('printToPDF'),
                            PRINT_TO_PDF,
                            defaultOrLastUsedPrinterName == PRINT_TO_PDF,
                            false,
                            false);
   addDestinationListOption('', '', false, true, true);
 
   // Add options to manage printers.
   if (!cr.isChromeOS) {
     addDestinationListOption(localStrings.getString('manageLocalPrinters'),
         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();
 }
 
 /**
  * Creates an option that can be added to the printer destination list.
  * @param {String} optionText specifies the option text content.
  * @param {String} optionValue specifies the option value.
  * @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,
     isDisabled, isSeparator) {
   var option = document.createElement('option');
   option.textContent = optionText;
   option.value = optionValue;
   option.selected = isDefault;
   option.disabled = isSeparator || isDisabled;
   // Adding attribute for improved accessibility.
@@ skipping 16 matching lines @@
                                            optionValue,
                                            isDefault,
                                            isDisabled,
                                            isSeparator);
   $('printer-list').add(option);
   return option;
 }
 
 /**
  * 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.
  * @param {boolean} isDisabled is true if the option needs to be disabled.
  * @return {Object} The created option.
  */
 function addDestinationListOptionAtPosition(position,
                                             optionText,
                                             optionValue,
                                             isDefault,
                                             isDisabled,
                                             isSeparator) {
   var option = createDestinationListOption(optionText,
                                            optionValue,
                                            isDefault,
                                            isDisabled,
                                            isSeparator);
   var printerList = $('printer-list');
   var before = printerList[position];
   printerList.add(option, before);
   return option;
 }
 
 /**
- * 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
  * cloud print server.
  * @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++,
           localStrings.getString('addCloudPrinter'),
           ADD_CLOUD_PRINTER,
           false,
           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++,
                                            '',
                                            '',
                                            false,
                                            true,
                                            true);
         addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
             localStrings.getString('morePrinters'),
             MORE_PRINTERS, false, false, false);
       }
     }
   } 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)
     return null;
   return printerList.options[selectedPrinter];
 }
 
 /**
  * Sets the color mode for the PDF plugin.
  * Called from PrintPreviewHandler::ProcessColorSetting().
@@ skipping 272 matching lines @@
 }
 
 /**
  * Takes a snapshot of the print settings.
  */
 PrintSettings.prototype.save = function() {
   this.deviceName = getSelectedPrinterName();
   this.isLandscape = layoutSettings.isLandscape();
 }