From 625f8a6f517ab164c9d33284dc7cb44cff7b3e74 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 4 Jul 2020 11:29:46 +0200 Subject: [PATCH 1/2] Refactor/simplify the "delayedFallback" handling in the default viewer There's a few things that could be improved in the current implementation, such as: - It's currently necessary to *both* manually track the `featureId`s which should trigger delayed fallback, as well as manually report telemetry in affected cases. Obviously there's only two call-sites as of now (forms and javaScript), but it still feels somewhat error-prone especially if more cases were to be added in the future. To address this, this patch adds a new (private) method which abstracts away these details from the call-sites. - Generally, it also seems nice to reduce *and* simplify the amount of state we need to store on `PDFViewerApplication` in order to support the "delayedFallback" functionality. Also, having to *manually* work with the "delayedFallback"-array in multiple places feels cumbersome and makes e.g. the `PDFViewerApplication.fallback` method less clear as to its behaviour. - Having code *outside* of `PDFViewerApplication`, i.e. in the event handlers, directly access properties which are marked as "private" via a leading underscore doesn't seem that great in general. Furthermore, having the event handlers directly deal with that should be internal state also seem unfortunate. To address this, the patch will instead make use of a new `PDFViewerApplication.triggerDelayedFallback` callback. - There's at least one code-path in the viewer, see `PDFViewerApplication.error`, where `fallback` can be called without an argument. It's currently possible (although maybe somewhat unlikely) that such a call *could* be overridden by the `featureId` of a pending "delayedFallback" call, thus not reporting the *correct* fallback reason. - The "delayedFallback"-state weren't being reset on document close (which shouldn't affect Firefox, but nonetheless it ought to be fixed). --- web/app.js | 63 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/web/app.js b/web/app.js index 68ce9acfc..7511eecc7 100644 --- a/web/app.js +++ b/web/app.js @@ -189,8 +189,7 @@ const PDFViewerApplication = { externalServices: DefaultExternalServices, _boundEvents: {}, contentDispositionFilename: null, - _hasInteracted: false, - _delayedFallbackFeatureIds: [], + triggerDelayedFallback: null, // Called once when the document is loaded. async initialize(appConfig) { @@ -690,6 +689,7 @@ const PDFViewerApplication = { this.url = ""; this.baseUrl = ""; this.contentDispositionFilename = null; + this.triggerDelayedFallback = null; this.pdfSidebar.reset(); this.pdfOutlineViewer.reset(); @@ -864,12 +864,29 @@ const PDFViewerApplication = { .catch(downloadByUrl); // Error occurred, try downloading with the URL. }, - _recordFallbackErrorTelemetry(featureId) { - if (typeof PDFJSDev === "undefined" || PDFJSDev.test("MOZCENTRAL")) { + /** + * For PDF documents that contain e.g. forms and javaScript, we should only + * trigger the fallback bar once the user has interacted with the page. + * @private + */ + _delayedFallback(featureId) { + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC") + ) { + // Ensure that telemetry is always reported, since it's not guaranteed + // that the fallback bar will be shown (depends on user interaction). this.externalServices.reportTelemetry({ type: "unsupportedFeature", featureId, }); + + if (!this.triggerDelayedFallback) { + this.triggerDelayedFallback = () => { + this.fallback(featureId); + this.triggerDelayedFallback = null; + }; + } } }, @@ -878,24 +895,16 @@ const PDFViewerApplication = { typeof PDFJSDev === "undefined" || PDFJSDev.test("MOZCENTRAL || GENERIC") ) { - if (featureId) { - this._recordFallbackErrorTelemetry(featureId); - } - - // For PDFs that contain script and form errors, we should only trigger - // the fallback once the user has interacted with the page. - if (this._delayedFallbackFeatureIds.length >= 1 && this._hasInteracted) { - featureId = this._delayedFallbackFeatureIds[0]; - // Reset to prevent all click events from showing fallback bar. - this._delayedFallbackFeatureIds = []; - } + this.externalServices.reportTelemetry({ + type: "unsupportedFeature", + featureId, + }); // Only trigger the fallback once so we don't spam the user with messages // for one PDF. if (this.fellback) { return; } - this.fellback = true; this.externalServices.fallback( { @@ -1259,8 +1268,7 @@ const PDFViewerApplication = { return false; } console.warn("Warning: JavaScript is not supported"); - this._delayedFallbackFeatureIds.push(UNSUPPORTED_FEATURES.javaScript); - this._recordFallbackErrorTelemetry(UNSUPPORTED_FEATURES.javaScript); + this._delayedFallback(UNSUPPORTED_FEATURES.javaScript); return true; }); @@ -1342,8 +1350,7 @@ const PDFViewerApplication = { if (info.IsAcroFormPresent) { console.warn("Warning: AcroForm/XFA is not supported"); - this._delayedFallbackFeatureIds.push(UNSUPPORTED_FEATURES.forms); - this._recordFallbackErrorTelemetry(UNSUPPORTED_FEATURES.forms); + this._delayedFallback(UNSUPPORTED_FEATURES.forms); } if ( @@ -2501,15 +2508,13 @@ function webViewerWheel(evt) { } function webViewerClick(evt) { - PDFViewerApplication._hasInteracted = true; - // Avoid triggering the fallback bar when the user clicks on the // toolbar or sidebar. if ( - PDFViewerApplication._delayedFallbackFeatureIds.length >= 1 && + PDFViewerApplication.triggerDelayedFallback && PDFViewerApplication.pdfViewer.containsElement(evt.target) ) { - PDFViewerApplication.fallback(); + PDFViewerApplication.triggerDelayedFallback(); } if (!PDFViewerApplication.secondaryToolbar.isOpen) { @@ -2527,12 +2532,10 @@ function webViewerClick(evt) { function webViewerKeyUp(evt) { if (evt.keyCode === 9) { - // The user is tabbing into the pdf. Display the error message - // if it has not already been displayed. - PDFViewerApplication._hasInteracted = true; - - if (PDFViewerApplication._delayedFallbackFeatureIds.length >= 1) { - PDFViewerApplication.fallback(); + // The user is tabbing into the viewer. Trigger the fallback bar if it has + // not already been displayed. + if (PDFViewerApplication.triggerDelayedFallback) { + PDFViewerApplication.triggerDelayedFallback(); } } } From f9157ec2436a99355045ae874da28fe67ac1f391 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 8 Jul 2020 15:35:09 +0200 Subject: [PATCH 2/2] Unconditionally report telemetry, in the viewer, regardless of build target Given the dummy-methods on `DefaultExternalServices`, there's no longer any compelling reason not to (attempt to) report telemetry unconditionally. The only larger change consists of moving the `KNOWN_VERSIONS` and `KNOWN_GENERATORS` arrays ouf of the `PDFViewerApplication._initializeMetadata` method. *Please note:* Most of this patch consists of whitespace-only changes. --- web/app.js | 254 ++++++++++++++++++++++++----------------------------- 1 file changed, 113 insertions(+), 141 deletions(-) diff --git a/web/app.js b/web/app.js index 7511eecc7..774fbf7d0 100644 --- a/web/app.js +++ b/web/app.js @@ -86,6 +86,51 @@ const ViewOnLoad = { INITIAL: 1, }; +// Keep these in sync with mozilla-central's Histograms.json. +const KNOWN_VERSIONS = [ + "1.0", + "1.1", + "1.2", + "1.3", + "1.4", + "1.5", + "1.6", + "1.7", + "1.8", + "1.9", + "2.0", + "2.1", + "2.2", + "2.3", +]; +// Keep these in sync with mozilla-central's Histograms.json. +const KNOWN_GENERATORS = [ + "acrobat distiller", + "acrobat pdfwriter", + "adobe livecycle", + "adobe pdf library", + "adobe photoshop", + "ghostscript", + "tcpdf", + "cairo", + "dvipdfm", + "dvips", + "pdftex", + "pdfkit", + "itext", + "prince", + "quarkxpress", + "mac os x", + "microsoft", + "openoffice", + "oracle", + "luradocument", + "pdf-xchange", + "antenna house", + "aspose.cells", + "fpdf", +]; + class DefaultExternalServices { constructor() { throw new Error("Cannot initialize DefaultExternalServices."); @@ -870,55 +915,45 @@ const PDFViewerApplication = { * @private */ _delayedFallback(featureId) { - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("MOZCENTRAL || GENERIC") - ) { - // Ensure that telemetry is always reported, since it's not guaranteed - // that the fallback bar will be shown (depends on user interaction). - this.externalServices.reportTelemetry({ - type: "unsupportedFeature", - featureId, - }); + // Ensure that telemetry is always reported, since it's not guaranteed + // that the fallback bar will be shown (depends on user interaction). + this.externalServices.reportTelemetry({ + type: "unsupportedFeature", + featureId, + }); - if (!this.triggerDelayedFallback) { - this.triggerDelayedFallback = () => { - this.fallback(featureId); - this.triggerDelayedFallback = null; - }; - } + if (!this.triggerDelayedFallback) { + this.triggerDelayedFallback = () => { + this.fallback(featureId); + this.triggerDelayedFallback = null; + }; } }, fallback(featureId) { - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("MOZCENTRAL || GENERIC") - ) { - this.externalServices.reportTelemetry({ - type: "unsupportedFeature", - featureId, - }); + this.externalServices.reportTelemetry({ + type: "unsupportedFeature", + featureId, + }); - // Only trigger the fallback once so we don't spam the user with messages - // for one PDF. - if (this.fellback) { - return; - } - this.fellback = true; - this.externalServices.fallback( - { - featureId, - url: this.baseUrl, - }, - function response(download) { - if (!download) { - return; - } - PDFViewerApplication.download(); - } - ); + // Only trigger the fallback once so we don't spam the user with messages + // for one PDF. + if (this.fellback) { + return; } + this.fellback = true; + this.externalServices.fallback( + { + featureId, + url: this.baseUrl, + }, + function response(download) { + if (!download) { + return; + } + PDFViewerApplication.download(); + } + ); }, /** @@ -1319,7 +1354,6 @@ const PDFViewerApplication = { ); let pdfTitle; - const infoTitle = info && info.Title; if (infoTitle) { pdfTitle = infoTitle; @@ -1339,7 +1373,6 @@ const PDFViewerApplication = { pdfTitle = metadataTitle; } } - if (pdfTitle) { this.setTitle( `${pdfTitle} - ${contentDispositionFilename || document.title}` @@ -1353,83 +1386,32 @@ const PDFViewerApplication = { this._delayedFallback(UNSUPPORTED_FEATURES.forms); } - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("MOZCENTRAL || GENERIC") - ) { - // Telemetry labels must be C++ variable friendly. - let versionId = "other"; - // Keep these in sync with mozilla central's Histograms.json. - const KNOWN_VERSIONS = [ - "1.0", - "1.1", - "1.2", - "1.3", - "1.4", - "1.5", - "1.6", - "1.7", - "1.8", - "1.9", - "2.0", - "2.1", - "2.2", - "2.3", - ]; - if (KNOWN_VERSIONS.includes(info.PDFFormatVersion)) { - versionId = `v${info.PDFFormatVersion.replace(".", "_")}`; - } - - let generatorId = "other"; - // Keep these in sync with mozilla central's Histograms.json. - const KNOWN_GENERATORS = [ - "acrobat distiller", - "acrobat pdfwriter", - "adobe livecycle", - "adobe pdf library", - "adobe photoshop", - "ghostscript", - "tcpdf", - "cairo", - "dvipdfm", - "dvips", - "pdftex", - "pdfkit", - "itext", - "prince", - "quarkxpress", - "mac os x", - "microsoft", - "openoffice", - "oracle", - "luradocument", - "pdf-xchange", - "antenna house", - "aspose.cells", - "fpdf", - ]; - if (info.Producer) { - const producer = info.Producer.toLowerCase(); - KNOWN_GENERATORS.some(function (generator) { - if (!producer.includes(generator)) { - return false; - } - generatorId = generator.replace(/[ .\-]/g, "_"); - return true; - }); - } - - let formType = null; - if (info.IsAcroFormPresent) { - formType = info.IsXFAPresent ? "xfa" : "acroform"; - } - this.externalServices.reportTelemetry({ - type: "documentInfo", - version: versionId, - generator: generatorId, - formType, + // Telemetry labels must be C++ variable friendly. + let versionId = "other"; + if (KNOWN_VERSIONS.includes(info.PDFFormatVersion)) { + versionId = `v${info.PDFFormatVersion.replace(".", "_")}`; + } + let generatorId = "other"; + if (info.Producer) { + const producer = info.Producer.toLowerCase(); + KNOWN_GENERATORS.some(function (generator) { + if (!producer.includes(generator)) { + return false; + } + generatorId = generator.replace(/[ .\-]/g, "_"); + return true; }); } + let formType = null; + if (info.IsAcroFormPresent) { + formType = info.IsXFAPresent ? "xfa" : "acroform"; + } + this.externalServices.reportTelemetry({ + type: "documentInfo", + version: versionId, + generator: generatorId, + formType, + }); }, /** @@ -1642,14 +1624,9 @@ const PDFViewerApplication = { printService.layout(); - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("MOZCENTRAL || GENERIC") - ) { - this.externalServices.reportTelemetry({ - type: "print", - }); - } + this.externalServices.reportTelemetry({ + type: "print", + }); }, afterPrint() { @@ -2091,22 +2068,17 @@ function webViewerPageRendered(evt) { }); } - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("MOZCENTRAL || GENERIC") - ) { + PDFViewerApplication.externalServices.reportTelemetry({ + type: "pageInfo", + timestamp: evt.timestamp, + }); + // It is a good time to report stream and font types. + PDFViewerApplication.pdfDocument.getStats().then(function (stats) { PDFViewerApplication.externalServices.reportTelemetry({ - type: "pageInfo", - timestamp: evt.timestamp, + type: "documentStats", + stats, }); - // It is a good time to report stream and font types. - PDFViewerApplication.pdfDocument.getStats().then(function (stats) { - PDFViewerApplication.externalServices.reportTelemetry({ - type: "documentStats", - stats, - }); - }); - } + }); } function webViewerPageMode({ mode }) {