Print preview page selection should not require a rerendering of draft pages.

chrome/browser/resources/print_preview/print_preview.js

 // Copyright (c) 2011 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.
 
 // require: cr/ui/print_preview_cloud.js
 
 var localStrings = new LocalStrings();
 
 // If useCloudPrint is true we attempt to connect to cloud print
 // and populate the list of printers with cloud print printers.
@@ skipping 44 matching lines @@
 var colorSettings;
 
 // True if the user has click 'Advanced...' in order to open the system print
 // dialog.
 var showingSystemDialog = false;
 
 // The range of options in the printer dropdown controlled by cloud print.
 var firstCloudPrintOptionPos = 0;
 var lastCloudPrintOptionPos = firstCloudPrintOptionPos;
 
+// Store the current previewUid.
+var currentPreviewUid = '';
+
+// True if we need to generate draft preview data.
+var generateDraftData = true;
 
 // TODO(abodenha@chromium.org) A lot of cloud print specific logic has
 // made its way into this file.  Refactor to create a cleaner boundary
 // between print preview and GCP code.  Reference bug 88098 when fixing.
 
 // A dictionary of cloud printers that have been added to the printer
 // dropdown.
 var addedCloudPrinters = {};
 
 // The maximum number of cloud printers to allow in the dropdown.
@@ skipping 229 matching lines @@
   var settings =
       {'deviceName': deviceName,
        'pageRange': pageSettings.selectedPageRanges,
        'printAll': pageSettings.allPagesRadioButton.checked,
        'duplex': copiesSettings.duplexMode,
        'copies': copiesSettings.numberOfCopies,
        'collate': copiesSettings.isCollated(),
        'landscape': layoutSettings.isLandscape(),
        'color': colorSettings.isColor(),
        'printToPDF': printToPDF,
-       'requestID': 0};
+       'requestID': 0,
+       'generateDraftData': generateDraftData};
 
   var printerList = $('printer-list');
   var selectedPrinter = printerList.selectedIndex;
   if (cloudprint.isCloudPrint(printerList.options[selectedPrinter])) {
     settings['cloudPrintID'] =
         printerList.options[selectedPrinter].value;
   }
   return settings;
 }
 
@@ skipping 89 matching lines @@
 function sendPrintDocumentRequest() {
   var printerList = $('printer-list');
   var printer = printerList[printerList.selectedIndex];
   chrome.send('saveLastPrinter', [printer.textContent,
                                   cloudprint.getData(printer)]);
   chrome.send('print', [JSON.stringify(getSettings()),
                         cloudprint.getPrintTicketJSON(printer)]);
 }
 
 /**
+ * Loads the selected preview pages.
+ */
+function LoadSelectedPages() {

[dpapad, 2011/08/01 17:51:40]

Nit: Function names start with lowercase. Also maybe a more descriptive name
would be displaySelectedPages.

[kmadhusu, 2011/08/02 22:53:45]

Done
I felt loadSelectedPages is more apt for this function, since we are asking the
plugin to load the preview urls.

+  hasPendingPreviewRequest = false;
+  pageSettings.updatePageSelection();
+  var pageSet = pageSettings.previouslySelectedPages;
+  var pageCount = pageSet.length;
+  if (pageCount == 0 || currentPreviewUid == '')
+    return;
+
+  for (var i =0; i < pageCount; i++)
+    onDidPreviewPage(pageSet[i]-1, currentPreviewUid);
+  addEventListeners();
+}
+
+/**
  * Asks the browser to generate a preview PDF based on current print settings.
  */
 function requestPrintPreview() {
   hasPendingPreviewRequest = true;
   removeEventListeners();
   printSettings.save();
+  generateDraftData = true;
   if (!isTabHidden)
     showLoadingAnimation();
 
+  if (previewModifiable && isOnlyPageSettingsChanged()) {
+    LoadSelectedPages();
+    generateDraftData = false;
+  }
+  if (!previewModifiable && pageSettings.totalPageCount > 0)
+    generateDraftData = false;
+
   var settings = getSettings();
   settings.requestID = generatePreviewRequestID();
   chrome.send('getPreview', [JSON.stringify(settings)]);
 }
 
 /**
  * Called from PrintPreviewUI::OnFileSelectionCancelled to notify the print
  * preview tab regarding the file selection cancel event.
  */
 function fileSelectionCancelled() {
@@ skipping 379 matching lines @@
  * Notification that a print preview page has been rendered.
  * Check if the settings have changed and request a regeneration if needed.
  * Called from PrintPreviewUI::OnDidPreviewPage().
  * @param {number} pageNumber The page number, 0-based.
  */
 function onDidPreviewPage(pageNumber, previewUid) {
   // Refactor
   if (!previewModifiable)
     return;
 
   var pageIndex = pageSettings.previouslySelectedPages.indexOf(pageNumber + 1);

[dpapad, 2011/08/01 17:51:40]

Move this to where |pageIndex| is first used.

[kmadhusu, 2011/08/02 22:53:45]

Done.

 
   if (checkIfSettingsChangedAndRegeneratePreview())
     return;
+
+  if (pageIndex == -1)

[dpapad, 2011/08/01 17:51:40]

Is this possible?

[kmadhusu, 2011/08/02 22:53:45]

Yes it is possible now. When the user changes the layout and enter some page
ranges, we generate the draft pages for all preview pages. We only need to load
the requested selected pages. 

+    return;
+
+  currentPreviewUid = previewUid;
   if (pageIndex == 0)
-    createPDFPlugin(previewUid);
+    createPDFPlugin(pageNumber);
 
   $('pdf-viewer').loadPreviewPage(
       getPageSrcURL(previewUid, pageNumber), pageIndex);
 }
 
 /**
  * Update the print preview when new preview data is available.
  * Create the PDF plugin as needed.
  * Called from PrintPreviewUI::PreviewDataIsAvailable().
  * @param {string} jobTitle The print job title.
  * @param {boolean} modifiable If the preview is modifiable.
  * @param {string} previewUid Preview unique identifier.
  * @param {number} previewResponseId The preview request id that resulted in
  *     this response.
  */
 function updatePrintPreview(jobTitle,
                             previewUid,
                             previewResponseId) {
   if (!isExpectedPreviewResponse(previewResponseId))
     return;
   hasPendingPreviewRequest = false;
+  if (!previewModifiable)
+    pageSettings.updatePageSelection();
 
   if (checkIfSettingsChangedAndRegeneratePreview())
     return;
 
   document.title = localStrings.getStringF('printPreviewTitleFormat', jobTitle);
 
   if (!previewModifiable) {
     // If the preview is not modifiable the plugin has not been created yet.
-    createPDFPlugin(previewUid);
+    currentPreviewUid = previewUid;
+    createPDFPlugin(-1);

[dpapad, 2011/08/01 17:51:40]

Create a constant
const ALL_PAGES = -1;
and use 
createPDFPlugin(ALL_PAGES);
for more readability.

[kmadhusu, 2011/08/02 22:53:45]

created const COMPLETE_METAFILE_DATA_INDEX

   }
 
   cr.dispatchSimpleEvent(document, 'updateSummary');
   cr.dispatchSimpleEvent(document, 'updatePrintButton');
   addEventListeners();
 
   if (hasPendingPrintDocumentRequest)
     requestToPrintPendingDocument();
 }
 
@@ skipping 13 matching lines @@
     setDefaultValuesAndRegeneratePreview();
     return true;
   }
   if (pageSettings.requestPrintPreviewIfNeeded())
     return true;
 
   return false;
 }
 
 /**
+ * Check if only page selection has been changed and is valid.

[dpapad, 2011/08/01 17:51:40]

Document since when the page selection is being checked (since last preview
request?).

[kmadhusu, 2011/08/02 22:53:45]

Done.

+ * @return {boolean} true if the nwe page selection is valid.

[dpapad, 2011/08/01 17:51:40]

s/nwe/new

[kmadhusu, 2011/08/02 22:53:45]

Done.

+ */
+function isOnlyPageSettingsChanged() {

[dpapad, 2011/08/01 17:51:40]

Nit (optional): Maybe haveOnlyPageSettingsChanged is more correct grammatically.

[kmadhusu, 2011/08/02 22:53:45]

changed to hasOnlyPageSettingsChanged

+  var tempPrintSettings = new PrintSettings();
+  tempPrintSettings.save();
+
+  return !!(printSettings.deviceName == tempPrintSettings.deviceName &&
+            printSettings.isLandscape == tempPrintSettings.isLandscape &&
+            pageSettings.hasPageSelectionChangedAndIsValid());
+}
+
+/**
  * Create the PDF plugin or reload the existing one.
- * @param {string} previewUid Preview unique identifier.
+ * @param {number} srcDataIndex Preview data source index.

[dpapad, 2011/08/01 17:51:40]

Document the meaning of -1.

[kmadhusu, 2011/08/02 22:53:45]

I think the const variable defined here is descriptive.

  */
-function createPDFPlugin(previewUid) {
+function createPDFPlugin(srcDataIndex) {
   var pdfViewer = $('pdf-viewer');
+  var srcURL = getPageSrcURL(currentPreviewUid, srcDataIndex);
   if (pdfViewer) {
     // Need to call this before the reload(), where the plugin resets its
     // internal page count.
     pdfViewer.goToPage('0');
+    pdfViewer.resetPrintPreviewUrl(srcURL);
     pdfViewer.reload();
     pdfViewer.grayscale(!colorSettings.isColor());
     return;
   }
 
-  // Get the complete preview document.
-  var dataIndex = previewModifiable ? '0' : '-1';
-
   pdfViewer = document.createElement('embed');
   pdfViewer.setAttribute('id', 'pdf-viewer');
+
   pdfViewer.setAttribute('type',
                          'application/x-google-chrome-print-preview-pdf');
-  pdfViewer.setAttribute('src', getPageSrcURL(previewUid, dataIndex));
+  pdfViewer.setAttribute('src', srcURL);
   pdfViewer.setAttribute('aria-live', 'polite');
   pdfViewer.setAttribute('aria-atomic', 'true');
   $('mainview').appendChild(pdfViewer);
   pdfViewer.onload('onPDFLoad()');
   pdfViewer.removePrintButton();
   pdfViewer.grayscale(true);
 }
 
 /**
  * @return {boolean} true if a compatible pdf plugin exists.
  */
 function checkCompatiblePluginExists() {
   var dummyPlugin = $('dummy-viewer')
   return !!(dummyPlugin.onload &&
             dummyPlugin.goToPage &&
             dummyPlugin.removePrintButton &&
             dummyPlugin.loadPreviewPage &&
             dummyPlugin.printPreviewPageCount);

[dpapad, 2011/08/01 17:51:40]

Check also for newly added method resetPrintPreviewUrl.

[kmadhusu, 2011/08/02 22:53:45]

Done.

 }
 
 window.addEventListener('DOMContentLoaded', onLoad);
 
 /**
  * Sets the default values and sends a request to regenerate preview data.
  */
 function setDefaultValuesAndRegeneratePreview() {
   pageSettings.resetState();
   requestPrintPreview();
 }
 
 /**
  * Class that represents the state of the print settings.
  */
 function PrintSettings() {
   this.deviceName = '';
   this.isLandscape = '';
 }
 
 /**
  * Takes a snapshot of the print settings.
  */
 PrintSettings.prototype.save = function() {
   this.deviceName = getSelectedPrinterName();
   this.isLandscape = layoutSettings.isLandscape();
 }