Simplified print preview printer selection to be more consistent with Chrome

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 232c2bfa5501c8d205bfd13c8ae0058b975a054f..7da2f0487bd701790c0ed33ecaee4c5d15f85369 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -85,10 +85,6 @@ var currentPreviewUid = '';
 // True if we need to generate draft preview data.
 var generateDraftData = true;
 
-// TODO(abodenha@chromium.org) A lot of cloud print specific logic has
-// made its way into this file.  Refactor to create a cleaner boundary
-// between print preview and GCP code.  Reference bug 88098 when fixing.
-
 // A dictionary of cloud printers that have been added to the printer
 // dropdown.
 var addedCloudPrinters = {};
@@ -223,12 +219,8 @@ function updateControlsWithSelectedPrinterCapabilities() {
   var skip_refresh = false;
   var selectedValue = printerList.options[selectedIndex].value;
   if (cloudprint.isCloudPrint(printerList.options[selectedIndex])) {
-    updateWithCloudPrinterCapabilities();
-    skip_refresh = true;
-  } else if (selectedValue == PRINT_WITH_CLOUD_PRINT) {
-    // If a preview is pending this will just disable controls.
-    // Once the preview completes we'll try again.
-    printWithCloudPrintDialog();
+    cloudprint.updatePrinterCaps(printerList.options[selectedIndex],
+                                 doUpdateCloudPrinterCapabilities);
     skip_refresh = true;
   } else if (selectedValue == SIGN_IN ||
              selectedValue == MANAGE_CLOUD_PRINTERS ||
@@ -236,7 +228,8 @@ function updateControlsWithSelectedPrinterCapabilities() {
     printerList.selectedIndex = lastSelectedPrinterIndex;
     chrome.send(selectedValue);
     skip_refresh = true;
-  } else if (selectedValue == PRINT_TO_PDF) {
+  } else if (selectedValue == PRINT_TO_PDF ||
+             selectedValue == PRINT_WITH_CLOUD_PRINT) {

[kmadhusu, 2011/09/21 21:57:41]

Adding this option here will hide copies, color and layout settings.  Some users
may want these options. 

[Albert Bodenhamer, 2011/09/22 23:23:39]

Layout works fine.  Did you mean duplex?
Copies, color, and duplex must be specified via the cloud print dialog if they
are supported on the specific printer so it makes no sense to ask for them here.

On 2011/09/21 21:57:41, kmadhusu wrote:

     updateWithPrinterCapabilities({
         'disableColorOption': true,
         'setColorAsDefault': true,
@@ -258,17 +251,6 @@ function updateControlsWithSelectedPrinterCapabilities() {
 }
 
 /**
- * Updates the printer capabilities for the currently selected
- * cloud print printer.
- */
-function updateWithCloudPrinterCapabilities() {
-  var printerList = $('printer-list');
-  var selectedIndex = printerList.selectedIndex;
-  cloudprint.updatePrinterCaps(printerList.options[selectedIndex],
-                               doUpdateCloudPrinterCapabilities);
-}
-
-/**
  * Helper function to do the actual work of updating cloud printer
  * capabilities.
  * @param {Object} printer The printer object to set capabilities for.
@@ -331,7 +313,8 @@ function finishedCloudPrinting() {
 function areSettingsValid() {
   return pageSettings.isPageSelectionValid() &&
       (copiesSettings.isValid() ||
-       getSelectedPrinterName() == PRINT_TO_PDF);
+       getSelectedPrinterName() == PRINT_TO_PDF ||
+       getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT);

[kmadhusu, 2011/09/21 21:57:41]

var selectedPrinter = getSelectedPrinterName();
and reuse "selectedPrinter" here.

[Albert Bodenhamer, 2011/09/22 23:23:39]

Done.

 }
 
 /**
@@ -342,6 +325,7 @@ function areSettingsValid() {
 function getSettings() {
   var deviceName = getSelectedPrinterName();
   var printToPDF = (deviceName == PRINT_TO_PDF);
+  var printWithCloudPrint = (deviceName == PRINT_WITH_CLOUD_PRINT);

[dpapad, 2011/09/22 18:11:57]

Nit: No need for parenthesis here, same for line 327. I t does enhance
readability but it is not consistent with lines 423, 424.

[Albert Bodenhamer, 2011/09/22 23:23:39]

Done.

 
   var settings =
       {'deviceName': deviceName,
@@ -352,6 +336,7 @@ function getSettings() {
        'landscape': layoutSettings.isLandscape(),
        'color': colorSettings.colorMode,
        'printToPDF': printToPDF,
+       'printWithCloudPrint': printWithCloudPrint,
        'isFirstRequest' : false,
        'headerFooterEnabled': headerFooterSettings.hasHeaderFooter(),
        'defaultMarginsSelected': marginSettings.isDefaultMarginsSelected(),
@@ -436,10 +421,13 @@ function getSelectedPrinterName() {
 function requestToPrintDocument() {
   hasPendingPrintDocumentRequest = !isPrintReadyMetafileReady;
   var printToPDF = getSelectedPrinterName() == PRINT_TO_PDF;
-
+  var printWithCloudPrint = getSelectedPrinterName() == PRINT_WITH_CLOUD_PRINT;
   if (hasPendingPrintDocumentRequest) {
     if (printToPDF) {
       sendPrintDocumentRequest();
+    } else if (printWithCloudPrint) {
+      showCustomMessage(localStrings.getString('printWithCloudPrintWait'));

[kmadhusu, 2011/09/21 21:57:41]

Along with the progress message, you also want to show a throbber near the link
so that the user can know that we are loading the dialog.

Eg: On linux, try to open a long document and press Ctrl+P. While the preview is
loading, click the "Print using system dialog" link. You will see a throbber
near the link till the system dialog is completely loaded. 

[dpapad, 2011/09/22 18:11:57]

We only show the throbber if the link was clicked and the metafile is not ready.
In this case the "Print" button was clicked, right? So I don't think a throbber
should be displayed.

[Albert Bodenhamer, 2011/09/22 23:23:39]

I'm going with dpapad@ on this one.  The option should behave very much like a
normal printer.  There are other status messages that indicate the wait
properly.

On 2011/09/22 18:11:57, dpapad wrote:

+      disableInputElementsInSidebar();
     } else {
       isTabHidden = true;
       chrome.send('hidePreview');
@@ -552,7 +540,7 @@ function setDefaultPrinter(printer_name, cloudPrintData) {
     if (cloudPrintData) {
       cloudprint.setDefaultPrinter(printer_name,
                                    cloudPrintData,
-                                   addCloudPrinters,
+                                   addDestinationListOptionAtPosition,
                                    doUpdateCloudPrinterCapabilities);
     } else {
       $('printer-list')[0].value = defaultOrLastUsedPrinterName;
@@ -598,14 +586,14 @@ function setPrinters(printers) {
   }
   // Add options to manage printers.
   if (!cr.isChromeOS) {
-    addDestinationListOption(localStrings.getString('manageLocalPrinters'),
+    addDestinationListOption(localStrings.getString('managePrinters'),
         MANAGE_LOCAL_PRINTERS, false, false, false);
   } else if (useCloudPrint) {
     // Fetch recent printers.
-    cloudprint.fetchPrinters(addCloudPrinters, false);
+    cloudprint.fetchPrinters(addDestinationListOptionAtPosition, false);
     // Fetch the full printer list.
-    cloudprint.fetchPrinters(addCloudPrinters, true);
-    addDestinationListOption(localStrings.getString('manageCloudPrinters'),
+    cloudprint.fetchPrinters(addDestinationListOptionAtPosition, true);
+    addDestinationListOption(localStrings.getString('managePrinters'),
         MANAGE_CLOUD_PRINTERS, false, false, false);
   }
 
@@ -683,108 +671,6 @@ function addDestinationListOptionAtPosition(position,
   printerList.add(option, before);
   return option;
 }
-
-/**
- * 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} True if this id has previously been passed to
- *     trackCloudPrinterAdded.
- */
-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} False if 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
- * cloud print server.
- * @param {Array} printers Array of printer info objects.
- * @return {Object} The currently selected printer.
- */
-function addCloudPrinters(printers) {
-  var isFirstPass = false;
-  var printerList = $('printer-list');
-
-  if (firstCloudPrintOptionPos == lastCloudPrintOptionPos) {
-    isFirstPass = true;
-    // Remove empty entry added by setDefaultPrinter.
-    if (printerList[0] && printerList[0].textContent == '')
-      printerList.remove(0);
-    var option = addDestinationListOptionAtPosition(
-        lastCloudPrintOptionPos++,
-        localStrings.getString('cloudPrinters'),
-        'Label',
-        false,
-        true,
-        false);
-    cloudprint.setCloudPrint(option, null, null);
-  }
-  if (printers != null) {
-    for (var i = 0; i < printers.length; i++) {
-      if (!cloudPrinterAlreadyAdded(printers[i]['id'])) {
-        if (!trackCloudPrinterAdded(printers[i]['id'])) {
-          break;
-        }
-        var option = addDestinationListOptionAtPosition(
-            lastCloudPrintOptionPos++,
-            printers[i]['name'],
-            printers[i]['id'],
-            printers[i]['name'] == defaultOrLastUsedPrinterName,
-            false,
-            false);
-        cloudprint.setCloudPrint(option,
-                                 printers[i]['name'],
-                                 printers[i]['id']);
-      }
-    }
-  } else {
-    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);
-  }
-  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().
@@ -909,9 +795,9 @@ function onDidGetDefaultPageLayout(pageLayout) {
  */
 function checkAndHideOverlayLayerIfValid() {
   var selectedPrinter = getSelectedPrinterName();
-  var printToPDF = selectedPrinter == PRINT_TO_PDF;
-  var printWithCloudPrint = selectedPrinter == PRINT_WITH_CLOUD_PRINT;
-  if ((printToPDF || printWithCloudPrint || !previewModifiable) &&
+  var printToDialog = selectedPrinter == PRINT_TO_PDF ||
+                      selectedPrinter == PRINT_WITH_CLOUD_PRINT;

[dpapad, 2011/09/22 18:11:57]

Nit: Do regular 4 space indent, since the styleguide does not specify any
exceptions for aligning conditionals. Same for line 906.

[Albert Bodenhamer, 2011/09/22 23:23:39]

Done.

+  if ((printToDialog || !previewModifiable) &&
       !isPrintReadyMetafileReady && hasPendingPrintDocumentRequest) {
     return;
   }
@@ -1016,19 +902,15 @@ function sendPrintDocumentRequestIfNeeded() {
   // printing. If the preview source is modifiable, we need to wait till all
   // the requested pages are loaded in the plugin for printing.
   var selectedPrinter = getSelectedPrinterName();
-  var printToPDF = selectedPrinter == PRINT_TO_PDF;
-  var printWithCloudPrint = selectedPrinter == PRINT_WITH_CLOUD_PRINT;
-  if (((printToPDF || !previewModifiable || printWithCloudPrint) &&
+  var printToDialog = selectedPrinter == PRINT_TO_PDF ||
+                      selectedPrinter == PRINT_WITH_CLOUD_PRINT;
+  if (((printToDialog || !previewModifiable) &&
            !isPrintReadyMetafileReady) ||

[dpapad, 2011/09/22 18:11:57]

This can fit in the previous line now.

[Albert Bodenhamer, 2011/09/22 23:23:39]

Done.

       (previewModifiable && hasPendingPreviewRequest)) {
     return;
   }
 
   hasPendingPrintDocumentRequest = false;
-  if (printWithCloudPrint) {
-    chrome.send('printWithCloudPrint');
-    return;
-  }
 
   if (!areSettingsValid()) {
     if (isTabHidden)
@@ -1141,21 +1023,6 @@ function setInitiatorTabTitle(initiatorTabTitle) {
       'printPreviewTitleFormat', initiatorTabTitle);
 }
 
-/**
- * Attempt to hide the preview tab and display the Cloud Print
- * dialog instead. Just disables controls if we're waiting on a new preview
- * to be generated.
- */
-function printWithCloudPrintDialog() {
-  if (isPrintReadyMetafileReady) {
-    chrome.send('printWithCloudPrint');
-  } else {
-    showCustomMessage(localStrings.getString('printWithCloudPrintWait'));
-    disableInputElementsInSidebar();
-    hasPendingPrintDocumentRequest = true;
-  }
-}
-
 /// Pull in all other scripts in a single shot.
 <include src="print_preview_animations.js"/>
 <include src="print_preview_cloud.js"/>