1224010 - Add UI Telemetry for setting a homepage

Status: VERIFIED FIXED

(Firefox for Android :: Settings and Preferences, defect)

Description

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

We added support or setting a hompage via Settings. The code uses a custom onPreferenceChanged listener [1] so we skip the UI Telemetry probe used by other settings [2]. Do we need a separate onPreferenceChanged handler? If so, add the probe.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#871
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#1184

Comment 1

[Varun Joshi (:vjoshi)]

Attachment: addUITelemetryForSettingHomepage.patch

Used the existing handler; didn't need to add a probe.

Comment 2

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

Comment on attachment 8690532 [details] [diff] [review]
addUITelemetryForSettingHomepage.patch

Thanks for cleaning this up. I assume the URL is still saved and works as before.

Comment 3

[Varun Joshi (:vjoshi)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8690532 [details] [diff] [review]
> addUITelemetryForSettingHomepage.patch
> 
> Thanks for cleaning this up. I assume the URL is still saved and works as
> before.

Yeah, it works as before for me.

Comment 4

[:Margaret Leibovic]

It would also be good to uplift this to 44 if we're uplifting bug 1227322.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6a3a062f53cb

Comment 6

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8690532 [details] [diff] [review]
addUITelemetryForSettingHomepage.patch

Finkle, what are the risks here? I'm unclear why we moved the summary code.

Approval Request Comment
[Feature/regressing bug #]: bug 1227322 getting uplifted into 44.
[User impact if declined]: We won't be able to detect how users use this preference.
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: (Leaving to Finkle, as the reviewer)
[String/UUID change made/needed]: None

Comment 7

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/6a3a062f53cb

Comment 8

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

(In reply to Michael Comella (:mcomella) from comment #6)
> Comment on attachment 8690532 [details] [diff] [review]
> addUITelemetryForSettingHomepage.patch
> 
> Finkle, what are the risks here? I'm unclear why we moved the summary code.
> 
> Approval Request Comment
> [Feature/regressing bug #]: bug 1227322 getting uplifted into 44.
> [User impact if declined]: We won't be able to detect how users use this
> preference.
> [Describe test coverage new/current, TreeHerder]: None
> [Risks and why]: (Leaving to Finkle, as the reviewer)
> [String/UUID change made/needed]: None

Risk is low.

For some reason, the Home page setting had a separate change listener. Many of the other settings used a common change listener. This patch moves the setHomePageSummary call to the common change listener.

Comment 9

[Ritu Kothari (:ritu)]

Comment on attachment 8690532 [details] [diff] [review]
addUITelemetryForSettingHomepage.patch

Looks like a straightforward patch, let's uplift to Aurora44.

Comment 10

[Carsten Book [:Tomcat]]

Hi, this has problems during uplift:

grafting 316682:6a3a062f53cb "Bug 1224010 - Add UI Telemetry for setting a homepage. r=mfinkle"
merging mobile/android/base/preferences/GeckoPreferences.java
warning: conflicts while merging mobile/android/base/preferences/GeckoPreferences.java! (edit, then use 'hg resolve --mark')

could you take a look, thanks!

Comment 11

[Varun Joshi (:vjoshi)]

(In reply to Carsten Book [:Tomcat] from comment #10)
> Hi, this has problems during uplift:
> 
> grafting 316682:6a3a062f53cb "Bug 1224010 - Add UI Telemetry for setting a
> homepage. r=mfinkle"
> merging mobile/android/base/preferences/GeckoPreferences.java
> warning: conflicts while merging
> mobile/android/base/preferences/GeckoPreferences.java! (edit, then use 'hg
> resolve --mark')
> 
> could you take a look, thanks!

Sure, I'll look into it right away!

Comment 12

[Varun Joshi (:vjoshi)]

Attachment: addUITelemetryForSettingHomepage.patch

Based the patch on aurora to resolve issues during uplift.

Comment 13

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

Comment on attachment 8692912 [details] [diff] [review]
addUITelemetryForSettingHomepage.patch


>diff --git a/mobile/android/base/preferences/GeckoPreferences.java b/mobile/android/base/preferences/GeckoPreferences.java

>                 } else if (PREFS_HOMEPAGE.equals(key)) {
>                     String setUrl = GeckoSharedPrefs.forProfile(getBaseContext()).getString(PREFS_HOMEPAGE, AboutPages.HOME);
>                     setHomePageSummary(pref, setUrl);
>-                    pref.setOnPreferenceChangeListener(new OnPreferenceChangeListener() {
>-                        @Override
>-                        public boolean onPreferenceChange(final Preference preference, final Object newValue) {
>-                            setHomePageSummary(pref, String.valueOf(newValue));
>-                            return true;
>-                        }
>-                    });
>+                    pref.setOnPreferenceChangeListener(this);

I missed this on the initial review, which landed on Nightly, but I don't think we even need this line. We already set the preference change listener here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#688

But since this doesn't hurt anything, and it does get Telemetry to start working, I'm OK to uplift the change. Let's get another bug filed to remove the redundant code from Nightly though.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3d6c1d78c44

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a3d6c1d78c44

Comment 16

[u421692]

Verified as fixed on Aurora 45.0a2 (2015-12-17) and Firefox 44 Beta 1