1408429 - Translation Telemetry shouldn't check the toolkit.telemetry.enabled pref

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Chris H-C :chutten]

browser/components/translation/Translation.jsm checks the extended telemetry pref before collecting telemetry. This is wasteful and unnecessary as Telemetry does it for you.

Remove all uses of the pref from Translation.jsm and its tests. (browser_translation_telemetry.js sets and resets the pref unnecessarily already)

Comment 1

[Aditya Bharti [:adbugger]]

Hi, I would like to start working on this bug.

I'm looking at the Translation.jsm file and I see multiple calls to the _canRecord function, which calls the Services.prefs.getBoolPref("toolkit.telemetry.enabled"). Is this the pref whose uses must be removed?

Also, just to ensure I understand correctly, this check is not needed because the Services.telemetry module performs the check for you? If that is the case, I'm assuming no data is returned if the pref is not enabled. Will the application handle this case correctly?

Comment 2

[Chris H-C :chutten]

You are right: _canRecord can go, as can all of the early returns that use it.

You are also right in that Telemetry handles whether or not to record or send this data internally.

I have assigned this bug to you.

Comment 3

[Aditya Bharti [:adbugger]]

Attachment: Removed definition and all uses of _canRecord from Translation.jsm

I have removed all uses of the toolkit.telemetry.enabled pref from Translation.jsm. The about:telemetry page works and nothing, as far as I can see, breaks. 

I'm not sure about which test file needs modification so will remove the pref from testfile browser_telemetry.js (https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_telemetry.js#6) in next patch file. Waiting for further info on this before proceeding.

Meanwhile, please take a look at these changes. Any other test files which need modification?

Comment 4

[Chris H-C :chutten]

The patch looks like a good draft!

Don't go ahead with changing browser_telemetry.js. Instead, the tests you change should be limited to just the translation tests found in browser_translation_telemetry.js (browser/components/translation/test/browser_translation_telemetry.js)

Comment 5

[Aditya Bharti [:adbugger]]

How do I know which tests to change? There don't seem to be any failing tests. I ran mochitests on that single file (browser/components/translation/test/browser_translation_telemetry.js) and all 24 tests passed. Plus, I'm not seeing the pref toolkit.telemetry.enabled being explicitly checked anywhere in the file?

Can you give me a little more info on which tests require a change and why? Thanks.

Comment 6

[Chris H-C :chutten]

Oh. Looks like I took care of that file during bug 1406391. :S

Sorry about that, looks like all you needed was the changes to the jsm itself. Carry on!

Comment 7

[Aditya Bharti [:adbugger]]

Attachment: Changed file Translation.jsm

Since Translation.jsm was the only file that needed to be changed, uploading a patch for review.

Comment 8

[Chris H-C :chutten]

Comment on attachment 8929076 [details] [diff] [review]
Changed file Translation.jsm

Review of attachment 8929076 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 9

[Chris H-C :chutten]

We're going to have to find you a harder one for next time. Does anything in this list of "good second bugs" appeal to you? https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed&namedcmd=goodSecondBug%20-%20unassigned&list_id=13885366

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c03f806d36
Fix Translation Telemetry shouldn't check toolkit.telemetry.enabled pref. r=chutten

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/56c03f806d36