Print Preview: Finish removing global Javascript functions.

chrome/browser/resources/print_preview/print_preview.js

Index: chrome/browser/resources/print_preview/print_preview.js
diff --git a/chrome/browser/resources/print_preview/print_preview.js b/chrome/browser/resources/print_preview/print_preview.js
index c60a4398a9eda8634dd4965a2dfc003aa32385df..dd04e202106f8db1a444e9293fd93b0704a31f5e 100644
--- a/chrome/browser/resources/print_preview/print_preview.js
+++ b/chrome/browser/resources/print_preview/print_preview.js
@@ -310,6 +310,13 @@ cr.define('print_preview', function() {
      * @private
      */
     this.showSystemDialogBeforeNextPrint_ = false;
+
+    /**
+     * Whether the preview is listening for the manipulate-settings-for-test
+     * UI event.
+     * @private {boolean}
+     */
+    this.isListeningForManipulateSettings_ = false;
   }
 
   PrintPreview.prototype = {
@@ -340,27 +347,24 @@ cr.define('print_preview', function() {
           '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',
@@ -824,7 +828,8 @@ cr.define('print_preview', function() {
     onPreviewGenerationDone_: function() {
       this.isPreviewGenerationInProgress_ = false;
       this.printHeader_.isPrintButtonEnabled = true;
-      this.nativeLayer_.previewReadyForTest();
+      if (this.isListeningForManipulateSettings_)
+        this.nativeLayer_.previewReadyForTest();
       this.printIfReady_();
     },
 
@@ -1002,41 +1007,46 @@ cr.define('print_preview', function() {
      * @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);
       }
     },
 
@@ -1052,15 +1062,25 @@ cr.define('print_preview', function() {
           loadTimeData.getString('couldNotPrint'));
     },
 
+    /**
+     * Called to start listening for the manipulate-settings-for-test WebUI
+     * event so that settings can be modified by this event.
+     * @private
+     */
+    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 {Event} event Event object that contains the option that is to
-     *     be changed and what to set that option.
+     * @param {!print_preview.PreviewSettings} settings Contains print preview
+     *     settings to change and the values to change them to.
      * @private
      */
-    onManipulateSettingsForTest_: function(event) {
-      var settings =
-          /** @type {print_preview.PreviewSettings} */ (event.settings);
+    onManipulateSettingsForTest_: function(settings) {
       if ('selectSaveAsPdfDestination' in settings) {
         this.saveAsPdfForTest_();  // No parameters.
       } else if ('layoutSettings' in settings) {