From f171d997994c063bfa017da654d6de280a24ff20 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 20 Jan 2020 11:37:54 +0100 Subject: [PATCH] Ensure that the viewer telemetry reporting, and fallback code, runs in development mode and GENERIC builds While only the `MOZCENTRAL` builds will actually do anything meaningful with the telemetry data, none of the code in question actually runs *at all* in e.g. development mode.[1] This seems bad since it essentially means that this code is completely untested, despite being quite important for the built-in Firefox PDF viewer, and this thus ought to be fixed. In this case, the explanation for the current state of the code should be "for historical reasons". Before the viewer was split into the current components and before the pre-processor was improved, back when all code resided in the `web/viewer.js` file, the telemetry reporting was done with *direct* `FirefoxCom` calls. However, with the dummy `DefaultExternalServices.reportTelemetry` method there's nothing actually preventing attempting to report telemetry in any type of build. NOTE: By running this code in GENERIC builds as well, in addition to just locally, the *viewer* part of telemetry reporting becomes tested e.g. in preview builds too which should help with reviewing. --- [1] When fixing bug 1606566, I had to edit the relevant `PDFJSDev` checks to be able to actually test the changes locally. --- web/app.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/web/app.js b/web/app.js index 13ce63b94..58984c9e3 100644 --- a/web/app.js +++ b/web/app.js @@ -850,8 +850,8 @@ const PDFViewerApplication = { fallback(featureId) { if ( - typeof PDFJSDev !== "undefined" && - PDFJSDev.test("FIREFOX || MOZCENTRAL") + typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC") ) { // Only trigger the fallback once so we don't spam the user with messages // for one PDF. @@ -1325,8 +1325,8 @@ const PDFViewerApplication = { } if ( - typeof PDFJSDev !== "undefined" && - PDFJSDev.test("FIREFOX || MOZCENTRAL") + typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC") ) { // Telemetry labels must be C++ variable friendly. let versionId = "other"; @@ -1556,8 +1556,8 @@ const PDFViewerApplication = { printService.layout(); if ( - typeof PDFJSDev !== "undefined" && - PDFJSDev.test("FIREFOX || MOZCENTRAL") + typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC") ) { this.externalServices.reportTelemetry({ type: "print", @@ -1870,8 +1870,8 @@ function webViewerInitialized() { } if ( - typeof PDFJSDev !== "undefined" && - PDFJSDev.test("FIREFOX || MOZCENTRAL") && + (typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC")) && !PDFViewerApplication.supportsDocumentFonts ) { AppOptions.set("disableFontFace", true); @@ -1998,8 +1998,8 @@ function webViewerPageRendered(evt) { } if ( - typeof PDFJSDev !== "undefined" && - PDFJSDev.test("FIREFOX || MOZCENTRAL") + typeof PDFJSDev === "undefined" || + PDFJSDev.test("MOZCENTRAL || GENERIC") ) { PDFViewerApplication.externalServices.reportTelemetry({ type: "pageInfo",