Update CUPS settings UI to allow USB printers to be added via discovery.

chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js

 // Copyright 2016 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.
 
 /**
  * @fileoverview 'settings-cups-add-printer-dialog' includes multiple dialogs to
  * set up a new CUPS printer.
  * Subdialogs include:
  * - 'add-printer-discovery-dialog' is a dialog showing discovered printers on
  *   the network that are available for setup.
@@ skipping 15 matching lines @@
   MANUFACTURER: 'add-printer-manufacturer-model-dialog',
 };
 
 /**
  * The maximum height of the discovered printers list when the searching spinner
  * is not showing.
  * @const {number}
  */
 var kPrinterListFullHeight = 350;
 
+/**
+ * Return a reset CupsPrinterInfo object.
+ *  @return {!CupsPrinterInfo}
+ */
+function getEmptyPrinter_() {
+  return {
+    printerAddress: '',
+    printerDescription: '',
+    printerId: '',
+    printerManufacturer: '',
+    printerModel: '',
+    printerName: '',
+    printerPPDPath: '',
+    printerProtocol: 'ipp',
+    printerQueue: '',
+    printerStatus: '',
+  };
+}
+
 Polymer({
   is: 'add-printer-discovery-dialog',
 
   behaviors: [WebUIListenerBehavior],
 
   properties: {
     /** @type {!Array<!CupsPrinterInfo>|undefined} */
     discoveredPrinters: {
       type: Array,
     },
@@ skipping 46 matching lines @@
   /** @private */
   stopDiscoveringPrinters_: function() {
     settings.CupsPrintersBrowserProxyImpl.getInstance().
         stopDiscoveringPrinters();
     this.discovering_ = false;
   },
 
   /** @private */
   switchToManualAddDialog_: function() {
     this.stopDiscoveringPrinters_();
+    // We're abandoning discovery in favor of manual specification, so
+    // drop the selection if one exists.
+    this.selectedPrinter = getEmptyPrinter_();
     this.$$('add-printer-dialog').close();
     this.fire('open-manually-add-printer-dialog');
   },
 
   /** @private */
   onCancelTap_: function() {
     this.stopDiscoveringPrinters_();
     this.$$('add-printer-dialog').close();
   },
 
   /** @private */
-  switchToConfiguringDialog_: function() {
+  switchToManufacturerDialog_: function() {
     this.stopDiscoveringPrinters_();
+    // If we're switching to the manufacturer/model dialog, clear the existing
+    // data we have about the PPD (if any), as we're dropping that in favor of
+    // user selections.
+    this.selectedPrinter.printerManufacturer = '';
+    this.selectedPrinter.printerModel = '';
+    this.selectedPrinter.printerPPDPath = '';

[xdai1, 2017/05/11 22:50:28]

You probably don't need these three lines. After the button changes to a back
button, currently there is no way to jump from manually-add-printer dialog to
auto-discovery dialog so the data from manually-add-printer flow won't affect
the auto-discovery flow.

[Carlson, 2017/05/11 23:57:10]

That's true, but when we actually do select a printer, we may end up with
partial information that we don't want to use.  This actually happens in the USB
case, so these lines are needed.

[xdai1, 2017/05/12 00:30:43]

Acknowledged.

     this.$$('add-printer-dialog').close();
-    this.fire('open-configuring-printer-dialog');
+    this.fire('open-manufacturer-model-dialog');
   },
 });
 
 Polymer({
   is: 'add-printer-manually-dialog',
 
   properties: {
     /** @type {!CupsPrinterInfo} */
     newPrinter: {
       type: Object,
       notify: true,
-      value: function() {
-        return {
-          printerAddress: '',
-          printerDescription: '',
-          printerId: '',
-          printerManufacturer: '',
-          printerModel: '',
-          printerName: '',
-          printerPPDPath: '',
-          printerProtocol: 'ipp',
-          printerQueue: '',
-          printerStatus: '',
-        };
-      },
+      value: getEmptyPrinter_

[xdai1, 2017/05/11 22:50:28]

getEmptyPrinter_()?

[Carlson, 2017/05/11 23:57:11]

I don't think so? 

https://www.polymer-project.org/1.0/docs/devguide/properties

says value needs to be a function that returns the proper type if the default
value should be an Array or Object, so I think we need to pass the function
itself, not the result of the function.

[xdai1, 2017/05/12 00:30:43]

Acknowledged.

     },
   },
 
   /** @private */
   switchToDiscoveryDialog_: function() {
     this.$$('add-printer-dialog').close();
     this.fire('open-discovery-printers-dialog');
   },
 
   /** @private */
@@ skipping 40 matching lines @@
     /** @type {?Array<string>} */
     manufacturerList: {
       type: Array,
     },
 
     /** @type {?Array<string>} */
     modelList: {
       type: Array,
     },
 
+    /** @type {string} */
+    previousDialog: String,
+
     setupFailed: {
       type: Boolean,
       value: false,
     },
   },
 
   observers: [
     'selectedManufacturerChanged_(newPrinter.printerManufacturer)',
   ],
 
@@ skipping 44 matching lines @@
   /**
    * @param {!ModelsInfo} modelsInfo
    * @private
    */
   modelListChanged_: function(modelsInfo) {
     if (modelsInfo.success)
       this.modelList = modelsInfo.models;
   },
 
   /** @private */
-  switchToManualAddDialog_: function() {
+  switchToPreviousDialog_: function() {
     this.$$('add-printer-dialog').close();
-    this.fire('open-manually-add-printer-dialog');
+    if (this.previousDialog == AddPrinterDialogs.DISCOVERY) {
+      this.fire('open-discovery-printers-dialog');
+    } else if (this.previousDialog == AddPrinterDialogs.MANUALLY) {
+      this.fire('open-manually-add-printer-dialog');
+    }
   },
 
   /** @private */
   onCancelTap_: function() {
     this.$$('add-printer-dialog').close();
   },
 
   /** @private */
   switchToConfiguringDialog_: function() {
     this.$$('add-printer-dialog').close();
@@ skipping 35 matching lines @@
   },
 });
 
 Polymer({
   is: 'settings-cups-add-printer-dialog',
 
   behaviors: [WebUIListenerBehavior],
 
   properties: {
     /** @type {!CupsPrinterInfo} */
-    selectedPrinter: {
-      type: Object,
-    },
-
-    /** @type {!CupsPrinterInfo} */
     newPrinter: {
       type: Object,
     },
 
     /** @type {boolean} whether the new printer setup is failed. */
     setupFailed: {
       type: Boolean,
       value: false,
     },
 
     configuringDialogTitle: String,
 
-    /** @private {string} */
-    previousDialog_: String,
+    /** @type {string} */
+    previousDialog: String,
 
     /** @private {string} */
     currentDialog_: String,
 
     /** @private {boolean} */
     showDiscoveryDialog_: Boolean,
 
     /** @private {boolean} */
     showManuallyAddDialog_: Boolean,
 
@@ skipping 13 matching lines @@
   },
 
   /** @override */
   ready: function() {
     this.addWebUIListener('on-add-cups-printer', this.onAddPrinter_.bind(this));
   },
 
   /** Opens the Add printer discovery dialog. */
   open: function() {
     this.resetData_();
-    this.switchDialog_(
-        '', AddPrinterDialogs.MANUALLY, 'showManuallyAddDialog_');
+    this.switchDialog_('', AddPrinterDialogs.DISCOVERY, 'showDiscoveryDialog_');
   },
 
   /**
    * Reset all the printer data in the Add printer flow.
    * @private
    */
   resetData_: function() {
-    if (this.selectedPrinter)
-      this.selectedPrinter = this.getEmptyPrinter_();
     if (this.newPrinter)
-      this.newPrinter = this.getEmptyPrinter_();
+      this.newPrinter = getEmptyPrinter_();
     this.setupFailed = false;
   },
 
-  /**
-   * @return {!CupsPrinterInfo}
-   * @private
-   */
-  getEmptyPrinter_: function() {
-    return {
-      printerAddress: '',
-      printerDescription: '',
-      printerId: '',
-      printerManufacturer: '',
-      printerModel: '',
-      printerName: '',
-      printerPPDPath: '',
-      printerProtocol: 'ipp',
-      printerQueue: '',
-      printerStatus: '',
-    };
-  },
-
   /** @private */
   openManuallyAddPrinterDialog_: function() {
     this.switchDialog_(this.currentDialog_, AddPrinterDialogs.MANUALLY,
                        'showManuallyAddDialog_');
   },
 
   /** @private */
   openDiscoveryPrintersDialog_: function() {
     this.switchDialog_(this.currentDialog_, AddPrinterDialogs.DISCOVERY,
                        'showDiscoveryDialog_');
   },
 
   /** @private */
   openConfiguringPrinterDialog_: function() {
     this.switchDialog_(

[xdai1, 2017/05/11 22:50:28]

After your change, the previous dialog for configuring-printer-dialog only can
be manufacturer-model-dialog. So this function won't do the right thing any
more.

[Carlson, 2017/05/11 23:57:11]

That's true right now, but isn't the right thing in the long term.  When network
discovered printers go in, we will want to try to configure them directly from
the discovery screen, we just don't do that for USB printers.

So I think leaving this as-is is the right thing?  I don't think it's wrong, I
think we just never hit the first conditional right now.

[xdai1, 2017/05/12 00:30:43]

Because the previous dialog will always be this.previousDialog ==
AddPrinterDialogs.MANUFACTURER, we set |this.configuringDialogTitle| to be the
manuall-add-printer title, which is incorrect.

[Carlson, 2017/05/12 17:12:46]

Took me a while, but I understand now.

I'm not sure what the right thing is here, it's really a question of what's
least confusing to the user and what it means to add a printer 'manually'.

We can sort of think of this as having 3 categories:

1)  Printers that magically work.  You select them in the discovery box, click
add, no further action is required.  (Or, like USB, they just show up as
available)

2)  Printers that aren't discovered at all.  The user puts in name, address,
protocol, manufacturer, model. 

3)  Printers that are discovered, but require the user to tell us
manufacturer/model before they can be configured.

2 is definitely manual.  1 is definitely *not* manual.

3 is less clear.  I've been calling 3 'manual' as well, which is why I was
confused; I think you tend to think of those as *not* 'manual'.  I think under
my definition this is correct and under yours it's not, but neither one is
obviously right.  

Also note that, as currently written, the manufacturer-model dialog always says
"Add a printer manually", even when we get there from discovery, which I've also
thought was the right thing.

You OK with me punting on this point by filing a bug to clarify dialog titles? 
I'd like to get Weifang's input on it.

[xdai1, 2017/05/12 17:56:55]

Yes. I'm fine to file a bug for this.

[Carlson, 2017/05/12 18:14:09]

Filed crbug/721841

         this.currentDialog_, AddPrinterDialogs.CONFIGURING,
         'showConfiguringDialog_');
-    if (this.previousDialog_ == AddPrinterDialogs.DISCOVERY) {
+    if (this.previousDialog == AddPrinterDialogs.DISCOVERY) {
       this.configuringDialogTitle =
           loadTimeData.getString('addPrintersNearbyTitle');
       settings.CupsPrintersBrowserProxyImpl.getInstance().addCupsPrinter(
           this.selectedPrinter);
-    } else if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER) {
+    } else if (this.previousDialog == AddPrinterDialogs.MANUFACTURER) {
       this.configuringDialogTitle =
           loadTimeData.getString('addPrintersManuallyTitle');
       settings.CupsPrintersBrowserProxyImpl.getInstance().addCupsPrinter(
           this.newPrinter);
     }
   },
 
   /** @private */
   openManufacturerModelDialog_: function() {
     this.switchDialog_(this.currentDialog_, AddPrinterDialogs.MANUFACTURER,
                        'showManufacturerDialog_');
   },
 
   /** @private */
   configuringDialogClosed_: function() {
-    if (this.previousDialog_ == AddPrinterDialogs.MANUALLY) {
+    if (this.previousDialog == AddPrinterDialogs.MANUALLY) {

[xdai1, 2017/05/11 22:50:28]

Same as above.

[Carlson, 2017/05/11 23:57:10]

Same reply here.  Eventually this will be needed and I think it's not incorrect
for now.

[xdai1, 2017/05/12 00:30:43]

Acknowledged.

[Carlson, 2017/05/12 17:12:46]

Actually, this one is a real problem looking more closely.  Eventually we'll be
getting to "configuring" from any of {DISCOVERY, MANUALLY, MANUFACTURER}, so we
should go back to the right place from any of those.  I've done that.  

(As an aside, I think I'm going to do a future change that alters how we manage
previousDialog and the domIfBooleanName stuff; I think there's a way to make
that a little more elegant)

       this.switchDialog_(
-          this.currentDialog_, this.previousDialog_, 'showManuallyAddDialog_');
-    } else if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER) {
+          this.currentDialog_, this.previousDialog, 'showManuallyAddDialog_');
+    } else if (this.previousDialog == AddPrinterDialogs.MANUFACTURER) {
       this.switchDialog_(
-          this.currentDialog_, this.previousDialog_, 'showManufacturerDialog_');
+          this.currentDialog_, this.previousDialog, 'showManufacturerDialog_');
     }
   },
 
   /**
    * Switch dialog from |fromDialog| to |toDialog|.
    * @param {string} fromDialog
    * @param {string} toDialog
    * @param {string} domIfBooleanName The name of the boolean variable
    *     corresponding to the |toDialog|.
    * @private
    */
   switchDialog_: function(fromDialog, toDialog, domIfBooleanName) {
-    this.previousDialog_ = fromDialog;
+    this.previousDialog = fromDialog;
     this.currentDialog_ = toDialog;
 
     this.set(domIfBooleanName, true);
     this.async(function() {
       var dialog = this.$$(toDialog);
       dialog.addEventListener('close', function() {
         this.set(domIfBooleanName, false);
       }.bind(this));
     });
   },
 
   /**
    * @return {string} The name of the current printer in configuration.
    * @private
    */
   getConfiguringPrinterName_: function() {

[xdai1, 2017/05/11 22:50:28]

This function can be removed now. And in configuring-printer-dialog, bind
printer-name="[[newPrinter.printerName]]"

[Carlson, 2017/05/11 23:57:10]

Good point, done.

-    if (this.previousDialog_ == AddPrinterDialogs.DISCOVERY)
-      return this.selectedPrinter.printerName;
-    if (this.previousDialog_ == AddPrinterDialogs.MANUALLY ||
-        this.previousDialog_ == AddPrinterDialogs.MANUFACTURER) {
+    if (this.previousDialog == AddPrinterDialogs.DISCOVERY ||
+        this.previousDialog == AddPrinterDialogs.MANUALLY ||
+        this.previousDialog == AddPrinterDialogs.MANUFACTURER) {
       return this.newPrinter.printerName;
     }
     return '';
   },
 
   /**
    * @param {boolean} success
    * @param {string} printerName
    * @private
    */
   onAddPrinter_: function(success, printerName) {
     this.$$('add-printer-configuring-dialog').close();
     if (success)
       return;
 
-    if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER) {
+    if (this.previousDialog == AddPrinterDialogs.MANUFACTURER) {
       this.setupFailed = true;
     }
   },
 });