Closed Bug 1454686 Opened 2 years ago Closed Last year

Apply App Level Push Notifications Opt-out for Leanplum for Android Users

Categories

(Firefox for Android :: Metrics, enhancement)

Firefox 62
All
Android
enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
relnote-firefox --- 62+
firefox62 --- verified

People

(Reporter: petru, Assigned: petru)

References

Details

Attachments

(6 files, 5 obsolete files)

122.60 KB, image/png
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
As Jean wrote on https://bugzilla.mozilla.org/show_bug.cgi?id=1445798#c0
"Currently users can only opt out of notifications on app level (OS settings) but we want to allow users to opt out of notifications within the Firefox settings."

The user option for this shall appear in "Settings -> Notifications".

The strings needed have been added as per bug 1445798.
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Depends on: 1445798
From my investigations the needed functionalities 
- keep LeanPlum running and logging events
- skipping showing LeanPlum banners or push notifications
are not a feature currently offered by their SDK.

I think the first step would be to ask LeanPlum if they have any advice on how to offer this, or if this is something on their agenda.

Otherwise we will have to modify the LeanPlum SDK included in the app and also support this feature going forward.
Flags: needinfo?(michael.l.comella)
> - keep LeanPlum running and logging events
> - skipping showing LeanPlum banners or push notifications

Why do we need to keep logging events if the notifications are disabled? Won't that skew the data (e.g. user makes changes but never saw the banner)?

> I think the first step would be to ask LeanPlum if they have any advice on how to offer this,

If there's a simple solution they have, that works for me! Maybe talk to Susheel or Kat about getting in contact? You could also try filing a bug if their issues are open.

If there is no easy solution, one thing we've done in the past is wrap all the calls in a class, say `LeanPlumWrapper` or `LeanPlumIntegration` which will be able to enable/disable specific functionality at runtime. This is hard to maintain though: people can call the LeanPlum APIs directly (unless you correct that with build system boundaries) and any SDK upgrades could make you miss functionality.
Flags: needinfo?(michael.l.comella) → needinfo?(petru.lingurar)
Thanks Mike, you do bring up a valid point.
From what I also understand LeanPlum reporting is only used for better targeting this messages and then check their impact.
If the messages won't be shown to the user I'd say there's no point in continuing receiving reports from him.

But I think this should be validated by Product? Kat?

If we are to stop LeanPlum from running when users opt-out from MMA messages a solution is not so complicated and I think the just uploaded patch should offer it.
Flags: needinfo?(petru.lingurar) → needinfo?(kplam)
Duplicate of this bug: 1455814
Blocks: 1456110
Susheel, can you follow up with Kat or provide an answer so I can review this?
Flags: needinfo?(sdaswani)
Yes, I have a list of items to review with Kat next week, stay tuned.
Flags: needinfo?(sdaswani)
Merge is in 5 days: Susheel, do you have a status update?
Flags: needinfo?(sdaswani)
No, the meeting was delayed, it's later this week.

That said, I think this is a fine solution: "If we are to stop LeanPlum from running when users opt-out from MMA messages a solution is not so complicated and I think the just uploaded patch should offer it." https://bugzilla.mozilla.org/show_bug.cgi?id=1454686#c4

Petru it might be nice to include a video of this new setting so I can show it off to Product later this week.
Flags: needinfo?(sdaswani) → needinfo?(petru.lingurar)
https://drive.google.com/file/d/1d1AMsr6ynSCwBNOLROUsfYIKWu7AsmM8/view?usp=sharing
This is a video showing the process of enabling/disabling the LeanPlum notifications by stopping LeanPlum altogether.
(Couldn't add it as attachment because "Secure Connection Failed")

Also reworked the patch a little to allow for restarting LeanPlum in the same app session (as opposed to having to restart the app) which offers a smooth experience for the entire process.
Flags: needinfo?(petru.lingurar)
Great video, thanks Petru.

Andreas and Bram, can you please look at this video and let us know if you have any comments from the Product or UX side? I would like to ship this in 61, if not 62.
Flags: needinfo?(bram)
Flags: needinfo?(abovens)
Can you confirm that this toggle also turns off Leanplum push notifications (not just in-app banners)? 

Some comments about strings:
- "New features, websites updates" should maybe better be "New features, product tips".
- There should be no mention of "Fennec" in the strings. The product name should be "Firefox" (or "Nightly").

Otherwise, this looks ok.
Flags: needinfo?(abovens)
UX-wise, this looks okay.
Flags: needinfo?(bram)
Petru, aren't you using the strings that we shipped here: https://bugzilla.mozilla.org/show_bug.cgi?id=1445798#c33

Also what's up with Fennec showing up instead of Firefox.

Thanks for the review Bram and Andreas!
Flags: needinfo?(petru.lingurar)
The strings this feature uses are indeed the ones from bug 1445798.
Is just that as Michael indicated [1] we should use them in a dynamic way, specific to each build. Because I have builded from mozilla-central I got Fennec instead of Firefox. But Firefox should appear elsewise.

I have rebased the patch and added the modification that Andreas suggested.

Also tested with LP push notifications (Send Preview from dashboard) and saw that when the notification toggle is off (and LP is basically stopped) neither the banners nor the push notifications are displayed anymore.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1445798#c35
Flags: needinfo?(petru.lingurar)
Petru where does this string come from: "New features, websites updates"? It's not in the screen with the toggle, but the settings screen where you get into the toggle screen.
Flags: needinfo?(petru.lingurar)
> "New features, websites updates"
is a hardcoded string used as description for the Notifications setting.
Changing this text was not part of bug 1445798 but indeed, after the new LP opt-out feature is implemented, I think "New features, product tips" would make more sense.
Flags: needinfo?(petru.lingurar)
Flags: needinfo?(kplam) → needinfo?(cpark)
Andreas the string (‘New features, websites updates’) is already set in the product. I would suggest we change it under another bug if you would like. I think this change is good to go though!
Petru when the user toggles LP off do we indeed keep logging events per Mike’s concern here? https://bugzilla.mozilla.org/show_bug.cgi?id=1454686#c2

If we shut LP off events won’t be logged I believe.
Flags: needinfo?(cpark) → needinfo?(petru.lingurar)
When the user toggles LP notifications off, all LP functionality will be stopped - notifications, in-app banners, events tracking.
Only when the user toggles LP notifications on will LP start again, with all it's functionality.

As Mike said, if the notifications are off, I think there is no need to continue tracking events, otherwise used for customizing the LP messages, and doing so would even skew the data overall.

The new string suggested by Andreas is included in the latest patch for this ticket as that change is tied with adding this new feature.
Flags: needinfo?(petru.lingurar)
petru i'd rather not change an existing string now as it will need to get translated. please remove that change.

i think shutting off everything is just fine.

after you remove that new string Mike can review this.
Flags: needinfo?(petru.lingurar)
> petru i'd rather not change an existing string now as it will need to get translated. please remove that change.

I think that's fine: we have to ride the trains anyway. That being said, if this slips to an uplift then yeah, we might need to remove the new string.
I think I'd like to try and get it in 61, and the settings string rode the 60 train, so I'm hoping we can uplift it if I beg the RMs hard enough :) .
Comment on attachment 8969321 [details]
Bug 1454686 - Allow users to stop and start LeanPlum and it's notifications;

https://reviewboard.mozilla.org/r/238050/#review247676

See my comment in GeckoPreferences: I think we're making this harder than it needs to be.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2327
(Diff revision 3)
>                  }
>  
>                  break;
>  
> +            case "NotificationSettings:UpdateFeatureTipsStatus":
> +                if (message.getBoolean("status")) {

nit: "status" doesn't give me a clear idea of what this boolean flag indicates. Maybe `isMmaEnabled`?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2328
(Diff revision 3)
>  
>                  break;
>  
> +            case "NotificationSettings:UpdateFeatureTipsStatus":
> +                if (message.getBoolean("status")) {
> +                    initSwitchboard(this, safeStartingIntent, isInAutomation);

nit: Add a comment indicating this also inits `MmaDelegate` too.

Alternatively, consider renaming `initSwitchboard` to indicate so you don't need a comment.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:186
(Diff revision 3)
>          if (context == null) {
>              return false;
>          }
>          final boolean healthReport = GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true);
>          final boolean inExperiment = SwitchBoard.isInExperiment(context, Experiments.LEANPLUM);
> +        final boolean messagesAllowed = GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_NOTIFICATIONS_FEATURES_TIPS, true);

nit: since we call this twice, it'd be great to have either a single wrapper function (e.g. `GeckoPreferences.isLeanplumEnabled(context)` but maybe there's a better place) so we're not duplicating the default value code OR have a static default value that all the implementations share (I prefer the former because it's easy to add new call sites without using the default value).

Having a helper function will also clarify the external name - Features Tips Status - from the internal function - disabling Leanplum.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:111
(Diff revision 3)
> -    public void stop() {
> +    public void stopEventsReporting() {
>          Leanplum.stop();
>      }
>  
>      @Override
> +    public void stopShowingSessionMessages() {

fwiw, I haven't had a chance to look into the methods in this file to ensure they actually turn off LP: I'll do this in my next review pass because I'm unfortunately backed up on reviews right now.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:823
(Diff revision 3)
>                      }
> +                } else if (PREFS_NOTIFICATIONS_FEATURES_TIPS.equals(key)) {
> +                    // Use the PreferenceScreen that will be displayed to access SharedPreferences and check other stored preferences
> +                    final SharedPreferences sharedPrefs = preferences.getSharedPreferences();
> +
> +                    if (!SwitchBoard.isInExperiment(this, Experiments.LEANPLUM_DEBUG) &&

This expression is really confusing. Can you break it out into separate variables to simplify it? e.g.
```java
final boolean isLeanplumEnabled = SwitchBoard.isInExperiment(this, Experiments.LEANPLUM_DEBUG) ||
    SwitchBoard.isInExperiment(this, Experiments.LEANPLUM)
...

if (!isLeanplumEnabled || ...
```

---

Similarly, can you add grouping operators? Maybe this is something I should know but I don't know how `&&` and `||` actually group implicitly. Looking at Wolfram, the explicit expression is: `(a && b) || c || (d && e)`, which was not obvious to me.

https://www.wolframalpha.com/input/?i=a+and+b+or+c+or+d+and+e

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:823
(Diff revision 3)
>                      }
> +                } else if (PREFS_NOTIFICATIONS_FEATURES_TIPS.equals(key)) {
> +                    // Use the PreferenceScreen that will be displayed to access SharedPreferences and check other stored preferences
> +                    final SharedPreferences sharedPrefs = preferences.getSharedPreferences();
> +
> +                    if (!SwitchBoard.isInExperiment(this, Experiments.LEANPLUM_DEBUG) &&

iiuc, we're allowing users to disable HR upload, which will prevent uploading leanplum probes, but still show them pending LP messages? I don't think we need to do this: if we can't record LP data, there's little need to show pending messages. This also creates a lot of different state our app can be in which is hard to get right and confusing for users trying to disable these messages.

Instead, I think we should simplify this: the HR pref should be a superset of LP. When HR is disabled, LP is disabled. When HR is enabled, LP can be enabled or disabled.

I also don't think we should be removing the preference entirely when the HR toggle is flipped: this is also confusing to users. Assuming we go with the approach I suggested, I think when the HR toggle is flipped, we should stop LP entirely, disable the LP pref, and set it to the unchecked state. It's probably confusing for users why this might be the case but I think it's better than having partial states like this.

This has an added benefit: in order to create this state, we're reaching into a lot of LP internals right now which is scary: it's hard to guarantee they do what we think it does and it's possible to change in future releases. It's best not to reach in!

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1147
(Diff revision 3)
>                  startActivityForResult(promptIntent, REQUEST_CODE_TAB_QUEUE);
>                  return false;
>              }
> +        } else if (PREFS_NOTIFICATIONS_FEATURES_TIPS.equals(prefName)) {
> +            final GeckoBundle newStatus = new GeckoBundle(1);
> +            newStatus.putBoolean("status", !((SwitchPreference) preference).isChecked());

nit: add a comment to indicate that `isChecked` is actually the old value, that it hasn't been updated yet.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1148
(Diff revision 3)
>                  return false;
>              }
> +        } else if (PREFS_NOTIFICATIONS_FEATURES_TIPS.equals(prefName)) {
> +            final GeckoBundle newStatus = new GeckoBundle(1);
> +            newStatus.putBoolean("status", !((SwitchPreference) preference).isChecked());
> +            EventDispatcher.getInstance().dispatch("NotificationSettings:UpdateFeatureTipsStatus", newStatus);

nit: -> `NotificationSettings:FeatureTipsStatusUpdated`
Attachment #8969321 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8969321 [details]
Bug 1454686 - Allow users to stop and start LeanPlum and it's notifications;

https://reviewboard.mozilla.org/r/238050/#review247696

By the way, this commit changed many things at once so I found it difficult to review this code: if you broke your code up into multiple commits that each did one thing, it'd make it way easier! That being said, don't waste a lot of time making the commits – only do it if it's actually convenient! For example, I noticed the code did these things that could conceivably be made into separate commits:
- Modify strings
- Add preference UI
- Modify init methods to take into account new preference
- Modify isMmaEnabled to take into account new preference
- Respond to changes in the preference state
Comment on attachment 8969321 [details]
Bug 1454686 - Allow users to stop and start LeanPlum and it's notifications;

https://reviewboard.mozilla.org/r/238050/#review247676

> nit: since we call this twice, it'd be great to have either a single wrapper function (e.g. `GeckoPreferences.isLeanplumEnabled(context)` but maybe there's a better place) so we're not duplicating the default value code OR have a static default value that all the implementations share (I prefer the former because it's easy to add new call sites without using the default value).
> 
> Having a helper function will also clarify the external name - Features Tips Status - from the internal function - disabling Leanplum.

I have added GeckoPreferences.isMmaAvailable(Context) that is easily available to BrowserApp() and MmaDelegate().

> iiuc, we're allowing users to disable HR upload, which will prevent uploading leanplum probes, but still show them pending LP messages? I don't think we need to do this: if we can't record LP data, there's little need to show pending messages. This also creates a lot of different state our app can be in which is hard to get right and confusing for users trying to disable these messages.
> 
> Instead, I think we should simplify this: the HR pref should be a superset of LP. When HR is disabled, LP is disabled. When HR is enabled, LP can be enabled or disabled.
> 
> I also don't think we should be removing the preference entirely when the HR toggle is flipped: this is also confusing to users. Assuming we go with the approach I suggested, I think when the HR toggle is flipped, we should stop LP entirely, disable the LP pref, and set it to the unchecked state. It's probably confusing for users why this might be the case but I think it's better than having partial states like this.
> 
> This has an added benefit: in order to create this state, we're reaching into a lot of LP internals right now which is scary: it's hard to guarantee they do what we think it does and it's possible to change in future releases. It's best not to reach in!

If the user is to disable health report and we keep the toggle on the screen but disabled, maybe we should modify it's summary to let the users know why it can't be enabled?
Attachment #8969321 - Attachment is obsolete: true
Thanks Mike!
Made the changes according to review and split the patch in more commits to ease the process going forward.

Filed bug 1459632 to address the Notifications setting summary update.
Flags: needinfo?(petru.lingurar)
Comment on attachment 8973663 [details]
Bug 1454686 - Add a new preference under Settings - Notifications;

https://reviewboard.mozilla.org/r/242030/#review248000

I think HealthReport should be enabled by default.

::: commit-message-43e0a:1
(Diff revision 1)
> +Bug 1454686 - Part 1 - Add a preference under Settings - Notifications; r?mcomella

nit: no need to number each part and it takes extra work to make sure the numbering is in order.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:820
(Diff revision 1)
>                          preferences.removePreference(pref);
>                          i--;
>                          continue;
>                      }
> +                } else if (PREFS_NOTIFICATIONS_FEATURES_TIPS.equals(key)) {
> +                    final boolean isLeanplumAvailable = SwitchBoard.isInExperiment(this, Experiments.LEANPLUM);

What happened to LEANPLUM_DEBUG?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:832
(Diff revision 1)
> +
> +                    // Use the PreferenceScreen that will be displayed to access SharedPreferences and check other stored preferences
> +                    final SharedPreferences sharedPrefs = preferences.getSharedPreferences();
> +                    final boolean isHealthReportEnabled =
> +                            sharedPrefs.contains(PREFS_HEALTHREPORT_UPLOAD_ENABLED) &&
> +                            sharedPrefs.getBoolean(PREFS_HEALTHREPORT_UPLOAD_ENABLED, false);

It looks like HealthReport is enabled by default: https://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+healthreport_upload_enabled&path= Don't we want to return true if sharedPrefs doesn't contain the pref (or just set the default value to true)?

This is a good time to have a wrapper method to call out too. :)

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:834
(Diff revision 1)
> +                    final SharedPreferences sharedPrefs = preferences.getSharedPreferences();
> +                    final boolean isHealthReportEnabled =
> +                            sharedPrefs.contains(PREFS_HEALTHREPORT_UPLOAD_ENABLED) &&
> +                            sharedPrefs.getBoolean(PREFS_HEALTHREPORT_UPLOAD_ENABLED, false);
> +                    // Mma can only work if Health Report is enabled
> +                    if (!isHealthReportEnabled) {

Will this update correctly if health report is toggled while the user is in settings? I assume this method is called for each PreferenceScreen so yes.
Attachment #8973663 - Flags: review?(michael.l.comella) → review-
> If the user is to disable health report and we keep the toggle on the screen but disabled,
> maybe we should modify it's summary to let the users know why it can't be enabled?

A good idea (though I wonder it it's worth the time investment, given we probably have higher priority issues) - can you file a follow-up and float it by Susheel to prioritize?
Comment on attachment 8973663 [details]
Bug 1454686 - Add a new preference under Settings - Notifications;

https://reviewboard.mozilla.org/r/242030/#review248000

> nit: no need to number each part and it takes extra work to make sure the numbering is in order.

Thanks! Was looking in commit history for other tickets resolved in more commits and though numbering them is the custom way to do it. Will avoid doing that in the future.

> Will this update correctly if health report is toggled while the user is in settings? I assume this method is called for each PreferenceScreen so yes.

Indeed, the method is called each time a PreferenceScreen is accessed.
Attachment #8973667 - Attachment is obsolete: true
Attachment #8973667 - Flags: review?(michael.l.comella)
Attachment #8973669 - Attachment is obsolete: true
Attachment #8973669 - Flags: review?(michael.l.comella)
Attachment #8973685 - Attachment is obsolete: true
Attachment #8973685 - Flags: review?(michael.l.comella)
Attachment #8973694 - Attachment is obsolete: true
Attachment #8973694 - Flags: review?(michael.l.comella)
Attachment #8973667 - Attachment is obsolete: false
Attachment #8973667 - Flags: review?(michael.l.comella)
Attachment #8973669 - Attachment is obsolete: false
Attachment #8973669 - Flags: review?(michael.l.comella)
Attachment #8973685 - Attachment is obsolete: false
Attachment #8973685 - Flags: review?(michael.l.comella)
Attachment #8973694 - Attachment is obsolete: false
Attachment #8973694 - Flags: review?(michael.l.comella)
Sorry for the spam, I don't think I've done anything to make the other patches obsolete but after I pushed an update to commit 1 all the others got obsoleted and I see now that they are not grouped together anymore in ReviewBoard. ?

Regarding updating the new setting summary for when it is disabled and the user cannot interact with it, I've filled bug 1459864
Blocks: 1459864
You always need to push the whole patch series to Mozreview. If you push only one commit, Mozreview assumes you're abandoning the other commits and discards them.
In case we don't include the "New features, product tips" string fix in the initial release, here's https://bugzilla.mozilla.org/show_bug.cgi?id=1459934 to keep track of it.
> Was looking in commit history for other tickets resolved in more commits and though numbering them is the custom way to do it.

We used to upload individual patches to Bugzilla so we used the numbers to keep the patches in order. Since then, we've upgraded to Mozreview and have the ability to land a series of changes instead where the numbering is now in the changeset metadata.
Comment on attachment 8973663 [details]
Bug 1454686 - Add a new preference under Settings - Notifications;

https://reviewboard.mozilla.org/r/242030/#review250156

r+ w/ nits.

::: commit-message-43e0a:3
(Diff revision 2)
> +Bug 1454686 - Part 1 - Add a new preference under Settings - Notifications; r?mcomella
> +
> +The behavior of this new preference is dynamic in that:

Good summary. :)

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:831
(Diff revision 2)
> +                        i--;
> +                        continue;
> +                    }
> +
> +                    // Mma can only work if Health Report is enabled
> +                    if (!isHealthReportEnabled(getApplicationContext())) {

It's safer to always set the value rather than assume the initial value, e.g.:

```java
boolean isEnabled = isHealthReportEnabled(...);
((SwitchPreference) pref).setChecked(isEnabled);
pref.setEnabled(isEnabled);
```
Attachment #8973663 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8973663 [details]
Bug 1454686 - Add a new preference under Settings - Notifications;

https://reviewboard.mozilla.org/r/242030/#review250162

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:831
(Diff revision 2)
> +                        i--;
> +                        continue;
> +                    }
> +
> +                    // Mma can only work if Health Report is enabled
> +                    if (!isHealthReportEnabled(getApplicationContext())) {

"It's safer to always..."

This should have been a nit. Since this is called each time the PreferenceScreen is shown and they're unlikely to break the API, this should be fine to ship. For time reasons, let's only fix this if there are other issues to fix.
Petru, wrt comment 39, see Jan's comment 40. I can't actually land this as is because they're not considered to be a single "revision set". mozreview should be able to figure out which commits to push automatically but if not, you can explicitly specify them with something like, e.g. `hg push -r <end-commit>:<other-commit-end> review`. If you need help with this, let me know.

Note that -r actually works the same way *for all hg commands* so you can verify the commits you select with `hg log -r <...>:<...>`. (note: it might actually be two colons, I can't remember).
Flags: needinfo?(petru.lingurar)
> Sorry for the spam,

Also, don't worry about it! Bugzilla is *your* canvas so if you need to make a few test pushes to get things right, so be it. :)
Comment on attachment 8973669 [details]
Bug 1454686 - Part 3 - Small refactoring: extract isMmaEnabled and renamed initSwitchboard;

https://reviewboard.mozilla.org/r/242038/#review250164

lgtm but it won't let me add r+ (see above).

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1450
(Diff revision 1)
>      }
> +
> +    /**
> +     * Get if Mma is available for the device and enabled by the user.
> +     */
> +    public static boolean isMmaAvailable(@NonNull Context context) {

nit: it's a little confusing that this, and MmaDelegate, have the same method name (as a caller, which one do I use?) but I don't have a better solution.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1451
(Diff revision 1)
> +
> +    /**
> +     * Get if Mma is available for the device and enabled by the user.
> +     */
> +    public static boolean isMmaAvailable(@NonNull Context context) {
> +        final boolean isInMmaExperiment = SwitchBoard.isInExperiment(context, Experiments.LEANPLUM);

nit: include debug?

This "check both leanplum experiments" is duplicated (with the first commit): I wonder if we can share code?
Comment on attachment 8973685 [details]
Bug 1454686 - Part 4 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/242056/#review250172

Looks good but wondering about "The process will check if all other requirements are met...".

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2328
(Diff revision 1)
>  
> +            case "NotificationSettings:FeatureTipsStatusUpdated":
> +                if (message.getBoolean("isMmaEnabled")) {
> +                    initSwitchboardAndMma(this, safeStartingIntent, isInAutomation);
> +                } else {
> +                    MmaDelegate.stopReportingEvents();

nit: I don't think these are called individually: can they be combined into one method (in part 1 if it's not too difficult)?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1134
(Diff revision 1)
>              final Boolean newBooleanValue = (Boolean) newValue;
>              AdjustConstants.getAdjustHelper().setEnabled(newBooleanValue);
> -            if (!newBooleanValue) {
> -                MmaDelegate.stop();
> -            }
> +            // If Health Report has been disabled Mma should also be stopped.
> +            // If it was just enabled, we should also try to start Mma and not wait until the app is restarted.
> +            // The process will check if all the other requirements are met before actually starting Mma.
> +            informMmaStatusChanged(newBooleanValue);

"The process will check if all the other requirements are met before actually starting Mma."

Where does the code do this? Sorry, I'm pressed for time atm and I haven't been able to find it.
Comment on attachment 8973694 [details]
Bug 1454686 - Part 5 - Remove Lint error suppression for unused strings;

https://reviewboard.mozilla.org/r/242070/#review250174

lgtm, thanks for cleaning up.
Comment on attachment 8973667 [details]
Bug 1454686 - Part 2 - Add MmaDelegate methods to allow stopping LeanPlum

Waiting for new reviewboard push before giving official r*: see above.
Attachment #8973667 - Flags: review?(michael.l.comella)
Comment on attachment 8973667 [details]
Bug 1454686 - Part 2 - Add MmaDelegate methods to allow stopping LeanPlum

https://reviewboard.mozilla.org/r/242036/#review250178

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:117
(Diff revision 1)
> +        VarCache.reset();
> +    }
> +
> +    @Override
> +    public void allowRestartInCurrentAppSession() {
> +        LeanplumInternal.setCalledStart(false);

I don't love that we're digging into leanplum internals to do this: did you consider alternatives?

Note: I haven't had a chance to dig into the leanplum code to find out if this all internal digging is doing what we expect it to.
Attachment #8973667 - Flags: review?(michael.l.comella)
Comment on attachment 8973685 [details]
Bug 1454686 - Part 4 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/242056/#review250172

> nit: I don't think these are called individually: can they be combined into one method (in part 1 if it's not too difficult)?

> in part 1

I meant part 2, sorry. The part where you add them. :)
NI self to look at Leanplum internals for part 2.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8973667 [details]
Bug 1454686 - Part 2 - Add MmaDelegate methods to allow stopping LeanPlum

https://reviewboard.mozilla.org/r/242036/#review250224

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:117
(Diff revision 1)
> +        VarCache.reset();
> +    }
> +
> +    @Override
> +    public void allowRestartInCurrentAppSession() {
> +        LeanplumInternal.setCalledStart(false);

Perused LeanPlum documentation [1] but found no APIs to offer what we needed.
I did extensive testing to try to understand the start and stop processes, their dependencies and find the best solution with the least of impact or internal methods usages.


[1] https://docs.leanplum.com/reference
Comment on attachment 8973669 [details]
Bug 1454686 - Part 3 - Small refactoring: extract isMmaEnabled and renamed initSwitchboard;

https://reviewboard.mozilla.org/r/242038/#review250164

> nit: it's a little confusing that this, and MmaDelegate, have the same method name (as a caller, which one do I use?) but I don't have a better solution.

Renamed MmaDelegate().isMmaEnabled() to isMmaAllowed() as that should be a more appropriate name (in regards to considering Mma as disabled for private tabs) and should help mitigate the confusion.
Comment on attachment 8973669 [details]
Bug 1454686 - Part 3 - Small refactoring: extract isMmaEnabled and renamed initSwitchboard;

https://reviewboard.mozilla.org/r/242038/#review250232

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1451
(Diff revision 1)
> +
> +    /**
> +     * Get if Mma is available for the device and enabled by the user.
> +     */
> +    public static boolean isMmaAvailable(@NonNull Context context) {
> +        final boolean isInMmaExperiment = SwitchBoard.isInExperiment(context, Experiments.LEANPLUM);

From what I saw in Switchboard experiments config [1] <LEANPLUM_DEBUG> is not used anymore? as an experiment, but kept it in the eventuality that it will be used later.

I created a new method in MmaDelegate() - as this class is the specialization for those experiments, to be the only place from which the LP experiments check should be called. Also, now this class will be the only one which accesses those string constants.

[1] https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/experiments/records
Comment on attachment 8973685 [details]
Bug 1454686 - Part 4 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/242056/#review250172

> "The process will check if all the other requirements are met before actually starting Mma."
> 
> Where does the code do this? Sorry, I'm pressed for time atm and I haven't been able to find it.

Thanks!
Reworded to avoid using "process" but kept the comment as I feel it's important to easily grasp what will happen next, and to show the link between Health Report and Mma.
("process" referred to all that would happen after calling the informMmaStatusChanged(..) method. That would propagate in BrowserApp() - initSwitchboardAndMma() which will check GeckoPreferences.isMmaAvailable(..) )
Comment on attachment 8973685 [details]
Bug 1454686 - Part 4 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/242056/#review250254

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1134
(Diff revision 1)
>              final Boolean newBooleanValue = (Boolean) newValue;
>              AdjustConstants.getAdjustHelper().setEnabled(newBooleanValue);
> -            if (!newBooleanValue) {
> -                MmaDelegate.stop();
> -            }
> +            // If Health Report has been disabled Mma should also be stopped.
> +            // If it was just enabled, we should also try to start Mma and not wait until the app is restarted.
> +            // The process will check if all the other requirements are met before actually starting Mma.
> +            informMmaStatusChanged(newBooleanValue);

Thanks!
Reworded to avoid using "process" but kept the comment as I feel it's important to easily grasp what will happen next, and to show the link between Health Report and Mma.
("process" referred to all that would happen after calling the informMmaStatusChanged(..) method)
Attachment #8973667 - Attachment is obsolete: true
Attachment #8973669 - Attachment is obsolete: true
Attachment #8973685 - Attachment is obsolete: true
Attachment #8973694 - Attachment is obsolete: true
Thanks Mike!
Managed to gather all those separate commits in a single "revision set" and also fixed the nits you found.

I think I understood the process now: 
- if you have more commits and want them grouped together, you need to push them individually (to not just create another changeset) and ReviewBoard will know to group them based on the bug number.
- if you edit one commit in the group, all of them need again to be pushed individually, from first to last (this also allows to easily update later commits to support the change you did below)
Flags: needinfo?(petru.lingurar)
Comment on attachment 8973667 [details]
Bug 1454686 - Part 2 - Add MmaDelegate methods to allow stopping LeanPlum

https://reviewboard.mozilla.org/r/242036/#review250224

> Perused LeanPlum documentation [1] but found no APIs to offer what we needed.
> I did extensive testing to try to understand the start and stop processes, their dependencies and find the best solution with the least of impact or internal methods usages.
> 
> 
> [1] https://docs.leanplum.com/reference

> I did extensive testing

Great! :) Thanks for digging in.

As part of reviewing use of "unofficial APIs", I need to verify that what Leanplum thinks their code is doing is what it's actually doing (i.e. review their code too!) and consider alternative solutions that are less tricky (e.g. it might be helpful to know other things you had considered).
Comment on attachment 8973669 [details]
Bug 1454686 - Part 3 - Small refactoring: extract isMmaEnabled and renamed initSwitchboard;

https://reviewboard.mozilla.org/r/242038/#review250232

> From what I saw in Switchboard experiments config [1] <LEANPLUM_DEBUG> is not used anymore? as an experiment, but kept it in the eventuality that it will be used later.
> 
> I created a new method in MmaDelegate() - as this class is the specialization for those experiments, to be the only place from which the LP experiments check should be called. Also, now this class will be the only one which accesses those string constants.
> 
> [1] https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/experiments/records

> From what I saw in Switchboard experiments config [1] `<LEANPLUM_DEBUG>` is not used anymore?

tbh, I'm not sure: the first time I saw it was when you wrote it into the code! :P I'll take your word for what should be used.

> Also, now this class will be the only one which accesses those string constants.

This is a good way to mitigate the problem. :)
Comment on attachment 8976099 [details]
Bug 1454686 - Small refactoring of Mma related methods;

https://reviewboard.mozilla.org/r/244284/#review250562

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:179
(Diff revision 1)
> +    }
> +
> +    // isMmaAllowed should use pass-in context. The context comes from
>      // 1. track(), it's called from UI (where we inject events). Context is from weak reference to Activity (assigned in init())
>      // 2. handleGcmMessage(), it's called from GcmListenerService (where we handle GCM messages). Context is from the Service
> -    private static boolean isMmaEnabled(Context context) {
> +    private static boolean isMmaAllowed(Context context) {

nit: The distinction between "allowed" and "available" is pretty subtle but this is better than it was before and this method is private anyway so let's take it. :) Thanks for the improvement.

(maybe "available" should be `isMmaSelectableAndUserEnabled`?)
Attachment #8976099 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8976105 [details]
Bug 1454686 - Remove Lint error suppression for unused strings;

https://reviewboard.mozilla.org/r/244292/#review250564
Attachment #8976105 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8976104 [details]
Bug 1454686 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/244290/#review250570

Some uncertainty but low risk: lgtm.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2326
(Diff revision 1)
>  
>                  break;
>  
> +            case "NotificationSettings:FeatureTipsStatusUpdated":
> +                if (message.getBoolean("isMmaEnabled")) {
> +                    initSwitchboardAndMma(this, safeStartingIntent, isInAutomation);

One problem I see here is that if Leanplum is enabled, then disabled, and then re-enabled in the same session, we call some methods redundantly.

For example, MmmDelegate.init calls `mmaHelper.init`, which calls `LeanplumHelperActivity.enableLifecycleCallbacks`, which registers an additional set of lifecycle callbacks. Looking briefly at the Leanplum code (these callbacks and elsewhere), it looks like it's careful about being registered twice so I'm more concerned about our code (of which there isn't much) and I'm generally less concerned about calling init twice.

Since the enabled -> disabled -> enabled case isn't likely to happen often (so is unlikely to skew data though Leanplum seems careful not to do so), if you've tested it and it doesn't crash or do anything crazy, then this is fine with me. If you think it's worth the churn, maybe we should just add a comment?
Attachment #8976104 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8976073 [details]
Bug 1454686 - MmaLeanPlumImp().stop() will now stop LP, stop showing messages and allow restart in same app session;

https://reviewboard.mozilla.org/r/244256/#review250572

I'm not convinced yet that leanplum isn't continuing to upload user data to the servers after we stop it here. Can you elaborate on how that's done (I left a comment below with my theories)? Solving this might be as simple as adding a more elaborate comment. :)

By the way, sorry for how long it's taking to land this bug - thanks for sticking in there! :)

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:113
(Diff revision 2)
> +        // This will stop LeanPlum talking to the server, reporting events,
> +        // but it will still work with the already downloaded data
>          Leanplum.stop();

I find it strange that Leanplum only has to post a request to the server [1] to stop itself from `track`ing events. Do you know how that works? Can you add to the comment?

In particular, I'm concerned that Leanplum might continue to upload user data to their servers.

fwiw:
- For events, it looks like we check the mma enabled state before sending events in `MmaDelegate.track`
- it looks like `setCalledStart(false)` prevents some background events from happening like POST `RESUME_SESSION` and `resumeHeartbeat`.

**Are there any other events that are happening in the background?**

[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/mobile/android/thirdparty/com/leanplum/Leanplum.java#1050

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:116
(Diff revision 2)
> +        // This will remove all cached messages to make sure that they will not be triggered in current session anymore
> +        VarCache.reset();

If you've tested it, wfm: it's hard to follow how the VarCache is used in leanplum code but flushing it seems reasonable.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:120
(Diff revision 2)
> +        LeanplumInternal.setCalledStart(false);
> +        LeanplumInternal.setHasStarted(false);

These seem primarily used to manage lifecycle callbacks.
Attachment #8976073 - Flags: review?(michael.l.comella) → review-
One possible alternative implementation that has more certainty: we could try swapping out the Leanplum network components for no-op ones when leanplum is disabled. I'm not sure it's worth our time though and it sounds like you've already tested this thoroughly. What do you think, Petru?
Flags: needinfo?(michael.l.comella)
Comment on attachment 8976073 [details]
Bug 1454686 - MmaLeanPlumImp().stop() will now stop LP, stop showing messages and allow restart in same app session;

https://reviewboard.mozilla.org/r/244256/#review250572

> I find it strange that Leanplum only has to post a request to the server [1] to stop itself from `track`ing events. Do you know how that works? Can you add to the comment?
> 
> In particular, I'm concerned that Leanplum might continue to upload user data to their servers.
> 
> fwiw:
> - For events, it looks like we check the mma enabled state before sending events in `MmaDelegate.track`
> - it looks like `setCalledStart(false)` prevents some background events from happening like POST `RESUME_SESSION` and `resumeHeartbeat`.
> 
> **Are there any other events that are happening in the background?**
> 
> [1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/mobile/android/thirdparty/com/leanplum/Leanplum.java#1050

Indeed, Leanplum.stop() is not sufficient.

I have investigated more and found an interesting discussion started by us where someone from LeanPlum suggested to use *enableTestMode* to "essentially stop the sdk from that point on" - [1].
Basically this method will set a boolean flag that is checked before the LP SDK does anything inside the app, and more, it will "prevent Leanplum from communicating with the server" - as the method comment says.

Tested this and went with it as it this method at least has a comment in code about what it does and deep implications for which I think it will be supported in the future also.

Because this method takes care of blocking any LP actions inside the app I removed the VarCache.reset() which was used for flushing the already downloaded messages so even if LP tried to show a message, it won't be there to show.


[1] https://github.com/Leanplum/Leanplum-iOS-SDK/issues/27#issuecomment-305632443

> If you've tested it, wfm: it's hard to follow how the VarCache is used in leanplum code but flushing it seems reasonable.

I was flushing VarCache so that even if LP tried to trigger the display of a message, no messages would exist anymore to be displayed.

> These seem primarily used to manage lifecycle callbacks.

Indeed.
But if LeanPlum knows it has already been started, when we call MmaDelagate().init() and so Leanplum.start() the second time, in the same app session, after toggling the preference off and on it will not display the messages as it would normally do after the start call.
With this two methods toggling the prefference off and on will stop and then start LeanPlum which will behave as it was started for the first time in that session.
Got to them only after debugging.
Comment on attachment 8976099 [details]
Bug 1454686 - Small refactoring of Mma related methods;

https://reviewboard.mozilla.org/r/244284/#review250562

> nit: The distinction between "allowed" and "available" is pretty subtle but this is better than it was before and this method is private anyway so let's take it. :) Thanks for the improvement.
> 
> (maybe "available" should be `isMmaSelectableAndUserEnabled`?)

Thanks!
Went with isMmaAvailableAndEnabled() which I think expresses exactly what it offers and better differentiates it from MmaDelegate().isMmaAllowed()
Comment on attachment 8976104 [details]
Bug 1454686 - Respond to changes in the new preference state state;

https://reviewboard.mozilla.org/r/244290/#review250570

> One problem I see here is that if Leanplum is enabled, then disabled, and then re-enabled in the same session, we call some methods redundantly.
> 
> For example, MmmDelegate.init calls `mmaHelper.init`, which calls `LeanplumHelperActivity.enableLifecycleCallbacks`, which registers an additional set of lifecycle callbacks. Looking briefly at the Leanplum code (these callbacks and elsewhere), it looks like it's careful about being registered twice so I'm more concerned about our code (of which there isn't much) and I'm generally less concerned about calling init twice.
> 
> Since the enabled -> disabled -> enabled case isn't likely to happen often (so is unlikely to skew data though Leanplum seems careful not to do so), if you've tested it and it doesn't crash or do anything crazy, then this is fine with me. If you think it's worth the churn, maybe we should just add a comment?

Tested this and saw no issues.
Made a small recording of spamming the setting toogle - https://drive.google.com/file/d/1Rh-aYqtys45D8xcLq9H0lT8iZln2EtMx/view?usp=sharing
For my device I receive those 3 notifications when LeanPlum starts and the helpful hint anytime the activity resumes.
Everything seems to run as intended.
Comment on attachment 8976073 [details]
Bug 1454686 - MmaLeanPlumImp().stop() will now stop LP, stop showing messages and allow restart in same app session;

https://reviewboard.mozilla.org/r/244256/#review251600

I looked through the LP code and the major methods I'm aware of (e.g. `LeanplumInternal.track`) check this value through `isNoop` before making an action so this should prevent both recording events to disk, uploading them to the server, and displaying data to the user. Note that I didn't thoroughly vet this.

This is a much simpler solution than before: thanks for doing the extra research, Petru! It looks great! :)

fwiw, I don't think fixing these nits is worth the churn so I didn't open them as issues and I'm just going to land as is.

::: commit-message-440b3:6
(Diff revision 3)
> +Bug 1454686 - MmaLeanPlumImp().stop() will now stop LP, stop showing messages and allow restart in same app session; r?mcomella
> +
> +According to current LP documentation there are no SDK APIs to allow users to fully stop LP: events reporting and message displaying there.
> +After extensive testing and investigations I think I found the least intrusive way to offer that.
> +
> +We will use internal methods but which are public so I hope they will be supported in the future also. Nevertheless we will need to maintain this in regards to future SDK updates.

I have similar concerns. fwiw, I commented in the bug you linked: https://github.com/Leanplum/Leanplum-iOS-SDK/issues/27#issuecomment-390807391

Thanks for finding that!

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:117
(Diff revision 3)
> +    //      - fully stop LeanPlum: tracking events and displaying messages
> +    //      - allow LP to be restarted in the same app session
> +    // Did extensive testing to ensure all will work as intended
> +    // but although this method uses LP public methods it should be maintained in accordance with LP SDK updates.
>      @Override
>      public void stop() {

nit: I think this method should actually be called `setDisabled()`: `stop` is misleading because `Activity.onStop` exists.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:126
(Diff revision 3)
> +        // As written in LeanPlum SDK documentation, "This prevents Leanplum from communicating with the server."
> +        // as this "isTestMode" flag is checked before LeanPlum SDK does anything.
> +        // Also has the benefit effect of blocking the display of already downloaded messages.
> +        //
> +        // The reverse of this - setIsTestModeEnabled(false) must be called before trying to init LP in the same session.
> +        Leanplum.setIsTestModeEnabled(true);

nit: It would be great to link to the conversation you linked in this bug that this is the preferred way to disable Leanplum (that was a good find! :).
Attachment #8976073 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a371b3bb0cd6
Add a new preference under Settings - Notifications; r=mcomella
https://hg.mozilla.org/integration/autoland/rev/84312c7af401
MmaLeanPlumImp().stop() will now stop LP, stop showing messages and allow restart in same app session; r=mcomella
https://hg.mozilla.org/integration/autoland/rev/c6586abb6773
Small refactoring of Mma related methods; r=mcomella
https://hg.mozilla.org/integration/autoland/rev/d21bb52a99dc
Respond to changes in the new preference state state; r=mcomella
https://hg.mozilla.org/integration/autoland/rev/de4e1b2221d7
Remove Lint error suppression for unused strings; r=mcomella
Flags: qe-verify+
Hello,

After enabling LeanPlum I did get the notifications and banners that I've set up for testing, but testing this is not currently possible as the "Product and feature tips" toggle is not displayed in the Notifications section of the Settings.

If toggling the Health Report from Privacy I did not get any more LeanPlum notifications.
Status: RESOLVED → REOPENED
Flags: qe-verify+ → qe-verify-
OS: Unspecified → Android
Hardware: Unspecified → All
Resolution: FIXED → ---
Version: unspecified → Firefox 62
Thanks Bogdan!

Something strange is happening, the only reason why that toggle would not show would be that the check to see if LeanPlum is available [1] would return that the device is not in a LeanPlum experiment [2].
But Bogdan had LeanPlum working, something that should have also checked [3] if the device is in a LeanPlum experiment [4] and if not, shouldn't proceed with starting LeanPlum.

Will investigate and update the patch accordingly.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java#823
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java#172
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1022
[4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java#1458
Investigated more with Bogdan the issue and it seems that the blocker for testing was because of the convoluted way of enabling LeanPlum on nightly builds.
Managed to find a clearer and more reliable path for this so I'm asking again for tests to confirm the patch.
Flags: qe-verify- → qe-verify+
Testing with the new method seems to work excellent!

This means enabling LeanPlum and updating via Play Store instead of using -r with adb. When we were using the adb update method the "Product and feature tips" were never displayed in the Notifications section in Settings. Updating Nightly via PS seems to work flawlessly regarding this manner.

By using this update method we will sometimes be limited to a previous build of Nightly until the new version is available on the PS.

As far as testing goes, everything worked as expected. @Susheel or Michael do you think we need to do further investigation to see what the problem is when updating via ADB or should we stick to the new method?

Marking as fixed and if no further investigation is needed please mark as verified as well.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Flags: qe-verify+
Flags: needinfo?(sdaswani)
Flags: needinfo?(michael.l.comella)
Resolution: --- → FIXED
Leaving prioritization decisions to Susheel. afaik, the problem is that the check we use to display the panel, `if (is in leanplum experiment)`, does not take into account the adb method so we'd just have to update that if statement to support the adb method. I don't know if the check has easy access to this variable and thus how hard it'd be. Maybe Petru has some idea.
Flags: needinfo?(michael.l.comella)
I have created bug https://bugzilla.mozilla.org/show_bug.cgi?id=1464990 to address the need of a easier way to test Switchboard experiments, Leanplum being among them.
clearing NI for me.
Flags: needinfo?(sdaswani)
Marking as verified since everything else will be addressed in Bug 1464990.
Status: RESOLVED → VERIFIED
Added to Fennec 62 release notes.
You need to log in before you can comment on or make changes to this bug.