Print Preview: Set settings.pages to the n-up pages

settings.pages is used outside of pages_settings.js for 3 reasons:
(1) To send page numbers to the PDF plugin
(2) To set the page count in the print header
(3) To set the page count in the print ticket

During the development of the N-up printing feature it has become
clear that the n-up pages need to be used in place of the original
document page indices in all 3 of these locations, so we should set
settings.pages to the n-up values.

Note that the print preview ticket still needs to use the page
indices for the original document, in order to know what pages
should be included in the preview. However, since PrintPreviewHandler
expects this setting to be in ranges format, the print preview
ticket uses settings.ranges, which this CL keeps the same as before
(i.e. using the original document's page numbers).

Bug: None
Change-Id: Ieaad8d75e5125c0dac185f407c665b336aaf4334
Reviewed-on: https://chromium-review.googlesource.com/1174704Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584633}

chrome/browser/resources/print_preview/new/header.js

@@ -49,8 +49,7 @@ Polymer({
 
   observers:
       ['update_(settings.copies.value, settings.duplex.value, ' +
-       'settings.pages.value, settings.pagesPerSheet.value, state, ' +
-       'destination.id)'],
+       'settings.pages.value, state, destination.id)'],
 
   /** @private {!print_preview_new.State} */
   lastState_: print_preview_new.State.NOT_READY,
@@ -98,14 +97,6 @@ Polymer({
       numSheets = Math.ceil(numPages / 2);
     }
 
-    const pagesPerSheet = parseInt(this.getSettingValue('pagesPerSheet'), 10);
-
-    if (!Number.isNaN(pagesPerSheet)) {
-      assert(pagesPerSheet > 0);
-      numSheets = Math.ceil(numSheets / pagesPerSheet);
-      numPages = Math.ceil(numPages / pagesPerSheet);
-    }
-
     const copies = parseInt(this.getSettingValue('copies'), 10);
     numSheets *= copies;
     numPages *= copies;

chrome/browser/resources/print_preview/new/pages_settings.html

@@ -2,6 +2,7 @@
 
 <link rel="import" href="checkbox_radio_css.html">
 <link rel="import" href="../data/document_info.html">
+<link rel="import" href="../print_preview_utils.html">
 <link rel="import" href="input_behavior.html">
 <link rel="import" href="input_css.html">
 <link rel="import" href="print_preview_shared_css.html">

chrome/browser/resources/print_preview/new/pages_settings.js

@@ -76,7 +76,8 @@ Polymer({
   },
 
   observers: [
-    'onRangeChange_(errorState_, rangesToPrint_, settings.pages)',
+    'onRangeChange_(errorState_, rangesToPrint_, settings.pages, ' +
+        'settings.pagesPerSheet.value)',
     'onRadioChange_(allSelected_, customSelected_)'
   ],
 
@@ -114,6 +115,15 @@ Polymer({
    * @private
    */
   computeAllPagesArray_: function() {
+    // This computed function will unnecessarily get triggered if
+    // this.documentInfo is set to a new object, which happens whenever the
+    // preview refreshes, but the page count is the same as before. We do not
+    // want to trigger all observers unnecessarily.
+    if (!!this.allPagesArray_ &&
+        this.allPagesArray_.length == this.documentInfo.pageCount) {
+      return this.allPagesArray_;
+    }
+
     const array = new Array(this.documentInfo.pageCount);
     for (let i = 0; i < array.length; i++)
       array[i] = i + 1;
@@ -216,17 +226,22 @@ Polymer({
   },
 
   /**
-   * @return {boolean} Whether pages setting and pagesToPrint_ match.
+   * @return {!Array<number>} The final page numbers, reflecting N-up setting.
+   *     Page numbers are 1 indexed, since these numbers are displayed to the
+   *     user.
+   * @private
    */
-  settingMatches_: function() {
-    const setting = this.getSetting('pages').value;
-    if (setting.length != this.pagesToPrint_.length)
-      return false;
-    for (let index = 0; index < this.pagesToPrint_.length; index++) {
-      if (this.pagesToPrint_[index] != setting[index])
-        return false;
-    }
-    return true;
+  getNupPages_: function() {
+    const pagesPerSheet =
+        /** @type {number} */ (this.getSettingValue('pagesPerSheet'));
+    if (pagesPerSheet <= 1 || this.pagesToPrint_.length == 0)
+      return this.pagesToPrint_;
+
+    const numPages = Math.ceil(this.pagesToPrint_.length / pagesPerSheet);
+    const nupPages = new Array(numPages);
+    for (let i = 0; i < nupPages.length; i++)
+      nupPages[i] = i + 1;
+    return nupPages;
   },
 
   /**
@@ -245,10 +260,16 @@ Polymer({
     }
     this.$.pageSettingsCustomInput.classList.remove('invalid');
     this.setSettingValid('pages', true);
-    if (!this.settingMatches_()) {
-      this.setSetting('pages', this.pagesToPrint_);
-      this.setSetting('ranges', this.rangesToPrint_);
+    const nupPages = this.getNupPages_();
+    const rangesChanged = !areRangesEqual(
+        this.rangesToPrint_,
+        /** @type {!Array} */ (this.getSettingValue('ranges')));
+    if (rangesChanged ||
+        nupPages.length != this.getSettingValue('pages').length) {
+      this.setSetting('pages', nupPages);
     }
+    if (rangesChanged)
+      this.setSetting('ranges', this.rangesToPrint_);
   },
 
   /** @private */

chrome/browser/resources/print_preview/new/preview_area.js

@@ -351,24 +351,10 @@ Polymer({
           this.onPreviewVisualStateChange_.bind(this));
     }
 
-    const pageNumbers =
-        /** @type {!Array<number>} */ (this.getSetting('pages').value);
-    let pageLabels = [];
-    const pagesPerSheet =
-        /** @type {number} */ (this.getSettingValue('pagesPerSheet'));
-    // When pagesPerSheet > 1, page labels should always refer to the N-up
-    // pages.
-    if (pagesPerSheet > 1) {
-      const nupPageCount = Math.ceil(pageNumbers.length / pagesPerSheet);
-      for (let index = 1; index <= nupPageCount; index++)
-        pageLabels.push(index);
-    } else {
-      pageLabels = pageNumbers;
-    }
-
     this.pluginLoaded_ = false;
     this.pluginProxy_.resetPrintPreviewMode(
-        previewUid, index, !this.getSettingValue('color'), pageLabels,
+        previewUid, index, !this.getSettingValue('color'),
+        /** @type {!Array<number>} */ (this.getSettingValue('pages')),
         this.documentInfo.isModifiable);
   },
 

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

@@ -42,13 +42,6 @@ cr.define('header_test', function() {
           available: true,
           key: '',
         },
-        pagesPerSheet: {
-          value: 1,
-          unavailableValue: 1,
-          valid: true,
-          available: true,
-          key: '',
-        },
       };
 
       PolymerTest.clearBody();
@@ -114,30 +107,6 @@ cr.define('header_test', function() {
       assertEquals('Total: 8 sheets of paper', summary.textContent);
     });
 
-    // Tests that the message is correctly adjusted for N-up.
-    test(assert(TestNames.HeaderWithNup), function() {
-      const summary = header.$$('.summary');
-      assertEquals('Total: 1 sheet of paper', summary.textContent);
-      header.setSetting('pagesPerSheet', 4);
-      assertEquals('Total: 1 sheet of paper', summary.textContent);
-      header.setSetting('pages', [1, 2, 3, 4, 5, 6]);
-      assertEquals('Total: 2 sheets of paper', summary.textContent);
-      header.setSetting('duplex', true);
-      assertEquals('Total: 1 sheet of paper', summary.textContent);
-      header.setSetting('pagesPerSheet', 2);
-      assertEquals('Total: 2 sheets of paper', summary.textContent);
-      header.setSetting('pagesPerSheet', 3);
-      assertEquals('Total: 1 sheet of paper', summary.textContent);
-      header.setSetting('copies', 2);
-      assertEquals('Total: 2 sheets of paper', summary.textContent);
-
-      // Check PDF destination
-      header.setSetting('copies', 1);
-      header.setSetting('duplex', false);
-      setPdfDestination();
-      assertEquals('Total: 2 pages', summary.textContent);
-    });
-
     // Tests that the correct message is shown for non-READY states, and that
     // the print button is disabled appropriately.
     test(assert(TestNames.HeaderChangesForState), function() {

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

@@ -270,6 +270,10 @@ TEST_F('PrintPreviewPagesSettingsTest', 'InvalidPageRanges', function() {
   this.runMochaTest(pages_settings_test.TestNames.InvalidPageRanges);
 });
 
+TEST_F('PrintPreviewPagesSettingsTest', 'NupChangesPages', function() {
+  this.runMochaTest(pages_settings_test.TestNames.NupChangesPages);
+});
+
 PrintPreviewRestoreStateTest = class extends NewPrintPreviewTest {
   /** @override */
   get browsePreload() {
@@ -838,11 +842,6 @@ TEST_F('PrintPreviewHeaderTest', 'HeaderWithCopies', function() {
   this.runMochaTest(header_test.TestNames.HeaderWithCopies);
 });
 
-TEST_F('PrintPreviewHeaderTest', 'HeaderWithNup', function() {
-  loadTimeData.overrideValues({pagesPerSheetEnabled: true});
-  this.runMochaTest(header_test.TestNames.HeaderWithNup);
-});
-
 TEST_F('PrintPreviewHeaderTest', 'HeaderChangesForState', function() {
   this.runMochaTest(header_test.TestNames.HeaderChangesForState);
 });

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

@@ -7,6 +7,7 @@ cr.define('pages_settings_test', function() {
   const TestNames = {
     ValidPageRanges: 'valid page ranges',
     InvalidPageRanges: 'invalid page ranges',
+    NupChangesPages: 'nup changes pages',
   };
 
   const suiteName = 'PagesSettingsTest';
@@ -39,6 +40,13 @@ cr.define('pages_settings_test', function() {
           available: true,
           key: '',
         },
+        pagesPerSheet: {
+          value: 1,
+          unavailableValue: 1,
+          valid: true,
+          available: true,
+          key: '',
+        },
       };
       pagesSection.documentInfo = documentInfo;
       pagesSection.disabled = false;
@@ -73,18 +81,18 @@ cr.define('pages_settings_test', function() {
       return test_util.eventToPromise('input-change', pagesSection);
     }
 
-    // Tests that the page ranges set are valid for different user inputs.
-    test(assert(TestNames.ValidPageRanges), function() {
     /** @param {!Array<number>} expectedPages The expected pages value. */
-      const validateState = function(expectedPages) {
+    function validateState(expectedPages) {
       const pagesValue = pagesSection.getSettingValue('pages');
       assertEquals(expectedPages.length, pagesValue.length);
       expectedPages.forEach((page, index) => {
         assertEquals(page, pagesValue[index]);
       });
       assertTrue(pagesSection.$$('.hint').hidden);
-      };
+    }
 
+    // Tests that the page ranges set are valid for different user inputs.
+    test(assert(TestNames.ValidPageRanges), function() {
       const oneToHundred = Array.from({length: 100}, (x, i) => i + 1);
       const tenToHundred = Array.from({length: 91}, (x, i) => i + 10);
 
@@ -126,7 +134,7 @@ cr.define('pages_settings_test', function() {
       const syntaxError = 'Invalid page range, use e.g. 1-5, 8, 11-13';
 
       /** @param {string} expectedMessage The expected error message. */
-      const validateState = function(expectedMessage) {
+      const validateErrorState = function(expectedMessage) {
         assertFalse(pagesSection.$$('.hint').hidden);
         assertEquals(
             expectedMessage, pagesSection.$$('.hint').textContent.trim());
@@ -134,35 +142,84 @@ cr.define('pages_settings_test', function() {
 
       return setupInput('10-100000', 100)
           .then(function() {
-            validateState(limitError + '100');
+            validateErrorState(limitError + '100');
             return setupInput('1, 100000', 100);
           })
           .then(function() {
-            validateState(limitError + '100');
+            validateErrorState(limitError + '100');
             return setupInput('1, 2, 0, 56', 100);
           })
           .then(function() {
-            validateState(syntaxError);
+            validateErrorState(syntaxError);
             return setupInput('-1, 1, 2,, 56', 100);
           })
           .then(function() {
-            validateState(syntaxError);
+            validateErrorState(syntaxError);
             return setupInput('1,2,56-40', 100);
           })
           .then(function() {
-            validateState(syntaxError);
+            validateErrorState(syntaxError);
             return setupInput('101-110', 100);
           })
           .then(function() {
-            validateState(limitError + '100');
+            validateErrorState(limitError + '100');
             return setupInput('1\u30012\u30010\u300156', 100);
           })
           .then(function() {
-            validateState(syntaxError);
+            validateErrorState(syntaxError);
             return setupInput('-1,1,2\u3001\u300156', 100);
           })
           .then(function() {
-            validateState(syntaxError);
+            validateErrorState(syntaxError);
+          });
+    });
+
+    // Tests that the pages are set correctly for different values of pages per
+    // sheet, and that ranges remain fixed (since they are used for generating
+    // the print preview ticket).
+    test(assert(TestNames.NupChangesPages), function() {
+      /**
+       * @param {string} rangesValue The desired stringified ranges setting
+       *     value.
+       */
+      const validateRanges = function(rangesValue) {
+        assertEquals(
+            rangesValue,
+            JSON.stringify(pagesSection.getSettingValue('ranges')));
+      };
+      return setupInput('1, 2, 3, 1, 56', 100)
+          .then(function() {
+            const rangesValue =
+                JSON.stringify(pagesSection.getSettingValue('ranges'));
+            validateState([1, 2, 3, 56]);
+            pagesSection.setSetting('pagesPerSheet', 2);
+            validateRanges(rangesValue);
+            validateState([1, 2]);
+            pagesSection.setSetting('pagesPerSheet', 4);
+            validateRanges(rangesValue);
+            validateState([1]);
+            pagesSection.setSetting('pagesPerSheet', 1);
+            return setupInput('1-3, 6-9, 6-10', 100);
+          })
+          .then(function() {
+            const rangesValue =
+                JSON.stringify(pagesSection.getSettingValue('ranges'));
+            validateState([1, 2, 3, 6, 7, 8, 9, 10]);
+            pagesSection.setSetting('pagesPerSheet', 2);
+            validateRanges(rangesValue);
+            validateState([1, 2, 3, 4]);
+            pagesSection.setSetting('pagesPerSheet', 3);
+            validateRanges(rangesValue);
+            validateState([1, 2, 3]);
+            return setupInput('1-3', 100);
+          })
+          .then(function() {
+            const rangesValue =
+                JSON.stringify(pagesSection.getSettingValue('ranges'));
+            validateState([1]);
+            pagesSection.setSetting('pagesPerSheet', 1);
+            validateRanges(rangesValue);
+            validateState([1, 2, 3]);
           });
     });
   });

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

@@ -110,6 +110,7 @@ cr.define('preview_generation_test', function() {
       return initialize()
           .then(function(args) {
             const originalTicket = JSON.parse(args.printTicket);
+            assertEquals(0, originalTicket.requestID);
             assertEquals(initialTicketValue, originalTicket[ticketKey]);
             nativeLayer.resetResolver('getPreview');
             assertEquals(
@@ -122,7 +123,7 @@ cr.define('preview_generation_test', function() {
                 updatedSettingValue, page.getSettingValue(settingName));
             const ticket = JSON.parse(args.printTicket);
             assertEquals(updatedTicketValue, ticket[ticketKey]);
-            assertEquals(2, ticket.requestID);
+            assertEquals(1, ticket.requestID);
           });
     }
 
@@ -183,6 +184,7 @@ cr.define('preview_generation_test', function() {
             assertEquals(
                 letterOption.height_microns,
                 originalTicket.mediaSize.height_microns);
+            assertEquals(0, originalTicket.requestID);
             nativeLayer.resetResolver('getPreview');
             const mediaSizeSetting = page.getSettingValue('mediaSize');
             assertEquals(
@@ -206,7 +208,7 @@ cr.define('preview_generation_test', function() {
             assertEquals(
                 squareOption.height_microns, ticket.mediaSize.height_microns);
             nativeLayer.resetResolver('getPreview');
-            assertEquals(2, ticket.requestID);
+            assertEquals(1, ticket.requestID);
           });
     });