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

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

Index: chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
diff --git a/chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js b/chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
index d2270f7c1a2fe01b487d5cc35d01b936cc3ae8d7..9c7f7cb2c28f87bc3e826145fb1f4a578a014704 100644
--- a/chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
+++ b/chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js
@@ -33,6 +33,25 @@ var AddPrinterDialogs = {
  */
 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',
 
@@ -99,6 +118,9 @@ Polymer({
   /** @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');
   },
@@ -110,10 +132,16 @@ Polymer({
   },
 
   /** @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');
   },
 });
 
@@ -125,20 +153,7 @@ Polymer({
     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.

     },
   },
 
@@ -199,6 +214,9 @@ Polymer({
       type: Array,
     },
 
+    /** @type {string} */
+    previousDialog: String,
+
     setupFailed: {
       type: Boolean,
       value: false,
@@ -263,9 +281,13 @@ Polymer({
   },
 
   /** @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 */
@@ -320,11 +342,6 @@ Polymer({
   behaviors: [WebUIListenerBehavior],
 
   properties: {
-    /** @type {!CupsPrinterInfo} */
-    selectedPrinter: {
-      type: Object,
-    },
-
     /** @type {!CupsPrinterInfo} */
     newPrinter: {
       type: Object,
@@ -338,8 +355,8 @@ Polymer({
 
     configuringDialogTitle: String,
 
-    /** @private {string} */
-    previousDialog_: String,
+    /** @type {string} */
+    previousDialog: String,
 
     /** @private {string} */
     currentDialog_: String,
@@ -373,8 +390,7 @@ Polymer({
   /** Opens the Add printer discovery dialog. */
   open: function() {
     this.resetData_();
-    this.switchDialog_(
-        '', AddPrinterDialogs.MANUALLY, 'showManuallyAddDialog_');
+    this.switchDialog_('', AddPrinterDialogs.DISCOVERY, 'showDiscoveryDialog_');
   },
 
   /**
@@ -382,32 +398,11 @@ Polymer({
    * @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,
@@ -425,12 +420,12 @@ Polymer({
     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(
@@ -446,12 +441,12 @@ Polymer({
 
   /** @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_');
     }
   },
 
@@ -464,7 +459,7 @@ Polymer({
    * @private
    */
   switchDialog_: function(fromDialog, toDialog, domIfBooleanName) {
-    this.previousDialog_ = fromDialog;
+    this.previousDialog = fromDialog;
     this.currentDialog_ = toDialog;
 
     this.set(domIfBooleanName, true);
@@ -481,10 +476,9 @@ Polymer({
    * @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 '';
@@ -500,7 +494,7 @@ Polymer({
     if (success)
       return;
 
-    if (this.previousDialog_ == AddPrinterDialogs.MANUFACTURER) {
+    if (this.previousDialog == AddPrinterDialogs.MANUFACTURER) {
       this.setupFailed = true;
     }
   },