Closed Bug 1423045 Opened 7 years ago Closed 6 years ago

Drop an event when User changes default browser to Firefox (from Leanplum contextual hints)

Categories

(Firefox for Android Graveyard :: Metrics, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Metrics
Platform:
Other
iOS
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jcollings, Assigned: petru)

Details

(Whiteboard: [Leanplum] [61])

Attachments

(1 file, 2 obsolete files)

Bug 1423045 - Drop an event when User changes default browser to Firefox;
59 bytes, text/x-review-board-request
mcomella
: review+
Details 
Looking to create an event when a user changes their default browser to Firefox. This will help us figure which message creates higher conversions.
Hi Joe, Wesly
Please help prioritize this.
Flags: needinfo?(wehuang)
Flags: needinfo?(jcheng)
Priority: -- → P3
Joe, would you help confirm if this is our priority for Fx60? (the coming Nightly cycle) Thanks.
Flags: needinfo?(wehuang)
[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.
Rank: 1
Priority: P3 → P1
Whiteboard: [Leanplum] [61]
Assignee: nobody → petru.lingurar
Flags: needinfo?(jcheng)
The app will now check everytime it starts (like when coming back to it from Settings) if Fennec has been set as default browser in the meantime and if so it will trigger a LeanPlum event: "E_Changed_Default_To_Fennec"
Attachment #8964301 - Flags: review?(michael.l.comella)
Mike I know this is a Leanplum change but it's really an application logic change that just sends a LP event, so I'm hoping you can ably review this. Let me know if not.
Flags: needinfo?(michael.l.comella)
Is there a mozreview for this?
Flags: needinfo?(michael.l.comella)
Attachment #8964301 - Attachment is obsolete: true
Attachment #8964301 - Flags: review?(michael.l.comella)
Attachment #8966495 - Flags: review?(sdaswani) → review?(michael.l.comella)
Comment on attachment 8966495 [details]
Bug 1423045 - Drop an event when User changes default browser to Firefox;

https://reviewboard.mozilla.org/r/235206/#review241152

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:73
(Diff revision 1)
>      public static final String KEY_ANDROID_PREF_STRING_LEANPLUM_DEVICE_ID = "android.not_a_preference.leanplum.device_id";
>      private static final String DEBUG_LEANPLUM_DEVICE_ID = "8effda84-99df-11e7-abc4-cec278b6b50a";
>  
>      private static final MmaInterface mmaHelper = MmaConstants.getMma();
>      private static WeakReference<Context> applicationContext;
> +    private static boolean defaultBrowserStatus;

nit: Java convention uses `is...` (and its various tenses) for boolean variable names

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:119
(Diff revision 1)
>  
> +    public static void informDefaultBrowserStatus() {
> +        // Method may be called before class was initialized
> +        if (applicationContext == null || applicationContext.get() == null) return;
> +
> +        if (!defaultBrowserStatus && isDefaultBrowser(applicationContext.get())) {

afaict, this won't cover the case that Fennec is not in memory when the default is switched, e.g. Fennec is cleared from memory by the system, the user clicks a link in an app and selects to Always open in Fennec, and then opens Fennec.

I'd think we want to cover this case too.

Should we use `SharedPreferences` to store the default browser state persistently rather than putting it in memory?
Attachment #8966495 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8966495 [details]
Bug 1423045 - Drop an event when User changes default browser to Firefox;

https://reviewboard.mozilla.org/r/235206/#review241152

> afaict, this won't cover the case that Fennec is not in memory when the default is switched, e.g. Fennec is cleared from memory by the system, the user clicks a link in an app and selects to Always open in Fennec, and then opens Fennec.
> 
> I'd think we want to cover this case too.
> 
> Should we use `SharedPreferences` to store the default browser state persistently rather than putting it in memory?

Indeed, if we are to cover the above case I think we'd need to use SharedPreferences.
But I didn't went with that because the ticket's title was "*Drop an event when User changes default browser to Firefox (from Leanplum contextual hints)*" which I understood as the need to log an event for when the user changes Fennec to be the default browser, after getting a LP hint, while the app is running.
So we would need to help tie this specific event with a *LP contextual hint*

As such, it might be possible that, irrespective of *LP contextual hints* the user changes Fennec to be the default browser and then starts it. This would trigger the *E_Launch_Browser* event but not *E_Changed_Default_To_Fennec*, because there is no clear correlation between a *LP contextual hint* and the setting Fennec as default action.

Looking forward to hear what you guys think.
Mike, I think Petru has a point - Jean just wanted to track the switch to the default browser so that when a campaign is running she can track the campaign effectiveness. If the patch reasonably accomplishes that (it's likely the user will either i) change their browser to default immediately after acting on the message, so that the app is now in memory or ii) not change it and ignore the message), I think we can go ahead with this patch.
Flags: needinfo?(michael.l.comella)
Jean/Kat, to address our assumptions:
- Is it okay if a very small number of probes are dropped? We have a simpler solution that we've already implemented or we could make sure to get 100% of the data but the solution is more complex (which takes away from other development efforts)
- The bug title is, "Drop an event when User changes default browser to Firefox (from Leanplum contextual hints)". If we provide a generic, "Fennec became the default browser" probe, can you correlate that with a "Make fennec the default browser" leanplum contextual hint was shown in the data analysis interface in order to address the issue here? Or do we have to keep track of if, and possible which, contextual hints are shown?

fwiw, I don't have a clear understanding of how Leanplum works and we've been having a lot of back-and-forth on this bug. If you think it'd be helpful to have a vidyo conversation with either Petru or myself to understand your specific needs, please let me know!

For how I came to these questions, see below.

---

> "*Drop an event when User changes default browser to Firefox (from Leanplum contextual hints)*"

Nice observation – thanks for being thorough. :)

Looking at `LauncherActivity.dispatchDeepLink` and our SUMO default browser page [1], it looks like we can't programmatically make Fennec a default browser: the user has to explicitly go to the default apps screen. Presumably, the Leanplum hints direct the user to this screen.

Petru's patch will work assuming the system doesn't remove us from memory when visiting the system default apps page (this is unlikely to happen on most modern devices).

Storing the data in SharedPreferences is more thorough but adds additional complexity that may not be needed if we don't mind potentially dropping a few pings.

Both these methods assume we can correlate, "A leanplum contextual hint was shown/clicked!" with "Firefox is the default browser!" in the Leanplum data analysis interface.

[1]: https://support.mozilla.org/kb/make-firefox-default-browser-android
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(kplam)
Flags: needinfo?(jcollings)
Mike, I think the just pushed patch should address your valid concerns.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8968211 [details]
Bug 1423045 - Drop an event when User changes default browser to Firefox;

https://reviewboard.mozilla.org/r/236906/#review242838

I think the way this is implemented, it has the same issues as the in-memory approach.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:75
(Diff revision 1)
>  
>      private static final MmaInterface mmaHelper = MmaConstants.getMma();
>      private static WeakReference<Context> applicationContext;
> -    private static boolean defaultBrowserStatus;
> +    private static SharedPreferences prefs;
>  
> -    public static void init(Activity activity) {
> +    public static void init(Activity activity, SharedPreferences preferences) {

You're missing the change to the call-site from this patch: did you forgot to add a file?

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:77
(Diff revision 1)
>      private static WeakReference<Context> applicationContext;
> -    private static boolean defaultBrowserStatus;
> +    private static SharedPreferences prefs;
>  
> -    public static void init(Activity activity) {
> +    public static void init(Activity activity, SharedPreferences preferences) {
>          applicationContext = new WeakReference<>(activity.getApplicationContext());
> -        defaultBrowserStatus = isDefaultBrowser(activity);
> +        prefs = preferences;

nit: afaik, you can retrieve SharedPrefs from the `applicationContext` so you don't need to keep a reference to the prefs or pass it into init.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:79
(Diff revision 1)
>  
> -    public static void init(Activity activity) {
> +    public static void init(Activity activity, SharedPreferences preferences) {
>          applicationContext = new WeakReference<>(activity.getApplicationContext());
> -        defaultBrowserStatus = isDefaultBrowser(activity);
> +        prefs = preferences;
> +
> +        prefs.edit().putBoolean(GeckoPreferences.PREFS_FENNEC_IS_DEFAULT, isDefaultBrowser(activity)).apply();

This patch has the same problem as holding it in memory: this code sets the default browser each time the app is started (like if we were storing it in memory).

I think we only want to set this if the value isn't already set, that way we can track changes between the app being on or off.

In the code in `informDefault...`, we'll also need to update the value each time it is updated so that we can catch an enable, disable, enable (unless we don't want to catch that but I don't know why we wouldn't).

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:123
(Diff revision 1)
>      public static void informDefaultBrowserStatus() {
>          // Method may be called before class was initialized
>          if (applicationContext == null || applicationContext.get() == null) return;
>  
> -        if (!defaultBrowserStatus && isDefaultBrowser(applicationContext.get())) {
> -            defaultBrowserStatus = !defaultBrowserStatus;
> +        if (!prefs.getBoolean(GeckoPreferences.PREFS_FENNEC_IS_DEFAULT, true) &&
> +                isDefaultBrowser(applicationContext.get())) {

Since `applicationContext` is a weak reference, this could crash if it's null.

tbh, I don't think `applicationContext` needs to be a weak reference if you just want to change that – the applicationContext should exist the entire time the application is open.
Attachment #8968211 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8968211 [details]
Bug 1423045 - Drop an event when User changes default browser to Firefox;

https://reviewboard.mozilla.org/r/236906/#review242844

Also, since the first commit is now unrelated, I'd prefer you squashed these two commits. The mozreview interface does interdiffs, which lets me see the difference between pushes so I don't lose data and seeing one commit makes it easier to review on my end.

If you're keeping the two commits around for your own efficiency (e.g. so you can push the first implementation if we dump this second one) because it's too hard to create two "branches", then I think that's fine and you can keep the commit. Let's optimize the metadat for time and not perfection. :)
Flags: needinfo?(michael.l.comella)
Attachment #8966495 - Attachment is obsolete: true
Comment on attachment 8968211 [details]
Bug 1423045 - Drop an event when User changes default browser to Firefox;

https://reviewboard.mozilla.org/r/236906/#review243170

I think we could slightly improve the code but I don't think the additional turn-around to correct and rereview is worth it: I'm going to land it as is.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:123
(Diff revision 2)
> +
> +        final SharedPreferences sharedPreferences = activity.getPreferences(Context.MODE_PRIVATE);
> +        final boolean isFennecDefaultBrowser = isDefaultBrowser(activity);
> +
> +        // Will only inform LeanPlum of the event if Fennec was not previously the default browser
> +        if (!sharedPreferences.getBoolean(KEY_ANDROID_PREF_BOOLEAN_FENNEC_IS_DEFAULT, true) && isFennecDefaultBrowser) {

nit: I'd prefer to explicitly handle the case where this value isn't set in sharedPrefs yet, e.g.:
```java
// If we don't have the key set, it's first run
// in which case we can't track a "change"
if (sp.contains(KEY)) {
  // Do value check and track if necessary
}

// Set SP value for KEY
```

Being explicit indicates that you *intended* to handle the default case whereas `(!sharedPreferences.getBoolean(KEY_ANDROID_PREF_BOOLEAN_FENNEC_IS_DEFAULT, true)` works but might make the reader 1) question if the default case was written intentionally or 2) forget about the default case when refactoring this code.
Attachment #8968211 - Flags: review?(michael.l.comella) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8e3dc4c7737c6cef06acd906bf50ace935e5a23a -d 5b68f5f5ed00: rebasing 458995:8e3dc4c7737c "Bug 1423045 - Drop an event when User changes default browser to Firefox; r=mcomella" (tip)
merging mobile/android/docs/mma.rst
warning: conflicts while merging mobile/android/docs/mma.rst! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased and added the better handling of the first LeanPlum run.
Flags: needinfo?(michael.l.comella)
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/924aabc981b6
Drop an event when User changes default browser to Firefox; r=mcomella
That feeling when a doc update forces a rebase. :d
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/924aabc981b6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: needinfo?(kplam)
Flags: needinfo?(jcollings)
Flags: needinfo?(cpark)
I see we're fixed for 61 - Is this ready for testing in Nightly?
Flags: needinfo?(cpark)
yes it is Cherry, sorry for the unnecessary NI :) .
Not a prob at all, good to see everything at once!
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: