Ensure that the viewer telemetry reporting, and fallback code, runs in development mode and GENERIC builds #11522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n 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.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/79e07c4ef0f9206/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/79e07c4ef0f9206/output.txt Total script time: 9.83 mins Published |
|
/botio-windows preview |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/bcbe382ce43e369/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/bcbe382ce43e369/output.txt Total script time: 3.76 mins Published |
timvandermeij
approved these changes
Jan 20, 2020
|
Looks good to me. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While only the
MOZCENTRALbuilds 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.jsfile, the telemetry reporting was done with directFirefoxComcalls. However, with the dummyDefaultExternalServices.reportTelemetrymethod 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
PDFJSDevchecks to be able to actually test the changes locally.