From 7fd5f2dd61a4ae906fcc09b2371fbbb5484af1e6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 27 Feb 2020 15:02:03 +0100 Subject: [PATCH 1/2] [api-minor] Remove the `getGlobalEventBus` viewer functionality (PR 11631 follow-up) The correct/intended way of working with the "viewer components" is by providing an `EventBus` instance upon initialization, and the `getGlobalEventBus` was only added for backwards compatibility. Note, for example, that using `getGlobalEventBus` doesn't really work at all well with a use-case where there's *multiple* `PDFViewer` instances on a one page, since it may then be difficult/impossible to tell which viewer a particular event originated from. All of the "viewer components" examples have been previously updated, such that there's no longer any code/examples which relies on the now removed `getGlobalEventBus` functionality. --- web/base_viewer.js | 3 +-- web/pdf_find_bar.js | 4 ++-- web/pdf_find_controller.js | 4 ++-- web/pdf_history.js | 3 +-- web/pdf_link_service.js | 4 ++-- web/pdf_page_view.js | 3 +-- web/text_layer_builder.js | 3 +-- web/ui_utils.js | 12 ------------ 8 files changed, 10 insertions(+), 26 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index f57cafe66..09257ac5a 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -17,7 +17,6 @@ import { CSS_UNITS, DEFAULT_SCALE, DEFAULT_SCALE_VALUE, - getGlobalEventBus, getVisibleElements, isPortraitOrientation, isValidRotation, @@ -145,7 +144,7 @@ class BaseViewer { this.container = options.container; this.viewer = options.viewer || options.container.firstElementChild; - this.eventBus = options.eventBus || getGlobalEventBus(); + this.eventBus = options.eventBus; this.linkService = options.linkService || new SimpleLinkService(); this.downloadManager = options.downloadManager || null; this.findController = options.findController || null; diff --git a/web/pdf_find_bar.js b/web/pdf_find_bar.js index 2a42d8573..c444c2541 100644 --- a/web/pdf_find_bar.js +++ b/web/pdf_find_bar.js @@ -13,8 +13,8 @@ * limitations under the License. */ -import { getGlobalEventBus, NullL10n } from "./ui_utils.js"; import { FindState } from "./pdf_find_controller.js"; +import { NullL10n } from "./ui_utils.js"; const MATCHES_COUNT_LIMIT = 1000; @@ -38,7 +38,7 @@ class PDFFindBar { this.findResultsCount = options.findResultsCount || null; this.findPreviousButton = options.findPreviousButton || null; this.findNextButton = options.findNextButton || null; - this.eventBus = eventBus || getGlobalEventBus(); + this.eventBus = eventBus; this.l10n = l10n; // Add event listeners to the DOM elements. diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index 1bc3a203b..2068de152 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -13,9 +13,9 @@ * limitations under the License. */ -import { getGlobalEventBus, scrollIntoView } from "./ui_utils.js"; import { createPromiseCapability } from "pdfjs-lib"; import { getCharacterType } from "./pdf_find_utils.js"; +import { scrollIntoView } from "./ui_utils.js"; const FindState = { FOUND: 0, @@ -69,7 +69,7 @@ class PDFFindController { */ constructor({ linkService, eventBus }) { this._linkService = linkService; - this._eventBus = eventBus || getGlobalEventBus(); + this._eventBus = eventBus; this._reset(); eventBus._on("findbarclose", this._onFindBarClose.bind(this)); diff --git a/web/pdf_history.js b/web/pdf_history.js index 4e8127b15..96f240bfc 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -14,7 +14,6 @@ */ import { - getGlobalEventBus, isValidRotation, parseQueryString, waitOnEventOrTimeout, @@ -59,7 +58,7 @@ class PDFHistory { */ constructor({ linkService, eventBus }) { this.linkService = linkService; - this.eventBus = eventBus || getGlobalEventBus(); + this.eventBus = eventBus; this._initialized = false; this._fingerprint = ""; diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index dc8d95732..97e0eb53e 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { getGlobalEventBus, parseQueryString } from "./ui_utils.js"; +import { parseQueryString } from "./ui_utils.js"; /** * @typedef {Object} PDFLinkServiceOptions @@ -44,7 +44,7 @@ class PDFLinkService { externalLinkEnabled = true, ignoreDestinationZoom = false, } = {}) { - this.eventBus = eventBus || getGlobalEventBus(); + this.eventBus = eventBus; this.externalLinkTarget = externalLinkTarget; this.externalLinkRel = externalLinkRel; this.externalLinkEnabled = externalLinkEnabled; diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index f0fb4528c..710e316cc 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -17,7 +17,6 @@ import { approximateFraction, CSS_UNITS, DEFAULT_SCALE, - getGlobalEventBus, getOutputScale, NullL10n, RendererType, @@ -92,7 +91,7 @@ class PDFPageView { this.useOnlyCssZoom = options.useOnlyCssZoom || false; this.maxCanvasPixels = options.maxCanvasPixels || MAX_CANVAS_PIXELS; - this.eventBus = options.eventBus || getGlobalEventBus(); + this.eventBus = options.eventBus; this.renderingQueue = options.renderingQueue; this.textLayerFactory = options.textLayerFactory; this.annotationLayerFactory = options.annotationLayerFactory; diff --git a/web/text_layer_builder.js b/web/text_layer_builder.js index 6c3be2f54..715135da0 100644 --- a/web/text_layer_builder.js +++ b/web/text_layer_builder.js @@ -13,7 +13,6 @@ * limitations under the License. */ -import { getGlobalEventBus } from "./ui_utils.js"; import { renderTextLayer } from "pdfjs-lib"; const EXPAND_DIVS_TIMEOUT = 300; // ms @@ -45,7 +44,7 @@ class TextLayerBuilder { enhanceTextSelection = false, }) { this.textLayerDiv = textLayerDiv; - this.eventBus = eventBus || getGlobalEventBus(); + this.eventBus = eventBus; this.textContent = null; this.textContentItemsStr = []; this.textContentStream = null; diff --git a/web/ui_utils.js b/web/ui_utils.js index 1ef9c45ef..1841860dd 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -884,17 +884,6 @@ class EventBus { } } -let globalEventBus = null; -function getGlobalEventBus(dispatchToDOM = false) { - console.error( - "getGlobalEventBus is deprecated, use a manually created EventBus instance instead." - ); - if (!globalEventBus) { - globalEventBus = new EventBus({ dispatchToDOM }); - } - return globalEventBus; -} - function clamp(v, min, max) { return Math.min(Math.max(v, min), max); } @@ -1013,7 +1002,6 @@ export { SpreadMode, NullL10n, EventBus, - getGlobalEventBus, clamp, ProgressBar, getPDFFileNameFromURL, From 664b79abe0ca4ea6437eda510989276d49a35961 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 27 Feb 2020 15:25:33 +0100 Subject: [PATCH 2/2] [api-minor] Remove the `eventBusDispatchToDOM` option/preference, and thus the general ability to dispatch "viewer components" events to the DOM This functionality was only added to the default viewer for backwards compatibility and to support the various PDF viewer tests in mozilla-central, with the intention to eventually remove it completely. While the different mozilla-central tests cannot be *easily* converted from DOM events, it's however possible to limit that functionality to only MOZCENTRAL builds *and* when tests are running. Rather than depending of the re-dispatching of internal events to the DOM, the default viewer can instead be used in e.g. the following way: ```javascript document.addEventListener("webviewerloaded", function() { PDFViewerApplication.initializedPromise.then(function() { // The viewer has now been initialized, and its properties can be accessed. PDFViewerApplication.eventBus.on("pagerendered", function(event) { console.log("Has rendered page number: " + event.pageNumber); }); }); }); ``` --- extensions/chromium/preferences_schema.json | 4 -- test/unit/ui_utils_spec.js | 50 +-------------------- web/app.js | 10 +++-- web/app_options.js | 5 --- web/firefoxcom.js | 7 +++ web/ui_utils.js | 27 +++++------ 6 files changed, 29 insertions(+), 74 deletions(-) diff --git a/extensions/chromium/preferences_schema.json b/extensions/chromium/preferences_schema.json index 5b428bc6e..add31dae3 100644 --- a/extensions/chromium/preferences_schema.json +++ b/extensions/chromium/preferences_schema.json @@ -58,10 +58,6 @@ "type": "boolean", "default": false }, - "eventBusDispatchToDOM": { - "type": "boolean", - "default": false - }, "pdfBugEnabled": { "title": "Enable debugging tools", "description": "Whether to enable debugging tools.", diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index b60c783d3..132aa3b72 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -312,7 +312,7 @@ describe("ui_utils", function() { expect(count).toEqual(2); }); - it("should not, by default, re-dispatch to DOM", function(done) { + it("should not re-dispatch to DOM", function(done) { if (isNodeJS) { pending("Document in not supported in Node.js."); } @@ -329,54 +329,6 @@ describe("ui_utils", function() { eventBus.dispatch("test"); - Promise.resolve().then(() => { - expect(count).toEqual(1); - - document.removeEventListener("test", domEventListener); - done(); - }); - }); - it("should re-dispatch to DOM", function(done) { - if (isNodeJS) { - pending("Document in not supported in Node.js."); - } - const eventBus = new EventBus({ dispatchToDOM: true }); - let count = 0; - eventBus.on("test", function(evt) { - expect(evt).toEqual(undefined); - count++; - }); - function domEventListener(evt) { - expect(evt.detail).toEqual({}); - count++; - } - document.addEventListener("test", domEventListener); - - eventBus.dispatch("test"); - - Promise.resolve().then(() => { - expect(count).toEqual(2); - - document.removeEventListener("test", domEventListener); - done(); - }); - }); - it("should re-dispatch to DOM, with arguments (without internal listeners)", function(done) { - if (isNodeJS) { - pending("Document in not supported in Node.js."); - } - const eventBus = new EventBus({ dispatchToDOM: true }); - let count = 0; - function domEventListener(evt) { - expect(evt.detail).toEqual({ abc: 123 }); - count++; - } - document.addEventListener("test", domEventListener); - - eventBus.dispatch("test", { - abc: 123, - }); - Promise.resolve().then(() => { expect(count).toEqual(1); diff --git a/web/app.js b/web/app.js index b1ebe23e1..155462fa3 100644 --- a/web/app.js +++ b/web/app.js @@ -125,6 +125,10 @@ class DefaultExternalServices { metaKey: true, }); } + + static get isInAutomation() { + return shadow(this, "isInAutomation", false); + } } const PDFViewerApplication = { @@ -339,13 +343,13 @@ const PDFViewerApplication = { async _initializeViewerComponents() { const appConfig = this.appConfig; - this.overlayManager = new OverlayManager(); - const eventBus = appConfig.eventBus || - new EventBus({ dispatchToDOM: AppOptions.get("eventBusDispatchToDOM") }); + new EventBus({ isInAutomation: this.externalServices.isInAutomation }); this.eventBus = eventBus; + this.overlayManager = new OverlayManager(); + const pdfRenderingQueue = new PDFRenderingQueue(); pdfRenderingQueue.onIdle = this.cleanup.bind(this); this.pdfRenderingQueue = pdfRenderingQueue; diff --git a/web/app_options.js b/web/app_options.js index 3e4736299..3fa1187a1 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -66,11 +66,6 @@ const defaultOptions = { value: false, kind: OptionKind.VIEWER + OptionKind.PREFERENCE, }, - eventBusDispatchToDOM: { - /** @type {boolean} */ - value: false, - kind: OptionKind.VIEWER + OptionKind.PREFERENCE, - }, externalLinkRel: { /** @type {string} */ value: "noopener noreferrer nofollow", diff --git a/web/firefoxcom.js b/web/firefoxcom.js index 75e55a2db..d0167f888 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -345,6 +345,13 @@ class FirefoxExternalServices extends DefaultExternalServices { ); return shadow(this, "supportedMouseWheelZoomModifierKeys", support); } + + static get isInAutomation() { + // Returns the value of `Cu.isInAutomation`, which is only `true` when e.g. + // various test-suites are running in mozilla-central. + const isInAutomation = FirefoxCom.requestSync("isInAutomation"); + return shadow(this, "isInAutomation", isInAutomation); + } } PDFViewerApplication.externalServices = FirefoxExternalServices; diff --git a/web/ui_utils.js b/web/ui_utils.js index 1841860dd..5ea91834a 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -760,6 +760,9 @@ const animationStarted = new Promise(function(resolve) { * NOTE: Only used to support various PDF viewer tests in `mozilla-central`. */ function dispatchDOMEvent(eventName, args = null) { + if (typeof PDFJSDev !== "undefined" && !PDFJSDev.test("MOZCENTRAL")) { + throw new Error("Not implemented: dispatchDOMEvent"); + } const details = Object.create(null); if (args && args.length > 0) { const obj = args[0]; @@ -784,19 +787,11 @@ function dispatchDOMEvent(eventName, args = null) { * and `off` methods. To raise an event, the `dispatch` method shall be used. */ class EventBus { - constructor({ dispatchToDOM = false } = {}) { + constructor(options) { this._listeners = Object.create(null); - this._dispatchToDOM = dispatchToDOM === true; - if ( - typeof PDFJSDev !== "undefined" && - !PDFJSDev.test("MOZCENTRAL || TESTING") && - dispatchToDOM - ) { - console.error( - "The `eventBusDispatchToDOM` option/preference is deprecated, " + - "add event listeners to the EventBus instance rather than the DOM." - ); + if (typeof PDFJSDev === "undefined" || PDFJSDev.test("MOZCENTRAL")) { + this._isInAutomation = (options && options.isInAutomation) === true; } } @@ -819,7 +814,10 @@ class EventBus { dispatch(eventName) { const eventListeners = this._listeners[eventName]; if (!eventListeners || eventListeners.length === 0) { - if (this._dispatchToDOM) { + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("MOZCENTRAL")) && + this._isInAutomation + ) { const args = Array.prototype.slice.call(arguments, 1); dispatchDOMEvent(eventName, args); } @@ -848,7 +846,10 @@ class EventBus { }); externalListeners = null; } - if (this._dispatchToDOM) { + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("MOZCENTRAL")) && + this._isInAutomation + ) { dispatchDOMEvent(eventName, args); } }