[CrOS Printing] Use checkboxes for boolean attributes.

Bug: 964919
Change-Id: I8d8ff498fb54fe07060fdff4c92524c97cea7c1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807116Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Vladislav Kuzkokov <vkuzkokov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701119}

chrome/browser/resources/print_preview/data/destination.js

@@ -94,6 +94,17 @@ cr.define('print_preview', function() {
    */
   let VendorCapabilitySelectOption;
 
+  /**
+   * Same as cloud_devices::printer::TypedValueVendorCapability::ValueType.
+   * @enum {string}
+   */
+  const VendorCapabilityValueType = {
+    BOOLEAN: 'BOOLEAN',
+    FLOAT: 'FLOAT',
+    INTEGER: 'INTEGER',
+    STRING: 'STRING',
+  };
+
   /**
    * Specifies a custom vendor capability.
    * @typedef {{
@@ -106,6 +117,7 @@ cr.define('print_preview', function() {
    *   }|undefined),
    *   typed_value_cap: ({
    *     default: (number | string | boolean | undefined),
+   *     value_type: (print_preview.VendorCapabilityValueType | undefined),
    *   }|undefined),
    *   range_cap: ({
    *     default: (number),
@@ -966,6 +978,7 @@ cr.define('print_preview', function() {
     makeRecentDestination: makeRecentDestination,
     RecentDestination: RecentDestination,
     VendorCapabilitySelectOption: VendorCapabilitySelectOption,
+    VendorCapabilityValueType: VendorCapabilityValueType,
     VendorCapability: VendorCapability,
 
     // <if expr="chromeos">

chrome/browser/resources/print_preview/ui/advanced_settings_item.html

 <link rel="import" href="chrome://resources/html/polymer.html">
 
 <link rel="import" href="chrome://resources/cr_elements/hidden_style_css.html">
+<link rel="import" href="chrome://resources/cr_elements/cr_checkbox/cr_checkbox.html">
 <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html">
 <link rel="import" href="chrome://resources/cr_elements/search_highlight_style_css.html">
 <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
@@ -76,11 +77,16 @@
           </select>
         </div>
       </template>
-      <span hidden$="[[isCapabilityTypeSelect_(capability)]]">
+      <span hidden$="[[!isCapabilityTypeInput_(capability)]]">
         <cr-input type="text" on-input="onUserInput_" spellcheck="false"
             placeholder="[[getCapabilityPlaceholder_(capability)]]">
         </cr-input>
       </span>
+      <span hidden$="[[!isCapabilityTypeCheckbox_(capability)]]">
+        <cr-checkbox on-change="onCheckboxInput_"
+            checked="[[isChecked_(currentValue_)]]">
+        </cr-checkbox>
+      </span>
     </div>
   </template>
   <script src="advanced_settings_item.js"></script>

chrome/browser/resources/print_preview/ui/advanced_settings_item.js

@@ -66,6 +66,33 @@ Polymer({
     return this.capability.type == 'SELECT';
   },
 
+  /**
+   * @return {boolean} Whether the capability represented by this item is
+   *     of type checkbox.
+   * @private
+   */
+  isCapabilityTypeCheckbox_: function() {
+    return this.capability.type == 'TYPED_VALUE' &&
+        this.capability.typed_value_cap.value_type == 'BOOLEAN';
+  },
+
+  /**
+   * @return {boolean} Whether the capability represented by this item is
+   *     of type input.
+   * @private
+   */
+  isCapabilityTypeInput_: function() {
+    return !this.isCapabilityTypeSelect_() && !this.isCapabilityTypeCheckbox_();
+  },
+
+  /**
+   * @return {boolean} Whether the checkbox setting is checked.
+   * @private
+   */
+  isChecked_: function() {
+    return this.currentValue_ == 'true';
+  },
+
   /**
    * @param {!print_preview.VendorCapabilitySelectOption} option The option
    *     for a select capability.
@@ -136,6 +163,14 @@ Polymer({
     this.currentValue_ = e.target.value;
   },
 
+  /**
+   * @param {!Event} e Event containing the new value.
+   * @private
+   */
+  onCheckboxInput_: function(e) {
+    this.currentValue_ = e.target.checked ? 'true' : 'false';
+  },
+
   /**
    * @return {string} The current value of the setting, or the empty string if
    *     it is not set.

chrome/test/data/webui/print_preview/advanced_item_test.js

@@ -7,8 +7,10 @@ cr.define('advanced_item_test', function() {
   const TestNames = {
     DisplaySelect: 'display select',
     DisplayInput: 'display input',
+    DisplayCheckbox: 'display checkbox',
     UpdateSelect: 'update select',
     UpdateInput: 'update input',
+    UpdateCheckbox: 'update checkbox',
     QueryName: 'query name',
     QueryOption: 'query option',
   };
@@ -50,9 +52,9 @@ cr.define('advanced_item_test', function() {
       assertEquals('Recycled', select.options[1].textContent.trim());
       assertEquals('Special', select.options[2].textContent.trim());
 
-      // The input should not be shown for a select capability.
-      const input = item.$$('cr-input');
-      assertTrue(input.parentElement.hidden);
+      // Don't show input or checkbox.
+      assertTrue(item.$$('cr-input').parentElement.hidden);
+      assertTrue(item.$$('cr-checkbox').parentElement.hidden);
     });
 
     test(assert(TestNames.DisplayInput), function() {
@@ -70,8 +72,29 @@ cr.define('advanced_item_test', function() {
       assertFalse(input.parentElement.hidden);
       assertEquals('', input.inputElement.value);
 
-      // No select.
+      // Don't show select or checkbox.
+      assertEquals(null, item.$$('select'));
+      assertTrue(item.$$('cr-checkbox').parentElement.hidden);
+    });
+
+    test(assert(TestNames.DisplayCheckbox), function() {
+      // Create capability
+      item.capability = print_preview_test_utils
+                            .getCddTemplateWithAdvancedSettings(4, 'FooDevice')
+                            .capabilities.printer.vendor_capability[3];
+      Polymer.dom.flush();
+
+      const label = item.$$('.label');
+      assertEquals('Staple', label.textContent);
+
+      // The checkbox should be shown.
+      const checkbox = item.$$('cr-checkbox');
+      assertFalse(checkbox.parentElement.hidden);
+      assertFalse(checkbox.checked);
+
+      // Don't show select or input.
       assertEquals(null, item.$$('select'));
+      assertTrue(item.$$('cr-input').parentElement.hidden);
     });
 
     // Test that a select capability updates correctly when the setting is
@@ -104,6 +127,24 @@ cr.define('advanced_item_test', function() {
       assertEquals('ABCD', input.inputElement.value);
     });
 
+    // Test that an checkbox capability updates correctly when the setting is
+    // updated (e.g. when sticky settings are set).
+    test(assert(TestNames.UpdateCheckbox), function() {
+      // Create capability
+      item.capability = print_preview_test_utils
+                            .getCddTemplateWithAdvancedSettings(4, 'FooDevice')
+                            .capabilities.printer.vendor_capability[3];
+      Polymer.dom.flush();
+
+      // Check that checkbox is unset.
+      const checkbox = item.$$('cr-checkbox');
+      assertFalse(checkbox.checked);
+
+      // Update the setting.
+      item.set('settings.vendorItems.value', {'finishings/4': 'true'});
+      assertTrue(checkbox.checked);
+    });
+
     // Test that the setting is displayed correctly when the search query
     // matches its display name.
     test(assert(TestNames.QueryName), function() {

chrome/test/data/webui/print_preview/print_preview_test_utils.js

@@ -151,6 +151,20 @@ cr.define('print_preview_test_utils', function() {
       }
     });
 
+    if (numSettings < 4) {
+      return template;
+    }
+
+    template.capabilities.printer.vendor_capability.push({
+      display_name: 'Staple',
+      id: 'finishings/4',
+      type: 'TYPED_VALUE',
+      typed_value_cap: {
+        default: '',
+        value_type: 'BOOLEAN',
+      }
+    });
+
     return template;
   }
 

components/printing/common/cloud_print_cdd_conversion.cc

@@ -179,15 +179,15 @@ base::Value PrinterSemanticCapsAndDefaultsToCdd(
           select_capability.AddDefaultOption(
               printer::SelectVendorCapabilityOption(value.name,
                                                     localized_value),
-              value.is_default);
+              value.name == capability.default_value);
         }
         vendor_capabilities.AddOption(printer::VendorCapability(
             capability.name, capability_name, std::move(select_capability)));
       } else {
-        vendor_capabilities.AddOption(printer::VendorCapability(
-            capability.name, capability_name,
-            printer::TypedValueVendorCapability(
-                ToCloudValueType(base::Value::Type::STRING))));
+        vendor_capabilities.AddOption(
+            printer::VendorCapability(capability.name, capability_name,
+                                      printer::TypedValueVendorCapability(
+                                          ToCloudValueType(capability.type))));
       }
     }
     vendor_capabilities.SaveTo(&description);

printing/backend/cups_ipp_advanced_caps.cc

@@ -36,30 +36,19 @@ void ScalarHandler(const CupsOptionProvider& printer,
   capabilities->emplace_back();
   AdvancedCapability& capability = capabilities->back();
   capability.name = attribute_name;
-}
-
-void PopulateBooleanCapability(AdvancedCapability* capability,
-                               bool default_value) {
-  // TODO(crbug.com/964919) Support checkbox in UI instead of making this
-  // two-value enum.
-  capability->values.emplace_back();
-  capability->values.back().name = kOptionFalse;
-  capability->values.back().is_default = !default_value;
-  capability->values.emplace_back();
-  capability->values.back().name = kOptionTrue;
-  capability->values.back().is_default = default_value;
+  capability.type = base::Value::Type::STRING;
+  // TODO(crbug.com/964919) Set defaults.
 }
 
 void BooleanHandler(const CupsOptionProvider& printer,
                     const char* attribute_name,
                     AdvancedCapabilities* capabilities) {
-  ipp_attribute_t* attr_default = printer.GetDefaultOptionValue(attribute_name);
-  bool default_value = attr_default && ippGetBoolean(attr_default, 0);
-
   capabilities->emplace_back();
   AdvancedCapability& capability = capabilities->back();
   capability.name = attribute_name;
-  PopulateBooleanCapability(&capability, default_value);
+  capability.type = base::Value::Type::BOOLEAN;
+  ipp_attribute_t* attr_default = printer.GetDefaultOptionValue(attribute_name);
+  capability.default_value = attr_default && ippGetBoolean(attr_default, 0);
 }
 
 void KeywordHandler(const CupsOptionProvider& printer,
@@ -69,17 +58,15 @@ void KeywordHandler(const CupsOptionProvider& printer,
   if (!attr)
     return;
 
+  capabilities->emplace_back();
+  AdvancedCapability& capability = capabilities->back();
+  capability.name = attribute_name;
   ipp_attribute_t* attr_default = printer.GetDefaultOptionValue(attribute_name);
-  std::string default_value;
   if (attr_default) {
     const char* value = ippGetString(attr_default, 0, nullptr);
     if (value)
-      default_value = value;
+      capability.default_value = value;
   }
-
-  capabilities->emplace_back();
-  AdvancedCapability& capability = capabilities->back();
-  capability.name = attribute_name;
   int num_values = ippGetCount(attr);
   for (int i = 0; i < num_values; i++) {
     const char* value = ippGetString(attr, i, nullptr);
@@ -88,7 +75,6 @@ void KeywordHandler(const CupsOptionProvider& printer,
 
     capability.values.emplace_back();
     capability.values.back().name = value;
-    capability.values.back().is_default = default_value == value;
   }
 }
 
@@ -99,18 +85,17 @@ void EnumHandler(const CupsOptionProvider& printer,
   if (!attr)
     return;
 
-  ipp_attribute_t* attr_default = printer.GetDefaultOptionValue(attribute_name);
-  int default_value = attr_default ? ippGetInteger(attr_default, 0) : 0;
-
   capabilities->emplace_back();
   AdvancedCapability& capability = capabilities->back();
   capability.name = attribute_name;
+  ipp_attribute_t* attr_default = printer.GetDefaultOptionValue(attribute_name);
+  capability.default_value =
+      base::NumberToString(attr_default ? ippGetInteger(attr_default, 0) : 0);
   int num_values = ippGetCount(attr);
   for (int i = 0; i < num_values; i++) {
     int value = ippGetInteger(attr, i);
     capability.values.emplace_back();
     capability.values.back().name = base::NumberToString(value);
-    capability.values.back().is_default = default_value == value;
   }
 }
 
@@ -133,9 +118,8 @@ void MultivalueEnumHandler(int none_value,
     AdvancedCapability& capability = capabilities->back();
     capability.name =
         std::string(attribute_name) + "/" + base::NumberToString(value);
-
-    // TODO(crbug.com/964919) Get correct defaults.
-    PopulateBooleanCapability(&capability, false);
+    capability.type = base::Value::Type::BOOLEAN;
+    // TODO(crbug.com/964919) Set defaults.
   }
 }
 

printing/backend/cups_ipp_constants.cc

@@ -30,8 +30,8 @@ constexpr char kIppJobAttributes[] = "job-creation-attributes";  // PWG 5100.11
 
 constexpr char kPinEncryptionNone[] = "none";
 
-constexpr char kOptionFalse[] = "no";
-constexpr char kOptionTrue[] = "yes";
+constexpr char kOptionFalse[] = "false";
+constexpr char kOptionTrue[] = "true";
 
 #endif  // defined(OS_CHROMEOS)
 

printing/backend/cups_ipp_util_unittest.cc

@@ -268,9 +268,13 @@ TEST_F(PrintBackendCupsIppUtilTest, AdvancedCaps) {
 
   EXPECT_EQ(6u, caps.advanced_capabilities.size());
   EXPECT_EQ("ipp-attribute-fidelity", caps.advanced_capabilities[0].name);
+  EXPECT_EQ(base::Value::Type::BOOLEAN, caps.advanced_capabilities[0].type);
   EXPECT_EQ("finishings/7", caps.advanced_capabilities[1].name);
+  EXPECT_EQ(base::Value::Type::BOOLEAN, caps.advanced_capabilities[1].type);
   EXPECT_EQ("finishings/10", caps.advanced_capabilities[2].name);
+  EXPECT_EQ(base::Value::Type::BOOLEAN, caps.advanced_capabilities[2].type);
   EXPECT_EQ("job-name", caps.advanced_capabilities[3].name);
+  EXPECT_EQ(base::Value::Type::STRING, caps.advanced_capabilities[3].type);
   EXPECT_EQ("output-bin", caps.advanced_capabilities[4].name);
   EXPECT_EQ(2u, caps.advanced_capabilities[4].values.size());
   EXPECT_EQ("print-quality", caps.advanced_capabilities[5].name);

printing/backend/print_backend.h

@@ -58,9 +58,6 @@ struct PRINTING_EXPORT AdvancedCapabilityValue {
 
   // Localized name for the value.
   std::string display_name;
-
-  // True iff this is default value.
-  bool is_default = false;
 };
 
 struct PRINTING_EXPORT AdvancedCapability {
@@ -74,6 +71,12 @@ struct PRINTING_EXPORT AdvancedCapability {
   // Localized name for the attribute.
   std::string display_name;
 
+  // Attribute type.
+  base::Value::Type type;
+
+  // Default value.
+  std::string default_value;
+
   // Values for enumerated attributes.
   std::vector<AdvancedCapabilityValue> values;
 };