Print Preview: change getPreview to cr.sendWithPromise

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

Index: chrome/browser/resources/print_preview/previewarea/preview_area.js
diff --git a/chrome/browser/resources/print_preview/previewarea/preview_area.js b/chrome/browser/resources/print_preview/previewarea/preview_area.js
index 994668d6547fee75fa9f4018832ebf7e02e3977d..6fcf41f903a2937c11137c546ed00577098e9423 100644
--- a/chrome/browser/resources/print_preview/previewarea/preview_area.js
+++ b/chrome/browser/resources/print_preview/previewarea/preview_area.js
@@ -171,6 +171,12 @@ cr.define('print_preview', function() {
      * @private
      */
     this.openSystemDialogButton_ = null;
+
+    /**
+     * If this is in a browser test (fake plugin).
+     * @private {boolean}
+     */
+    this.isBrowserTest_ = false;

[dpapad, 2017/06/29 20:58:31]

Is this new boolean necessary? How did the tests work before without it?

[rbpotter, 2017/06/29 22:31:22]

This is used for the InvalidSettings test. Most tests do not care
about what happens after a preview request is sent, so they do not
care if PREVIEW_GENERATION_DONE is ever fired.

I wanted to use the new getPreview() function with resolve/reject
instead of manually triggering PREVIEW_GENERATION_DONE and
SETTINGS_INVALID in this test. onDocumentReady_ is called when 
getPreview is resolved but it also checks the plugin loading state
before issuing PREVIEW_GENERATION_DONE. Since the plugin does not
really exist it looked like the preview was never finished so print
was never called. Thus this boolean was added so for testing
PreviewArea just manually sets the pluginReload state to done as soon
as onDocumentReady_ is called.

As a result this test now checks the full initialize -> getPreview ->
reject and change destination -> getPreview -> resolve logic/event
sequences, instead of just creating the events on its own and
manually setting parts of the UI (e.g. forcing print header to
enabled at the start of the test).

The other thought I had was to create a "PluginStub" to fill in all
the necessary plugin functions and watch for resolutions of getPreview
from the NativeLayerStub to call onLoadCallback each time. Then
PreviewArea.createPlugin_ would be overridden to use the constructor
of this fake plugin. However, this seemed like it might be more 
complicated than necessary. Would that be a better approach?

[dpapad, 2017/06/29 23:28:31]

Thanks for the explanation. I think the way the InvalidSettings test works now
is better than before. Regarding PluginStub, probably a good idea, but I think
we should attempt it separately (in a follow up). Also not sure if we need to
monitor NativeLayerStub to call onLoadCallback. Can't our fake plugin call
onLoadCallback immediately?

   }
 
   /**
@@ -192,7 +198,10 @@ cr.define('print_preview', function() {
 
     // Dispatched when a new document preview is being generated.
     PREVIEW_GENERATION_IN_PROGRESS:
-        'print_preview.PreviewArea.PREVIEW_GENERATION_IN_PROGRESS'
+        'print_preview.PreviewArea.PREVIEW_GENERATION_IN_PROGRESS',
+
+    // Dispatched when invalid printer settings are detected.
+    SETTINGS_INVALID: 'print_preview.PreviewArea.SETTINGS_INVALID'
   };
 
   /**
@@ -349,10 +358,6 @@ cr.define('print_preview', function() {
             this.previewGenerator_,
             print_preview.PreviewGenerator.EventType.PAGE_READY,
             this.onPagePreviewReady_.bind(this));
-        this.tracker.add(
-            this.previewGenerator_,
-            print_preview.PreviewGenerator.EventType.FAIL,
-            this.onPreviewGenerationFail_.bind(this));
         this.tracker.add(
             this.previewGenerator_,
             print_preview.PreviewGenerator.EventType.DOCUMENT_READY,
@@ -523,18 +528,43 @@ cr.define('print_preview', function() {
      * @private
      */
     onTicketChange_: function() {
-      if (this.previewGenerator_ && this.previewGenerator_.requestPreview()) {

[dpapad, 2017/06/29 20:58:31]

Do we know if checking if previewGenerator_ exists is necessary? Also, it seems
a bit odd that in the case where previewGenerator is not there, we still go to
the else case.

If legitimately |previewGenerator_| can be null/undefined here, should the logic
be as follows?

if (!this.previewGenerator_)
  return;

if (this.previewGenerator_.requestPreview()) {
 ...
} else {
 ...
}

Asking because this complicates a bit the code you are modifying.

[rbpotter, 2017/06/29 22:31:22]

Think so, as if the plugin is not compatible preview generator
will not exist. See line 349. I suspect showing margin controls
in this case was needed for the CustomMargins tests as
previewGenerator_ was null for all tests previously. However
now that this has changed it appears we can just return immediately
in the case of a null generator.

-        cr.dispatchSimpleEvent(
-            this, PreviewArea.EventType.PREVIEW_GENERATION_IN_PROGRESS);
-        if (this.loadingTimeout_ == null) {
-          this.loadingTimeout_ = setTimeout(
-              this.showMessage_.bind(
-                  this, print_preview.PreviewAreaMessageId_.LOADING),
-              PreviewArea.LOADING_TIMEOUT_);
-        }
-      } else {
+      var requestPreview = this.previewGenerator_ ?

[dpapad, 2017/06/29 20:58:31]

Nit (optional): |requestPreview| sounds like a function name. Maybe reverse, as
|previewRequest|, or simply |request|.

[rbpotter, 2017/06/29 22:31:22]

Done.

+          this.previewGenerator_.requestPreview() :
+          null;
+      var requestId = requestPreview ? requestPreview.id : -1;
+      if (requestId == -1) {
         this.marginControlContainer_.showMarginControlsIfNeeded();
+        return;
+      }
+      cr.dispatchSimpleEvent(
+          this, PreviewArea.EventType.PREVIEW_GENERATION_IN_PROGRESS);
+      if (this.loadingTimeout_ == null) {
+        this.loadingTimeout_ = setTimeout(
+            this.showMessage_.bind(
+                this, print_preview.PreviewAreaMessageId_.LOADING),
+            PreviewArea.LOADING_TIMEOUT_);
       }
+      requestPreview.request.then(
+          /** @param {number} previewUid The unique id of the preview. */
+          function(previewUid) {
+            this.previewGenerator_.onPreviewGenerationDone(
+                requestPreview.id, previewUid);
+          }.bind(this),
+          /**
+           * @param {*} type The type of print preview failure that
+           *     occurred.
+           */
+          function(type) {
+            if (/** @type{string} */ (type) == 'SETTINGS_INVALID') {
+              this.cancelTimeout();
+              this.showCustomMessage(
+                  loadTimeData.getString('invalidPrinterSettings'));
+              cr.dispatchSimpleEvent(
+                  this, PreviewArea.EventType.SETTINGS_INVALID);
+            } else {
+              this.onPreviewGenerationFail_();
+            }
+          }.bind(this));
     },
 
     /**
@@ -574,6 +604,8 @@ cr.define('print_preview', function() {
      */
     onDocumentReady_: function(event) {
       this.isDocumentReady_ = true;
+      if (this.isBrowserTest_)
+        this.isPluginReloaded_ = true;
       this.dispatchPreviewGenerationDoneIfReady_();
     },
 
@@ -641,6 +673,11 @@ cr.define('print_preview', function() {
       // we don't want this to happen as it can cause the margin to stop
       // being draggable.
       this.plugin_.style.pointerEvents = isDragging ? 'none' : 'auto';
+    },
+
+    /** @param {boolean} isTest Whether this instance is in a browser test. */
+    setIsBrowserTest: function(isTest) {
+      this.isBrowserTest_ = isTest;
     }
   };