992258 - Trigger a refresh of the experiments view in the addon manager when experiments change

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We currently don't trigger a refresh for the experiments view.
PreviousExperimentProvider should trigger that for on the "experiments-changed" observer topic.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

If this is only about historical experiments needing updating in the UI, then it's a bug in the PreviousExperimentProvider. The UI should never deal with this itself - it should always be driven by standard events coming from providers.

I assume the issue is with an experiment changing from being active to being completed? PreviousExperimentProvider needs to fire some install events to make the new addon appear - using onExternalInstall makes this *far* easier than the usual install process. See LightweightThemeManager.jsm or SocialService.jsm for examples.

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #1)
> I assume the issue is with an experiment changing from being active to being
> completed? PreviousExperimentProvider needs to fire some install events to
> make the new addon appear - using onExternalInstall makes this *far* easier
> than the usual install process.

Yes - i am already testing around with onExternalInstall, but there are apparently issues with our current experiment addon handling and i need to figure out why i am not receiving onUninstalled or onDisabled properly.

At the moment i think have the refresh handled, but i can't trigger it properly.

Comment 3

[Georg Fritzsche [:gfritzsche]]

gps, i'm wondering does this partially or fully overlap with your work in bug 990111 or other bugs?
This should be just observing "experiments-changed" from the PreviousExperimentsProvider, tracking the changes & triggering onExternalInstall/onExternalUninstall accordingly.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Today i at least learned that my actual usage of triggering "onExternalInstall" etc. as directly causing this in the console is fine (as well as other means like disabling the experiment through the API):

> Cu.import("resource:///modules/experiments/Experiments.jsm");
> Experiments.instance().onUninstalled({id: "[e-mail]"});

However, clicking remove on an experiment item in the AddonManagers list view doesn't trigger Experiments.onUninstalled() until i switch the Addon category.
I'm not quite sure yet where this could fail/hang/... irving, unfocused, do you have any ideas here?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> However, clicking remove on an experiment item in the AddonManagers list
> view doesn't trigger Experiments.onUninstalled() until i switch the Addon
> category.
> I'm not quite sure yet where this could fail/hang/... irving, unfocused, do
> you have any ideas here?

This is expected - this is how it works at the moment for restartless extensions to support undo-uninstall in the UI, until we fix bug 612168. What happens currently when you hit the uninstall button for a restartless extension is that the UI disables the extension, and marks it for future uninstall - when you switch views (or close the UI), it performs the uninstall.

You shouldn't need any browser-chrome tests to cover this, so it shouldn't be a problem - existing Add-on Manager UI tests cover this fine. All that's needed is xpcshell tests to ensure the API works as expected.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #5)
> This is expected - this is how it works at the moment for restartless
> extensions to support undo-uninstall in the UI, until we fix bug 612168.
> What happens currently when you hit the uninstall button for a restartless
> extension is that the UI disables the extension, and marks it for future
> uninstall - when you switch views (or close the UI), it performs the
> uninstall.

Ok, this is... awkward, but good to know!

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> (In reply to Blair McBride [:Unfocused] from comment #5)
> > This is expected - this is how it works at the moment for restartless
> > extensions to support undo-uninstall in the UI, until we fix bug 612168.
> > What happens currently when you hit the uninstall button for a restartless
> > extension is that the UI disables the extension, and marks it for future
> > uninstall - when you switch views (or close the UI), it performs the
> > uninstall.
> 
> Ok, this is... awkward, but good to know!

Hm, interestingly using remove from the context menu on an addon list item already triggers onUninstalled() right away.
Given that, i'm not sure why the remove button on the item can't just trigger the same behavior?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Hm, interestingly using remove from the context menu on an addon list item
> already triggers onUninstalled() right away.
> Given that, i'm not sure why the remove button on the item can't just
> trigger the same behavior?

Hah, shows how often people use the context menu... that's a bug, mind filing one for that? It should let you undo the uninstall still.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Make provider work from snapshot & trigger install/uninstall events

Irving, can you look into this?

This makes the addon provider for historical ("previous") experiments work from a snapshot of the experiments.
That allows it to detect what changed when "experiments-changed" is fired and trigger the install/uninstall events required for the addon manager UI.

Comment 10

[Georg Fritzsche [:gfritzsche]]

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

Comment 11

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

Comment on attachment 8417433 [details] [diff] [review]
Make provider work from snapshot & trigger install/uninstall events

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

r+ with the nits below fixed, but I sure feel like this would have been a lot easier if we'd extended XPIProvider to know about a new add-on state for expired experiments rather than trying to build it around the existing APIs.

::: browser/experiments/Experiments.jsm
@@ +2058,5 @@
>  
>  this.Experiments.PreviousExperimentProvider.prototype = Object.freeze({
> +  startup: function () {
> +    this._log.trace("startup()");
> +    Services.obs.addObserver(this, OBSERVER_TOPIC, false);

Existing code, but please rename this constant to something more descriptive; *which* observer topic are we interested in?

@@ +2105,5 @@
> +      let added = [e.id for (e of list) if (!oldMap.has(e.id))];
> +      let removed = [e.id for (e of this._experimentList) if (!newMap.has(e.id))];
> +
> +      for (let id of added) {
> +        this._log.trace("updateExperimentList() - adding " + id + "\n");

Log.jsm does its own newlines, so you don't need (or want) to put them on the messages you pass in.

Comment 12

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/571814852fe8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/571814852fe8

Comment 14

[Andrei Vaida [:avaida]]

Georg, it was my understanding that removing an experiment should automatically update the "Experiments" tab from Add-ons Manager and display it grayed out, with its state "Complete". On Nightly 32 2014-05-07 [1], removing the experiment results in a completely empty (blank) "Experiments" tab. 

On the other hand, if you switch from "Experiments" to "Services" for example and then back, the "Experiments" tab is refreshed, with the previously removed experiment displayed as "Complete" and disabled.

Could you please confirm if this behavior is intended?


[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0

Comment 15

[Georg Fritzsche [:gfritzsche]]

You mean when using the remove button in the addon manager UI?
Per comment 5 this currently expected.

Refresh should work fine though when e.g. an experiment runs out or gets disabled by other means like using the API or flipping the experiments.enabled pref.

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> You mean when using the remove button in the addon manager UI?
> Per comment 5 this currently expected.
> 
> Refresh should work fine though when e.g. an experiment runs out or gets
> disabled by other means like using the API or flipping the
> experiments.enabled pref.
That's right, thanks for confirming.

This issue is verified fixed on Nightly 32 2014-05-07, using:
  * Windows 7 64-bit [1],
  * Mac OS X 10.8.5 [2],
  * Ubuntu 12.04 32-bit [3].

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:32.0) Gecko/20100101 Firefox/32.0
[3] Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0

Comment 17

[Benjamin Smedberg]

I don't understand comment 15. Clicking the button to disable an experiment should never leave the user with a completely empty UI. Per comment 5 it may make sense for us to wait to actually disable the experiment until the user switches panes. But either way there must always be a row showing.

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> I don't understand comment 15. Clicking the button to disable an experiment
> should never leave the user with a completely empty UI. Per comment 5 it may
> make sense for us to wait to actually disable the experiment until the user
> switches panes. But either way there must always be a row showing.

Follow-up in bug 1007663.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Make provider work from snapshot & trigger install/uninstall events

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Telemetry experiments.
User impact if declined: Experiment changes are not reflected in the addon manager UI unless explicitly reloading or switching categories.
Testing completed (on m-c, etc.): Verified on m-c.
Risk to taking this patch (and alternatives if risky): Low-medium - the recent experiment changes were well tested.
String or IDL/UUID changes made by this patch: None.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Attachment: Make provider work from snapshot & trigger install/uninstall events

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Telemetry experiments.
User impact if declined: Experiment changes are not reflected in the addon manager UI unless explicitly reloading or switching categories.
Testing completed (on m-c, etc.): Verified on m-c.
Risk to taking this patch (and alternatives if risky): Low-medium - the recent experiment changes were well tested.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09ee10146bbe