From 898172e9d2f48fd40535554ed3b47f1da2a62bdc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 7 Feb 2024 13:53:29 +0100 Subject: [PATCH 1/3] Re-factor `PDFPrintServiceFactory` to use import maps This is very old code, which can (ever so slightly) be simplified now that import maps are available. --- web/app.js | 17 ++++------------- web/firefox_print_service.js | 22 +++++++++++----------- web/interfaces.js | 21 ++++++++++++++++++++- web/pdf_print_service.js | 21 +++++++++++++-------- web/viewer-geckoview.js | 1 - web/viewer.js | 1 - 6 files changed, 48 insertions(+), 35 deletions(-) diff --git a/web/app.js b/web/app.js index f1736f04b..ea7ec64d1 100644 --- a/web/app.js +++ b/web/app.js @@ -70,6 +70,7 @@ import { PDFHistory } from "./pdf_history.js"; import { PDFLayerViewer } from "web-pdf_layer_viewer"; import { PDFOutlineViewer } from "web-pdf_outline_viewer"; import { PDFPresentationMode } from "web-pdf_presentation_mode"; +import { PDFPrintServiceFactory } from "web-print_service"; import { PDFRenderingQueue } from "./pdf_rendering_queue.js"; import { PDFScriptingManager } from "./pdf_scripting_manager.js"; import { PDFSidebar } from "web-pdf_sidebar"; @@ -731,7 +732,7 @@ const PDFViewerApplication = { }, get supportsPrinting() { - return PDFPrintServiceFactory.instance.supportsPrinting; + return PDFPrintServiceFactory.supportsPrinting; }, get supportsFullscreen() { @@ -1786,7 +1787,7 @@ const PDFViewerApplication = { const optionalContentConfigPromise = this.pdfViewer.optionalContentConfigPromise; - const printService = PDFPrintServiceFactory.instance.createPrintService( + const printService = PDFPrintServiceFactory.createPrintService( this.pdfDocument, pagesOverview, printContainer, @@ -3234,14 +3235,4 @@ function webViewerReportTelemetry({ details }) { PDFViewerApplication.externalServices.reportTelemetry(details); } -/* Abstract factory for the print service. */ -const PDFPrintServiceFactory = { - instance: { - supportsPrinting: false, - createPrintService() { - throw new Error("Not implemented: createPrintService"); - }, - }, -}; - -export { PDFPrintServiceFactory, PDFViewerApplication }; +export { PDFViewerApplication }; diff --git a/web/firefox_print_service.js b/web/firefox_print_service.js index 476839c83..aa1428458 100644 --- a/web/firefox_print_service.js +++ b/web/firefox_print_service.js @@ -20,7 +20,6 @@ import { shadow, } from "pdfjs-lib"; import { getXfaHtmlForPrinting } from "./print_utils.js"; -import { PDFPrintServiceFactory } from "./app.js"; // Creates a placeholder with div and canvas with right size for the page. function composePage( @@ -194,15 +193,16 @@ class FirefoxPrintService { } } -PDFPrintServiceFactory.instance = { - get supportsPrinting() { +/** + * @implements {IPDFPrintServiceFactory} + */ +class PDFPrintServiceFactory { + static get supportsPrinting() { const canvas = document.createElement("canvas"); - const value = "mozPrintCallback" in canvas; + return shadow(this, "supportsPrinting", "mozPrintCallback" in canvas); + } - return shadow(this, "supportsPrinting", value); - }, - - createPrintService( + static createPrintService( pdfDocument, pagesOverview, printContainer, @@ -218,7 +218,7 @@ PDFPrintServiceFactory.instance = { optionalContentConfigPromise, printAnnotationStoragePromise ); - }, -}; + } +} -export { FirefoxPrintService }; +export { PDFPrintServiceFactory }; diff --git a/web/interfaces.js b/web/interfaces.js index 1be741d2e..10c9bb023 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -217,4 +217,23 @@ class IL10n { resume() {} } -export { IDownloadManager, IL10n, IPDFLinkService, IRenderableView }; +/** + * @interface + */ +class IPDFPrintServiceFactory { + static get supportsPrinting() { + return false; + } + + static createPrintService() { + throw new Error("Not implemented: createPrintService"); + } +} + +export { + IDownloadManager, + IL10n, + IPDFLinkService, + IPDFPrintServiceFactory, + IRenderableView, +}; diff --git a/web/pdf_print_service.js b/web/pdf_print_service.js index 845444573..389418f66 100644 --- a/web/pdf_print_service.js +++ b/web/pdf_print_service.js @@ -13,9 +13,9 @@ * limitations under the License. */ -import { AnnotationMode, PixelsPerInch } from "pdfjs-lib"; -import { PDFPrintServiceFactory, PDFViewerApplication } from "./app.js"; +import { AnnotationMode, PixelsPerInch, shadow } from "pdfjs-lib"; import { getXfaHtmlForPrinting } from "./print_utils.js"; +import { PDFViewerApplication } from "./app.js"; let activeService = null; let dialog = null; @@ -355,10 +355,15 @@ function ensureOverlay() { return overlayPromise; } -PDFPrintServiceFactory.instance = { - supportsPrinting: true, +/** + * @implements {IPDFPrintServiceFactory} + */ +class PDFPrintServiceFactory { + static get supportsPrinting() { + return shadow(this, "supportsPrinting", true); + } - createPrintService( + static createPrintService( pdfDocument, pagesOverview, printContainer, @@ -378,7 +383,7 @@ PDFPrintServiceFactory.instance = { printAnnotationStoragePromise ); return activeService; - }, -}; + } +} -export { PDFPrintService }; +export { PDFPrintServiceFactory }; diff --git a/web/viewer-geckoview.js b/web/viewer-geckoview.js index 39edaf9fb..735f66af9 100644 --- a/web/viewer-geckoview.js +++ b/web/viewer-geckoview.js @@ -14,7 +14,6 @@ */ import "web-com"; -import "web-print_service"; import { RenderingStates, ScrollMode, SpreadMode } from "./ui_utils.js"; import { AppOptions } from "./app_options.js"; import { LinkTarget } from "./pdf_link_service.js"; diff --git a/web/viewer.js b/web/viewer.js index fea12640f..dcd13132a 100644 --- a/web/viewer.js +++ b/web/viewer.js @@ -14,7 +14,6 @@ */ import "web-com"; -import "web-print_service"; import { RenderingStates, ScrollMode, SpreadMode } from "./ui_utils.js"; import { AppOptions } from "./app_options.js"; import { LinkTarget } from "./pdf_link_service.js"; From 6da9448f6c1acd8b1bc867eab06fb98de5adba5b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 7 Feb 2024 16:07:27 +0100 Subject: [PATCH 2/3] Remove the `web-com` import map (PR 17588 follow-up) With the changes in PR 17588 we're already importing the relevant code via the `web/app.js` file. --- gulpfile.mjs | 4 ---- test/unit/unit_test.html | 1 - web/viewer-geckoview.html | 1 - web/viewer-geckoview.js | 1 - web/viewer.html | 1 - web/viewer.js | 1 - 6 files changed, 9 deletions(-) diff --git a/gulpfile.mjs b/gulpfile.mjs index 8112e8b15..df641c746 100644 --- a/gulpfile.mjs +++ b/gulpfile.mjs @@ -269,7 +269,6 @@ function createWebpackConfig( const viewerAlias = { "web-alt_text_manager": "web/alt_text_manager.js", "web-annotation_editor_params": "web/annotation_editor_params.js", - "web-com": "", "web-download_manager": "", "web-external_services": "", "web-null_l10n": "", @@ -291,7 +290,6 @@ function createWebpackConfig( libraryAlias["display-fetch_stream"] = "src/display/fetch_stream.js"; libraryAlias["display-network"] = "src/display/network.js"; - viewerAlias["web-com"] = "web/chromecom.js"; viewerAlias["web-download_manager"] = "web/download_manager.js"; viewerAlias["web-external_services"] = "web/chromecom.js"; viewerAlias["web-null_l10n"] = "web/l10n.js"; @@ -306,7 +304,6 @@ function createWebpackConfig( libraryAlias["display-node_stream"] = "src/display/node_stream.js"; libraryAlias["display-node_utils"] = "src/display/node_utils.js"; - viewerAlias["web-com"] = "web/genericcom.js"; viewerAlias["web-download_manager"] = "web/download_manager.js"; viewerAlias["web-external_services"] = "web/genericcom.js"; viewerAlias["web-null_l10n"] = "web/genericl10n.js"; @@ -321,7 +318,6 @@ function createWebpackConfig( viewerAlias[key] = gvAlias[key] || "web/stubs-geckoview.js"; } } - viewerAlias["web-com"] = "web/firefoxcom.js"; viewerAlias["web-download_manager"] = "web/firefoxcom.js"; viewerAlias["web-external_services"] = "web/firefoxcom.js"; viewerAlias["web-null_l10n"] = "web/l10n.js"; diff --git a/test/unit/unit_test.html b/test/unit/unit_test.html index d88bd5bfc..2ddbd960a 100644 --- a/test/unit/unit_test.html +++ b/test/unit/unit_test.html @@ -27,7 +27,6 @@ "web-alt_text_manager": "../../web/alt_text_manager.js", "web-annotation_editor_params": "../../web/annotation_editor_params.js", - "web-com": "../../web/genericcom.js", "web-download_manager": "../../web/download_manager.js", "web-external_services": "../../web/genericcom.js", "web-null_l10n": "../../web/genericl10n.js", diff --git a/web/viewer-geckoview.html b/web/viewer-geckoview.html index 57e28e172..16387bba6 100644 --- a/web/viewer-geckoview.html +++ b/web/viewer-geckoview.html @@ -60,7 +60,6 @@ See https://github.com/adobe-type-tools/cmap-resources "web-alt_text_manager": "./stubs-geckoview.js", "web-annotation_editor_params": "./stubs-geckoview.js", - "web-com": "./genericcom.js", "web-download_manager": "./download_manager.js", "web-external_services": "./genericcom.js", "web-null_l10n": "./genericl10n.js", diff --git a/web/viewer-geckoview.js b/web/viewer-geckoview.js index 735f66af9..31694567a 100644 --- a/web/viewer-geckoview.js +++ b/web/viewer-geckoview.js @@ -13,7 +13,6 @@ * limitations under the License. */ -import "web-com"; import { RenderingStates, ScrollMode, SpreadMode } from "./ui_utils.js"; import { AppOptions } from "./app_options.js"; import { LinkTarget } from "./pdf_link_service.js"; diff --git a/web/viewer.html b/web/viewer.html index d65e56e63..d5545f4ae 100644 --- a/web/viewer.html +++ b/web/viewer.html @@ -69,7 +69,6 @@ See https://github.com/adobe-type-tools/cmap-resources "web-alt_text_manager": "./alt_text_manager.js", "web-annotation_editor_params": "./annotation_editor_params.js", - "web-com": "./genericcom.js", "web-download_manager": "./download_manager.js", "web-external_services": "./genericcom.js", "web-null_l10n": "./genericl10n.js", diff --git a/web/viewer.js b/web/viewer.js index dcd13132a..52879fde7 100644 --- a/web/viewer.js +++ b/web/viewer.js @@ -13,7 +13,6 @@ * limitations under the License. */ -import "web-com"; import { RenderingStates, ScrollMode, SpreadMode } from "./ui_utils.js"; import { AppOptions } from "./app_options.js"; import { LinkTarget } from "./pdf_link_service.js"; From e98b9b019a1238871189e49cb7fa6a4c3816e2b1 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 9 Feb 2024 12:01:59 +0100 Subject: [PATCH 3/3] Break import cycles, in the viewer, for `PDFViewerApplication` Currently the `web/app.js` file pulls in various build-specific dependencies, via the use of import maps, and those files in turn import from `web/app.js` thus creating undesirable import cycles. To avoid this we instead pass in a `PDFViewerApplication`-reference, immediately after it's been created, to the relevant code. Note that we use an ESLint plugin rule, see `import/no-cycle`, that is normally able to catch import cycles. However, in this case import maps are involved which is why this wasn't caught. --- web/app.js | 8 +++++++- web/chromecom.js | 29 ++++++++++++++--------------- web/firefoxcom.js | 32 ++++++++++++++++++-------------- web/genericcom.js | 4 ++-- web/interfaces.js | 2 ++ web/pdf_print_service.js | 8 ++++++-- 6 files changed, 49 insertions(+), 34 deletions(-) diff --git a/web/app.js b/web/app.js index ea7ec64d1..39d73a096 100644 --- a/web/app.js +++ b/web/app.js @@ -53,12 +53,12 @@ import { } from "pdfjs-lib"; import { AppOptions, OptionKind } from "./app_options.js"; import { AutomationEventBus, EventBus } from "./event_utils.js"; +import { ExternalServices, initCom } from "web-external_services"; import { LinkTarget, PDFLinkService } from "./pdf_link_service.js"; import { AltTextManager } from "web-alt_text_manager"; import { AnnotationEditorParams } from "web-annotation_editor_params"; import { CaretBrowsingMode } from "./caret_browsing.js"; import { DownloadManager } from "web-download_manager"; -import { ExternalServices } from "web-external_services"; import { OverlayManager } from "./overlay_manager.js"; import { PasswordPrompt } from "./password_prompt.js"; import { PDFAttachmentViewer } from "web-pdf_attachment_viewer"; @@ -2146,6 +2146,12 @@ const PDFViewerApplication = { }, }; +initCom(PDFViewerApplication); + +if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { + PDFPrintServiceFactory.initGlobals(PDFViewerApplication); +} + if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { const HOSTED_VIEWER_ORIGINS = [ "null", diff --git a/web/chromecom.js b/web/chromecom.js index 6ba500509..c1e3b3f10 100644 --- a/web/chromecom.js +++ b/web/chromecom.js @@ -19,7 +19,6 @@ import { BaseExternalServices } from "./external_services.js"; import { BasePreferences } from "./preferences.js"; import { GenericL10n } from "./genericl10n.js"; import { GenericScripting } from "./generic_scripting.js"; -import { PDFViewerApplication } from "./app.js"; if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("CHROME")) { throw new Error( @@ -42,10 +41,16 @@ if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("CHROME")) { } AppOptions.set("defaultUrl", defaultUrl); +})(); + +let viewerApp = { initialized: false }; +function initCom(app) { + viewerApp = app; + // Ensure that PDFViewerApplication.initialBookmark reflects the current hash, // in case the URL rewrite above results in a different hash. - PDFViewerApplication.initialBookmark = location.hash.slice(1); -})(); + viewerApp.initialBookmark = location.hash.slice(1); +} const ChromeCom = { /** @@ -77,10 +82,9 @@ const ChromeCom = { * Resolves a PDF file path and attempts to detects length. * * @param {string} file - Absolute URL of PDF file. - * @param {OverlayManager} overlayManager - Manager for the viewer overlays. * @param {Function} callback - A callback with resolved URL and file length. */ - resolvePDFFile(file, overlayManager, callback) { + resolvePDFFile(file, callback) { // Expand drive:-URLs to filesystem URLs (Chrome OS) file = file.replace( /^drive:/i, @@ -104,13 +108,9 @@ const ChromeCom = { // Even without this check, the file load in frames is still blocked, // but this may change in the future (https://crbug.com/550151). if (origin && !/^file:|^chrome-extension:/.test(origin)) { - PDFViewerApplication._documentError( - "Blocked " + - origin + - " from loading " + - file + - ". Refused to load a local file in a non-local page " + - "for security reasons." + viewerApp._documentError( + `Blocked ${origin} from loading ${file}. Refused to load ` + + "a local file in a non-local page for security reasons." ); return; } @@ -118,7 +118,7 @@ const ChromeCom = { if (isAllowedAccess) { callback(file); } else { - requestAccessToLocalFile(file, overlayManager, callback); + requestAccessToLocalFile(file, viewerApp.overlayManager, callback); } }); }); @@ -420,7 +420,6 @@ class ExternalServices extends BaseExternalServices { // defaultUrl is set in viewer.js ChromeCom.resolvePDFFile( AppOptions.get("defaultUrl"), - PDFViewerApplication.overlayManager, function (url, length, originalUrl) { callbacks.onOpenWithURL(url, length, originalUrl); } @@ -436,4 +435,4 @@ class ExternalServices extends BaseExternalServices { } } -export { ChromeCom, ExternalServices, Preferences }; +export { ExternalServices, initCom, Preferences }; diff --git a/web/firefoxcom.js b/web/firefoxcom.js index b788f2f29..e16238664 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -18,7 +18,6 @@ import { BaseExternalServices } from "./external_services.js"; import { BasePreferences } from "./preferences.js"; import { DEFAULT_SCALE_VALUE } from "./ui_utils.js"; import { L10n } from "./l10n.js"; -import { PDFViewerApplication } from "./app.js"; if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { throw new Error( @@ -26,6 +25,11 @@ if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { ); } +let viewerApp = { initialized: false }; +function initCom(app) { + viewerApp = app; +} + class FirefoxCom { /** * Creates an event that the extension is listening for and will @@ -167,14 +171,14 @@ class Preferences extends BasePreferences { const findLen = "find".length; const handleEvent = function ({ type, detail }) { - if (!PDFViewerApplication.initialized) { + if (!viewerApp.initialized) { return; } if (type === "findbarclose") { - PDFViewerApplication.eventBus.dispatch(type, { source: window }); + viewerApp.eventBus.dispatch(type, { source: window }); return; } - PDFViewerApplication.eventBus.dispatch("find", { + viewerApp.eventBus.dispatch("find", { source: window, type: type.substring(findLen), query: detail.query, @@ -194,18 +198,18 @@ class Preferences extends BasePreferences { (function listenZoomEvents() { const events = ["zoomin", "zoomout", "zoomreset"]; const handleEvent = function ({ type, detail }) { - if (!PDFViewerApplication.initialized) { + if (!viewerApp.initialized) { return; } // Avoid attempting to needlessly reset the zoom level *twice* in a row, // when using the `Ctrl + 0` keyboard shortcut. if ( type === "zoomreset" && - PDFViewerApplication.pdfViewer.currentScaleValue === DEFAULT_SCALE_VALUE + viewerApp.pdfViewer.currentScaleValue === DEFAULT_SCALE_VALUE ) { return; } - PDFViewerApplication.eventBus.dispatch(type, { source: window }); + viewerApp.eventBus.dispatch(type, { source: window }); }; for (const event of events) { @@ -215,10 +219,10 @@ class Preferences extends BasePreferences { (function listenSaveEvent() { const handleEvent = function ({ type, detail }) { - if (!PDFViewerApplication.initialized) { + if (!viewerApp.initialized) { return; } - PDFViewerApplication.eventBus.dispatch("download", { source: window }); + viewerApp.eventBus.dispatch("download", { source: window }); }; window.addEventListener("save", handleEvent); @@ -226,10 +230,10 @@ class Preferences extends BasePreferences { (function listenEditingEvent() { const handleEvent = function ({ detail }) { - if (!PDFViewerApplication.initialized) { + if (!viewerApp.initialized) { return; } - PDFViewerApplication.eventBus.dispatch("editingaction", { + viewerApp.eventBus.dispatch("editingaction", { source: window, name: detail.name, }); @@ -242,9 +246,9 @@ if (PDFJSDev.test("GECKOVIEW")) { (function listenQueryEvents() { window.addEventListener("pdf.js.query", async ({ detail: { queryId } }) => { let result = null; - if (queryId === "canDownloadInsteadOfPrint") { + if (viewerApp.initialized && queryId === "canDownloadInsteadOfPrint") { result = false; - const { pdfDocument, pdfViewer } = PDFViewerApplication; + const { pdfDocument, pdfViewer } = viewerApp; if (pdfDocument) { try { const hasUnchangedAnnotations = @@ -411,4 +415,4 @@ class ExternalServices extends BaseExternalServices { } } -export { DownloadManager, ExternalServices, FirefoxCom, Preferences }; +export { DownloadManager, ExternalServices, initCom, Preferences }; diff --git a/web/genericcom.js b/web/genericcom.js index f4c48f478..def4988a3 100644 --- a/web/genericcom.js +++ b/web/genericcom.js @@ -25,7 +25,7 @@ if (typeof PDFJSDev !== "undefined" && !PDFJSDev.test("GENERIC")) { ); } -const GenericCom = {}; +function initCom(app) {} class Preferences extends BasePreferences { async _writeToStorage(prefObj) { @@ -47,4 +47,4 @@ class ExternalServices extends BaseExternalServices { } } -export { ExternalServices, GenericCom, Preferences }; +export { ExternalServices, initCom, Preferences }; diff --git a/web/interfaces.js b/web/interfaces.js index 10c9bb023..4c4403128 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -221,6 +221,8 @@ class IL10n { * @interface */ class IPDFPrintServiceFactory { + static initGlobals() {} + static get supportsPrinting() { return false; } diff --git a/web/pdf_print_service.js b/web/pdf_print_service.js index 389418f66..2e2af18d2 100644 --- a/web/pdf_print_service.js +++ b/web/pdf_print_service.js @@ -15,11 +15,11 @@ import { AnnotationMode, PixelsPerInch, shadow } from "pdfjs-lib"; import { getXfaHtmlForPrinting } from "./print_utils.js"; -import { PDFViewerApplication } from "./app.js"; let activeService = null; let dialog = null; let overlayManager = null; +let viewerApp = { initialized: false }; // Renders the page to the canvas of the given print service, and returns // the suggested dimensions of the output page. @@ -338,7 +338,7 @@ function ensureOverlay() { ); } if (!overlayPromise) { - overlayManager = PDFViewerApplication.overlayManager; + overlayManager = viewerApp.overlayManager; if (!overlayManager) { throw new Error("The overlay manager has not yet been initialized."); } @@ -359,6 +359,10 @@ function ensureOverlay() { * @implements {IPDFPrintServiceFactory} */ class PDFPrintServiceFactory { + static initGlobals(app) { + viewerApp = app; + } + static get supportsPrinting() { return shadow(this, "supportsPrinting", true); }