Print Preview: change getPreview to cr.sendWithPromise

chrome/browser/resources/print_preview/previewarea/preview_area.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.
 
 cr.exportPath('print_preview');
 
 /**
  * Enumeration of IDs shown in the preview area.
  * @enum {string}
  * @private
@@ skipping 153 matching lines @@
      * @private
      */
     this.overlayEl_ = null;
 
     /**
      * The "Open system dialog" button.
      * @type {HTMLButtonElement}
      * @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?

   }
 
   /**
    * Event types dispatched by the preview area.
    * @enum {string}
    */
   PreviewArea.EventType = {
     // Dispatched when the "Open system dialog" button is clicked.
     OPEN_SYSTEM_DIALOG_CLICK:
         'print_preview.PreviewArea.OPEN_SYSTEM_DIALOG_CLICK',
 
     // Dispatched when the document preview is complete.
     PREVIEW_GENERATION_DONE:
         'print_preview.PreviewArea.PREVIEW_GENERATION_DONE',
 
     // Dispatched when the document preview failed to be generated.
     PREVIEW_GENERATION_FAIL:
         'print_preview.PreviewArea.PREVIEW_GENERATION_FAIL',
 
     // 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'
   };
 
   /**
    * CSS classes used by the preview area.
    * @enum {string}
    * @private
    */
   PreviewArea.Classes_ = {
     OUT_OF_PROCESS_COMPATIBILITY_OBJECT:
         'preview-area-compatibility-object-out-of-process',
@@ skipping 138 matching lines @@
         this.tracker.add(
             this.previewGenerator_,
             print_preview.PreviewGenerator.EventType.PREVIEW_START,
             this.onPreviewStart_.bind(this));
         this.tracker.add(
             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,
             this.onDocumentReady_.bind(this));
       } else {
         this.showCustomMessage(loadTimeData.getString('noPlugin'));
       }
     },
 
     /** @override */
     exitDocument: function() {
       print_preview.Component.prototype.exitDocument.call(this);
@@ skipping 148 matching lines @@
       setIsVisible(openSystemDialogThrobber, true);
       cr.dispatchSimpleEvent(
           this, PreviewArea.EventType.OPEN_SYSTEM_DIALOG_CLICK);
     },
 
     /**
      * Called when the print ticket changes. Updates the preview.
      * @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));
     },
 
     /**
      * Called when the preview generator begins loading the preview.
      * @param {Event} event Contains the URL to initialize the plugin to.
      * @private
      */
     onPreviewStart_: function(event) {
       this.isDocumentReady_ = false;
       this.isPluginReloaded_ = false;
@@ skipping 19 matching lines @@
       this.plugin_.loadPreviewPage(event.previewUrl, event.previewIndex);
     },
 
     /**
      * Called when the preview generation is complete and the document is ready
      * to print.
      * @private
      */
     onDocumentReady_: function(event) {
       this.isDocumentReady_ = true;
+      if (this.isBrowserTest_)
+        this.isPluginReloaded_ = true;
       this.dispatchPreviewGenerationDoneIfReady_();
     },
 
     /**
      * Cancels the timeout so that an error message can be shown.
      */
     cancelTimeout: function() {
       if (this.loadingTimeout_) {
         clearTimeout(this.loadingTimeout_);
         this.loadingTimeout_ = null;
@@ skipping 47 matching lines @@
      */
     onMarginDragChanged_: function(isDragging) {
       if (!this.plugin_)
         return;
 
       // When hovering over the plugin (which may be in a separate iframe)
       // pointer events will be sent to the frame. When dragging the margins,
       // 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;
     }
   };
 
   // Export
   return {PreviewArea: PreviewArea};
 });