990111 - Addon Provider for previously-active experiments

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

We need an Addon Provider that returns previously-active experiments. This is the easy way to get old experiments listed in about:addons.

The first step of this (and possibly the scope of this bug) is to ensure we retain the necessary "required" properties for Addon objects in the experiments' local JSON file. See https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/Addon

When I very casually looked at this a few weeks ago, we were missing some elements. We should probably add those to the JSON.

Please note we may want to expose some of the optional properties and methods as well (if they make sense).

Comment 1

[Georg Fritzsche [:gfritzsche]]

This has quite a few properties that we never want to show in the addon manager. Presumably we can just have dummy values on most of these.
Which of them do we really need for the UX? We already have all that are needed to do the UI in attachment 8387109 [details].

Comment 2

[Gregory Szorc [:gps]]

I think I'm done with bug 989137. I'll work on this for the next few hours. Hopefully I'll have a patch to report by EOD.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Previous experiments addon provider

First draft posted on RB. Tests fail hard for yet-to-be-determined reasons.

Comment 4

[Georg Fritzsche [:gfritzsche]]

That pull command on RB doesn't work.

hg pull -r gps/telex/addmonmanager https://hg.stage.mozaws.net/gecko-collab
pulling from https://hg.stage.mozaws.net/gecko-collab
abort: unknown revision 'gps/telex/addmonmanager'!

Comment 5

[Georg Fritzsche [:gfritzsche]]

Ah |hg pull -r c952c81b1d17 https://hg.stage.mozaws.net/gecko-collab| it is.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Hm, on the above revision the following tests are fine for me:
browser/experiments/test/xpcshell/
toolkit/mozapps/extensions/test/browser/browser_experiments.js
toolkit/mozapps/extensions/test/xpcshell/test_experiment.js

Not sure what else to look for.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Nevermind, got it now, failure is in browser/experiments/test/xpcshell/test_api.js.

> Browser.Experiments.Experiments	TRACE	Experiments #16::updateExperiments() - discarding entry for [e-mail]
> Browser.Experiments.Experiments	TRACE	Experiments #16::_evaluateExperiments
> Browser.Experiments.Experiments	TRACE	ExperimentEntry #16::maybeStop()
> Browser.Experiments.Experiments	TRACE	ExperimentEntry #16::isApplicable() - now=1411120800, randomValue=0.8392389094471487, data={"id":"[e-mail]","xpiURL":"http://localhost:50900/data/experiment-1.xpi","xpiHash":"sha1:babfbcd37d380311e5888d1016f8254fe14d0df3","startTime":1411120740,"endTime":1411120860,"maxActiveSeconds":864000,"appName":["XPCShell"],"channel":["nightly"]}
> Browser.Experiments.Experiments	TRACE	ExperimentEntry #16::ensureActive() for [e-mail]
> Browser.Experiments.Experiments	WARN	ExperimentEntry #16::Experiment is not installed: [e-mail]
> Browser.Experiments.Experiments	ERROR	Experiments #16::_main caught error: Error: Experiment is not installed: [e-mail]
> TEST-UNEXPECTED-FAIL | /Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js | Unexpected exception Error: Experiment is not installed: [e-mail] at resource:///modules/experiments/Experiments.jsm:1638 - See following stack:
>  Experiments.ExperimentEntry.prototype.ensureActive<@resource:///modules/experiments/Experiments.jsm:1638:7
>  TaskImpl_run@resource://gre/modules/Task.jsm:282:1
>  Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:748:11
>  this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:627:7
>  _do_main@/.../testing/xpcshell/head.js:174:5
>  _execute_test@/.../testing/xpcshell/head.js:378:5

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Fixup 1, addon provider (un)registration

This fixes the first issue (nevermind all the tracing etc.), next up is a more obvious issue with |addon.userDisabled=false| triggering a "Unknown add-on ID".

The problem should be that the new provider only delivers previous experiments, but is the sole provider for "experiment" addons (AFAICT).

Comment 9

[Gregory Szorc [:gps]]

I got all the existing tests to pass by unregistering the add-on provider when Experiment.uninit() is called!

Now, to write new tests and ensure all this works as intended...

Comment 10

[Marco Mucci [:MarcoM]]

Hi Greg, can you provide a point value for this bug.

Comment 11

[u279076]

(In reply to Gregory Szorc [:gps] from comment #9)
> Now, to write new tests and ensure all this works as intended...

Kamil, please work with Gregory to make sure this has enough tests. If it's sufficiently covered via automation then please mark this as [qa-]. Thanks.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Previous experiments addon provider

This is rebased and with the issues in RB addressed:
https://reviewboard.allizom.org/r/54/

I'm trying to get ownership of the RB reviews, while i'm waiting this i'm just putting it for review up here.

Comment 13

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=0a7e8a13d2fe

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8407624 [details] [diff] [review]
Previous experiments addon provider

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

Think the only major thing left to do is what I brought up in bug 992258 comment 1.

::: browser/experiments/Experiments.jsm
@@ +464,5 @@
> +
> +    if (!gAddonProvider) {
> +      this._log.trace("Registering previous experiment add-on provider.");
> +      gAddonProvider = new Experiments.PreviousExperimentProvider(this, [
> +          new AddonManagerPrivate.AddonType("experiment",

You'll want to add a comment that the properties of this AddonType should be kept in sync with the experiment AddonType registered in XPIProvider.

@@ +465,5 @@
> +    if (!gAddonProvider) {
> +      this._log.trace("Registering previous experiment add-on provider.");
> +      gAddonProvider = new Experiments.PreviousExperimentProvider(this, [
> +          new AddonManagerPrivate.AddonType("experiment",
> +                                            "type.%ID%.name",

You're missing the string bundle URI parameter before this. Only reason this is working now is because if you register two AddonTypes with the same type identifier, we currently completely ignore the second one (which may change in the future).

@@ +2051,5 @@
> +/**
> + * An add-on that represents a previously-installed experiment.
> + */
> +function PreviousExperimentAddon(experiment) {
> +  this._author = "dummy author";

Presumably we want this to be something, uh, different :)

@@ +2122,5 @@
> +    return true;
> +  },
> +
> +  get version() {
> +    return this._version;

Should just directly return null from this, rather than the indirection.

::: toolkit/mozapps/extensions/test/browser/browser_experiments.js
@@ +282,5 @@
> +  Assert.ok(addons[0].appDisabled, "It is a previous experiment.");
> +  Assert.equal(addons[0].id, "experiment-1", "Add-on ID matches expected.");
> +
> +  // Verify the UI looks sane.
> +  // TODO remove the pane cycle once the UI refreshes automatically.

Are you planning on handling that here or in bug 992258?

@@ +291,5 @@
> +
> +  let item = get_addon_element(gManagerWindow, "experiment-1");
> +  Assert.ok(item, "Got add-on element.");
> +
> +  // TODO verify experiment is disabled/shaded out. How?

Just check the "active" attribute on the richlistitem is set to "false"

@@ +294,5 @@
> +
> +  // TODO verify experiment is disabled/shaded out. How?
> +
> +  // User control buttons should not be present because previous experiments
> +  // should have no permissions.

May as well check the preferences button too.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #14)
> Think the only major thing left to do is what I brought up in bug 992258
> comment 1.

Per bug 992258, comment 2, it mostly needs to trigger onExternalInstall properly and will be handled in that bug, not here.

> @@ +2051,5 @@
> > +/**
> > + * An add-on that represents a previously-installed experiment.
> > + */
> > +function PreviousExperimentAddon(experiment) {
> > +  this._author = "dummy author";
> 
> Presumably we want this to be something, uh, different :)

We won't show the author and don't care about it. Could just have the author be an empty string if you prefer that?

> ::: toolkit/mozapps/extensions/test/browser/browser_experiments.js
> @@ +282,5 @@
> > +  Assert.ok(addons[0].appDisabled, "It is a previous experiment.");
> > +  Assert.equal(addons[0].id, "experiment-1", "Add-on ID matches expected.");
> > +
> > +  // Verify the UI looks sane.
> > +  // TODO remove the pane cycle once the UI refreshes automatically.
> 
> Are you planning on handling that here or in bug 992258?

Bug 992258.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Previous experiments addon provider

Above points addressed.

https://tbpl.mozilla.org/?tree=Try&rev=741b6ba1c9bc

Comment 17

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8410260 [details] [diff] [review]
Previous experiments addon provider

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

gfritzsche asked me to steal the re-review...

::: browser/experiments/Experiments.jsm
@@ +204,5 @@
>  
>  // Returns a promise that is resolved with an Array<Addon> of the installed
>  // experiment addons.
>  function installedExperimentAddons() {
>    let deferred = Promise.defer();

I realize this is from a previous patch, and Blair didn't flag it, but Promise.defer() isn't in the ES6 Promise standard; they only support

let promise = new Promise((resolve, reject) => {
  ... your code, invoking resolve() and reject() as required
});

You don't need to change this for r+, just a heads up that I don't think new code should use Promise.defer().

@@ +2057,5 @@
> +/**
> + * An add-on that represents a previously-installed experiment.
> + */
> +function PreviousExperimentAddon(experiment) {
> +  this._author = "";

Don't really need the _author field at all, get creator() can just return new AMP.AddonAuthor("");

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Previous experiments addon provider

Review comment addressed, try looking good so far.

Comment 19

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/3d7485d2bb5c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d7485d2bb5c

Comment 21

[Marco Mucci [:MarcoM]]

Hey Kamil, will you have time to verify this bug before the desktop iteration ends this Monday?

Comment 22

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the latest Nightly and Aurora builds from the following location:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-01-03-02-02-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-01-00-40-04-mozilla-aurora/

- Installed 10 instances of the experiment and ensured that all the experiments were being listed under "about:support" and "about:addons"
- Ensured that the current experiment appears as active:true after restarting firefox
- Ensured that the disabled experiments appear as active:false under "about:support" and are disabled under "about:addons"
- Ensured that the current experiment is disabled/uninstalled once the time limit has been reached (moved the computers date ahead day by day)
- Ensured that the correct "days left" is being displayed for all the experiments currently being listed in "about:addons"
- Ensured that the UI is being displayed correctly and the user can tell between an active and disabled experiment
- Ensured that all the tiles are appearing when creating a new tab via "CTRL + T", "+" or "File -> New Tab"
- Ensured that the enabled/disabled experiments are not appearing in "extensions.bootstrappedAddons"
- Ensured that "experiments.activeExperiment" is displaying the correct value under "about:config"
- Ensured that "Learn More" and "Telemetry Settings" buttons are working correctly
- Ensured that the "More" button is working correctly for each enabled and disabled experiments
- Went through the above test cases using OSX 10.9.2 and Windows 8.1

Going through the above manual test cases plus the automated tests that have been added by Georg should be sufficient to mark this as verified.