Closed Bug 1386192 Opened 7 years ago Closed 6 years ago

Test Leanplum Custom Message for Onborading

Categories

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

Product:
Firefox for Android Graveyard
Component:
Metrics
Version:
56 Branch
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: cnevinchen, Assigned: petru)

References

Details

(Whiteboard: [Leanplum])

Attachments

(4 files, 2 obsolete files)

Fennec Onboarding with LeanPlum overwritten values
4.08 MB, video/mp4
Details
test.png
330.01 KB, image/png
Details
no_image.png
62.51 KB, image/png
Details
Bug 1386192 - Test Leanplum Custom Message for Onboarding;
59 bytes, text/x-review-board-request
cnevinchen
: review+
Details 
This bug is to test the possibility to do run-time onboarding using custom message templates.
Priority: -- → P2
[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.
Assignee: cnevinchen → nobody
Rank: 1
Priority: P2 → P1
Whiteboard: [Leanplum]
Whiteboard: [Leanplum] → [Leanplum] [61]
Closing my bug, but here's my comment:
We want to be able to leverage LP's tool to quickly A/B test the onboarding flow for our Android users. 

The thought is to integrate custom onboarding slides (the first 2 or 3 that we currently have as default) into Leanplum. They would need to be added in as "variables". 

What we would love to do is test the whole onboarding flow (first 2 slides and then a few snippet banners) in Leanplum.
Whiteboard: [Leanplum] [61] → [Leanplum]
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Attachment #8892390 - Attachment is obsolete: true
I've pushed a patch to enable this functionality but we should take into consideration that connecting to LeanPlum and downloading the Variables might take even a few seconds and so this change might not be suited for the first screen that the users should see.
Flags: needinfo?(kplam)
(In reply to Petru-Mugurel Lingurar[:petru] from comment #6)
> Created attachment 8971970 [details]
> Fennec Onboarding with LeanPlum overwritten values
> 
> I've pushed a patch to enable this functionality but we should take into
> consideration that connecting to LeanPlum and downloading the Variables
> might take even a few seconds and so this change might not be suited for the
> first screen that the users should see.

Looks cool, I'll talk to Kat about this.
Attachment #8971966 - Flags: review?(sdaswani) → review?(michael.l.comella)
Flags: needinfo?(kplam) → needinfo?(cpark)
> connecting to LeanPlum and downloading the Variables might take even a few seconds

A few alternative implementations:
- We ship with all the startup Variables built in, the device randomly picks one, and notifies the server which one it chose. This requires the strings to land on the trains, limiting the quick feedback cycle
- We limit Variable loading to some short duration (2 seconds?) and if it doesn't come, we use a hard-coded fallback string. We might end up seeing the fallback string more often than we'd like though
Mike I like the second suggestion.

Cherry have you seen the video attached to this video? I wanted to see if you had any comments before we merge this work in.
https://bugzilla.mozilla.org/attachment.cgi?id=8971970
Ahh I see what you mean by the delay. Will get the panel's thoughts and let you know today.
To be noted that for people with low speed internet/low spec'd devices, with the current version of the patch, the delay for actually showing the onboarding screens could take visibly more time than in my video in which I've tested with full 100 Mb wi-fi speed. The app will wait to show the onboarding screens until LeanPlum finishes it's startup process and downloads all data from the server.

Mike's second solution could indeed help to manage slow loading times / slow LeanPlum startup times but I think we should do intensive testing to make sure the fallback values will be shown as little as possible without affecting the UX.
I think this makes a lot of sense Petru if you can integrate the 2 second check in your patch:

"- We limit Variable loading to some short duration (2 seconds?) and if it doesn't come, we use a hard-coded fallback string. We might end up seeing the fallback string more often than we'd like though"
Updated the patch with the 2 seconds delay before starting to show the onboarding screens using the default values.

In regards to Michael's concern, I was thinking that maybe even before the code gets reviewed positively I could share a dev build with QA to test on more devices and in more conditions if the change that the ticket intended is even feasible. 
Because if this 2 seconds delay is too little and the users would get to see the default values too often, there is little that this feature would help with.
I think that makes a lot of sense Petru - can you have the QA folks test under different devices and network conditions (3G, 4G, LTE, WiFi) and see if the LP onboarding is working?
FYI I will wait to have this code reviewed until QA reports back on how the 2 second delay is working.
Hey Team, 

I checked this out with Kat and we're fine with the 2-sec delay + Mike's 2nd option: "We limit Variable loading to some short duration (2 seconds?) and if it doesn't come, we use a hard-coded fallback string. We might end up seeing the fallback string more often than we'd like though".

Are we still waiting to hear back from QA?
Flags: needinfo?(cpark) → needinfo?(sdaswani)
See Jess's comment above - clearing my NI
Flags: needinfo?(sdaswani)
Awesome, we have the code, we just need to QA it. I need to file a PI request for that, which I'll be doing soon.
Attached image test.png — 
Hey all,

Here are the testing details for this feature: https://goo.gl/EGzfgp

As you can see the full default values were returned a few times but most of the times we got mixed values (overridden text with default images as seen in the attachment).

There were 3 exceptions when we got the overridden text with no images at all.

Testing was done by using a fresh install of the app each time before launching it.
Thanks Bogdan!

The mixed values are probably because of concurrency issues. I thought LeanPlum would override the values after it completes the init + downloading part, but it seems that it overrides the values progressively, as they get downloaded from the server.
This means the patch must be updated to properly support the feature.

What needs to be noted though:
Because from 140 runs Bogdan got 55 defaults and 71 mixed, while only 14 tests got the full overridden values
> In 88% of the cases the 2 second delay was not enough, even though he tested with full internet.

When updating the patch to avoid the concurrency issue that initial delay of 2 seconds can be increased to maybe 3-4 seconds to try to give enough time for LeanPlum to complete but this will start to affect the general onboarding experience I think.

I think we should try to test how much of an increase for the delay is needed to get the overriden values for at least 75% ? of the tries just to have a general baseline and appreciate then the opportunity of this patch?
Flags: needinfo?(sdaswani)
Petru, I think adding some concurrency is strictly necessary - we can't have onboarding in a state where it's essentially corrupt due to network races. We can discuss this further live.
Flags: needinfo?(sdaswani)
Hey all,

After doing some more tests today with a new build and increased time of 3 seconds here are the results: https://goo.gl/zbGzWx

As you can see in 70% of the cases we got the default onboarding for the devices as for the rest of the 30% of the cases most of them were limited to 2 devices.

As a note, having a bigger increase than 3 seconds is not worth it as even a 3 second delay for the onboarding to kick in is quite noticeable, is we could get this to work with the 2 second delay (or less) as mentioned in Comment 8 by Michael that would be great, but 3 seconds or ever more is too much to wait for the onboarding to kick in as the user might start browsing or doing something else in the mean time. As an example with the 3 second time we tested now:

Open Fennec > Tap on a tile from Top Sites > Onboarding is displayed > Page from Top Sites starts loading and onboarding is dismissed.
Susheel, I have updated my patch and shared dev builds with Bogdan to test this again.
Also logged how much time is needed for LeanPlum to finish the init process and resource downloading (for which we are using Leanplum.addVariablesChangedAndNoDownloadsPendingHandler).
According to Bogdan's tests results https://goo.gl/zbGzWx the median time needed to get overriden values from Leanplum is 3.82 seconds which might be a bit too much for the onboarding process. And this was with the devices being connected to a 100Mbps network.
Flags: needinfo?(sdaswani)
Very interesting Petru. Can we make the leanplum controlled slides be at the middle / end of the onboarding flow? I'm thinking we can take advantage of the slow human and wait 5 seconds for the LP variables to be initialized. This may change your patch. Thoughts?
Flags: needinfo?(sdaswani) → needinfo?(petru.lingurar)
Indeed, in the current implementation (and what I used for my dev build shared with Bogdan) we would start the onboarding process with the screens knowing beforehand what data should they show.
We could make it so that in the constructing phase they would check if LeanPlum data is available and use that to override the default.

But there are only 3 or 4 screens that we might show for onboarding and when one is shown the next immediate one needs to also be created and available. 
So at most we could have the last 1/2 screens use LeanPlum overridden data which I think might not help that much with what this feature was intended to.

Maybe we could go with the other suggestion from Mike,
> We ship with all the startup Variables built in, the device randomly picks one, and notifies the server which one it chose. 
But this would increase APK size depending on the number images which would possibly be used for the onboarding slides and also requires the strings to land on the trains, limiting the quick feedback cycle.
Flags: needinfo?(petru.lingurar) → needinfo?(sdaswani)
Cherry and Kat, can we discuss exactly how much you want to customize the onboarding screen? Do you want to be able to set the first and second screens, or is setting the third and forth just fine? We can have a quick meeting to discuss.
Flags: needinfo?(sdaswani)
Flags: needinfo?(kplam)
Flags: needinfo?(cpark)
We'd like to be able to set and customize all the screens.
Flags: needinfo?(cpark)
Cherry if you want to use LP to customize onboarding, it's not fast enough in initialization to customize the the first and second screens. There is just a incompatibility with showing a user brand new content that is downloaded from the internet as soon as the App launches (e.g., you have to load a website in a browser before you see the content).

Can we start off with just customizing the third screen and on?

The other option is we have the Softvision engineers hard code the different screens in an app release, but then you are tied to the train schedule.
Flags: needinfo?(kplam) → needinfo?(cpark)
Let's proceed with LP and only having ability to customize the third screen and on...

We don't want to be tied to the trains.
Flags: needinfo?(cpark)
I spoke to LP and they said it shouldn't take more than half a second to get all the LP variables set. I'm going to set up a meeting for you all to chat with them.
Attachment #8971966 - Attachment is obsolete: true
Attachment #8971966 - Flags: review?(sdaswani)
Updated the existing patch with the changes made to avoid concurrency issues.

Did some tests again, I don't see how we could reliable get the data needed under those 3 seconds.
Would be great if LP could offer some input on I'm using to know when LP finished downloading and we can use the overridden data - Leanplum.addVariablesChangedAndNoDownloadsPendingHandler() - https://www.leanplum.com/dashboard/static/javadoc/com/leanplum/Leanplum.html#addVariablesChangedAndNoDownloadsPendingHandler-com.leanplum.callbacks.VariablesChangedCallback-
Talked with LeanPlum's support who suggested to use a callback firing as soon as the String variables are synced to the app.

Worked with :BogdanS to do some tests with this and the results are encouraging:
https://docs.google.com/spreadsheets/d/1C1cD9UIjwdb9Xm0x0Itw91mb9wyETEevWvF9stZXlO4/edit#gid=2094450222
vs
https://docs.google.com/spreadsheets/d/1C1cD9UIjwdb9Xm0x0Itw91mb9wyETEevWvF9stZXlO4/edit#gid=1152091155

The only issue with this approach is that we loose the possibility to also override the images used for the onboarding screens.
We would only have the option to modify the text but going with this route it's probable that we can fit in that 3 seconds window. Again, tested with full 100 Mb/s internet speed.

Cherry, does this sounds ok?
Is overriding the text (possibly all the fields of the onboarding screens) enough?
Also, I think new events to help distinguish if the onboarding screens showed default data or overriden data would be helpful.
Flags: needinfo?(cpark)
I'm also going to NI Jess as she may have an opinion.
Flags: needinfo?(josorio)
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review259352

This is a brief review cause I haven't try the patch locally. I'll do it when I come back from PTO.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:75
(Diff revision 1)
>  import android.widget.Button;
>  import android.widget.ListView;
>  import android.widget.ViewFlipper;
>  
> +import com.leanplum.Leanplum;
> +import com.leanplum.callbacks.VariablesChangedCallback;

Maybe use MmaDelegate to wrapp Leanplum impl and stub impl? If we turn Leanplum off using the build flag, this won't compile.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1105
(Diff revision 1)
> -     * Code to actually show the first run pager, separated
> -     * for distribution purposes.
> +     * Code to actually show the first run pager, separated for distribution purposes.<br>
> +     * If Mma is available will first check to make sure we have the most recent values from the server.
>       */
>      @UiThread
>      private void checkFirstrunInternal() {
> -        showFirstrunPager();
> +        if (SwitchBoard.isInExperiment(this, Experiments.LEANPLUM) && NetworkUtils.isConnected(this)) {

At this time, is Switchboard completed it's initialization? What will happen if Kinto config hasn't completed? Could this scenario cause misleading A/B testing result?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3073
(Diff revision 1)
> +
> +        if (HardwareUtils.isTablet()) {
> +            mTabStrip.setOnTabChangedListener(new TabStripInterface.OnTabAddedOrRemovedListener() {
> +                @Override
> +                public void onTabChanged() {
> +                    hideFirstrunPager(TelemetryContract.Method.BUTTON);

What will happen if this happens before the firstRun is is displayed?

::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstRunPanelConfigProviderStrategy.java:1
(Diff revision 1)
> +package org.mozilla.gecko.firstrun;

Missing the MPL license part here. 
/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

::: mobile/android/base/java/org/mozilla/gecko/firstrun/LocalFirstRunPanelProvider.java:1
(Diff revision 1)
> +package org.mozilla.gecko.firstrun;

Missing the MPL license part here.

::: mobile/android/base/java/org/mozilla/gecko/firstrun/LocalFirstRunPanelProvider.java:15
(Diff revision 1)
> +public class LocalFirstRunPanelProvider implements FirstRunPanelConfigProviderStrategy {
> +    @SuppressWarnings("fallthrough")
> +    public PanelConfig getPanelConfig(@NonNull Context context, PanelConfig.TYPE type, final boolean useLocalValues) {
> +        final Resources resources = context.getResources();
> +        switch (type) {
> +            default:    // Unreachable. Just to make Lint happy.

I'd like to put default at the end and add a comment for it. For example ( default: // This will also be the case for "WELCOME")

::: mobile/android/base/java/org/mozilla/gecko/firstrun/PanelConfig.java:1
(Diff revision 1)
> +package org.mozilla.gecko.firstrun;

Missing the MPL license part here.

::: mobile/android/base/java/org/mozilla/gecko/firstrun/PanelConfig.java:32
(Diff revision 1)
> +    }
> +
> +    @SuppressWarnings("fallthrough")
> +    public String getClassName() {
> +        switch (type) {
> +            default:    // Unreachable. Just to make Lint happy.

ditto

::: mobile/android/base/java/org/mozilla/gecko/firstrun/RemoteFirstRunPanelConfig.java:1
(Diff revision 1)
> +package org.mozilla.gecko.firstrun;

Missing the MPL license part here.

::: mobile/android/base/java/org/mozilla/gecko/mma/LeanplumVariables.java:1
(Diff revision 1)
> +package org.mozilla.gecko.mma;

Missing the MPL license part here.
Attachment #8984849 - Flags: review?(cnevinchen)
Hey Susheel,

Being able to test copy is certainly a start and a step in the right direction. If this is the best option right now (only being able to test copy) then lets run with it for now as an MVP/V1. We can start there while we explore other ways to leverage LP's onboarding testing features. I have my first touch base with our *new* LP Customer Service Manager tomorrow. I want to ask him what was done for iOS and see if there's anything we can leverage from that work. 

I can followup on this tomorrow after syncing with LP. 

Thank you,

Jess Osorio
Flags: needinfo?(josorio)
Flags: needinfo?(cpark)
Thanks Jess.

So Petru given this is your patch ready to go? If so I will have it reviewed.
Flags: needinfo?(petru.lingurar)
(In reply to :sdaswani only needinfo from comment #40)
> Thanks Jess.
> 
> So Petru given this is your patch ready to go? If so I will have it reviewed.

This is not the final version of the patch.
Nevin raised some interesting scenarios and I'm in the process of updating the patch to account for them.
Flags: needinfo?(petru.lingurar)
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review259352

> Maybe use MmaDelegate to wrapp Leanplum impl and stub impl? If we turn Leanplum off using the build flag, this won't compile.

This Leanplum callback now sits in MmaLeanplumImp.
LeanplumVariables class will now also not be compiled when Leanplum is turned off with the `MOZ_ANDROID_MMA` flag.
Thanks!

> At this time, is Switchboard completed it's initialization? What will happen if Kinto config hasn't completed? Could this scenario cause misleading A/B testing result?

Only after the Kinto config completed and 
only if the Leanplum experiment is available 
does the initialization of MmaDelegate happen (which means registering for variable updates).

Without waiting for this, if in 3 seconds after ~the app starts the listener for `Leanplum.addVariablesChangedHandler` is not called the Onboarding screens will be shown populated with the default local values.

> What will happen if this happens before the firstRun is is displayed?

The firstRun will be retried at the next app start.
Updated my patch to resolve all issues raised by Nevin and polish the previous mvp patch.

The general flow of the Onboarding screens after the first app start:
- the splashscreen is shown
- the 3 seconds countdown to show the onboarding screens with the default local values starts
- download the Kinto config, if the Leanplum experiment is available - initialize Leanplum and listen for when the variables have been updated.

- if before that 3 seconds
	- Kinto config download fails (maybe Internet not available)
	- we know that the Mma experiment is not available
    the Onboarding screens, populated with the default local values will be shown
	- the variables are updated
    the Onboarding screens, populated with the server values will be shown

- if the user touches the address bar before the Onboarding screens are shown:
    the splashscreen will be hidden and the the Onboarding will be retried at the next app start

- if the user touches the address bar after the Onboarding screens are shown but before the user finishing the process
    the Onboarding will be closed and treated as finished - it will not be retried

Other than the code changes the most important addition is the usage of the already existing splashscreen until the onboarding will be shown, and I think that this and the general workflow should be vetted by UX.

To easily get a feel of the new Onboarding process:
- video recording of the new Onboarding with server values: https://drive.google.com/file/d/1yg0SZDRtGkLILQWGuC1aBvwOaj01-kVq/view?usp=sharing
- apk to test: https://drive.google.com/file/d/1ENkBjtxTdiTb4VjJRcbDetbHRLvc91cU/view?usp=sharing
Flags: needinfo?(bram)
Hi Petru,

I’ve given this a look. The behaviour you’ve specified looks good, and using the existing splashscreen before the onboarding is shown is also a good idea.

This gets an r+ from me. Thank you!
Flags: needinfo?(bram)
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review260968

I just pulled the patch and build it locally today. Hope I can finish the review tomorrow.

::: mobile/android/base/java/org/mozilla/gecko/firstrun/OnboardingHelper.java:51
(Diff revision 2)
> +    private static final String FIRSTRUN_UUID = "firstrun_uuid";
> +
> +    // Speculative timeout for showing the Onboarding screens with the default local values.
> +    public static final int DELAY_SHOW_DEFAULT_ONBOARDING = 3 * 1000;
> +
> +    private AppCompatActivity activity;

Could this reference leak the activity?
Attachment #8984849 - Flags: review?(cnevinchen)
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review261208

I'm curious how do you measure the A/B testing result for variables. Can you tell from Leanplum backend that how many people have succesfully fetch the variable when they launch the app for the first time?
How do you tell from Telemetry/Redash that how many people have complete the onboarding? Since the onboarding may happen in the second launch?
I only have above question, other than that, the patch looks good after some minor fix.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1013
(Diff revision 2)
>  
>          final String serverExtra = intent.getStringExtra(INTENT_KEY_SWITCHBOARD_SERVER);
>          final String serverUrl = TextUtils.isEmpty(serverExtra) ? SWITCHBOARD_SERVER : serverExtra;
> -        new AsyncConfigLoader(context, serverUrl) {
> +        final SwitchBoard.ConfigStatusListener configStatuslistener = mOnboardingHelper;
> +        final MmaDelegate.MmaVariablesChangedListener variablesChangedListener = mOnboardingHelper;
> +        new AsyncConfigLoader(context, serverUrl, configStatuslistener) {

I made a mistake and inroduce an anonymous class here, which could leak the activity. I may need to fix this later.

::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPanel.java:16
(Diff revision 2)
>  import android.view.LayoutInflater;
>  import android.view.View;
>  import android.view.ViewGroup;
>  import android.widget.ImageView;
>  import android.widget.TextView;
> +

It'll be better to make this kind of re-format code in a separate patch. Otherwise it'll make back-out/merge code hard.

::: mobile/android/base/java/org/mozilla/gecko/firstrun/LocalFirstRunPanelProvider.java:16
(Diff revision 2)
> +import android.support.annotation.NonNull;
> +
> +import org.mozilla.gecko.R;
> +
> +public class LocalFirstRunPanelProvider implements FirstRunPanelConfigProviderStrategy {
> +    @SuppressWarnings("fallthrough")

Maybe remove @SuppressWarnings?

::: mobile/android/base/java/org/mozilla/gecko/mma/LeanplumVariables.java:19
(Diff revision 2)
> +
> +import com.leanplum.annotations.Variable;
> +
> +import org.mozilla.gecko.R;
> +
> +import java.io.IOException;

unused import

::: mobile/android/base/java/org/mozilla/gecko/mma/LeanplumVariables.java:20
(Diff revision 2)
> +import com.leanplum.annotations.Variable;
> +
> +import org.mozilla.gecko.R;
> +
> +import java.io.IOException;
> +import java.io.InputStream;

ditto

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:158
(Diff revision 2)
> +        if (useLocalValues) {
> +            throw new UnsupportedOperationException("Cannot build remote panel config with local values");
> +        }
> +
> +        switch (type) {
> +            default:    // Unreachable. Just to make Lint happy.

Maybe you want to also move default to the end?
Attachment #8984849 - Flags: review?(cnevinchen) → review+
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review260968

> Could this reference leak the activity?

Indeed I think a leak would be possible.
Used WeakReferences for both the Activity in `OnboardingHelper` and the OnboardingHelper in `MmaLeanplumImp` to avoid it.
Comment on attachment 8984849 [details]
Bug 1386192 - Test Leanplum Custom Message for Onboarding;

https://reviewboard.mozilla.org/r/250652/#review261208

For the A/B tests:
`FirstrunAnimationContainer` already tracked to LeanPLum when the Onboarding was dismissed [1].
Added new events tracking
- "E_Onboarding_With_Default_Values" - for when the Onboarding screens are shown using the default local values
- "E_Onboarding_With_Remote_Values" - for when the Onboarding screens are shown using the server values
I think this can be combined with data available in LeanPlum to approximate how many people saw certain Onboarding screens.

For Onboarding Telemetry:
`FirstrunPager` already tracks when the Onboarding panels are displayed [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/a0e47ebc4c06e652b919dabee711fdbd6bfd31b5/mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunAnimationContainer.java#66
[2] https://dxr.mozilla.org/mozilla-central/rev/a0e47ebc4c06e652b919dabee711fdbd6bfd31b5/mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPager.java#113

> I made a mistake and inroduce an anonymous class here, which could leak the activity. I may need to fix this later.

From what I saw, while this may leak the activity, the entire code executes so quickly that there isn't really time for the leak to happen. Maybe only if the user has a really slow Internet..
Nevertheless, I've filed bug 1473254 to address this issue.

> It'll be better to make this kind of re-format code in a separate patch. Otherwise it'll make back-out/merge code hard.

Looked into it, but I feel that all changes are deeply tied to the purpose of this ticket and also refer to specific parts of the app's code that shouldn't be changed that often so no conflicts should appear.
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84683bf540a5
Test Leanplum Custom Message for Onboarding; r=cnevinchen+582291
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84683bf540a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
An important thing to note is that currently the Leanplum experiment is configured to only be available on Firefox and Firefox Beta [1].

To be able to test this on Nightly the experiment's availability would need to be changed.

[1] https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/experiments/records
Regressions: 1542776
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: