From b5e00e1faec2cb2f0d7f0031079371138e5d6d7f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 1 Mar 2024 13:40:15 +0100 Subject: [PATCH 1/2] Move the error message localization into `PDFViewerApplication._otherError` When reporting errors in the viewer we currently localize the error messages "manually" at every call-site, which seems like unnecessary repetition. --- web/app.js | 44 ++++++++++++++++++++++---------------------- web/chromecom.js | 9 +++++---- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/web/app.js b/web/app.js index 2e87b64ce5f90..2c5a120aa629f 100644 --- a/web/app.js +++ b/web/app.js @@ -838,9 +838,7 @@ const PDFViewerApplication = { this.open({ url, length, originalUrl }); }, onError: err => { - this.l10n.get("pdfjs-loading-error").then(msg => { - this._documentError(msg, err); - }); + this._documentError("pdfjs-loading-error", err); }, onProgress: (loaded, total) => { this.progress(loaded / total); @@ -1050,10 +1048,11 @@ const PDFViewerApplication = { } else if (reason instanceof UnexpectedResponseException) { key = "pdfjs-unexpected-response-error"; } - return this.l10n.get(key).then(msg => { - this._documentError(msg, { message: reason?.message }); - throw reason; - }); + return this._documentError(key, { message: reason.message }).then( + () => { + throw reason; + } + ); } ); }, @@ -1134,10 +1133,13 @@ const PDFViewerApplication = { * Report the error; used for errors affecting loading and/or parsing of * the entire PDF document. */ - _documentError(message, moreInfo = null) { + async _documentError(key, moreInfo = null) { this._unblockDocumentLoadEvent(); - this._otherError(message, moreInfo); + const message = await this._otherError( + key || "pdfjs-loading-error", + moreInfo + ); this.eventBus.dispatch("documenterror", { source: this, @@ -1148,12 +1150,15 @@ const PDFViewerApplication = { /** * Report the error; used for errors affecting e.g. only a single page. - * @param {string} message - A message that is human readable. + * @param {string} key - The localization key for the error. * @param {Object} [moreInfo] - Further information about the error that is * more technical. Should have a 'message' and * optionally a 'stack' property. + * @returns {string} A (localized) error message that is human readable. */ - _otherError(message, moreInfo = null) { + async _otherError(key, moreInfo = null) { + const message = await this.l10n.get(key); + const moreInfoText = [`PDF.js v${version || "?"} (build: ${build || "?"})`]; if (moreInfo) { moreInfoText.push(`Message: ${moreInfo.message}`); @@ -1171,6 +1176,7 @@ const PDFViewerApplication = { } console.error(`${message}\n\n${moreInfoText.join("\n")}`); + return message; }, progress(level) { @@ -1387,9 +1393,7 @@ const PDFViewerApplication = { this._initializeAutoPrint(pdfDocument, openActionPromise); }, reason => { - this.l10n.get("pdfjs-loading-error").then(msg => { - this._documentError(msg, { message: reason?.message }); - }); + this._documentError("pdfjs-loading-error", { message: reason.message }); } ); @@ -1778,9 +1782,7 @@ const PDFViewerApplication = { } if (!this.supportsPrinting) { - this.l10n.get("pdfjs-printing-not-supported").then(msg => { - this._otherError(msg); - }); + this._otherError("pdfjs-printing-not-supported"); return; } @@ -2242,8 +2244,8 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { throw new Error("file origin does not match viewer's"); } } catch (ex) { - PDFViewerApplication.l10n.get("pdfjs-loading-error").then(msg => { - PDFViewerApplication._documentError(msg, { message: ex?.message }); + PDFViewerApplication._documentError("pdfjs-loading-error", { + message: ex.message, }); throw ex; } @@ -2308,9 +2310,7 @@ function webViewerPageRendered({ pageNumber, error }) { } if (error) { - PDFViewerApplication.l10n.get("pdfjs-rendering-error").then(msg => { - PDFViewerApplication._otherError(msg, error); - }); + PDFViewerApplication._otherError("pdfjs-rendering-error", error); } } diff --git a/web/chromecom.js b/web/chromecom.js index 403a9e1a9a8ea..5a4ebfbf5c55f 100644 --- a/web/chromecom.js +++ b/web/chromecom.js @@ -108,10 +108,11 @@ 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)) { - viewerApp._documentError( - `Blocked ${origin} from loading ${file}. Refused to load ` + - "a local file in a non-local page for security reasons." - ); + viewerApp._documentError(null, { + message: + `Blocked ${origin} from loading ${file}. Refused to load ` + + "a local file in a non-local page for security reasons.", + }); return; } isAllowedFileSchemeAccess(function (isAllowedAccess) { From ea1c910a66e7a19c247c5f0c330e8bfdd78dc357 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 2 Mar 2024 09:49:05 +0100 Subject: [PATCH 2/2] Remove `PDFViewerApplication.initPassiveLoading` and directly invoke the `open`-method from the extension-specific code This old method is essentially just adding, a small amount of, unnecessary indirection and we can easily invoke `PDFViewerApplication.open` directly from the extension-specific code instead. --- web/app.js | 44 ++++++++-------------------------------- web/chromecom.js | 5 ++--- web/external_services.js | 2 +- web/firefoxcom.js | 12 +++++------ 4 files changed, 17 insertions(+), 46 deletions(-) diff --git a/web/app.js b/web/app.js index 2c5a120aa629f..920c95bde356c 100644 --- a/web/app.js +++ b/web/app.js @@ -685,7 +685,9 @@ const PDFViewerApplication = { this._hideViewBookmark(); } } else if (PDFJSDev.test("MOZCENTRAL || CHROME")) { - this.initPassiveLoading(file); + this.setTitleUsingUrl(file, /* downloadUrl = */ file); + + this.externalServices.initPassiveLoading(); } else { throw new Error("Not implemented: run"); } @@ -815,37 +817,6 @@ const PDFViewerApplication = { this._caretBrowsing.moveCaret(isUp, select); }, - initPassiveLoading(file) { - if ( - typeof PDFJSDev === "undefined" || - !PDFJSDev.test("MOZCENTRAL || CHROME") - ) { - throw new Error("Not implemented: initPassiveLoading"); - } - this.setTitleUsingUrl(file, /* downloadUrl = */ file); - - this.externalServices.initPassiveLoading({ - onOpenWithTransport: range => { - this.open({ range }); - }, - onOpenWithData: (data, contentDispositionFilename) => { - if (isPdfFile(contentDispositionFilename)) { - this._contentDispositionFilename = contentDispositionFilename; - } - this.open({ data }); - }, - onOpenWithURL: (url, length, originalUrl) => { - this.open({ url, length, originalUrl }); - }, - onError: err => { - this._documentError("pdfjs-loading-error", err); - }, - onProgress: (loaded, total) => { - this.progress(loaded / total); - }, - }); - }, - setTitleUsingUrl(url = "", downloadUrl = null) { this.url = url; this.baseUrl = url.split("#", 1)[0]; @@ -987,10 +958,11 @@ const PDFViewerApplication = { const workerParams = AppOptions.getAll(OptionKind.WORKER); Object.assign(GlobalWorkerOptions, workerParams); - if ( - (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) && - args.url - ) { + if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) { + if (args.data && isPdfFile(args.filename)) { + this._contentDispositionFilename = args.filename; + } + } else if (args.url) { // The Firefox built-in viewer always calls `setTitleUsingUrl`, before // `initPassiveLoading`, and it never provides an `originalUrl` here. this.setTitleUsingUrl( diff --git a/web/chromecom.js b/web/chromecom.js index 5a4ebfbf5c55f..7f26b70dbccc3 100644 --- a/web/chromecom.js +++ b/web/chromecom.js @@ -417,12 +417,11 @@ class Preferences extends BasePreferences { } class ExternalServices extends BaseExternalServices { - initPassiveLoading(callbacks) { - // defaultUrl is set in viewer.js + initPassiveLoading() { ChromeCom.resolvePDFFile( AppOptions.get("defaultUrl"), function (url, length, originalUrl) { - callbacks.onOpenWithURL(url, length, originalUrl); + viewerApp.open({ url, length, originalUrl }); } ); } diff --git a/web/external_services.js b/web/external_services.js index 1fa42cd43dafa..509e084700873 100644 --- a/web/external_services.js +++ b/web/external_services.js @@ -24,7 +24,7 @@ class BaseExternalServices { updateFindMatchesCount(data) {} - initPassiveLoading(callbacks) {} + initPassiveLoading() {} reportTelemetry(data) {} diff --git a/web/firefoxcom.js b/web/firefoxcom.js index ecced78367640..bb1b5f46ff81d 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -323,7 +323,7 @@ class ExternalServices extends BaseExternalServices { FirefoxCom.request("updateFindMatchesCount", data); } - initPassiveLoading(callbacks) { + initPassiveLoading() { let pdfDataRangeTransport; window.addEventListener("message", function windowMessage(e) { @@ -340,7 +340,7 @@ class ExternalServices extends BaseExternalServices { switch (args.pdfjsLoadAction) { case "supportsRangedLoading": if (args.done && !args.data) { - callbacks.onError(); + viewerApp._documentError(null); break; } pdfDataRangeTransport = new FirefoxComDataRangeTransport( @@ -350,7 +350,7 @@ class ExternalServices extends BaseExternalServices { args.filename ); - callbacks.onOpenWithTransport(pdfDataRangeTransport); + viewerApp.open({ range: pdfDataRangeTransport }); break; case "range": pdfDataRangeTransport.onDataRange(args.begin, args.chunk); @@ -369,14 +369,14 @@ class ExternalServices extends BaseExternalServices { pdfDataRangeTransport?.onDataProgressiveDone(); break; case "progress": - callbacks.onProgress(args.loaded, args.total); + viewerApp.progress(args.loaded / args.total); break; case "complete": if (!args.data) { - callbacks.onError(args.errorCode); + viewerApp._documentError(null, { message: args.errorCode }); break; } - callbacks.onOpenWithData(args.data, args.filename); + viewerApp.open({ data: args.data, filename: args.filename }); break; } });