Closed
Bug 992258
Opened 11 years ago
Closed 11 years ago
Trigger a refresh of the experiments view in the addon manager when experiments change
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla32
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
Details
(Whiteboard: p=5 s=it-32c-31a-30b.1 [qa!])
Attachments
(1 file, 2 obsolete files)
12.39 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
(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.
Assignee | ||
Comment 3•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(gps)
Updated•11 years ago
|
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?]
Updated•11 years ago
|
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?] → p=5 s=it-31c-30a-29b.3 [qa?]
Updated•11 years ago
|
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa?] → p=5 s=it-31c-30a-29b.3 [qa+]
Updated•11 years ago
|
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa+] → p=5 s=it-32c-31a-30b.1 [qa+]
Assignee | ||
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
(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)
Assignee | ||
Comment 6•11 years ago
|
||
(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!
Assignee | ||
Comment 7•11 years ago
|
||
(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)
Comment 8•11 years ago
|
||
(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)
Updated•11 years ago
|
QA Contact: andrei.vaida
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
tracking-firefox31:
--- → ?
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•11 years ago
|
Updated•11 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
(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!]
Comment 17•11 years ago
|
||
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.
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
[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?
Assignee | ||
Comment 20•11 years ago
|
||
[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?
Updated•11 years ago
|
Attachment #8423891 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 21•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•