Closed Bug 990111 Opened 10 years ago Closed 10 years ago

Addon Provider for previously-active experiments

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 31

People

(Reporter: gps, Assigned: gfritzsche)

References

Details

(Whiteboard: p=5 s=it-32c-31a-30b.1 [qa!])

Attachments

(1 file, 4 obsolete files)

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).
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].
Flags: needinfo?(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.
Flags: needinfo?(gps)
Attached file Previous experiments addon provider (obsolete) —
First draft posted on RB. Tests fail hard for yet-to-be-determined reasons.
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'!
Ah |hg pull -r c952c81b1d17 https://hg.stage.mozaws.net/gecko-collab| it is.
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.
Nevermind, got it now, failure is in browser/experiments/test/xpcshell/test_api.js.

> Browser.Experiments.Experiments	TRACE	Experiments #16::updateExperiments() - discarding entry for test-experiment-1@tests.mozilla.org
> 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":"test-experiment-1@tests.mozilla.org","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 test-experiment-1@tests.mozilla.org
> Browser.Experiments.Experiments	WARN	ExperimentEntry #16::Experiment is not installed: test-experiment-1@tests.mozilla.org
> Browser.Experiments.Experiments	ERROR	Experiments #16::_main caught error: Error: Experiment is not installed: test-experiment-1@tests.mozilla.org
> TEST-UNEXPECTED-FAIL | /Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js | Unexpected exception Error: Experiment is not installed: test-experiment-1@tests.mozilla.org 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
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).
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...
Blocks: 988873
Blocks: 986677
Assignee: georg.fritzsche → gps
Whiteboard: p=0 s=it-31c-30a-29b.2
Hi Greg, can you provide a point value for this bug.
Flags: needinfo?(gps)
Whiteboard: p=0 s=it-31c-30a-29b.2 → p=5 s=it-31c-30a-29b.2
Whiteboard: p=5 s=it-31c-30a-29b.2 → p=5 s=it-31c-30a-29b.2 [qa?]
(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.
QA Contact: kamiljoz
Whiteboard: p=5 s=it-31c-30a-29b.2 [qa?] → p=5 s=it-31c-30a-29b.2 [qa+]
Flags: needinfo?(gps) → firefox-backlog+
Whiteboard: p=5 s=it-31c-30a-29b.2 [qa+] → p=5 s=it-31c-30a-29b.3 [qa+]
Assignee: gps → georg.fritzsche
Attachment #8400019 - Attachment is obsolete: true
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.
Attachment #8399712 - Attachment is obsolete: true
Attachment #8407624 - Flags: review?(bmcbride)
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.
Attachment #8407624 - Flags: review?(bmcbride) → review-
(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.
Above points addressed.

https://tbpl.mozilla.org/?tree=Try&rev=741b6ba1c9bc
Attachment #8407624 - Attachment is obsolete: true
Attachment #8410260 - Flags: review?(bmcbride)
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("");
Attachment #8410260 - Flags: review?(bmcbride) → review+
Review comment addressed, try looking good so far.
Attachment #8410260 - Attachment is obsolete: true
Attachment #8410342 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3d7485d2bb5c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Hey Kamil, will you have time to verify this bug before the desktop iteration ends this Monday?
Flags: needinfo?(kamiljoz)
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa+] → p=5 s=it-32c-31a-30b.1 [qa+]
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Whiteboard: p=5 s=it-32c-31a-30b.1 [qa+] → p=5 s=it-32c-31a-30b.1 [qa!]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.