Closed Bug 737596 Opened 9 years ago Closed 8 years ago

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

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tchevalier, Assigned: tchevalier)

References

Details

(Whiteboard: [telemetry][mozfr])

Attachments

(1 file, 4 obsolete files)

This bug is part of bug 699806, to enable telemetry by default on Nightly/Aurora for mobile.
Whiteboard: [telemetry]
Assignee: nobody → theo.chevalier11
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch Patch V1 (obsolete) — Splinter Review
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...
Attached patch Patch V2 (obsolete) — Splinter Review
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.
Attachment #607756 - Attachment is obsolete: true
Just tried the build on a phone of a friend, and it seems to work! :)
Attachment #610753 - Flags: review?(gavin.sharp)
Depends on: 702284
Attached patch Patch V3 (obsolete) — Splinter Review
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.
Attachment #610753 - Attachment is obsolete: true
Attachment #610753 - Flags: review?(gavin.sharp)
Attached patch Patch V4 (obsolete) — Splinter Review
Attachment #673631 - Attachment is obsolete: true
Depends on: 725987, 737600
Attached patch Patch V5Splinter Review
Fixed tests (Not sure if remotereftest.py & runreftestb2g.py are well parsed)

Order to apply is: this patch, then 725987, then 737600 (mobile)
Attachment #676026 - Attachment is obsolete: true
Attachment #681238 - Flags: review?(mark.finkle)
Comment on attachment 681238 [details] [diff] [review]
Patch V5

passing to Brian, who is a good reviewer for these changes.
Attachment #681238 - Flags: review?(mark.finkle) → review?(bnicholson)
Comment on attachment 681238 [details] [diff] [review]
Patch V5

># HG changeset patch
># Parent 2820c92a61981518a0a973e8be2682b78bc298f3
># User Theo Chevalier <theo.chevalier11@gmail.com>
>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.
(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 on attachment 681238 [details] [diff] [review]
Patch V5

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

::: mobile/android/app/profile/extensions/feedback@mobile.mozilla.org/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/feedback@mobile.mozilla.org/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
Attachment #681238 - Flags: review?(bnicholson) → review+
Blocks: 819732
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3ffe203ebf
Whiteboard: [telemetry] → [telemetry][mozfr]
https://hg.mozilla.org/mozilla-central/rev/7b3ffe203ebf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Summary: Enable Telemetry by default on Nightly and Aurora channels (mobile) → make it possible to enable Telemetry by default on Nightly and Aurora channels (mobile)
Blocks: 821998
This caused bug 821998.
You need to log in before you can comment on or make changes to this bug.