Closed
Bug 1224010
Opened 6 years ago
Closed 6 years ago
Add UI Telemetry for setting a homepage
Categories
(Firefox for Android Graveyard :: Settings and Preferences, defect)
Firefox for Android Graveyard
Settings and Preferences
Tracking
(firefox44 verified, firefox45 verified, b2g-v2.5 fixed, fennec44+)
VERIFIED
FIXED
Firefox 45
People
(Reporter: mfinkle, Assigned: vjoshi, Mentored)
References
Details
(Whiteboard: [lang=java])
Attachments
(1 file, 1 obsolete file)
2.77 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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
Updated•6 years ago
|
Mentor: mark.finkle
Whiteboard: [lang=java]
Assignee | ||
Comment 1•6 years ago
|
||
Used the existing handler; didn't need to add a probe.
Attachment #8690532 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 2•6 years ago
|
||
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+
Assignee | ||
Comment 3•6 years ago
|
||
(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•6 years ago
|
||
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?
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a3a062f53cb
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Reporter | ||
Comment 8•6 years ago
|
||
(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)
status-firefox44:
--- → affected
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+
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
(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)
Assignee | ||
Comment 12•6 years ago
|
||
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)
Reporter | ||
Comment 13•6 years ago
|
||
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+
Comment 14•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3d6c1d78c44
Comment 15•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a3d6c1d78c44
status-b2g-v2.5:
--- → fixed
Updated•5 years ago
|
tracking-fennec: ? → 44+
Comment 16•5 years ago
|
||
Verified as fixed on Aurora 45.0a2 (2015-12-17) and Firefox 44 Beta 1
Updated•4 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•