From ecb09d62fc1fe5d52964ee498a54b2d97df4c017 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 30 Jan 2026 08:02:24 +0100 Subject: [PATCH] Add the current loading percentage to the `onPassword` callback The percentage calculation is currently "spread out" across various viewer functionality, which we can avoid by having the API handle that instead. Also, remove the `this.#lastProgress` special-case[1] and just register a "normal" `fullReader.onProgress` callback unconditionally. Once `headersReady` is resolved the callback can simply be removed when not needed, since the "worst" thing that could theoretically happen is that the loadingBar (in the viewer) updates sooner this way. In practice though, since `fullReader.read` cannot return data until `headersReady` is resolved, this change is not actually observable in the API. --- [1] This was added in PR 8617, close to a decade ago, but it's not obvious to me that it was ever necessary to implement it that way. --- examples/mobile-viewer/viewer.mjs | 111 ++++++++++++------------------ src/display/api.js | 48 +++++-------- test/unit/api_spec.js | 25 ++++--- web/app.js | 7 +- web/firefoxcom.js | 10 ++- web/ui_utils.js | 2 +- 6 files changed, 92 insertions(+), 111 deletions(-) diff --git a/examples/mobile-viewer/viewer.mjs b/examples/mobile-viewer/viewer.mjs index bf496c91e..edf7d7079 100644 --- a/examples/mobile-viewer/viewer.mjs +++ b/examples/mobile-viewer/viewer.mjs @@ -46,19 +46,13 @@ const PDFViewerApplication = { * @returns {Promise} - Returns the promise, which is resolved when document * is opened. */ - open(params) { + async open(params) { if (this.pdfLoadingTask) { - // We need to destroy already opened document - return this.close().then( - function () { - // ... and repeat the open() call. - return this.open(params); - }.bind(this) - ); + // We need to destroy already opened document. + await this.close(); } - const url = params.url; - const self = this; + const { url } = params; this.setTitleUsingUrl(url); // Loading document. @@ -70,24 +64,22 @@ const PDFViewerApplication = { }); this.pdfLoadingTask = loadingTask; - loadingTask.onProgress = function (progressData) { - self.progress(progressData.loaded / progressData.total); - }; + loadingTask.onProgress = evt => this.progress(evt.percent); return loadingTask.promise.then( - function (pdfDocument) { + pdfDocument => { // Document loaded, specifying document for the viewer. - self.pdfDocument = pdfDocument; - self.pdfViewer.setDocument(pdfDocument); - self.pdfLinkService.setDocument(pdfDocument); - self.pdfHistory.initialize({ + this.pdfDocument = pdfDocument; + this.pdfViewer.setDocument(pdfDocument); + this.pdfLinkService.setDocument(pdfDocument); + this.pdfHistory.initialize({ fingerprint: pdfDocument.fingerprints[0], }); - self.loadingBar.hide(); - self.setTitleUsingMetadata(pdfDocument); + this.loadingBar.hide(); + this.setTitleUsingMetadata(pdfDocument); }, - function (reason) { + reason => { let key = "pdfjs-loading-error"; if (reason instanceof pdfjsLib.InvalidPDFException) { key = "pdfjs-invalid-file-error"; @@ -96,10 +88,10 @@ const PDFViewerApplication = { ? "pdfjs-missing-file-error" : "pdfjs-unexpected-response-error"; } - self.l10n.get(key).then(msg => { - self.error(msg, { message: reason?.message }); + this.l10n.get(key).then(msg => { + this.error(msg, { message: reason.message }); }); - self.loadingBar.hide(); + this.loadingBar.hide(); } ); }, @@ -109,9 +101,9 @@ const PDFViewerApplication = { * @returns {Promise} - Returns the promise, which is resolved when all * destruction is completed. */ - close() { + async close() { if (!this.pdfLoadingTask) { - return Promise.resolve(); + return; } const promise = this.pdfLoadingTask.destroy(); @@ -128,7 +120,7 @@ const PDFViewerApplication = { } } - return promise; + await promise; }, get loadingBar() { @@ -152,48 +144,36 @@ const PDFViewerApplication = { this.setTitle(title); }, - setTitleUsingMetadata(pdfDocument) { - const self = this; - pdfDocument.getMetadata().then(function (data) { - const info = data.info, - metadata = data.metadata; - self.documentInfo = info; - self.metadata = metadata; + async setTitleUsingMetadata(pdfDocument) { + const { info, metadata } = await pdfDocument.getMetadata(); + this.documentInfo = info; + this.metadata = metadata; - // Provides some basic debug information - console.log( - "PDF " + - pdfDocument.fingerprints[0] + - " [" + - info.PDFFormatVersion + - " " + - (info.Producer || "-").trim() + - " / " + - (info.Creator || "-").trim() + - "]" + - " (PDF.js: " + - (pdfjsLib.version || "-") + - ")" - ); + // Provides some basic debug information + console.log( + `PDF ${pdfDocument.fingerprints[0]} [${info.PDFFormatVersion} ` + + `${(metadata?.get("pdf:producer") || info.Producer || "-").trim()} / ` + + `${(metadata?.get("xmp:creatortool") || info.Creator || "-").trim()}` + + `] (PDF.js: ${pdfjsLib.version || "?"} [${pdfjsLib.build || "?"}])` + ); - let pdfTitle; - if (metadata && metadata.has("dc:title")) { - const title = metadata.get("dc:title"); - // Ghostscript sometimes returns 'Untitled', so prevent setting the - // title to 'Untitled. - if (title !== "Untitled") { - pdfTitle = title; - } + let pdfTitle; + if (metadata && metadata.has("dc:title")) { + const title = metadata.get("dc:title"); + // Ghostscript sometimes returns 'Untitled', so prevent setting the + // title to 'Untitled. + if (title !== "Untitled") { + pdfTitle = title; } + } - if (!pdfTitle && info && info.Title) { - pdfTitle = info.Title; - } + if (!pdfTitle && info && info.Title) { + pdfTitle = info.Title; + } - if (pdfTitle) { - self.setTitle(pdfTitle + " - " + document.title); - } - }); + if (pdfTitle) { + this.setTitle(pdfTitle + " - " + document.title); + } }, setTitle: function pdfViewSetTitle(title) { @@ -223,8 +203,7 @@ const PDFViewerApplication = { console.error(`${message}\n\n${moreInfoText.join("\n")}`); }, - progress: function pdfViewProgress(level) { - const percent = Math.round(level * 100); + progress(percent) { // Updating the bar if value increases. if (percent > this.loadingBar.percent || isNaN(percent)) { this.loadingBar.percent = percent; diff --git a/src/display/api.js b/src/display/api.js index 4f1ad871c..f088b9dc1 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -25,6 +25,7 @@ import { getVerbosityLevel, info, isNodeJS, + MathClamp, RenderingIntentFlag, setVerbosityLevel, shadow, @@ -525,6 +526,8 @@ function getDocument(src = {}) { * @typedef {Object} OnProgressParameters * @property {number} loaded - Currently loaded number of bytes. * @property {number} total - Total number of bytes in the PDF file. + * @property {number} percent - Currently loaded percentage, as an integer value + * in the [0, 100] range. If `total` is undefined, the percentage is `NaN`. */ /** @@ -2396,8 +2399,6 @@ class WorkerTransport { #fullReader = null; - #lastProgress = null; - #methodPromises = new Map(); #networkStream = null; @@ -2492,6 +2493,14 @@ class WorkerTransport { return promise; } + #onProgress({ loaded, total }) { + this.loadingTask.onProgress?.({ + loaded, + total, + percent: MathClamp(Math.round((loaded / total) * 100), 0, 100), + }); + } + get annotationStorage() { return shadow(this, "annotationStorage", new AnnotationStorage()); } @@ -2624,12 +2633,10 @@ class WorkerTransport { "GetReader - no `BasePDFStream` instance available." ); this.#fullReader = this.#networkStream.getFullReader(); - this.#fullReader.onProgress = evt => { - this.#lastProgress = { - loaded: evt.loaded, - total: evt.total, - }; - }; + // If stream or range turn out to be disabled, once `headersReady` is + // resolved, this is our only way to report loading progress. + this.#fullReader.onProgress = evt => this.#onProgress(evt); + sink.onPull = () => { this.#fullReader .read() @@ -2669,20 +2676,9 @@ class WorkerTransport { const { isStreamingSupported, isRangeSupported, contentLength } = this.#fullReader; - // If stream or range are disabled, it's our only way to report - // loading progress. - if (!isStreamingSupported || !isRangeSupported) { - if (this.#lastProgress) { - loadingTask.onProgress?.(this.#lastProgress); - } - this.#fullReader.onProgress = evt => { - loadingTask.onProgress?.({ - loaded: evt.loaded, - total: evt.total, - }); - }; + if (isStreamingSupported && isRangeSupported) { + this.#fullReader.onProgress = null; // See comment in "GetReader" above. } - return { isStreamingSupported, isRangeSupported, contentLength }; }); @@ -2779,10 +2775,7 @@ class WorkerTransport { messageHandler.on("DataLoaded", data => { // For consistency: Ensure that progress is always reported when the // entire PDF file has been loaded, regardless of how it was fetched. - loadingTask.onProgress?.({ - loaded: data.length, - total: data.length, - }); + this.#onProgress({ loaded: data.length, total: data.length }); this.downloadInfoCapability.resolve(data); }); @@ -2905,10 +2898,7 @@ class WorkerTransport { if (this.destroyed) { return; // Ignore any pending requests if the worker was terminated. } - loadingTask.onProgress?.({ - loaded: data.loaded, - total: data.total, - }); + this.#onProgress(data); }); messageHandler.on("FetchBinaryData", async data => { diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index c3f6a7d2d..801577518 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -160,14 +160,18 @@ describe("api", function () { progressReportedCapability.resolve(progressData); }; - const data = await Promise.all([ - progressReportedCapability.promise, + const [pdfDoc, progress] = await Promise.all([ loadingTask.promise, + progressReportedCapability.promise, ]); - expect(data[0].loaded / data[0].total >= 0).toEqual(true); - expect(data[1] instanceof PDFDocumentProxy).toEqual(true); - expect(loadingTask).toEqual(data[1].loadingTask); + expect(pdfDoc instanceof PDFDocumentProxy).toEqual(true); + expect(pdfDoc.loadingTask).toBe(loadingTask); + + expect(progress.loaded).toBeGreaterThanOrEqual(0); + expect(progress.total).toEqual(basicApiFileLength); + expect(progress.percent).toBeGreaterThanOrEqual(0); + expect(progress.percent).toBeLessThanOrEqual(100); await loadingTask.destroy(); }); @@ -218,12 +222,17 @@ describe("api", function () { progressReportedCapability.resolve(data); }; - const data = await Promise.all([ + const [pdfDoc, progress] = await Promise.all([ loadingTask.promise, progressReportedCapability.promise, ]); - expect(data[0] instanceof PDFDocumentProxy).toEqual(true); - expect(data[1].loaded / data[1].total).toEqual(1); + + expect(pdfDoc instanceof PDFDocumentProxy).toEqual(true); + expect(pdfDoc.loadingTask).toBe(loadingTask); + + expect(progress.loaded).toEqual(basicApiFileLength); + expect(progress.total).toEqual(basicApiFileLength); + expect(progress.percent).toEqual(100); // Check that the TypedArray was transferred. expect(typedArrayPdf.length).toEqual(0); diff --git a/web/app.js b/web/app.js index e882ad150..a06ba8c12 100644 --- a/web/app.js +++ b/web/app.js @@ -1236,9 +1236,7 @@ const PDFViewerApplication = { this.passwordPrompt.open(); }; - loadingTask.onProgress = ({ loaded, total }) => { - this.progress(loaded / total); - }; + loadingTask.onProgress = evt => this.progress(evt.percent); return loadingTask.promise.then( pdfDocument => { @@ -1374,8 +1372,7 @@ const PDFViewerApplication = { return message; }, - progress(level) { - const percent = Math.round(level * 100); + progress(percent) { // When we transition from full request to range requests, it's possible // that we discard some of the loaded data. This can cause the loading // bar to move backwards. So prevent this by only updating the bar if it diff --git a/web/firefoxcom.js b/web/firefoxcom.js index 253965f16..8db4d8965 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { isPdfFile, PDFDataRangeTransport } from "pdfjs-lib"; +import { isPdfFile, MathClamp, PDFDataRangeTransport } from "pdfjs-lib"; import { AppOptions } from "./app_options.js"; import { BaseExternalServices } from "./external_services.js"; import { BasePreferences } from "./preferences.js"; @@ -627,7 +627,13 @@ class ExternalServices extends BaseExternalServices { pdfDataRangeTransport?.onDataProgressiveDone(); break; case "progress": - viewerApp.progress(args.loaded / args.total); + const percent = MathClamp( + Math.round((args.loaded / args.total) * 100), + 0, + 100 + ); + + viewerApp.progress(percent); break; case "complete": if (!args.data) { diff --git a/web/ui_utils.js b/web/ui_utils.js index 0251c37b6..a4199e611 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -709,7 +709,7 @@ class ProgressBar { } set percent(val) { - this.#percent = MathClamp(val, 0, 100); + this.#percent = val; if (isNaN(val)) { this.#classList.add("indeterminate");