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

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox31+ fixed, firefox32 verified)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

We currently don't trigger a refresh for the experiments view.
PreviousExperimentProvider should trigger that for on the "experiments-changed" observer topic.
Flags: firefox-backlog?
Blocks: telex
Component: Client: Desktop → Add-ons Manager
Depends on: 986677
Product: Firefox Health Report → Toolkit
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
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.
(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.
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.
Flags: needinfo?(gps)
Flags: firefox-backlog? → firefox-backlog+
Flags: needinfo?(gps)
Blocks: 1000114
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?]
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?] → p=5 s=it-31c-30a-29b.3 [qa?]
Blocks: 1001529
Depends on: 1001787
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa?] → p=5 s=it-31c-30a-29b.3 [qa+]
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa+] → p=5 s=it-32c-31a-30b.1 [qa+]
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: "test-experiment-1@tests.mozilla.org"});

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?
Flags: needinfo?(irving)
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(irving)
Flags: needinfo?(bmcbride)
(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!
(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?
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
QA Contact: andrei.vaida
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.
Attachment #8417433 - Flags: review?(irving)
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.
Attachment #8417433 - Flags: review?(irving) → review+
https://hg.mozilla.org/mozilla-central/rev/571814852fe8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Flags: needinfo?(georg.fritzsche)
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.
Flags: needinfo?(georg.fritzsche)
(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
Status: RESOLVED → VERIFIED
Whiteboard: p=5 s=it-32c-31a-30b.1 [qa+] → p=5 s=it-32c-31a-30b.1 [qa!]
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.
Blocks: 1007663
(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.
[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.
Attachment #8417433 - Attachment is obsolete: true
Attachment #8423888 - Flags: approval-mozilla-aurora?
[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.
Attachment #8423888 - Attachment is obsolete: true
Attachment #8423888 - Flags: approval-mozilla-aurora?
Attachment #8423891 - Flags: approval-mozilla-aurora?
Attachment #8423891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.