737596 - make it possible to enable Telemetry by default on Nightly and Aurora channels (mobile)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Théo Chevalier]

This bug is part of bug 699806, to enable telemetry by default on Nightly/Aurora for mobile.

Comment 1

[Théo Chevalier]

Attachment: Patch V1

Like for 699806, this needs to be fixed:
+if test "$MOZ_UPDATE_CHANNEL" = nightly -o
+  test "$MOZ_UPDATE_CHANNEL" = aurora; then

Maybe quotes? Or no space between '=' and 'nightly'? Out of ideas...

Comment 2

[Théo Chevalier]

Attachment: Patch V2

Including some stuff from bug 699806 to make it works while it's not landed. (Preprocessor Define, and prefs)

I'll check if res/xml/preferences.xml is well parsed. If yes, this patch is more or less ready.

Comment 3

[Théo Chevalier]

Just tried the build on a phone of a friend, and it seems to work! :)

Comment 4

[Théo Chevalier]

If you want to confirm: https://tbpl.mozilla.org/?tree=Try&rev=af7a388cd3a7

Comment 5

[Théo Chevalier]

Attachment: Patch V3

Some part of the code is in patch attached on bug 725987 (this avoid me to rebase everytime)

I kept string changes, until we find a solution in bug 699806.

Comment 6

[Théo Chevalier]

Attachment: Patch V4

Comment 7

[Théo Chevalier]

Attachment: Patch V5

Fixed tests (Not sure if remotereftest.py & runreftestb2g.py are well parsed)

Order to apply is: this patch, then 725987, then 737600 (mobile)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 681238 [details] [diff] [review]
Patch V5

passing to Brian, who is a good reviewer for these changes.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 681238 [details] [diff] [review]
Patch V5

># HG changeset patch
># Parent 2820c92a61981518a0a973e8be2682b78bc298f3
># User Theo Chevalier <[e-mail]>
>Bug 737596 - Enable Telemetry by default on Nightly and Aurora channels (mobile)
>
>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in

>+#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
>+            <setting pref="toolkit.telemetry.enabledByDefault" title="&feedback.allowTelemetry.title;" type="bool"/>
>+#else
>             <setting pref="toolkit.telemetry.enabled" title="&feedback.allowTelemetry.title;" type="bool"/>
>+#endif

> #ifdef MOZ_TELEMETRY_REPORTING
>-        <CheckBoxPreference android:key="toolkit.telemetry.enabled"
>+        <CheckBoxPreference
>+#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
>+                            android:key="toolkit.telemetry.enabledByDefault"
>+#else
>+                            android:key="toolkit.telemetry.enabled"
>+#endif

I don't think it makes sense to use a separate pref for this (also see https://bugzilla.mozilla.org/show_bug.cgi?id=725987#c26). Wouldn't it be easier to always use "toolkit.telemetry.enabled" so we don't need all of these checks? "enabledByDefault" doesn't match its meaning if we're actually using it as an enabled flag.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> I don't think it makes sense to use a separate pref for this (also see
> https://bugzilla.mozilla.org/show_bug.cgi?id=725987#c26). Wouldn't it be
> easier to always use "toolkit.telemetry.enabled" so we don't need all of
> these checks? "enabledByDefault" doesn't match its meaning if we're actually
> using it as an enabled flag.

See the discussion in bug 699806. Using the same pref with different defaults across channels causes a lot of trouble when users switch between channels with the same profile. That might not be a factor on Android (IIRC each version gets it own profile?), but since a lot of this code is shared, I imagine we probably can't use a different solution there.

(The name is a good point - I had a similar concern. Not sure I have a better suggestion, though - enabledPreRelease?)

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 681238 [details] [diff] [review]
Patch V5

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

::: mobile/android/app/profile/extensions/[e-mail]/content/overlay.xul
@@ +30,5 @@
>              </setting>
>            </settings>
>            <settings id="feedback-tools" label="&feedback.tools.title;">
> +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> +            <setting pref="toolkit.telemetry.enabledByDefault" title="&feedback.allowTelemetry.title;" type="bool"/>

Can you split this to remove redundancy? e.g.:

    <setting
#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
        pref="toolkit.telemetry.enabledByDefault"
#else
        pref="toolkit.telemetry.enabled"
#endif
        title="&feedback.allowTelemetry.title;"
...

::: mobile/android/chrome/content/about.xhtml
@@ +48,2 @@
>      </div>
> +

nit: I'd drop this line

::: mobile/xul/app/profile/extensions/[e-mail]/content/overlay.xul
@@ +30,5 @@
>              </setting>
>            </settings>
>            <settings id="feedback-tools" label="&feedback.tools.title;">
> +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> +            <setting pref="toolkit.telemetry.enabledByDefault" title="&feedback.allowTelemetry.title;" type="bool"/>

Can you split this to remove redundancy?

::: mobile/xul/chrome/content/about.xhtml
@@ +35,5 @@
>      <span id="update-message-none">&aboutPage.checkForUpdates.none;</span>
>      <span id="update-message-found">&aboutPage.checkForUpdates.found;</span>
>  #endif
>  
> +

Nit: Don't need this extra line

@@ +47,3 @@
>      </div>
>  
> +

Or this one

Comment 12

[Théo Chevalier]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3ffe203ebf

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b3ffe203ebf

Comment 14

[Wesley Johnston (:wesj)]

This caused bug 821998.