From c4b95d925fb741bbe4cd1fd9808ce40470bcce4d Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 1 Jan 2021 23:03:18 +0100 Subject: [PATCH 1/2] Add a new helper method, on `PDFViewerApplication`, to determine the document filename Currently this code is duplicated no less than three times in the `web/app.js` file, and by introducing a helper method we can avoid unnecessary repetition. --- web/app.js | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/web/app.js b/web/app.js index ace56420e..2166e8f21 100644 --- a/web/app.js +++ b/web/app.js @@ -763,6 +763,12 @@ const PDFViewerApplication = { document.title = title; }, + get _docFilename() { + // Use `this.url` instead of `this.baseUrl` to perform filename detection + // based on the reference fragment as ultimate fallback if needed. + return this._contentDispositionFilename || getPDFFileNameFromURL(this.url); + }, + /** * @private */ @@ -984,12 +990,9 @@ const PDFViewerApplication = { downloadManager.downloadUrl(url, filename); } - const url = this.baseUrl; - // Use this.url instead of this.baseUrl to perform filename detection based - // on the reference fragment as ultimate fallback if needed. - const filename = - this._contentDispositionFilename || getPDFFileNameFromURL(this.url); - const downloadManager = this.downloadManager; + const downloadManager = this.downloadManager, + url = this.baseUrl, + filename = this._docFilename; downloadManager.onerror = err => { // This error won't really be helpful because it's likely the // fallback won't work either (or is already open). @@ -1017,12 +1020,9 @@ const PDFViewerApplication = { return; } - const url = this.baseUrl; - // Use this.url instead of this.baseUrl to perform filename detection based - // on the reference fragment as ultimate fallback if needed. - const filename = - this._contentDispositionFilename || getPDFFileNameFromURL(this.url); - const downloadManager = this.downloadManager; + const downloadManager = this.downloadManager, + url = this.baseUrl, + filename = this._docFilename; downloadManager.onerror = err => { // This error won't really be helpful because it's likely the // fallback won't work either (or is already open). @@ -1587,8 +1587,6 @@ const PDFViewerApplication = { } this._contentLength = length; } - const filename = - this._contentDispositionFilename || getPDFFileNameFromURL(this.url); try { await scripting.createSandbox({ @@ -1602,7 +1600,7 @@ const PDFViewerApplication = { ...this.documentInfo, baseURL: this.baseUrl, filesize: this._contentLength, - filename, + filename: this._docFilename, metadata: this.metadata, numPages: pdfDocument.numPages, URL: this.url, From 775d45b36ac6b75795f55c143e21034ff4370a9a Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 2 Jan 2021 12:02:58 +0100 Subject: [PATCH 2/2] Remove the `DownloadManager.onerror` functionality, since its only usage is unlikely to be helpful Note how the `onerror` functionality is not being used in the GENERIC `DownloadManager`, since we have no way of knowing if downloading succeeded. Hence this functionality is only *possibly* useful in MOZCENTRAL builds, however as outlined in the existing comments it's unlikely to be helpful in practice. Generally speaking, if downloading failed once in [`PdfStreamConverter.jsm`](https://searchfox.org/mozilla-central/rev/809ac3660845fef6faf18ec210232fdadc0f1ad9/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#294-406) it seems very likely that it would fail again; all-in-all I'm thus suggesting that we just remove the `onerror` functionality altogether here. --- web/app.js | 10 ---------- web/download_manager.js | 1 - web/firefoxcom.js | 6 ++++-- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/web/app.js b/web/app.js index 2166e8f21..1c44dc548 100644 --- a/web/app.js +++ b/web/app.js @@ -993,11 +993,6 @@ const PDFViewerApplication = { const downloadManager = this.downloadManager, url = this.baseUrl, filename = this._docFilename; - downloadManager.onerror = err => { - // This error won't really be helpful because it's likely the - // fallback won't work either (or is already open). - this.error(`PDF failed to download: ${err}`); - }; // When the PDF document isn't ready, or the PDF file is still downloading, // simply download using the URL. @@ -1023,11 +1018,6 @@ const PDFViewerApplication = { const downloadManager = this.downloadManager, url = this.baseUrl, filename = this._docFilename; - downloadManager.onerror = err => { - // This error won't really be helpful because it's likely the - // fallback won't work either (or is already open). - this.error(`PDF failed to be saved: ${err}`); - }; // When the PDF document isn't ready, or the PDF file is still downloading, // simply download using the URL. diff --git a/web/download_manager.js b/web/download_manager.js index 3c287f856..0261e4580 100644 --- a/web/download_manager.js +++ b/web/download_manager.js @@ -71,7 +71,6 @@ class DownloadManager { this.downloadUrl(url, filename); return; } - const blobUrl = URL.createObjectURL(blob); download(blobUrl, filename); } diff --git a/web/firefoxcom.js b/web/firefoxcom.js index afca70122..5ea019c61 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -130,8 +130,10 @@ class DownloadManager { filename, sourceEventType, }).then(error => { - if (error && this.onerror) { - this.onerror(error); + if (error) { + // If downloading failed in `PdfStreamConverter.jsm` it's very unlikely + // that attempting to fallback and re-download would be helpful here. + console.error("`ChromeActions.download` failed."); } URL.revokeObjectURL(blobUrl); });