Simplified print preview printer selection to be more consistent with Chrome

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 67 matching lines @@
 // The range of options in the printer dropdown controlled by cloud print.
 var firstCloudPrintOptionPos = 0;
 var lastCloudPrintOptionPos = firstCloudPrintOptionPos;
 
 // Store the current previewUid.
 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 = {};
 
 // The maximum number of cloud printers to allow in the dropdown.
 const maxCloudPrinters = 10;
 
 const MIN_REQUEST_ID = 0;
 const MAX_REQUEST_ID = 32000;
 
@@ skipping 114 matching lines @@
  * Gets the selected printer capabilities and updates the controls accordingly.
  */
 function updateControlsWithSelectedPrinterCapabilities() {
   var printerList = $('printer-list');
   var selectedIndex = printerList.selectedIndex;
   if (selectedIndex < 0)
     return;
   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 ||
              selectedValue == MANAGE_LOCAL_PRINTERS) {
     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,
         'setDuplexAsDefault': false,
         'printerColorModelForColor': colorSettings.COLOR,
         'printerDefaultDuplexValue': copiesSettings.UNKNOWN_DUPLEX_MODE,
         'disableCopiesOption': true});
   } else {
     // This message will call back to 'updateWithPrinterCapabilities'
     // function.
     chrome.send('getPrinterCapabilities', [selectedValue]);
   }
   if (!skip_refresh) {
     lastSelectedPrinterIndex = selectedIndex;
 
     // Regenerate the preview data based on selected printer settings.
     setDefaultValuesAndRegeneratePreview(true);
   }
 }
 
 /**
- * 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.
  */
 function doUpdateCloudPrinterCapabilities(printer) {
   var settings = {'disableColorOption': !cloudprint.supportsColor(printer),
                   'setColorAsDefault': cloudprint.colorIsDefault(printer),
                   'disableCopiesOption': true,
                   'disableLandscapeOption': true};
   updateWithPrinterCapabilities(settings);
@@ skipping 42 matching lines @@
 }
 
 /**
  * Checks whether the specified settings are valid.
  *
  * @return {boolean} true if settings are valid, false if not.
  */
 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.

 }
 
 /**
  * Creates an object based on the values in the printer settings.
  *
  * @return {Object} Object containing print job settings.
  */
 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,
        'pageRange': pageSettings.selectedPageRanges,
        'duplex': copiesSettings.duplexMode,
        'copies': copiesSettings.numberOfCopies,
        'collate': copiesSettings.isCollated(),
        'landscape': layoutSettings.isLandscape(),
        'color': colorSettings.colorMode,
        'printToPDF': printToPDF,
+       'printWithCloudPrint': printWithCloudPrint,
        'isFirstRequest' : false,
        'headerFooterEnabled': headerFooterSettings.hasHeaderFooter(),
        'defaultMarginsSelected': marginSettings.isDefaultMarginsSelected(),
        'margins': marginSettings.customMargins,
        'requestID': -1,
        'generateDraftData': generateDraftData};
 
   var printerList = $('printer-list');
   var selectedPrinter = printerList.selectedIndex;
   if (cloudprint.isCloudPrint(printerList.options[selectedPrinter])) {
@@ skipping 64 matching lines @@
 }
 
 /**
  * Asks the browser to print the preview PDF based on current print
  * settings. If the preview is still loading, printPendingFile() will get
  * called once the preview loads.
  */
 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');
     }
     return;
   }
 
   if (printToPDF) {
     sendPrintDocumentRequest();
   } else {
@@ skipping 92 matching lines @@
  *                 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,
+                                   addDestinationListOptionAtPosition,
                                    doUpdateCloudPrinterCapabilities);
     } else {
       $('printer-list')[0].value = defaultOrLastUsedPrinterName;
       updateControlsWithSelectedPrinterCapabilities();
     }
   }
   chrome.send('getPrinters');
 }
 
 /**
@@ skipping 25 matching lines @@
   if (useCloudPrint) {
     addDestinationListOption(localStrings.getString('printWithCloudPrint'),
                              PRINT_WITH_CLOUD_PRINT,
                              false,
                              false,
                              false);
     addDestinationListOption('', '', false, true, true);
   }
   // 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);
   }
 
   printerList.disabled = false;
 
   if (!hasRequestedPreview())
     updateControlsWithSelectedPrinterCapabilities();
 }
 
 /**
@@ skipping 57 matching lines @@
   var option = createDestinationListOption(optionText,
                                            optionValue,
                                            isDefault,
                                            isDisabled,
                                            isSeparator);
   var printerList = $('printer-list');
   var before = printerList[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().
  * @param {boolean} color is true if the PDF plugin should display in color.
  */
 function setColor(color) {
   var pdfViewer = $('pdf-viewer');
   if (!pdfViewer) {
     return;
   }
@@ skipping 104 matching lines @@
   // TODO(aayushkumar): Do something here!
 }
 
 /**
  * This function sends a request to hide the overlay layer only if there is no
  * pending print document request and we are not waiting for the print ready
  * metafile.
  */
 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;
   }
   hideOverlayLayer();
 }
 
 /**
  * Called when no pipelining previewed pages.
  * @param {string} previewUid Preview unique identifier.
  * @param {number} previewResponseId The preview request id that resulted in
@@ skipping 84 matching lines @@
  */
 function sendPrintDocumentRequestIfNeeded() {
   if (!hasPendingPrintDocumentRequest || !isFirstPageLoaded)
     return;
 
   // If the selected printer is PRINT_TO_PDF or PRINT_WITH_CLOUD_PRINT or
   // the preview source is not modifiable, we need the print ready data for
   // 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)
       cancelPendingPrintRequest();
     return;
   }
   sendPrintDocumentRequest();
 }
 
 /**
@@ skipping 92 matching lines @@
  * 'getInitiatorTabTitle' message.
  * @param {string} initiatorTabTitle The title of the initiator tab.
  */
 function setInitiatorTabTitle(initiatorTabTitle) {
   if (initiatorTabTitle == '')
     return;
   document.title = localStrings.getStringF(
       '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"/>
 <include src="print_preview_utils.js"/>
 <include src="print_header.js"/>
 <include src="page_settings.js"/>
 <include src="copies_settings.js"/>
 <include src="header_footer_settings.js"/>
 <include src="layout_settings.js"/>
 <include src="color_settings.js"/>
 <include src="margin_settings.js"/>