Skip to content

Commit

Permalink
Surface print preview error for too many copies selected
Browse files Browse the repository at this point in the history
Print preview currently has a constant maximum copies allowed (of 1000).
Since we're now reliably getting the max copies supported from each
printer, this change plumbs that max to the print preview UI and
errors the user appropriately.

Main changes:
 - Added separate CopiesCapability and CopiesTicketItem representations,
as per the spec (https://developers.google.com/cloud-print/docs/cdd#cdd)

Bug: chromium:1027830
Test: manually confirmed behavior and tests pass
Change-Id: Ibcf53b9a43328e1846fd485c2e300420832499a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2055351
Commit-Queue: Luum Habtemariam <luum@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Nikita Podguzov <nikitapodguzov@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747969}
  • Loading branch information
Luum Habtemariam authored and Commit Bot committed Mar 7, 2020
1 parent 089a416 commit 9504238
Show file tree
Hide file tree
Showing 27 changed files with 217 additions and 72 deletions.
2 changes: 1 addition & 1 deletion chrome/app/printing_strings.grdp
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
Out of bounds page reference, limit is <ph name="MAXIMUM_PAGE">$1<ex>1</ex></ph>
</message>
<message name="IDS_PRINT_PREVIEW_COPIES_INSTRUCTION" desc="Instruction shown when the user enters an invalid number of copies.">
Use a number to indicate how many copies to print (1 to 999).
Use a number to indicate how many copies to print (1 to <ph name="MAX_COPIES">$1<ex>1</ex></ph>).
</message>
<message name="IDS_PRINT_PREVIEW_SCALING_INSTRUCTION" desc="Instruction shown when the user enters an invalid scaling value.">
Scale amount must be a number between 10 and 200.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
31d6aad61ceca5cbbaf082ac81304b514655f59b
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ TEST_F(PrinterCapabilitiesProviderTest, GetPrinterInfo_NotInstalledPrinter) {

auto capabilities =
std::make_unique<printing::PrinterSemanticCapsAndDefaults>();
capabilities->copies_capable = true;
capabilities->copies_max = 2;
capabilities->color_changeable = true;
// Add printer capabilities to |test_backend_|.
test_backend_->AddValidPrinter(kPrinterId, std::move(capabilities));
Expand All @@ -143,7 +143,7 @@ TEST_F(PrinterCapabilitiesProviderTest, GetPrinterInfo_NotInstalledPrinter) {

EXPECT_TRUE(printer_configurer_->IsConfigured(kPrinterId));
ASSERT_TRUE(capabilities_);
EXPECT_TRUE(capabilities_->copies_capable);
EXPECT_EQ(2, capabilities_->copies_max);
EXPECT_TRUE(capabilities_->color_changeable);
EXPECT_FALSE(capabilities_->color_default);
}
Expand All @@ -169,7 +169,7 @@ TEST_F(PrinterCapabilitiesProviderTest, GetPrinterInfo_InstalledPrinter) {

EXPECT_FALSE(printer_configurer_->IsConfigured(kPrinterId));
ASSERT_TRUE(capabilities_);
EXPECT_FALSE(capabilities_->copies_capable);
EXPECT_EQ(1, capabilities_->copies_max);
}

// Tests that capabilities are cached but not fetched after every
Expand All @@ -195,7 +195,7 @@ TEST_F(PrinterCapabilitiesProviderTest, GetPrinterInfo_CachedCapabilities) {
// capabilities.
EXPECT_EQ(1, test_backend_->capabilities_requests_counter());
ASSERT_TRUE(capabilities_);
EXPECT_FALSE(capabilities_->copies_capable);
EXPECT_EQ(1, capabilities_->copies_max);

base::RunLoop cached_capabilities_run_loop;
printer_capabilities_provider_->GetPrinterCapabilities(
Expand All @@ -208,7 +208,7 @@ TEST_F(PrinterCapabilitiesProviderTest, GetPrinterInfo_CachedCapabilities) {
// GetPrinterSemanticCapsAndDefaults() shouldn't be called again.
EXPECT_EQ(1, test_backend_->capabilities_requests_counter());
ASSERT_TRUE(capabilities_);
EXPECT_FALSE(capabilities_->copies_capable);
EXPECT_EQ(1, capabilities_->copies_max);
}

} // namespace extensions
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ ConstructPrinterCapabilities() {
std::make_unique<printing::PrinterSemanticCapsAndDefaults>();
capabilities->color_model = printing::COLOR;
capabilities->duplex_modes.push_back(printing::SIMPLEX);
capabilities->copies_capable = true;
capabilities->copies_max = 2;
capabilities->dpis.push_back(gfx::Size(kHorizontalDpi, kVerticalDpi));
printing::PrinterSemanticCapsAndDefaults::Paper paper;
paper.vendor_id = kMediaSizeVendorId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@ constexpr char kKind[] = "kind";
constexpr char kIdPattern[] = "idPattern";
constexpr char kNamePattern[] = "namePattern";

constexpr int kMinCopies = 1;
constexpr int kMaxCopies = 999;

idl::PrinterSource PrinterSourceToIdl(chromeos::Printer::Source source) {
switch (source) {
case chromeos::Printer::Source::SRC_USER_PREFS:
Expand Down Expand Up @@ -190,9 +187,7 @@ std::unique_ptr<printing::PrintSettings> ParsePrintTicket(base::Value ticket) {
}

cloud_devices::printer::CopiesTicketItem copies;
if (!copies.LoadFrom(description))
return nullptr;
if (copies.value() < kMinCopies || copies.value() > kMaxCopies)
if (!copies.LoadFrom(description) || copies.value() < 1)
return nullptr;
settings->set_copies(copies.value());

Expand Down Expand Up @@ -229,7 +224,7 @@ bool CheckSettingsAndCapabilitiesCompatibility(
if (settings.collate() && !capabilities.collate_capable)
return false;

if (settings.copies() != 1 && !capabilities.copies_capable)
if (settings.copies() > capabilities.copies_max)
return false;

if (!base::Contains(capabilities.duplex_modes, settings.duplex_mode()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ printing::PrinterSemanticCapsAndDefaults ConstructPrinterCapabilities() {
printing::PrinterSemanticCapsAndDefaults capabilities;
capabilities.color_model = printing::COLOR;
capabilities.duplex_modes.push_back(printing::LONG_EDGE);
capabilities.copies_capable = true;
capabilities.copies_max = 2;
capabilities.dpis.push_back(gfx::Size(kHorizontalDpi, kVerticalDpi));
printing::PrinterSemanticCapsAndDefaults::Paper paper;
paper.vendor_id = kMediaSizeVendorId;
Expand Down Expand Up @@ -210,7 +210,7 @@ TEST(PrintingApiUtilsTest, CheckSettingsAndCapabilitiesCompatibility_Copies) {
std::unique_ptr<printing::PrintSettings> settings = ConstructPrintSettings();
printing::PrinterSemanticCapsAndDefaults capabilities =
ConstructPrinterCapabilities();
capabilities.copies_capable = false;
capabilities.copies_max = 1;
EXPECT_FALSE(
CheckSettingsAndCapabilitiesCompatibility(*settings, capabilities));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ ConstructPrinterCapabilities() {
std::make_unique<printing::PrinterSemanticCapsAndDefaults>();
capabilities->color_model = printing::COLOR;
capabilities->duplex_modes.push_back(printing::SIMPLEX);
capabilities->copies_capable = true;
capabilities->copies_max = 2;
capabilities->dpis.push_back(gfx::Size(kHorizontalDpi, kVerticalDpi));
printing::PrinterSemanticCapsAndDefaults::Paper paper;
paper.vendor_id = kMediaSizeVendorId;
Expand Down
20 changes: 20 additions & 0 deletions chrome/browser/resources/print_preview/data/destination.js
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,17 @@ export class Destination {
return this.isEnterprisePrinter_;
}

/**
* @return (Object} Copies capability of this destination.
* @private
*/
copiesCapability_() {
return this.capabilities && this.capabilities.printer &&
this.capabilities.printer.copies ?
this.capabilities.printer.copies :
null;
}

/**
* @return {Object} Color capability of this destination.
* @private
Expand Down Expand Up @@ -807,6 +818,15 @@ export class Destination {

// </if>

/** @return {boolean} Whether the printer supports copies. */
get hasCopiesCapability() {
const capability = this.copiesCapability_();
if (!capability) {
return false;
}
return capability.max ? capability.max > 1 : true;
}

/**
* @return {boolean} Whether the printer supports both black and white and
* color printing.
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/resources/print_preview/data/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,8 @@ Polymer({
* @private
*/
updateSettingsAvailabilityFromDestination_(caps) {
this.setSettingPath_('copies.available', !!caps && !!caps.copies);
this.setSettingPath_(
'copies.available', this.destination.hasCopiesCapability);
this.setSettingPath_('collate.available', !!caps && !!caps.collate);
this.setSettingPath_(
'color.available', this.destination.hasColorCapability);
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/resources/print_preview/print_preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export {Size} from './data/size.js';
export {Error, State} from './data/state.js';
export {BackgroundGraphicsModeRestriction, NativeLayer} from './native_layer.js';
export {getSelectDropdownBackground} from './print_preview_utils.js';
export {DEFAULT_MAX_COPIES} from './ui/copies_settings.js';
export {DestinationState} from './ui/destination_settings.js';
export {PluginProxy} from './ui/plugin_proxy.js';
export {PreviewAreaState} from './ui/preview_area.js';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
margin-inline-start: 16px;
}
</style>
<print-preview-number-settings-section max-value="999" min-value="1"
<print-preview-number-settings-section max-value="[[copiesMax_]]" min-value="1"
default-value="1" input-label="$i18n{copiesLabel}"
input-aria-label="$i18n{copiesLabel}"
disabled="[[disabled]]" current-value="{{currentValue_}}"
input-valid="{{inputValid_}}" hint-message="$i18n{copiesInstruction}">
input-valid="{{inputValid_}}"
hint-message="[[getHintMessage_(copiesMax_)]]">
<div slot="opt-inside-content" class="checkbox" aria-live="polite"
hidden$="[[collateHidden_(currentValue_, inputValid_)]]">
<cr-checkbox id="collate" on-change="onCollateChange_"
Expand Down
38 changes: 36 additions & 2 deletions chrome/browser/resources/print_preview/ui/copies_settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,18 @@ import 'chrome://resources/cr_elements/cr_checkbox/cr_checkbox.m.js';
import './number_settings_section.js';
import './print_preview_shared_css.js';

import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';

import {SettingsBehavior} from './settings_behavior.js';

/**
* Maximum number of copies supported by the printer if not explicitly
* specified.
* @type {number}
*/
export const DEFAULT_MAX_COPIES = 999;

Polymer({
is: 'print-preview-copies-settings',

Expand All @@ -18,6 +26,14 @@ Polymer({
behaviors: [SettingsBehavior],

properties: {
capability: Object,

/** @private {number} */
copiesMax_: {
type: Number,
computed: 'computeCopiesMax_(capability)',
},

/** @private {string} */
currentValue_: {
type: String,
Expand All @@ -30,8 +46,26 @@ Polymer({
disabled: Boolean,
},

observers:
['onSettingsChanged_(settings.copies.value, settings.collate.value)'],
observers: [
'onSettingsChanged_(settings.copies.value, settings.collate.value)',
],

/**
* @return {number} The maximum number of copies this printer supports.
* @private
*/
computeCopiesMax_() {
return (this.capability && this.capability.max) ? this.capability.max :
DEFAULT_MAX_COPIES;
},

/**
* @return {string} The message to show as hint.
* @private
*/
getHintMessage_() {
return loadTimeData.getStringF('copiesInstruction', this.copiesMax_);
},

/**
* Updates the input string when the setting has been initialized.
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/resources/print_preview/ui/sidebar.html
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
hidden$="[[!settings.pages.available]]" class="settings-section">
</print-preview-pages-settings>
<print-preview-copies-settings settings="[[settings]]"
capability="[[destination.capabilities.printer.copies]]"
disabled="[[controlsDisabled_]]"
hidden$="[[!settings.copies.available]]" class="settings-section">
</print-preview-copies-settings>
Expand Down
2 changes: 1 addition & 1 deletion chrome/common/chrome_utility_printing_messages.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ IPC_STRUCT_TRAITS_END()
IPC_STRUCT_TRAITS_BEGIN(printing::PrinterSemanticCapsAndDefaults)
IPC_STRUCT_TRAITS_MEMBER(collate_capable)
IPC_STRUCT_TRAITS_MEMBER(collate_default)
IPC_STRUCT_TRAITS_MEMBER(copies_capable)
IPC_STRUCT_TRAITS_MEMBER(copies_max)
IPC_STRUCT_TRAITS_MEMBER(duplex_modes)
IPC_STRUCT_TRAITS_MEMBER(duplex_default)
IPC_STRUCT_TRAITS_MEMBER(color_changeable)
Expand Down
38 changes: 37 additions & 1 deletion chrome/test/data/webui/print_preview/copies_settings_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'chrome://print/print_preview.js';
import {DEFAULT_MAX_COPIES} from 'chrome://print/print_preview.js';

import {assert} from 'chrome://resources/js/assert.m.js';
import {triggerInputEvent} from 'chrome://test/print_preview/print_preview_test_utils.js';
Expand All @@ -25,6 +25,42 @@ suite('CopiesSettingsTest', function() {
document.body.appendChild(copiesSection);
});

/**
* Confirms that |max| is currently set as copiesSection's maxCopies.
* @param {number} Expected maximum copies value to check.
*/
async function checkCopiesMax(max) {
const input =
copiesSection.$$('print-preview-number-settings-section').getInput();

// Check that |max| copies is valid.
await triggerInputEvent(input, max.toString(), copiesSection);
assertTrue(copiesSection.getSetting('copies').valid);

// Check that |max| + 1 copies is invalid.
await triggerInputEvent(input, (max + 1).toString(), copiesSection);
assertFalse(copiesSection.getSetting('copies').valid);
}

// Verifies that the copies capability is correctly parsed for the max copies
// supported.
test('set copies max', async () => {
const copiesInput =
copiesSection.$$('print-preview-number-settings-section').getInput();
assertEquals('1', copiesInput.value);
assertFalse(copiesSection.getSetting('copies').setFromUi);

copiesSection.capability = {max: 1234};
await checkCopiesMax(1234);

// Missing and empty capabilities should choose default max copies.
copiesSection.capability = null;
await checkCopiesMax(DEFAULT_MAX_COPIES);

copiesSection.capability = {};
await checkCopiesMax(DEFAULT_MAX_COPIES);
});

test('collate visibility', async () => {
const collateSection = copiesSection.$$('.checkbox');
assertTrue(collateSection.hidden);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,29 @@ suite('ModelSettingsAvailabilityTest', function() {
test('copies', function() {
assertTrue(model.settings.copies.available);

// Remove copies capability.
let capabilities = getCddTemplate(model.destination.id).capabilities;
delete capabilities.printer.copies;
model.set('destination.capabilities', capabilities);
// Set max copies to 1.
let caps = getCddTemplate(model.destination.id).capabilities;
const copiesCap = {max: 1};
caps.printer.copies = copiesCap;
model.set('destination.capabilities', caps);
assertFalse(model.settings.copies.available);

// Copies is no longer available.
// Set max copies to 2 (> 1).
caps = getCddTemplate(model.destination.id).capabilities;
copiesCap.max = 2;
caps.printer.copies = copiesCap;
model.set('destination.capabilities', caps);
assertTrue(model.settings.copies.available);

// Remove copies capability.
caps = getCddTemplate(model.destination.id).capabilities;
delete caps.printer.copies;
model.set('destination.capabilities', caps);
assertFalse(model.settings.copies.available);

// Copies is restored.
capabilities = getCddTemplate(model.destination.id).capabilities;
model.set('destination.capabilities', capabilities);
caps = getCddTemplate(model.destination.id).capabilities;
model.set('destination.capabilities', caps);
assertTrue(model.settings.copies.available);
assertFalse(model.settings.copies.setFromUi);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import {Destination, DestinationCertificateStatus, DestinationConnectionStatus, DestinationOrigin, DestinationStore, DestinationType} from 'chrome://print/print_preview.js';
import {DEFAULT_MAX_COPIES, Destination, DestinationCertificateStatus, DestinationConnectionStatus, DestinationOrigin, DestinationStore, DestinationType} from 'chrome://print/print_preview.js';
import {isChromeOS} from 'chrome://resources/js/cr.m.js';
import {WebUIListenerBehavior} from 'chrome://resources/js/web_ui_listener_behavior.m.js';
import {Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
Expand Down Expand Up @@ -46,7 +46,7 @@ export function getCddTemplate(printerId, opt_printerName) {
printer: {
supported_content_type: [{content_type: 'application/pdf'}],
collate: {default: true},
copies: {default: 1, max: 1000},
copies: {default: 1, max: DEFAULT_MAX_COPIES},
color: {
option: [
{type: 'STANDARD_COLOR', is_default: true},
Expand Down
Loading

0 comments on commit 9504238

Please sign in to comment.