Change the PDF Viewer's PluginController to a singleton

Currently, there is only one instance of a PluginController at a time,
but references to it only live inside PDFViewerBaseElement and derived
classes. Therefore, descendants of the <pdf-viewer> element fire an
event handled by the PDFViewerElement. Making the controller a
singleton allows other elements to directly access the controller using
PluginController.getInstance().

In making PluginController a singleton, the body of the constructor is
moved to an init() method, because getInstance() creates a new object
using the default constructor. Consumers of the PluginController would
need to make sure the singleton has been initialized before using it.

Eventually, InkController will also be altered to be a singleton,
similarly to PluginController.

Bug: 1134208
Change-Id: I5f8a6b565e6fd565e67c41d841ab92fec43e46c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495504Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820941}

chrome/browser/resources/pdf/BUILD.gn

@@ -121,6 +121,7 @@ js_library("controller") {
   deps = [
     ":viewport",
     "//ui/webui/resources/js:assert.m",
+    "//ui/webui/resources/js:cr.m",
     "//ui/webui/resources/js:load_time_data.m",
     "//ui/webui/resources/js:promise_resolver.m",
     "//ui/webui/resources/js/cr:event_target.m",

chrome/browser/resources/pdf/controller.js

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import {assert} from 'chrome://resources/js/assert.m.js';
+import {addSingletonGetter} from 'chrome://resources/js/cr.m.js';
 import {NativeEventTarget as EventTarget} from 'chrome://resources/js/cr/event_target.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {PromiseResolver} from 'chrome://resources/js/promise_resolver.m.js';
@@ -66,6 +67,12 @@ function createToken() {
 export class ContentController {
   constructor() {}
 
+  /** @return {boolean} */
+  get isActive() {}
+
+  /** @param {boolean} isActive */
+  set isActive(isActive) {}
+
   beforeZoom() {}
 
   afterZoom() {}
@@ -121,9 +128,10 @@ export class ContentController {
 }
 
 /**
- * PDF plugin controller, responsible for communicating with the embedded plugin
- * element. Dispatches a 'plugin-message' event containing the message from the
- * plugin, if a message type not handled by this controller is received.
+ * PDF plugin controller singleton, responsible for communicating with the
+ * embedded plugin element. Dispatches a 'plugin-message' event containing the
+ * message from the plugin, if a message type not handled by this controller is
+ * received.
  * @implements {ContentController}
  */
 export class PluginController {
@@ -133,7 +141,10 @@ export class PluginController {
    * @param {function():boolean} getIsUserInitiatedCallback
    * @param {function():?Promise} getLoadedCallback
    */
-  constructor(plugin, viewport, getIsUserInitiatedCallback, getLoadedCallback) {
+  init(plugin, viewport, getIsUserInitiatedCallback, getLoadedCallback) {
+    /** @private {boolean} */
+    this.isActive_ = false;
+
     /** @private {!HTMLEmbedElement} */
     this.plugin_ = plugin;
 
@@ -164,6 +175,23 @@ export class PluginController {
     this.requestResolverMap_ = new Map();
   }
 
+  /**
+   * @return {boolean}
+   * @override
+   */
+  get isActive() {
+    // Check whether `plugin_` is defined as a signal that `init()` was called.
+    return !!this.plugin_ && this.isActive_;
+  }
+
+  /**
+   * @param {boolean} isActive
+   * @override
+   */
+  set isActive(isActive) {
+    this.isActive_ = isActive;
+  }
+
   /**
    * @return {number} A new unique ID.
    * @private
@@ -401,6 +429,7 @@ export class PluginController {
     this.plugin_.style.display = 'block';
     try {
       await this.getLoadedCallback_();
+      this.isActive = true;
     } finally {
       URL.revokeObjectURL(url);
     }
@@ -409,6 +438,7 @@ export class PluginController {
   /** @override */
   unload() {
     this.plugin_.style.display = 'none';
+    this.isActive = false;
   }
 
   /**
@@ -496,3 +526,5 @@ export class PluginController {
     resolver.resolve(messageData);
   }
 }
+
+addSingletonGetter(PluginController);

chrome/browser/resources/pdf/ink_controller.js

@@ -55,6 +55,23 @@ export class InkController {
     this.tool_ = null;
   }
 
+  /**
+   * @return {boolean}
+   * @override
+   */
+  get isActive() {
+    // TODO(crbug.com/1134208): Implement when InkController is a singleton.
+    return false;
+  }
+
+  /**
+   * @param {boolean} isActive
+   * @override
+   */
+  set isActive(isActive) {
+    // TODO(crbug.com/1134208): Implement when InkController is a singleton.
+  }
+
   /** @return {!EventTarget} */
   getEventTarget() {
     return this.eventTarget_;

chrome/browser/resources/pdf/pdf_viewer.js

@@ -24,6 +24,7 @@ import {html} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.
 import {Bookmark} from './bookmark_type.js';
 import {BrowserApi} from './browser_api.js';
 import {Attachment, FittingType, Point, SaveRequestType} from './constants.js';
+import {PluginController} from './controller.js';
 import {ViewerPdfSidenavElement} from './elements/viewer-pdf-sidenav.js';
 import {ViewerPdfToolbarNewElement} from './elements/viewer-pdf-toolbar-new.js';
 import {ViewerThumbnailElement} from './elements/viewer-thumbnail.js';
@@ -381,6 +382,9 @@ export class PDFViewerElement extends PDFViewerBaseElement {
   init(browserApi) {
     super.init(browserApi);
 
+    /** @private {?PluginController} */
+    this.pluginController_ = PluginController.getInstance();
+
     // <if expr="chromeos">
     this.inkController_ = new InkController(
         this.viewport, /** @type {!HTMLDivElement} */ (this.getContent()));
@@ -480,7 +484,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
     switch (e.key) {
       case 'a':
         if (e.ctrlKey || e.metaKey) {
-          this.pluginController.selectAll();
+          this.pluginController_.selectAll();
           // Since we do selection ourselves.
           e.preventDefault();
         }
@@ -559,7 +563,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
     const annotationMode = e.detail;
     if (annotationMode) {
       // Enter annotation mode.
-      assert(this.currentController === this.pluginController);
+      assert(this.pluginController_.isActive);
       // TODO(dstockwell): set plugin read-only, begin transition
       this.updateProgress(0);
 
@@ -574,7 +578,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
 
       // TODO(dstockwell): handle save failure
       const saveResult =
-          await this.pluginController.save(SaveRequestType.ANNOTATION);
+          await this.pluginController_.save(SaveRequestType.ANNOTATION);
       // Data always exists when save is called with requestType = ANNOTATION.
       const result = /** @type {!RequiredSaveResult} */ (saveResult);
       if (result.hasUnsavedChanges) {
@@ -595,11 +599,12 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       this.updateProgress(50);
       await this.inkController_.load(result.fileName, result.dataToSave);
       this.currentController = this.inkController_;
-      this.pluginController.unload();
+      this.pluginController_.unload();
       this.updateProgress(100);
     } else {
       // Exit annotation mode.
       PDFMetrics.record(UserAction.EXIT_ANNOTATION_MODE);
+      assert(!this.pluginController_.isActive);
       assert(this.currentController === this.inkController_);
       // TODO(dstockwell): set ink read-only, begin transition
       this.updateProgress(0);
@@ -607,7 +612,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       // This runs separately to allow other consumers of `loaded` to queue
       // up after this task.
       this.loaded.then(() => {
-        this.currentController = this.pluginController;
+        this.currentController = this.pluginController_;
         this.inkController_.unload();
       });
       // TODO(dstockwell): handle save failure
@@ -618,9 +623,9 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       if (this.pdfViewerUpdateEnabled_) {
         await this.restoreSidenav_();
       }
-      await this.pluginController.load(result.fileName, result.dataToSave);
+      await this.pluginController_.load(result.fileName, result.dataToSave);
       // Ensure the plugin gets the initial viewport.
-      this.pluginController.afterZoom();
+      this.pluginController_.afterZoom();
     }
   }
 
@@ -654,9 +659,8 @@ export class PDFViewerElement extends PDFViewerBaseElement {
    * @private
    */
   onScroll_(e) {
-    if (this.currentController === this.pluginController &&
-        !this.annotationMode_) {
-      this.pluginController.updateScroll(
+    if (this.pluginController_.isActive && !this.annotationMode_) {
+      this.pluginController_.updateScroll(
           e.target.scrollLeft, e.target.scrollTop);
     }
   }
@@ -742,7 +746,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
    * @private
    */
   onPasswordSubmitted_(event) {
-    this.pluginController.getPasswordComplete(event.detail.password);
+    this.pluginController_.getPasswordComplete(event.detail.password);
   }
 
   /** @override */
@@ -785,21 +789,21 @@ export class PDFViewerElement extends PDFViewerBaseElement {
 
     switch (message.data.type.toString()) {
       case 'getSelectedText':
-        this.pluginController.getSelectedText().then(
+        this.pluginController_.getSelectedText().then(
             this.handleSelectedTextReply.bind(this));
         break;
       case 'getThumbnail':
         const getThumbnailData =
             /** @type {GetThumbnailMessageData} */ (message.data);
         const page = getThumbnailData.page;
-        this.pluginController.requestThumbnail(page).then(
+        this.pluginController_.requestThumbnail(page).then(
             this.sendScriptingMessage.bind(this));
         break;
       case 'print':
-        this.pluginController.print();
+        this.pluginController_.print();
         break;
       case 'selectAll':
-        this.pluginController.selectAll();
+        this.pluginController_.selectAll();
         break;
     }
   }
@@ -1088,10 +1092,10 @@ export class PDFViewerElement extends PDFViewerBaseElement {
    * @private
    */
   onPaintThumbnail_(e) {
-    assert(this.currentController === this.pluginController);
+    assert(this.pluginController_.isActive);
     assert(!this.annotationMode_);
     const thumbnail = e.detail;
-    this.pluginController.requestThumbnail(thumbnail.pageNumber)
+    this.pluginController_.requestThumbnail(thumbnail.pageNumber)
         .then(response => {
           const array = new Uint8ClampedArray(response.imageData);
           const imageData = new ImageData(array, response.width);

chrome/browser/resources/pdf/pdf_viewer_base.js

@@ -103,9 +103,6 @@ export class PDFViewerBaseElement extends PolymerElement {
     /** @private {?Viewport} */
     this.viewport_ = null;
 
-    /** @private {?PluginController} */
-    this.pluginController_ = null;
-
     /** @private {?HTMLEmbedElement} */
     this.plugin_ = null;
 
@@ -226,7 +223,7 @@ export class PDFViewerBaseElement extends PolymerElement {
 
     // Parse open pdf parameters.
     this.paramsParser = new OpenPdfParamsParser(destination => {
-      return this.pluginController_.getNamedDestination(destination);
+      return PluginController.getInstance().getNamedDestination(destination);
     });
 
     // Can only reload if we are in a normal tab.
@@ -274,12 +271,16 @@ export class PDFViewerBaseElement extends PolymerElement {
     // Create the plugin.
     this.plugin_ = this.createPlugin_(pdfViewerUpdateEnabled);
     this.getContent().appendChild(this.plugin_);
-    this.pluginController_ = new PluginController(
+
+    const pluginController = PluginController.getInstance();
+    pluginController.init(
         this.plugin_, this.viewport_, () => this.isUserInitiatedEvent,
         () => this.loaded);
-    this.currentController = this.pluginController_;
+    pluginController.isActive = true;
+    this.currentController = pluginController;
+
     this.tracker.add(
-        this.pluginController_.getEventTarget(), 'plugin-message',
+        pluginController.getEventTarget(), 'plugin-message',
         e => this.handlePluginMessage(e));
 
     document.body.addEventListener('change-page-and-xy', e => {
@@ -449,14 +450,6 @@ export class PDFViewerBaseElement extends PolymerElement {
     return assert(this.viewport_);
   }
 
-  /**
-   * @return {!PluginController}
-   * @protected
-   */
-  get pluginController() {
-    return assert(this.pluginController_);
-  }
-
   /**
    * Updates the load state and triggers completion of the `loaded`
    * promise if necessary.

chrome/browser/resources/pdf/pdf_viewer_pp.js

@@ -15,7 +15,7 @@ import {html} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.
 
 import {BrowserApi} from './browser_api.js';
 import {FittingType} from './constants.js';
-import {MessageData, PrintPreviewParams} from './controller.js';
+import {MessageData, PluginController, PrintPreviewParams} from './controller.js';
 import {DeserializeKeyEvent, LoadState, SerializeKeyEvent} from './pdf_scripting_api.js';
 import {PDFViewerBaseElement} from './pdf_viewer_base.js';
 import {DestinationMessageData, DocumentDimensionsMessageData, MessageObject, shouldIgnoreKeyEvents} from './pdf_viewer_utils.js';
@@ -78,6 +78,9 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
   init(browserApi) {
     super.init(browserApi);
 
+    /** @private {?PluginController} */
+    this.pluginController_ = PluginController.getInstance();
+
     this.toolbarManager_ =
         new ToolbarManager(window, null, this.getZoomToolbar_());
 
@@ -112,7 +115,7 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
         break;  // Ensure escape falls through to the print-preview handler.
       case 'a':
         if (e.ctrlKey || e.metaKey) {
-          this.pluginController.selectAll();
+          this.pluginController_.selectAll();
           // Since we do selection ourselves.
           e.preventDefault();
         }
@@ -148,7 +151,7 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
 
   /** @private */
   sendBackgroundColorForPrintPreview_() {
-    this.pluginController.backgroundColorChanged(
+    this.pluginController_.backgroundColorChanged(
         this.dark_ ? PRINT_PREVIEW_DARK_BACKGROUND_COLOR :
                      PRINT_PREVIEW_BACKGROUND_COLOR);
   }
@@ -192,7 +195,7 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
       pageIndicator.style.visibility = 'hidden';
     }
 
-    this.pluginController.viewportChanged();
+    this.pluginController_.viewportChanged();
   }
 
   /** @override */
@@ -209,11 +212,11 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
 
     switch (message.data.type.toString()) {
       case 'getSelectedText':
-        this.pluginController.getSelectedText().then(
+        this.pluginController_.getSelectedText().then(
             this.sendScriptingMessage.bind(this));
         break;
       case 'selectAll':
-        this.pluginController.selectAll();
+        this.pluginController_.selectAll();
         break;
     }
   }
@@ -230,7 +233,7 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
       case 'loadPreviewPage':
         messageData =
             /** @type {{ url:  string, index: number }} */ (messageData);
-        this.pluginController.loadPreviewPage(
+        this.pluginController_.loadPreviewPage(
             messageData.url, messageData.index);
         return true;
       case 'resetPrintPreviewMode':
@@ -249,7 +252,7 @@ class PDFViewerPPElement extends PDFViewerBaseElement {
         this.lastViewportPosition = this.viewport.position;
         this.$$('#page-indicator').pageLabels = messageData.pageNumbers;
 
-        this.pluginController.resetPrintPreviewMode(messageData);
+        this.pluginController_.resetPrintPreviewMode(messageData);
         return true;
       case 'sendKeyEvent':
         this.handleKeyEvent_(/** @type {!KeyboardEvent} */ (DeserializeKeyEvent(