Print Preview: Finish removing global Javascript functions.

chrome/browser/resources/print_preview/print_preview.js

 // Copyright (c) 2012 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.
 
 // TODO(rltoscano): Move data/* into print_preview.data namespace
 
 // <include src="component.js">
 // <include src="print_preview_focus_manager.js">
 //
 
@@ skipping 292 matching lines @@
      * @private
      */
     this.isPreviewGenerationInProgress_ = true;
 
     /**
      * Whether to show system dialog before next printing.
      * @type {boolean}
      * @private
      */
     this.showSystemDialogBeforeNextPrint_ = false;
+
+    /**
+     * Whether the preview is listening for the manipulate-settings-for-test
+     * UI event.
+     * @private {boolean}
+     */
+    this.isListeningForManipulateSettings_ = false;
   }
 
   PrintPreview.prototype = {
     __proto__: print_preview.Component.prototype,
     /**
      * @return {!print_preview.PreviewArea} The preview area. Used for tests.
      */
     getPreviewArea: function() {
       return this.previewArea_;
     },
@@ skipping 11 matching lines @@
       this.listenerTracker.add('print-failed', this.onPrintFailed_.bind(this));
       this.listenerTracker.add(
           'privet-printer-added',
           this.destinationStore_.onPrivetPrinterAdded_.bind(
               this.destinationStore_));
       this.listenerTracker.add(
           'extension-printers-added',
           this.destinationStore_.onExtensionPrintersAdded_.bind(
               this.destinationStore_));
       this.listenerTracker.add(
+          'reload-printer-list',
+          this.destinationStore_.onDestinationsReload_.bind(

[dpapad, 2017/07/07 17:28:43]

Two questions:
1) If onDestinationsReload_ has to be accessed from here, then it shouldn't be
private (and compiler should catch this in theory). Did it not complain?
2) Why are we adding the listener here, instead of within destinationStore_
itself? I see that we are re-using the existing listenerTracker, but this
creates an implicit dependency (in the reverse direction) between
print_preview.js and destination_store.js.

For example: Say there is a destinationStore unit test (meaning there is no
PrintPreview instance, just a DestinationStore instance) that fires
'reload-printer-list' and expects something to happen. It will not happen,
because DestinationStore does not register the listener itself, it relies on its
owning object to register listeners for it.

Same question for PreviewArea and PreviewGenerator.

[rbpotter, 2017/07/07 17:40:13]

1) No, compiler did not complain. Will make it public, as it is also
accessed in line 1011.
2) Listener is added here for the same reason as privet-printer-added above;
DestinationStore (like PreviewGenerator) is not a component so does not have
a listenerTracker or an "exitDocument" function where the listeners can be
removed. Would it be better to add a "removeListeners" public function for
the DestinationStore and PreviewGenerator that PrintPreview and PreviewArea
respectively could call during exitDocument? I think in this case registering
listeners would no longer be dependent on the owning object, although
removing them still would be.

[dpapad, 2017/07/07 18:04:29]

Ok, that's odd. I am wondering if this is related to concatenating files before
sending them to the compiler, since @private does have file-level effect AFAIK.
Thanks for the explanation, it makes sense now. I think what you are proposing
makes the code cleaner.

Mentioning a similar (maybe shorter) alternative: Keep explicitly adding
listeners from the owning object, but minimize the code residing on the owning
objects, as follows:

 - Expose a public addWebUIEventListeners(listenerTracker) from DestinationStore
and PreviewGenerator, passing PrintePreview's and PreviewArea's listenerTracker
respectively.
 - Call that method here, like
this.destinationStore_.addWebUIEventListeners(this.listenerTracker_);
 - Within DestinationStore/PreviewGenerator, register listeners on the
|listenerTracker| that is passed as a parameter, which belongs to the parent.
 - You no longer need to expose a public removeEventListeners(), since they will
be automatically removed on the owning Component's exitDocument().

WDYT?

Also I noticed some potentially existing leaking listeners in
destination_store.js setCloudPrintInterface, see [1] where we basically use an
EventTracker instance, but we never call removeAll(). So, for example if
setCloudPrintInterface() is called twice, we blindly re-register the listeners.
We should either assert that cloudPrintInterface_ is null when the method is
called (if it is only ever called once), or allow multiple calls by adding a
this.tracker_.removeAll() before re-adding the listeners. If that is indeed a
problem, let's leave this for a separate CL.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...

[rbpotter, 2017/07/07 20:45:10]

Done. That does seem a bit cleaner than having to remove in the owning
object, and we can pass in a listener if we need to test these objects
separately for some reason.

RE: setCloudPrintInterface, this should only ever be called once. The
use-cloud-print event is triggered by SendCloudPrintEnable [1], which
is only called in response to getInitialSettings, which happens at
initialization. So I think we should be able to assert both in
DestinationStore and InvitationStore, which has a similar issue, that
cloudPrintInterface_ is null before adding the listeners.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr...

+              this.destinationStore_));
+      this.listenerTracker.add(
           'use-cloud-print', this.onCloudPrintEnable_.bind(this));
+      this.listenerTracker.add(
+          'print-preset-options',
+          this.onPrintPresetOptionsFromDocument_.bind(this));
+      this.listenerTracker.add(
+          'preview-page-count', this.onPageCountReady_.bind(this));
+      this.listenerTracker.add(
+          'enable-manipulate-settings-for-test',
+          this.onEnableManipulateSettingsForTest_.bind(this));
     },
 
     /** @override */
     enterDocument: function() {
-      // Native layer events.
-      var nativeLayerEventTarget = this.nativeLayer_.getEventTarget();
-      this.tracker.add(
-          nativeLayerEventTarget,
-          print_preview.NativeLayer.EventType.PRINT_PRESET_OPTIONS,
-          this.onPrintPresetOptionsFromDocument_.bind(this));
-      this.tracker.add(
-          nativeLayerEventTarget,
-          print_preview.NativeLayer.EventType.PAGE_COUNT_READY,
-          this.onPageCountReady_.bind(this));
-      this.tracker.add(
-          nativeLayerEventTarget,
-          print_preview.NativeLayer.EventType.MANIPULATE_SETTINGS_FOR_TEST,
-          this.onManipulateSettingsForTest_.bind(this));
-
       if ($('system-dialog-link')) {
         this.tracker.add(
             getRequiredElement('system-dialog-link'), 'click',
             this.openSystemPrintDialog_.bind(this));
       }
       if ($('open-pdf-in-preview-link')) {
         this.tracker.add(
             getRequiredElement('open-pdf-in-preview-link'), 'click',
             this.onOpenPdfInPreviewLinkClick_.bind(this));
       }
@@ skipping 443 matching lines @@
       this.isPreviewGenerationInProgress_ = true;
     },
 
     /**
      * Called when the preview area's preview generation is complete.
      * @private
      */
     onPreviewGenerationDone_: function() {
       this.isPreviewGenerationInProgress_ = false;
       this.printHeader_.isPrintButtonEnabled = true;
-      this.nativeLayer_.previewReadyForTest();
+      if (this.isListeningForManipulateSettings_)
+        this.nativeLayer_.previewReadyForTest();
       this.printIfReady_();
     },
 
     /**
      * Called when the preview area's preview failed to load.
      * @private
      */
     onPreviewGenerationFail_: function() {
       this.isPreviewGenerationInProgress_ = false;
       this.printHeader_.isPrintButtonEnabled = false;
@@ skipping 157 matching lines @@
     },
 
     /**
      * Called when the user wants to sign in to Google Cloud Print. Calls the
      * corresponding native layer event.
      * @param {boolean} addAccount Whether to open an 'add a new account' or
      *     default sign in page.
      * @private
      */
     onCloudPrintSignInActivated_: function(addAccount) {
-      this.nativeLayer_.startCloudPrintSignIn(addAccount);
+      this.nativeLayer_.signIn(addAccount)
+          .then(this.destinationStore_.onDestinationsReload_.bind(
+              this.destinationStore_));
     },
 
     /**
      * Updates printing options according to source document presets.
-     * @param {Event} event Contains options from source document.
+     * @param {boolean} disableScaling Whether the document disables scaling.
+     * @param {number} copies The default number of copies from the document.
+     * @param {number} duplex The default duplex setting from the document.
      * @private
      */
-    onPrintPresetOptionsFromDocument_: function(event) {
-      if (event.optionsFromDocument.disableScaling)
+    onPrintPresetOptionsFromDocument_: function(
+        disableScaling, copies, duplex) {
+      if (disableScaling)
         this.documentInfo_.updateIsScalingDisabled(true);
 
-      if (event.optionsFromDocument.copies > 0 &&
-          this.printTicketStore_.copies.isCapabilityAvailable()) {
-        this.printTicketStore_.copies.updateValue(
-            event.optionsFromDocument.copies);
+      if (copies > 0 && this.printTicketStore_.copies.isCapabilityAvailable()) {
+        this.printTicketStore_.copies.updateValue(copies);
       }
 
-      if (event.optionsFromDocument.duplex >= 0 &&
-          this.printTicketStore_.duplex.isCapabilityAvailable()) {
-        this.printTicketStore_.duplex.updateValue(
-            event.optionsFromDocument.duplex);
+      if (duplex >= 0 & this.printTicketStore_.duplex.isCapabilityAvailable()) {
+        this.printTicketStore_.duplex.updateValue(duplex);
       }
     },
 
     /**
      * Called when the Page Count Ready message is received to update the fit to
      * page scaling value in the scaling settings.
-     * @param {Event} event Event object representing the page count ready
-     *     message
+     * @param {number} pageCount The document's page count (unused).
+     * @param {number} previewResponseId The request ID that corresponds to this
+     *     page count (unused).
+     * @param {number} fitToPageScaling The scaling required to fit the document
+     *     to page.
      * @private
      */
-    onPageCountReady_: function(event) {
-      if (event.fitToPageScaling >= 0) {
-        this.scalingSettings_.updateFitToPageScaling(event.fitToPageScaling);
+    onPageCountReady_: function(
+        pageCount, previewResponseId, fitToPageScaling) {
+      if (fitToPageScaling >= 0) {
+        this.scalingSettings_.updateFitToPageScaling(fitToPageScaling);
       }
     },
 
     /**
      * Called when printing to a privet or extension printer fails.
      * @param {*} httpError The HTTP error code, or -1 or a string describing
      *     the error, if not an HTTP error.
      * @private
      */
     onPrintFailed_: function(httpError) {
       console.error('Privet printing failed with error code ' + httpError);
       this.printHeader_.setErrorMessage(
           loadTimeData.getString('couldNotPrint'));
     },
 
     /**
-     * Called when the print preview settings need to be changed for testing.
-     * @param {Event} event Event object that contains the option that is to
-     *     be changed and what to set that option.
+     * Called to start listening for the manipulate-settings-for-test WebUI
+     * event so that settings can be modified by this event.
      * @private
      */
-    onManipulateSettingsForTest_: function(event) {
-      var settings =
-          /** @type {print_preview.PreviewSettings} */ (event.settings);
+    onEnableManipulateSettingsForTest_: function() {
+      this.listenerTracker.add(
+          'manipulate-settings-for-test',
+          this.onManipulateSettingsForTest_.bind(this));
+      this.isListeningForManipulateSettings_ = true;
+    },
+
+    /**
+     * Called when the print preview settings need to be changed for testing.
+     * @param {!print_preview.PreviewSettings} settings Contains print preview
+     *     settings to change and the values to change them to.
+     * @private
+     */
+    onManipulateSettingsForTest_: function(settings) {
       if ('selectSaveAsPdfDestination' in settings) {
         this.saveAsPdfForTest_();  // No parameters.
       } else if ('layoutSettings' in settings) {
         this.setLayoutSettingsForTest_(settings.layoutSettings.portrait);
       } else if ('pageRange' in settings) {
         this.setPageRangeForTest_(settings.pageRange);
       } else if ('headersAndFooters' in settings) {
         this.setHeadersAndFootersForTest_(settings.headersAndFooters);
       } else if ('backgroundColorsAndImages' in settings) {
         this.setBackgroundColorsAndImagesForTest_(
@@ skipping 286 matching lines @@
 // <include src="search/destination_search.js">
 // <include src="search/provisional_destination_resolver.js">
 
 window.addEventListener('DOMContentLoaded', function() {
   var previewWindow = /** @type {{isTest: boolean}} */ (window);
   if (!previewWindow.isTest) {
     var printPreview = new print_preview.PrintPreview();
     printPreview.initialize();
   }
 });