Splitting the work from bug 1415284, this covers the removing the non-addons-manager code handing Telemetry Experiments.

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. https://reviewboard.mozilla.org/r/233430/#review239018 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/telemetry/docs/fhr/dataformat.rst:1900 (Diff revision 1) > :: > > "org.mozilla.uitour.treatment": { > "_v": 1, > "treatment": [ > "optin", Warning: Optin ==> option [codespell]

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. https://reviewboard.mozilla.org/r/233430/#review239074 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/telemetry/docs/fhr/dataformat.rst:1900 (Diff revision 2) > :: > > "org.mozilla.uitour.treatment": { > "_v": 1, > "treatment": [ > "optin", Warning: Optin ==> option [codespell]

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. Taking this review.

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. https://reviewboard.mozilla.org/r/233430/#review239160 Cheers for doing this! I only have small notes here. I'd like to double-check pipeline impact, so i called out to not remove the data point from the TelemetryEnvironment yet. We can look at this in a follow-up bug and still remove the bulk of the code here. I also still see Experiments.jsm being referenced in: - toolkit/mozapps/extensions/content/extensions.js - toolkit/mozapps/extensions/test/browser/browser_experiments.js ::: toolkit/components/telemetry/TelemetryEnvironment.jsm (Diff revision 2) > let addons = { > activeAddons: await this._getActiveAddons(), > theme: await this._getActiveTheme(), > activePlugins: this._getActivePlugins(), > activeGMPlugins: await this._getActiveGMPlugins(), > - activeExperiment: this._getActiveExperiment(), Let's remove this property in a followup and always set it to `{}` here. We'll need to double-check if we can just leave this out or if this will have pipeline impact. It's optional in the schema, but we should double-check if our data jobs handle this properly. https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/dev/templates/include/telemetry/environment.1.schema.json ::: toolkit/components/telemetry/docs/data/environment.rst (Diff revision 2) > - activeExperiment: { // obsolete in firefox 61, section is empty if there's no active experiment > - id: <string>, // id > - branch: <string>, // branch name > - }, Let's leave this for the follow-up bug. ::: toolkit/components/telemetry/docs/fhr/dataformat.rst (Diff revision 2) > "bartext": { > "google": 1 > }, > "_v": "4" > - }, > - "org.mozilla.experiment": { This is deprecated documentation, kept around for archeological reasons. Let's leave this file as is. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1671 (Diff revision 2) > > // ... but that if a cohort identifier is set, we send it. > deferred = PromiseUtils.defer(); > TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve); > Services.prefs.setCharPref("browser.search.cohort", "testcohort"); > - Services.obs.notifyObservers(null, "browser-search-service", "init-complete"); > + Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current"); This change looks unrelated. This is probably for a different patch/bug? ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1682 (Diff revision 2) > > // Check that when changing the cohort identifier... > deferred = PromiseUtils.defer(); > TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve); > Services.prefs.setCharPref("browser.search.cohort", "testcohort2"); > - Services.obs.notifyObservers(null, "browser-search-service", "init-complete"); > + Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current"); This also looks unrelated. ::: toolkit/content/aboutTelemetry.js (Diff revision 2) > addonSection.setAttribute("class", "subsection-data subdata"); > let addons = ping.environment.addons; > this.renderAddonsObject(addons.activeAddons, addonSection, "activeAddons"); > this.renderActivePlugins(addons.activePlugins, addonSection, "activePlugins"); > this.renderKeyValueObject(addons.theme, addonSection, "theme"); > - this.renderKeyValueObject(addons.activeExperiment, addonSection, "activeExperiment"); Let's leave that for the follow-up.

:kmag, can you address the comments from Georg's review? Once this is done I can move on with the final steps to remove legacy Telemetry APIs.

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. https://reviewboard.mozilla.org/r/233430/#review239160 Andrew removed the toolkit/mozapps/extensions references in bug 1450801, but landing that is blocked on this bug, now, since the other experiments tests wind up registering an add-on provider and breaking some other add-on manager tests. > This change looks unrelated. > This is probably for a different patch/bug? It was part of the first version of the patch. I meant to remove it when I updated.

I applied the patch locally to ensure that it works. I run the test file :gfritzsche mentioned above, it passed. `Experiments.jsm` is still referenced, but in both places usage is if-guarded to check for its availability before use (`"@mozilla.org/browser/experiments-service;1" in Cc`). This is ok, these files will then be removed in bug 1450801 anyway. However, when applying the patch locally, I had to fix it in certain places. It seems to be based on changes from bug 1450801 (see below). This patch should be rebased on latest central to apply cleanly, and then bug 1450801 can be applied on top of that. * Patch not applying cleanly This patch: removing 3 preferences and empty line https://reviewboard.mozilla.org/r/233430/diff/3#chunk1.2 Patch from bug 1450801: removing remaining preferences, leaving 3 "old" preferences in place https://reviewboard.mozilla.org/r/233142/diff/3#chunk1.2

The patch on mozreview is on top of bug 1450801. The two will need to land at the same time, so there's no point in rebasing on top of m-c.

Comment on attachment 8964714 [details] Bug 1420908: Remove telemetry experiments. https://reviewboard.mozilla.org/r/233430/#review242202 Thanks, this looks good to me! Happy to see this cleaned up for us all! I assume you and Andrew have the dependencies between this and bug 1450801 covered.

https://hg.mozilla.org/integration/mozilla-inbound/rev/98389f291fe61e622267ba3e5e99792e6f5d6350 Bug 1420908: Remove telemetry experiments. r=gfritzsche

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a77da7f1488ef1c8011f049c5e536ca3716a619 Bug 1420908: Follow-up: Remove missing directory from codespell.yml. r=bustage DONTBUILD CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/98389f291fe6 https://hg.mozilla.org/mozilla-central/rev/8a77da7f1488

https://hg.mozilla.org/mozilla-central/rev/98389f291fe6 https://hg.mozilla.org/mozilla-central/rev/8a77da7f1488