Closed Bug 1224010 Opened 4 years ago Closed 4 years ago

Add UI Telemetry for setting a homepage

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 45
Tracking Status
firefox44 --- verified
firefox45 --- verified
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: mfinkle, Assigned: vjoshi, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

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
Blocks: homepage
Mentor: mark.finkle
Whiteboard: [lang=java]
Used the existing handler; didn't need to add a probe.
Attachment #8690532 - Flags: review?(mark.finkle)
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.
Attachment #8690532 - Flags: review?(mark.finkle) → review+
(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.
It would also be good to uplift this to 44 if we're uplifting bug 1227322.
Assignee: nobody → varunj.1011
tracking-fennec: --- → ?
Flags: needinfo?(michael.l.comella)
Keywords: checkin-needed
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
Flags: needinfo?(michael.l.comella) → needinfo?(mark.finkle)
Attachment #8690532 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6a3a062f53cb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(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.
Flags: needinfo?(mark.finkle)
Comment on attachment 8690532 [details] [diff] [review]
addUITelemetryForSettingHomepage.patch

Looks like a straightforward patch, let's uplift to Aurora44.
Attachment #8690532 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(varunj.1011)
(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!
Flags: needinfo?(varunj.1011)
Based the patch on aurora to resolve issues during uplift.
Attachment #8690532 - Attachment is obsolete: true
Attachment #8692912 - Flags: review?(mark.finkle)
Attachment #8692912 - Flags: review?(margaret.leibovic)
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.
Attachment #8692912 - Flags: review?(mark.finkle)
Attachment #8692912 - Flags: review?(margaret.leibovic)
Attachment #8692912 - Flags: review+
tracking-fennec: ? → 44+
Verified as fixed on Aurora 45.0a2 (2015-12-17) and Firefox 44 Beta 1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.