From 5b94ed54878c22c80befad999ed633b2751fee93 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 6 Aug 2020 12:43:59 +0200 Subject: [PATCH] Remove the `disableCreateObjectURL` option from `web/app_options.js` Prior to PR 11601, the `disableCreateObjectURL` option was present on `getDocument` in the API, since it was (potentially) used when decoding JPEG images natively in the browser. Hence setting this option, which was done automatically using compatibility-code, were in some browsers necessary in order for e.g. JPEG images to be correctly rendered. The downside of the `disableCreateObjectURL` option is that memory usage increases significantly, since we're forced to build and use `data:` URIs (rather than `blob:` URLs). However, at this point in time the `disableCreateObjectURL` option is only necessary for *some* (non-essential) functionality in the default viewer; in particular: - The openfile functionality, used only when manually opening a new file in the default viewer. - The download functionality, used when downloading either the PDF document itself or its attached files (if such exists). - The print functionality, in the generic `PDFPrintService` implementation. Hence neither the general PDF.js library, nor the *basic* functionality of the default viewer, depends on the `disableCreateObjectURL` option any more; which is why I'm thus proposing that we remove the option since using it is a performance footgun. *Please note:* To not outright break currently "supported" browsers, which lack proper `URL.createObjectURL` support, this patch purposely keeps the compatibility-code to explicitly disable `URL.createObjectURL` usage *only* for browsers which are known to not work correctly.[1] While it's certainly possible that there's additional, likely older, browsers with broken `URL.createObjectURL` support, the last time that these types of problems were reported was over *three* years ago.[2] Hence in the *very* unlikely event that additional problems occur, as a result of these changes, we can either add a new case in the compatibility-code or simply declare the affected browser as unsupported. --- [1] Which are IE11 (see issue 3977), and Google Chrome on iOS (see PR 8081). [2] Given that `URL.createObjectURL` is used by default, you'd really expect more reports if these problems were widespread. --- web/app.js | 7 +++---- web/app_options.js | 6 ------ web/download_manager.js | 11 ++--------- web/firefoxcom.js | 4 ---- web/pdf_attachment_viewer.js | 3 ++- web/pdf_print_service.js | 8 +++++--- 6 files changed, 12 insertions(+), 27 deletions(-) diff --git a/web/app.js b/web/app.js index 1dc3859db..b8b1a0a1e 100644 --- a/web/app.js +++ b/web/app.js @@ -72,6 +72,7 @@ import { PDFThumbnailViewer } from "./pdf_thumbnail_viewer.js"; import { PDFViewer } from "./pdf_viewer.js"; import { SecondaryToolbar } from "./secondary_toolbar.js"; import { Toolbar } from "./toolbar.js"; +import { viewerCompatibilityParams } from "./viewer_compatibility.js"; import { ViewHistory } from "./view_history.js"; const DEFAULT_SCALE_DELTA = 1.1; @@ -408,9 +409,7 @@ const PDFViewerApplication = { }); this.pdfLinkService = pdfLinkService; - const downloadManager = this.externalServices.createDownloadManager({ - disableCreateObjectURL: AppOptions.get("disableCreateObjectURL"), - }); + const downloadManager = this.externalServices.createDownloadManager(); this.downloadManager = downloadManager; const findController = new PDFFindController({ @@ -2225,7 +2224,7 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { } const file = evt.fileInput.files[0]; - if (!AppOptions.get("disableCreateObjectURL")) { + if (!viewerCompatibilityParams.disableCreateObjectURL) { let url = URL.createObjectURL(file); if (file.name) { url = { url, originalUrl: file.name }; diff --git a/web/app_options.js b/web/app_options.js index a38542649..27433d1ed 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -42,12 +42,6 @@ const defaultOptions = { value: "", kind: OptionKind.VIEWER + OptionKind.PREFERENCE, }, - disableCreateObjectURL: { - /** @type {boolean} */ - value: false, - compatibility: viewerCompatibilityParams.disableCreateObjectURL, - kind: OptionKind.VIEWER, - }, disableHistory: { /** @type {boolean} */ value: false, diff --git a/web/download_manager.js b/web/download_manager.js index 00ec1675d..d5b6565f1 100644 --- a/web/download_manager.js +++ b/web/download_manager.js @@ -23,9 +23,6 @@ if (typeof PDFJSDev !== "undefined" && !PDFJSDev.test("CHROME || GENERIC")) { ); } -const DISABLE_CREATE_OBJECT_URL = - viewerCompatibilityParams.disableCreateObjectURL || false; - function download(blobUrl, filename) { const a = document.createElement("a"); if (!a.click) { @@ -46,10 +43,6 @@ function download(blobUrl, filename) { } class DownloadManager { - constructor({ disableCreateObjectURL = DISABLE_CREATE_OBJECT_URL }) { - this.disableCreateObjectURL = disableCreateObjectURL; - } - downloadUrl(url, filename) { if (!createValidAbsoluteUrl(url, "http://example.com")) { return; // restricted/invalid URL @@ -66,7 +59,7 @@ class DownloadManager { const blobUrl = createObjectURL( data, contentType, - this.disableCreateObjectURL + viewerCompatibilityParams.disableCreateObjectURL ); download(blobUrl, filename); } @@ -80,7 +73,7 @@ class DownloadManager { return; } - if (this.disableCreateObjectURL) { + if (viewerCompatibilityParams.disableCreateObjectURL) { // URL.createObjectURL is not supported this.downloadUrl(url, filename); return; diff --git a/web/firefoxcom.js b/web/firefoxcom.js index e14cdcfc2..985ca0530 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -88,10 +88,6 @@ const FirefoxCom = (function FirefoxComClosure() { })(); class DownloadManager { - constructor(options) { - this.disableCreateObjectURL = false; - } - downloadUrl(url, filename) { FirefoxCom.request("download", { originalUrl: url, diff --git a/web/pdf_attachment_viewer.js b/web/pdf_attachment_viewer.js index b9bd851bf..0543cd6c5 100644 --- a/web/pdf_attachment_viewer.js +++ b/web/pdf_attachment_viewer.js @@ -15,6 +15,7 @@ import { createPromiseCapability, getFilenameFromUrl } from "pdfjs-lib"; import { BaseTreeViewer } from "./base_tree_viewer.js"; +import { viewerCompatibilityParams } from "./viewer_compatibility.js"; /** * @typedef {Object} PDFAttachmentViewerOptions @@ -169,7 +170,7 @@ class PDFAttachmentViewer extends BaseTreeViewer { const element = document.createElement("a"); if ( /\.pdf$/i.test(filename) && - !this.downloadManager.disableCreateObjectURL + !viewerCompatibilityParams.disableCreateObjectURL ) { this._bindPdfLink(element, { content: item.content, filename }); } else { diff --git a/web/pdf_print_service.js b/web/pdf_print_service.js index 2321f0cf3..7f2dff0ca 100644 --- a/web/pdf_print_service.js +++ b/web/pdf_print_service.js @@ -15,7 +15,7 @@ import { CSS_UNITS, NullL10n } from "./ui_utils.js"; import { PDFPrintServiceFactory, PDFViewerApplication } from "./app.js"; -import { AppOptions } from "./app_options.js"; +import { viewerCompatibilityParams } from "./viewer_compatibility.js"; let activeService = null; let overlayManager = null; @@ -78,7 +78,6 @@ function PDFPrintService( this.printContainer = printContainer; this._printResolution = printResolution || 150; this.l10n = l10n || NullL10n; - this.disableCreateObjectURL = AppOptions.get("disableCreateObjectURL"); this.currentPage = -1; // The temporary canvas where renderPage paints one page at a time. this.scratchCanvas = document.createElement("canvas"); @@ -188,7 +187,10 @@ PDFPrintService.prototype = { img.style.height = printItem.height; const scratchCanvas = this.scratchCanvas; - if ("toBlob" in scratchCanvas && !this.disableCreateObjectURL) { + if ( + "toBlob" in scratchCanvas && + !viewerCompatibilityParams.disableCreateObjectURL + ) { scratchCanvas.toBlob(function (blob) { img.src = URL.createObjectURL(blob); });