1.73 KB, patch
|Details | Diff | Splinter Review|
28.77 KB, patch
|Details | Diff | Splinter Review|
khuey suggests that functionality be gated on MOZ_UPDATE_CHANNEL
Why should this behavior differ based on the update channel? How do you see it differing (always on for non-release, opt-in for release)?
(In reply to comment #1) > How do you see > it differing (always on for non-release, opt-in for release)? IIUC, we'd want always *off* for non-release (that is -- for local debug builds, one-off tryserver builds with random hacky changes, etc), and opt-in for release.
It should be turned on if MOZ_UPDATE_CHANNEL != default, IMO.
So the goal is to disable it by default, but enable it for moco-shipped builds?
(In reply to comment #4) > So the goal is to disable it by default, but enable it for moco-shipped > builds? yes
That's probably best done with a separate configure flag that we set in the MoCo build configs. I don't think it makes much sense to tie it to the update channel.
Why not? That's how Test Pilot is done. This seems similar to me.
Well, I'm not a fan of that setup either, but aside from that I think the requirements are somewhat different. Don't we want telemetry on in official Nightly builds? Tying stuff to the update channel because it happens to mostly match the scenarios we care about is hacky, and doing this properly isn't particularly hard.
It exactly matches the scenarios we care about. All MoCo branded versions are shipped with MOZ_UPDATE_CHANNEL != "default". I don't care enough to argue it beyond this last comment though. I'll r+ a patch for any reasonable solution.
How about MOZILLA_OFFICIAL?
That conflicts with bug 588862 (admittedly I have a bit of a bias here!). The proliferation of magical overloaded variables that mean different things to different people ends up being much more confusing than the proliferation of configure options, IMO.
The problem with adding configure options or env variables or whatever, is that it would require a modification of our mozconfigs, and further modifications to propagate across branches at merge time. Note that in practice, MOZ_OFFICIAL_BRANDING can't replace MOZILLA_OFFICIAL, since the latter is defined for all our builds, while the former is only defined for betas and releases. MOZ_UPDATE_CHANNEL could work for us here, but it wouldn't work for Linux distros (except if we don't want to get telemetry from them, but I doubt it).
Adding things to the MoCo mozconfigs isn't difficult, and there's no need to tie it to merge dates (it can just be added now to all configs).
Created attachment 542837 [details] [diff] [review] Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined
Created attachment 542838 [details] [diff] [review] Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)
Comment on attachment 542838 [details] [diff] [review] Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only) Hmm, I thought there was a way to set a config more globally (without having to change every single mozconfig file). In some buildbot config/script, maybe?
You could ifdef the _showTelemetryNotification implementation as well.
Comment on attachment 542838 [details] [diff] [review] Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only) This is getting set on branches that don't even support telemetry (e.g. 1.9.1, 1.9.2, 2.0) And do you want telemetry enabled for debug or qt builds?
Created attachment 544466 [details] [diff] [review] Corresponding mozconfig change Stripped down to release, nightly and nightly-rpm on mozilla-aurora, mozilla-beta, mozilla-central and mozilla-release. That could be mozilla-central only for now, but it doesn't change anything when the build patch is not applied and avoids having to care about the propagation of this particular change when merging from central to aurora, then aurora to beta, etc.
Comment on attachment 544466 [details] [diff] [review] Corresponding mozconfig change >diff --git a/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig b/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig >--- a/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig >+++ b/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig >@@ -24,16 +24,22 @@ export CXXFLAGS="-gdwarf-2" > > # For NSS symbols > export MOZ_DEBUG_SYMBOLS=1 > ac_add_options --enable-debug-symbols="-gdwarf-2" > > # Needed to enable breakpad in application.ini > export MOZILLA_OFFICIAL=1 > >+export MOZ_TELEMETRY_REPORTING=1 >+ >+export MOZ_TELEMETRY_REPORTING=1 >+ >+export MOZ_TELEMETRY_REPORTING=1 Looks like something went horribly wrong here
Created attachment 545691 [details] [diff] [review] Corresponding mozconfig change Sorry for the previous mess.
http://hg.mozilla.org/build/buildbot-configs/rev/29bf5a28e69e Landed the mozconfig changes.
(In reply to comment #2) > IIUC, we'd want always *off* for non-release (that is -- for local debug > builds, one-off tryserver builds with random hacky changes, etc), and opt-in > for release. If the reason for this is to avoid misleading telemetry pings for potentially broken/alternative builds, then surely not just the UI, but also the ping itself needs to be made conditional on MOZ_TELEMETRY_REPORTING - since an existing profile with toolkit.telemetry.enabled set (either from before this change, or one shared with an official release), will still send telemetry pings. Shall I file a follow-up?
Build id: Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110717 Firefox/8.0a1 Setting this as verified fixed since the changes related to the MOZ_TELEMETRY_REPORTING definition and use (opt-in ui for release builds only), landed on central.
It might be better to get this in ff7, so that custom builds of the source tarballs don't yield the opt-in prompt.
Comment on attachment 542837 [details] [diff] [review] Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined See comment 27