Closed Bug 973992 Opened 6 years ago Closed 6 years ago

Telemetry experiments: addon manager UX

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: benjamin, Assigned: gps)

References

Details

Attachments

(4 files, 4 obsolete files)

The current experiment must be shown to the user and the user must have the ability to turn off the current experiment. The addon manager seems like the right place to do this, but we need more detailed UX design for this. bwinton please hand this bug back to me once that's done.
(nice to haves, for Bwinton....

* real / hashtag urls for 'parts of about:addon', so that "about:addon#some@id" works?
* new side tab:  "experiment"
Assigning to Mike to do the actual design.  ;)
Assignee: bwinton → mmaslaney
Design attached.

Suggested copy:

"What's this? You currently have Telemetry Experiments enabled, which shares your performance, usage, hardware and customization data about your browser with Mozilla to help us make Firefox better. If you wish to opt-out of the following experiments, please click here."

Currently working on the Experiments logo for the left nav.
jishnu/gpiper, the privacy policy currently calls this "Usage Statistics" but the in-product name in prefs is still "Telemetry". Does that matter at all in this context?

Also the sentence at "click here" isn't quite right, because you can opt out of *individual* experiments by choosing the "disable" button below. What you'd really be doing in prefs is opting out of Telemetry completely.

I know that I'm terrible at in-product wording, which is why I hate to be wriging this, but maybe:

"What's this? You currently have Telemetry enabled, which may install custom experiments from time to time. If you want to opt out of Telemetry, please click here. Learn More..." which goes to https://www.mozilla.org/en-US/legal/privacy/firefox.html#telemetry which is the same link we already have in the addon manager.

Thoughts?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(jmenon)
Flags: needinfo?(gpiper)
Works for me, Benjamin.
Flags: needinfo?(mmaslaney)
Assignee: mmaslaney → gps
Priority: -- → P2
Target Milestone: --- → mozilla30
Hi Ben

Lets ship consistent with the other language in the product.

For the notice - can we make this change:

"What's this? You currently have Telemetry enabled, which may install custom experiments from time to time. If you want to disable Telemetry, please click here. Learn More..." 

(opt-out -> disable)

From my perspective, opt-out isn't really precise - that's more of a conclusion - we generally use "disable" or "enable"
Flags: needinfo?(jmenon)
Here is a sketch/proposal of the API that the experiments service will provide for this consumer. 
From what i can tell from XPIProvider.jsm, accessing/showing the addon preferences can be done from the addon manager code (looking at "optionsURL", "options.xul", ...).
Keeping it as simple as possible for now based on the design here:


* The experiments service will fire an notifyObservers() event (topic "experiments-changed", null subject, null data) when the list of experiments or the activity status changed

* The experiments manager provides a method that returns an array of objects that describe the experiments that are or were active. The data format is like follows:

[
  {
    id: <string>,
    name: <string>,
    description: <string>,
    active: <boolean>,
    endDate: <Date>,
    ... // possibly later extended for FHR data source
  },
  ...
]

* The experiments manager provides a method to disable an experiment by its id.
Will the method that fetches the experiment list be async? I imagine we're going to be saving the history data on disk and so we'll need to do I/O in order to fetch it.
Good point, i hadn't thought of that. We should make the method async then, returning a promise that is resolved with the array described in comment 7.
(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Here is a sketch/proposal of the API that the experiments service will
> provide for this consumer.

I put up a more complete sketch of the API on bug 974009.
Talked to Unfocused about how this would work on a technical level.

The rows/tabs in about:addons are essentially populated by the add-on type of the Addon coming from AddonManager/XPIProvider land. You create a new add-on type and a new row magically appears. Or something like that.

If we create our experiments with a new XPI type, we get most of the UX for little cost.

Complicating things is the historical experiments list. We'll need to hack up XPIProvider to talk to the experiments API to effectively create dummy addon entries so they get reported in about:addons.

The alternative is to hack up the XUL for about:addons to manually add a new row/tab. But there is XBL involved and that's beyond my knowledge set and likely not something I could hash out in the next few days (unless I got a lot of hand holding). Besides, Unfocused seemed to advise going the hacked up XPIProvider route.

Either way, Unfocused says he's refactoring all this stuff in 31. So any code we write will be temporary. Incidentally, it sounds like his refactoring will make implementing our use case much easier going forward. Too bad TelEx isn't landing in 31!
https://hg.mozilla.org/mozilla-central/rev/6f4259bdadf5 is a commit that adds a new add-on type.
Questions as I'm implementing this:

1) Should the new Experiments tab not appear if an experiment has never executed?
2) Should the new Experiments tab not appear if Telemetry is disabled?
3) If someone has Telemetry disabled but has executed previous experiments and we show the Experiments tab, the message "You currently have Telemetry enabled" doesn't make any sense. What should be the behavior here?

These questions are all related.

I think we should have a message at the top of the tab that says something like "Experiments allow Mozilla to conduct targeted studies... Experiments are {enabled,disabled} because Telemetry is {enabled, disabled}."
Flags: needinfo?(benjamin)
about:addons uses an icon to delimit each add-on type. Existing icons live at toolkit/themes/*/mozapps/extensions. e.g. toolkit/themes/windows/mozapps/extensions/extensionGeneric.png.

Should I use an existing icon or would you like to provide one?
Flags: needinfo?(mmaslaney)
The experiments tab should not show if there are no experiments (past 180 days).
The experiments tab should show up if the user has past experiments but telemetry is currently disabled. In that case please just hide the entire heading and list the past experiments.
Flags: needinfo?(benjamin)
gps, have you picked a type id yet to use in the install manifest?
https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#type
Flags: needinfo?(gps)
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> gps, have you picked a type id yet to use in the install manifest?
> https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#type

I'm using 128 locally with the string name of "experiment".
Flags: needinfo?(gps)
Attached patch Support experiments add-ons (obsolete) — Splinter Review
This patch adds support for experiment add-ons to the XPI Provider and
about:addons.

The about:addons page still needs some work. Specifically, I need to
fully implement the help text. I'm still trying to wrap my head around
exactly how the "global-info" and "global-warning" system works. I think
if I keep poking around I'll find the solution. Putting this up for
feedback in case Unfocused comes back and tells me sooner.

I have another patch that adds a new add-ons provider for historical
experiments. Still working on that one. For now, this implementation
doesn't deal with old experiments.
Attachment #8391510 - Flags: feedback?(bmcbride)
And I found the CSS magic doing the global-info foo.
Attached patch Support experiments add-ons (obsolete) — Splinter Review
I believe this is feature complete (except for the historical add-ons
bit). But it's very rough around the edges. The layout of the message
in particular is crap. One of the buttons if off-window and requires a
resize. My frontend/XUL/CSS skills are beginner level (I do know DOM!),
so someone will likely have to spoon feed me a solution if we want this
to land in 30.

The lists page now has a link and button to disable Telemetry and to
open the Telemetry privacy policy.

Sadly, Telemetry doesn't have an API for disabling. I think this is the
3rd time I've had to perform the boilerplate for mucking around with
Telemetry's preferences (which isn't trivial since you either need to
involve the preprocessor or catch an import failure). I really wish
nsITelemetry would have an enabled flag. Oy.

The test fails due to silliness. But it's mostly working.

I don't agree with the opinion in comment #15 that we should hide the
entire introductory message (including the link to the privacy policy)
if Telemetry is disabled - even if experiments have been active at one
time. I think a better design would be to always have that message
present on the tab (the tab will only be displayed if experiments have
ever executed) and to have a separate message about telemetry toggling.
It may make sense to have that go both ways instead of just disable (we
want to make it easy for people to re-enable the feature if they see
it's benefit and want to help). But I'll hold off changing things until
told otherwise.
Attachment #8391613 - Flags: feedback?(bmcbride)
Attachment #8391613 - Flags: feedback?(benjamin)
Attachment #8391510 - Attachment is obsolete: true
Attachment #8391510 - Flags: feedback?(bmcbride)
Depends on: 974009
Attached patch Support experiments add-ons (obsolete) — Splinter Review
Improved the user interaction. We now have two "rows" in the UI for
TelEx. The first row is the "what's this" message including the "learn
more" link. The second row contains a "Telemetry is {enabled, disabled}
[{Disable, Enable} Telemetry]" message + button.

Still needs love in the cosmetics department. Learn more open tab test
also needs finished. Other than that, I'm optimistic this is feature
complete.
Attachment #8391782 - Flags: feedback?(bmcbride)
Attachment #8391782 - Flags: feedback?(benjamin)
Attachment #8391613 - Attachment is obsolete: true
Attachment #8391613 - Flags: feedback?(bmcbride)
Attachment #8391613 - Flags: feedback?(benjamin)
Comment on attachment 8391782 [details] [diff] [review]
Support experiments add-ons

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

(In reply to Gregory Szorc [:gps] from comment #20)
> I don't agree with the opinion in comment #15 that we should hide the
> entire introductory message (including the link to the privacy policy)
> if Telemetry is disabled

+1

(In reply to Gregory Szorc [:gps] from comment #21)
> Other than that, I'm optimistic this is feature
> complete.

Are you aiming on doing the historical experiments here or in another bug?


What about tweaks to the add-on items in the list/details view? This bug or another bug? Some are easy one-liners, some are a bit more involved.

::: toolkit/mozapps/extensions/content/extensions.css
@@ +191,5 @@
>  #addons-page .view-pane:not([type="plugin"]) .global-info-container {
>    display: none;
>  }
>  
> +#addons-page .view-pane:not[type="experiment"] .experiments-info-container {

Bad syntax here. Should be:
  :not([type="experiment"])

This is causing it to show on every view. Add a test for this?

::: toolkit/mozapps/extensions/content/extensions.js
@@ +16,5 @@
>  Cu.import("resource://gre/modules/DownloadUtils.jsm");
>  Cu.import("resource://gre/modules/AddonManager.jsm");
>  Cu.import("resource://gre/modules/addons/AddonRepository.jsm");
> +#ifdef MOZ_TELEMETRY_REPORTING
> +Cu.import("resource://gre/modules/TelemetryPing.jsm");

Should lazy import this.

@@ +210,5 @@
> +
> +  Services.prefs.setBoolPref(pref, !!enabled);
> +#endif
> +
> +  gListView.updateExperimentState();

This should be done via a pref observer.

@@ +1239,5 @@
>      },
> +
> +    cmd_experimentsEnable: {
> +      isEnabled: function () { return true; },
> +      doCommand: function () { setTelemetryEnabled(true); },

Name all these functions as per the convention established (in a stack trace, a dozen functions auto-named "doCommand" isn't useful).

::: toolkit/mozapps/extensions/content/extensions.xul
@@ +355,5 @@
>                        command="cmd_pluginCheck"/>
>                <spacer flex="5000"/> <!-- Necessary to allow the message to wrap -->
>              </hbox>
>            </hbox>
> +          <hbox class="view-header experiment-info-container">

Also use the global-info-container class for this, both for the style and for consistency.

@@ +358,5 @@
>            </hbox>
> +          <hbox class="view-header experiment-info-container">
> +            <vbox>
> +              <hbox class="global-info" flex="1">
> +                <label value="&experiment.info.label;"/>

Need to vertically align this with the text of the button. Also think we should consider adding an info icon to be consistent with most other alert/info bars - which, incidentally, should fix the vertical alignment issue for you.

@@ +364,5 @@
> +                        class="button-link"
> +                        label="&experiment.info.learnmore;"
> +                        tooltiptext="&experiment.info.learnmore;"
> +                        command="cmd_experimentsLearnMore"/>
> +                <spacer flex="5000"/> <!-- Necessary to allow the message to wrap. -->

Don't assume all locales on all window sizes won't need this on the other messages too.

@@ +366,5 @@
> +                        tooltiptext="&experiment.info.learnmore;"
> +                        command="cmd_experimentsLearnMore"/>
> +                <spacer flex="5000"/> <!-- Necessary to allow the message to wrap. -->
> +              </hbox>
> +              <hbox id="experiment-state-container" class="global-info" flex="1" align="center">

Need a little bit of white space between this and the above message.

@@ +372,5 @@
> +                  <label value="&experiment.info.state.enabled;"/>
> +                  <button id="experiment-disable-button"
> +                          label="&experiment.info.button.disable;"
> +                          tooltiptext="&experiment.info.button.disable;"
> +                          command="cmd_experimentsDisable"/>

I somewhat loathe to suggest this, because I have an old bug/patch to convert all links-that-are-really-buttons to buttons-that-look-like-buttons (bug 677170). But that also makes the buttons less in-your-face. So in lieu of that fix, converting these enable/disable buttons to look like links would help streamline the look of this, making it look much less awkward.

::: toolkit/mozapps/extensions/test/browser/browser_experiment_type.js
@@ +27,5 @@
> +
> +function check_experiment_state() {
> +  if (!is_telemetry_present()) {
> +    let container = gManagerWindow.document.getElementById("experiment-state-container");
> +    is_element_hidden(container, "Telemetry state hidden when Telemetry not available.");

This should be an early return.

::: toolkit/mozapps/extensions/test/browser/head.js
@@ +419,5 @@
> +/**
> + * Determines whether the Telemetry feature is available in the application.
> + */
> +function is_telemetry_present() {
> +  return "@mozilla.org/base/telemetry;1" in Components.classes;

This isn't adequate - I don't have MOZ_TELEMETRY_REPORTING in my build, but this does return true.

Also, seems toolkit.telemetry.infoURL still points to mozilla.org whenever we run tests. Because of the way the test fails on my machine, it somehow ends up clicking the Learn More button (presumably because it tries clicking a button that's hidden), which opens a tab for that URL. Should set it to a dummy URL in testing/profiles/prefs_general.js

::: toolkit/themes/windows/mozapps/extensions/extensions.css
@@ +276,5 @@
>  }
>  #category-recentUpdates > .category-icon {
>    list-style-image: url("chrome://mozapps/skin/extensions/category-recent.png");
>  }
> +#category-experiment > .category-icon {

Move this up so it's with the other auto-generated categories, to avoid confusion.
Attachment #8391782 - Flags: feedback?(bmcbride) → feedback+
gps do you have a proposed language/separation from comment 20? I don't mind doing the better version, but since "experiment history but telemetry disabled" is going to be a small edge case, I don't want to spend too much energy on it when we could have good-enough.
The language I used is in the patch.

Having separate strings for each element/control (as now implemented) is easier to implement (and test) than having one element/control and building up the content dynamically.
(In reply to Blair McBride [:Unfocused] from comment #22)
> (In reply to Gregory Szorc [:gps] from comment #21)
> > Other than that, I'm optimistic this is feature
> > complete.
> 
> Are you aiming on doing the historical experiments here or in another bug?

It's at least a part 2, at most another bug.

The historical experiments stuff relies on the Experiments API being checked in and should have minimal impact on the UI (including no strings). It's important, but it can be uplifted.

> What about tweaks to the add-on items in the list/details view? This bug or
> another bug? Some are easy one-liners, some are a bit more involved.

Gah, I missed those requirements. IMO those can be a separate patch. I'd like to get the new tab implemented as a nice standalone patch.

> ::: toolkit/mozapps/extensions/content/extensions.css
> @@ +191,5 @@
> >  #addons-page .view-pane:not([type="plugin"]) .global-info-container {
> >    display: none;
> >  }
> >  
> > +#addons-page .view-pane:not[type="experiment"] .experiments-info-container {
> 
> Bad syntax here. Should be:
>   :not([type="experiment"])
> 
> This is causing it to show on every view. Add a test for this?

Good catch. (BTW, I know very little CSS and was just copying existing patterns.) Will write a test.

> ::: toolkit/mozapps/extensions/test/browser/head.js
> @@ +419,5 @@
> > +/**
> > + * Determines whether the Telemetry feature is available in the application.
> > + */
> > +function is_telemetry_present() {
> > +  return "@mozilla.org/base/telemetry;1" in Components.classes;
> 
> This isn't adequate - I don't have MOZ_TELEMETRY_REPORTING in my build, but
> this does return true.

I haven't tested with Telemetry disabled yet. Will fix this.

> Also, seems toolkit.telemetry.infoURL still points to mozilla.org whenever
> we run tests. Because of the way the test fails on my machine, it somehow
> ends up clicking the Learn More button (presumably because it tries clicking
> a button that's hidden), which opens a tab for that URL. Should set it to a
> dummy URL in testing/profiles/prefs_general.js

Thanks. I was going to ask how we test these things :)
So it turns out my understanding of how Telemetry is shipped as a feature is misunderstood.

The Telemetry feature/component is *always* present. MOZ_TELEMETRY_REPORTING appears to merely control whether the build can *submit* Telemetry. However, references to Telemetry in the UI are behind MOZ_TELEMETRY_REPORTING.

So, TelemetryPing.jsm is always present. However, it has no API for querying/setting active state nor differentiating whether Telemetry should be "user visible." So, we need #ifdef MOZ_TELEMETRY_REPORTING in any UI involving Telemetry. What an ugly mess.
Forgot to mention: Make sure to make the new add-on type restartless (bootstrap=true). Will want this sooner rather than later, as it'll be impacting Georg's tests over in bug 974009.

eg: https://hg.mozilla.org/mozilla-central/diff/6f4259bdadf5/toolkit/mozapps/extensions/XPIProvider.jsm
(In reply to Gregory Szorc [:gps] from comment #25)
> The historical experiments stuff relies on the Experiments API being checked
> in and should have minimal impact on the UI (including no strings). It's
> important, but it can be uplifted.

I can see two strings you'll need for that: 
* "Complete"
* "%S ago"
Comment on attachment 8391782 [details] [diff] [review]
Support experiments add-ons

The UI should not expose a direct button to enable/disable telemetry. It must open the preferences pane. This is important because we're likely to change the data choices pane to be a slider and I don't want to duplicate the explanations here.
Attachment #8391782 - Flags: feedback?(benjamin) → feedback-
(In reply to Gregory Szorc [:gps] from comment #14)
> about:addons uses an icon to delimit each add-on type. Existing icons live
> at toolkit/themes/*/mozapps/extensions. e.g.
> toolkit/themes/windows/mozapps/extensions/extensionGeneric.png.
> 
> Should I use an existing icon or would you like to provide one?

I will be providing one, Gregory.
Flags: needinfo?(mmaslaney)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> The UI should not expose a direct button to enable/disable telemetry. It
> must open the preferences pane. This is important because we're likely to
> change the data choices pane to be a slider and I don't want to duplicate
> the explanations here.

Addons Manager is inside Toolkit (application agnostic) and Preferences are specific to the application. How do you propose we open an application-specific window from a generic feature?

(I believe the answer in the past is we don't do such a thing. I'm looking for code that opens the preferences pane and all I see are callers under browser/.)
No manual opening of about:preferences or preferences.xul outside of browser:

http://dxr.mozilla.org/mozilla-central/search?q=preferences.xul&redirect=true
http://dxr.mozilla.org/mozilla-central/search?q=about%3Apreferences&redirect=true

The (recommended) wrapper to open preferences (openPreferences.js from browser/base/content/utilityOverlay.js) is also not used outside of browser:

http://dxr.mozilla.org/mozilla-central/search?q=openPreferences&redirect=true

I am ignoring comment #29 because I don't believe the requirement is rationale (however sensible it may seem from a UX perspective). Sad panda is sad.
Per IRC conversation, we should be able to look for openPreferences() on the parent chrome window of the AddonManager. If it's not there, don't show the link/button to open the preferences.
(In reply to Gregory Szorc [:gps] from comment #33)
> Per IRC conversation, we should be able to look for openPreferences() on the
> parent chrome window of the AddonManager. If it's not there, don't show the
> link/button to open the preferences.

Yep. We inspect the parent chrome window in one or two cases already, for integration. Bug 730127 would hopefully introduce a mediation module to help with this sort of thing.
Moving review to RB.

New patch should incorporate most feedback and I believe is close to review worthy.

Still waiting on an icon and a (likely minor) fix to make the test pass.
Attachment #8391782 - Attachment is obsolete: true
Attachment #8392539 - Flags: feedback?(bmcbride)
Attachment #8392539 - Flags: feedback?(benjamin)
Attachment #8392539 - Flags: feedback?(bmcbride) → feedback+
Flags: needinfo?(gpiper)
(In reply to Blair McBride [:Unfocused] from bug 974009, comment #97)
> (In reply to Georg Fritzsche [:gfritzsche] from bug 974009, comment #93)
> > The AddonManager experiments UI should disable experiments use the public
> > API provided here.
> 
> But it can't :) That's not how the Add-ons Manager works. The Add-ons
> Manager UI is built to be generic in how it handles different add-on types.
> The patches in bug 973992 that result in the experiment add-ons showing up
> in the Add-ons Manager UI actually barely touchs the UI code - most of it
> is/will be dealing with backend support that's needed anyway, and the UI
> just automatically does everything for us.

Alright, this might be side-tracking... but:
We need fake entries for the historical experiments anyway. In addition to the historical entries, we have a special case of possibly having an experiment active.

From my understanding of this bug, we do the historical entries & the active one separately here and i'm wondering why we would if we can just handle it a) consistently and b) by directly using the experiment managers API.
Comment on attachment 8392539 [details]
Display experiments add-on in Addon Manager

Was there something in particular you wanted feedback on? I'm not a reviewer for this code.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #37)
> Was there something in particular you wanted feedback on? I'm not a reviewer
> for this code.

I wanted your approval for the new UX.
Can I see a screenshot or two?
Attached image AddonManager screenshot
Here's what the current patch on RB looks like.

The icon, historical experiments, and the compatibility note will be addressed in subsequent patches/bugs.
Blocks: 986093
Where will the "2 days remaining" bit go?
Flags: needinfo?(gps)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> Where will the "2 days remaining" bit go?

Is that a launch time feature? I think it involves XBL work and is thus considerable effort (at least to me - I don't know XBL).
Flags: needinfo?(gps)
Is there something we can do more simply, such as sticking it into the existing description field?
Blocks: 986530
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #43)
> Is there something we can do more simply, such as sticking it into the
> existing description field?

That hack just might work. Let's defer to a follow-up, if possible. (I'm trying to prevent churn on the existing patch.)
Comment on attachment 8392539 [details]
Display experiments add-on in Addon Manager

ok. I don't want to ship this to beta without that, though.
Attachment #8392539 - Flags: feedback?(benjamin)
Blocks: 986677
Attached patch experiment-addons-support.patch (obsolete) — Splinter Review
Small changes to support the "experiment" addon type.

Part of this try push:
https://tbpl.mozilla.org/?tree=Try&rev=4ffee5ea7661
Attachment #8395389 - Flags: review?(bmcbride)
OS: Linux → All
Hardware: x86_64 → All
Also cancel addon installation if the addon type is incorrect.

This needs to land with the main patch here anyway so it seemed easiest to just put this small change in here.
Attachment #8395389 - Attachment is obsolete: true
Attachment #8395389 - Flags: review?(bmcbride)
Attachment #8395558 - Flags: review?(bmcbride)
Comment on attachment 8395558 [details] [diff] [review]
browser/experiments/ support for experiment addons

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

::: browser/experiments/Experiments.jsm
@@ +1305,5 @@
>            gLogger.trace("ExperimentEntry::start() - onInstallStarted for " + this.id);
> +          if (install.addon.type !== "experiment") {
> +            gLogger.error("ExperimentEntry::start() - wrong addon type");
> +            failureHandler({state: -1, error: -1}, "onInstallStarted");
> +            install.cancel();

There is a race condition here. AddonManager listeners are global. I reckon you could receive an onInstallStarted from an unrelated addon. This may not hold true today because of the implementation of AddonManager. But you never know what the future may bring.

I think you should ignore events related to irrelevant-to-us add-on types.
Duplicate of this bug: 986093
(In reply to Gregory Szorc [:gps] from comment #48)
> There is a race condition here. AddonManager listeners are global. I reckon
> you could receive an onInstallStarted from an unrelated addon. This may not
> hold true today because of the implementation of AddonManager. But you never
> know what the future may bring.

This listener is only added to the AddonInstall here, not globally to the AddonManager (which limits it to this AddonInstalls events).
Comment on attachment 8395558 [details] [diff] [review]
browser/experiments/ support for experiment addons

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

Yea, what Georg said. The Add-ons Manager has several different kinds of listeners, for different domains. InstallListeners are local to their instance of AddonInstall. And we *do* support updating from one add-on type to another - which the experiments system really doesn't want, so that sanity check is warranted. 

Saying that, I think we should have an ID check in there anyway - but for a different reason. We support changing an add-on's ID via an update - while the Add-ons Manager copes perfectly fine with that, Experiments.jsm will *not* handle it (and, IMO, it isn't worth the complexity).
Attachment #8395558 - Flags: review?(bmcbride) → review-
Comment on attachment 8395558 [details] [diff] [review]
browser/experiments/ support for experiment addons

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

(In reply to Blair McBride [:Unfocused] from comment #51)
> Saying that, I think we should have an ID check in there anyway - but for a
> different reason. We support changing an add-on's ID via an update - while
> the Add-ons Manager copes perfectly fine with that, Experiments.jsm will
> *not* handle it (and, IMO, it isn't worth the complexity).

Ah, seems I missed bug 985670.

But also, my brain forgot a crucial aspect there: add-ons can only change IDs via normal update mechanism, which experiment add-ons don't use.

So this is good to go.
Attachment #8395558 - Flags: review- → review+
Comment on attachment 8392539 [details]
Display experiments add-on in Addon Manager

Formally requesting review for patch on ReviewBoard.
Attachment #8392539 - Attachment description: . → Display experiments add-on in Addon Manager
Attachment #8392539 - Flags: review?(bmcbride)
Attachment #8392539 - Flags: review?(bmcbride)
Comment on attachment 8392539 [details]
Display experiments add-on in Addon Manager

New patch up on RB.
Attachment #8392539 - Flags: review?(bmcbride)
Attachment #8392539 - Flags: review?(bmcbride) → review+
And Georg's patch:

https://hg.mozilla.org/integration/fx-team/rev/d3c2115ffc94
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9da8ac69f99b
https://hg.mozilla.org/mozilla-central/rev/d3c2115ffc94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla30 → mozilla31
I tried to scan the bug to find a discussion about the message, but I wasn't able to find it besides comment 6.

I confess that I found the current message scary: "Telemetry may install and run experiments from time to time."

My first reaction reading the string: why a function supposed to "measure from distance" is using my browser as a guinea pig and doing things like installing and running add-ons? 

Can we use something less intimidating, along the line of "Telemetry uses these experimental add-ons to collect data."?
Deferring string questions to bsmedberg.
Flags: needinfo?(benjamin)
> I confess that I found the current message scary: "Telemetry may install and
> run experiments from time to time."
> 
> My first reaction reading the string: why a function supposed to "measure
> from distance" is using my browser as a guinea pig and doing things like
> installing and running add-ons? 
> 
> Can we use something less intimidating, along the line of "Telemetry uses
> these experimental add-ons to collect data."?

We do in fact intend to use people with Telemetry enabled as potential guinea-pigs. We're in the process of updating various bits of the online documentation to reflect that. For example soon we're hoping to deploy an experiment to a subset of users that would enable automatic web-site translation. This certainly goes beyond just addons which collect data.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #60)
> We do in fact intend to use people with Telemetry enabled as potential
> guinea-pigs. We're in the process of updating various bits of the online
> documentation to reflect that. For example soon we're hoping to deploy an
> experiment to a subset of users that would enable automatic web-site
> translation. This certainly goes beyond just addons which collect data.

Thanks for the explanation: based on this, the string is definitely correct as it is.
Thanks for your frankness, Benjamin, but after reading your comment #60 I'm feeling caught in a dilemma: Should I enable telemetry so that my unusual use cases (e.g. 79 add-ons, counting only the enabled ones; more than 100 browser tabs) have a chance to find their way into Mozilla's statistics? Or should I disable it because I'm scared by something so similar to malware practices (installing unspecified software into my browser behind my back to do I-don't-know what)?
Oh, and BTW, isn't this whole telemetry system a security risk? What if a blackhat addon surreptitiously changes toolkit.telemetry.server and also sets toolkit.telemetry.enabled to true and toolkit.telemetry.debugSlowSql to whatever it takes to enable transmission of sensitive data?
Addons can do worse things already on their own, that's what the review process on AMO and blocklists are for.
But i don't think this is the place to discuss such concerns - bsmedberg can you recommend a mailing list here?
This project was announced to firefox-dev and should be discussed there.
Depends on: 1001529
No longer depends on: 1001529
Depends on: 1001787
You need to log in before you can comment on or make changes to this bug.