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 83c541d025a16541f0d2ac63acd81a2480d4bf8c..eacd5b2b6f5e91830c5b35f884654aef20c4c74c 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -16,6 +16,10 @@ var lastSelectedPrinterIndex = 0;
 // Used to disable some printing options when the preview is not modifiable.
 var previewModifiable = false;
 
+// True if a request to generate a preview has been sent to the browser in
+// this session.
+var previewRequested = false;

[dpapad, 2011/07/19 18:44:55]

Instead of adding another state variable, you could use the existing
|lastPreviewRequestID| to determine if a preview has been requested (it will be
greater than -1). I would prefer having a function 
function hasRequestedPreview() { return lastPreviewRequestID > -1; }
to keep the number of state variables to a minimum.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

+
 // Destination list special value constants.
 const ADD_CLOUD_PRINTER = 'addCloudPrinter';
 const MANAGE_CLOUD_PRINTERS = 'manageCloudPrinters';
@@ -56,9 +60,17 @@ 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;
 
+// A dictionary of cloud printers that have been added to the printer
+// dropdown.
+var addedCloudPrinters = {};

[dpapad, 2011/07/19 18:44:55]

Could these be moved within print_preview_cloud.js? Also it seems that there is
a lot of cloud print related logic in this file.
  In general there has been an effort to encapsulate related options to their
own separate class, please take a look at bug 88098 and the CLs that refer to
that bug. I would consider creating a class like CloudPrintSettings and put
everything that is cloud print related in there.
  I am not saying this has to be done in this CL necessarily, but if you think
you can follow the same pattern, please put this in your task queue.

[Albert Bodenhamer, 2011/07/19 21:53:19]

The boundary has definitely gotten muddy as things have come together.  My
original intent was that code that interacted directly with the print preview UI
would live in print_preview.js while logic related to GCP would live in
print_preview_cloud.js.

Thanks for being flexible on the refactoring.  I've added a todo to keep track
of it.

On 2011/07/19 18:44:55, dpapad wrote:

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

 The UI logic has migrated (to some degree) outside print_preview.js, which
should act more like a controller (in the MVC pattern), the UI should receive
generic events and update itself. 
 Thanks for taking a note on this, please refer your CL to the same issue
(88098), when you get to it.

+
+// 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 +205,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,
@@ -428,6 +443,7 @@ function requestPrintPreview() {
 
   var settings = getSettings();
   settings.requestID = generatePreviewRequestID();
+  previewRequested = true;
   chrome.send('getPreview', [JSON.stringify(settings)]);
 }
 
@@ -446,8 +462,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 +470,11 @@ function setDefaultPrinter(printer_name, cloudPrintData) {
                                    addCloudPrinters,
                                    doUpdateCloudPrinterCapabilities);
     } else {
-      $('printer-list')[0].value = defaultOrLastUsedPrinterName;
+      addDestinationListOption('',
+                               defaultOrLastUsedPrinterName,
+                               true,
+                               true,
+                               true);
       updateControlsWithSelectedPrinterCapabilities();
     }
   }
@@ -469,20 +487,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);
 
@@ -501,13 +511,15 @@ function setPrinters(printers) {
   }
   if (useCloudPrint) {
     cloudprint.fetchPrinters(addCloudPrinters, false);
+    cloudprint.fetchPrinters(addCloudPrinters, true);

[dpapad, 2011/07/19 18:44:55]

Why fetching printers twice? Is this for getting a short list quickly and then
fetch the full list?

[Albert Bodenhamer, 2011/07/19 21:53:19]

Yep.  We fetch the recents and the full list separately and then merge them to
fill out the list.  I've added comments to make it more clear.

On 2011/07/19 18:44:55, dpapad wrote:

     addDestinationListOption(localStrings.getString('manageCloudPrinters'),
         MANAGE_CLOUD_PRINTERS, false, false, false);
   }
 
+  var printerList = $('printer-list');

[dpapad, 2011/07/19 18:44:55]

Since $('printer-list') is only used once here, no need to cache it to a local
var.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

   printerList.disabled = false;
 
-  if (needPreview)
+  if (!previewRequested)
     updateControlsWithSelectedPrinterCapabilities();
 }
 
@@ -581,16 +593,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.

[dpapad, 2011/07/19 18:44:55]

Nit: No need for "-", just use a space between parameter name and comment, here
and elsewhere.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

+ * @return {boolean} Returns true if this id has previously
+ *                   been passed to trackCloudPrinterAdded.

[dpapad, 2011/07/19 18:44:55]

Nit: Use 4 space indent as usual, here and elsewhere.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

  */
-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
+ *                   would exceed maxCloudPrinters.
+ */
+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 +626,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++,
@@ -620,16 +651,22 @@ function addCloudPrinters(printers, showMorePrintersOption) {
           false);
     } else {
       for (printer in printers) {

[dpapad, 2011/07/19 18:44:55]

Do not use for in statement with arrays, since it does not behave as expected,
see JS styleguide
http://www.corp.google.com/eng/doc/javascriptguide.xml?showone=for-in_loop#fo....

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

-        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 (!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 +681,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)