1351585 - Code review of open-source Leanplum SDK

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

As soon as we land the SDK and use it in the app it's part of the code we maintain. We should do a review of this code (quality and privacy).

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review commit: https://reviewboard.mozilla.org/r/138964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138964/

Comment 2

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/1-2/

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/2-3/

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module


Due to the tight schedule, I want to modify Leanplum's SDK code base to reduce the effort of adding dependencies to our code base. Also make less impact on our APK size/method count.
Besides, we’ll shift to Gradle in near future, I don’t want to spend too many effort on Mozbuild,

They are couple for dependencies/code I've stripped from their code base here: 

1. Legacy http client: they have server-side compatibility issue so they can't change it. I'm going to use our ch.boye.httpclientandroidlib version to replace that ( change import).

2. BuildConfig: Their code use two build config in their code, one accept a system variable and use it as their user-agent name. The other just printing debug info. I've remove those code and it works fine.

3. Google Play Location services (Geofence): Since we don't need this feature now, I don't want to add it. 

My idea is to make their SDK easy for us to adpot via moz-build. Once we shift to Gradle build, I'll use the approach in Bug 1351573. 

Since this bug is for Leanplum code review, The patch I provided is the proposed modification of their source code and mozbuild config to put it into our repository.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

Review commit: https://reviewboard.mozilla.org/r/139082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139082/

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 3. Add proguard for leanplum.

Review commit: https://reviewboard.mozilla.org/r/139084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139084/

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 4. Use Switchboard to limit Leanplum startup/init

Review commit: https://reviewboard.mozilla.org/r/139086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139086/

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 5. Add build flag to inject mma implementation.

Review commit: https://reviewboard.mozilla.org/r/139088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139088/

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/3-4/

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139082/diff/1-2/

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139084/diff/1-2/

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867534 [details]
Bug 1351585 - Part 4. Use Switchboard to limit Leanplum startup/init

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139086/diff/1-2/

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867535 [details]
Bug 1351585 - Part 5. Add build flag to inject mma implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139088/diff/1-2/

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Hi Nick
According to https://bugzilla.mozilla.org/show_bug.cgi?id=1143888
I still need the puppet code and buildbot-configs code to work.
Can you please teach me how to add leanplum app_id and access_key to build?
Thank you!

Comment 15

[Nevin Chen(Not active on Bugzilla)]

I also found I don't know how to set the flag to make MOZ_INSTALL_MMA to true...
The reason why I use MMA instead of Leanplum is because we might change the implementation someday,

Comment 16

[:Grisha Kruglov]

(In reply to Nevin Chen [:nechen] from comment #4)
> 1. Legacy http client: they have server-side compatibility issue so they
> can't change it. I'm going to use our ch.boye.httpclientandroidlib version
> to replace that ( change import).

Every now and then we circle around to the idea of removing ch.boye.httpclientandroidlib from our codebase entirely. So far that work hasn't been taken up in earnest, but let's not make it more difficult by creating new dependencies here.

I haven't looked at their code base, but any chance we could get away with just using HttpURLConnection and friends?

Comment 17

[Max Liu [:maliu]]

Comment on attachment 8867535 [details]
Bug 1351585 - Part 5. Add build flag to inject mma implementation.

https://reviewboard.mozilla.org/r/139088/#review142718

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:888
(Diff revision 1)
>              protected Void doInBackground(Void... params) {
>                  super.doInBackground(params);
>                  if (SwitchBoard.isInExperiment(context, Experiments.LEANPLUM) &&
>                          GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true)) {
>                      // Do LeanPlum start/init here
> +                    MmaDelegate.init(context.getApplicationContext());

Why are we not put this into org.mozilla.gecko.switchboard.AsyncConfigLoader#doInBackground ?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:870
(Diff revision 2)
>  
>      /**
>       * Initializes the default Switchboard URLs the first time.
>       * @param intent
>       */
> -    private static void initSwitchboard(final Context context, final SafeIntent intent, final boolean isInAutomation) {
> +    private void initSwitchboard(final Context context, final SafeIntent intent, final boolean isInAutomation) {

Do we still need to remove static if we move MmaDelegate.init into AsyncConfigLoader?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:888
(Diff revision 2)
>              protected Void doInBackground(Void... params) {
>                  super.doInBackground(params);
>                  if (SwitchBoard.isInExperiment(context, Experiments.LEANPLUM) &&
>                          GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true)) {
>                      // Do LeanPlum start/init here
> +                    MmaDelegate.init(BrowserApp.this.getApplication());

Why are we not put the init into AsyncConfigLoader#doInBackground after SwitchBoard.loadConfig?

Comment 18

[Nevin Chen(Not active on Bugzilla)]

(In reply to :Grisha Kruglov from comment #16)
> Every now and then we circle around to the idea of removing
> ch.boye.httpclientandroidlib from our codebase entirely. So far that work
> hasn't been taken up in earnest, but let's not make it more difficult by
> creating new dependencies here.
Totally agree with you

> 
> I haven't looked at their code base, but any chance we could get away with
> just using HttpURLConnection and friends?

They use socket as keep alive. And there is some helper method can let them get Http Header and Status code to do some checking from that socket.
HttpURLConnection didn't provide that feature. And doing it manually means we gonna have our own logic to do that ( InputStream -> String -> parsing...).

The short term solution is to use our ch.boye library. longer term is to work with them to change their implementation.

Comment 19

[Max Liu [:maliu]]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

https://reviewboard.mozilla.org/r/139082/#review142814

::: mobile/android/base/AndroidManifest.xml.in:485
(Diff revision 2)
> +                <action android:name="com.leanplum.LeanplumPushListenerService" />
> +            </intent-filter>
> +        </receiver>
> +
> +        <!-- Leanplum Local Push Notification Service-->
> +        <service android:name="com.leanplum.LeanplumLocalPushListenerService" />

Should we explicitly declare this component exported and enabled?

::: mobile/android/base/AndroidManifest.xml.in:504
(Diff revision 2)
> +                <action android:name="com.google.android.gms.iid.InstanceID" />
> +            </intent-filter>
> +        </service>
> +
> +        <!-- Leanplum GCM/FCM Registration Service -->
> +        <service android:name="com.leanplum.LeanplumPushRegistrationService" />

Should we explicitly declare this component exported and enabled?

::: mobile/android/base/AndroidManifest.xml.in:508
(Diff revision 2)
> +        <!-- Leanplum GCM/FCM Registration Service -->
> +        <service android:name="com.leanplum.LeanplumPushRegistrationService" />
> +
> +
> +        <!-- Geofencing Service -->
> +        <service android:name="com.leanplum.ReceiveTransitionsIntentService" />

Should we explicitly declare this component exported and enabled?

::: mobile/android/base/Makefile.in:143
(Diff revision 2)
>  
>  ifdef MOZ_INSTALL_TRACKING
>  GECKOVIEW_JARS += gecko-thirdparty-adjust_sdk.jar
>  endif
>  
> +

nit: not necessary white space

::: mobile/android/base/Makefile.in:164
(Diff revision 2)
>  
>  ifdef MOZ_ANDROID_MLS_STUMBLER
>  FENNEC_JARS += ../stumbler/stumbler.jar
>  endif
>  
> +

nit: not necessary white space

::: mobile/android/base/moz.build:182
(Diff revision 2)
>  else:
>      constants_jar.sources += ['java/org/mozilla/gecko/' + x for x in [
>          'adjust/StubAdjustHelper.java',
>      ]]
>  
> +

nit: not necessary white space

Comment 20

[Max Liu [:maliu]]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

https://reviewboard.mozilla.org/r/139084/#review142818

Comment 21

[Max Liu [:maliu]]

Comment on attachment 8867534 [details]
Bug 1351585 - Part 4. Use Switchboard to limit Leanplum startup/init

https://reviewboard.mozilla.org/r/139086/#review142820

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:884
(Diff revision 2)
>          final String serverUrl = TextUtils.isEmpty(serverExtra) ? SWITCHBOARD_SERVER : serverExtra;
> -        new AsyncConfigLoader(context, serverUrl).execute();
> +        new AsyncConfigLoader(context, serverUrl) {
> +            @Override
> +            protected Void doInBackground(Void... params) {
> +                super.doInBackground(params);
> +                if (SwitchBoard.isInExperiment(context, Experiments.LEANPLUM) &&

Why are we not put the init into AsyncConfigLoader#doInBackground after SwitchBoard.loadConfig?

Comment 22

[Max Liu [:maliu]]

Comment on attachment 8867535 [details]
Bug 1351585 - Part 5. Add build flag to inject mma implementation.

https://reviewboard.mozilla.org/r/139088/#review142826

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:14
(Diff revision 2)
> +import android.content.Context;
> +
> +
> +public class MmaLeanplumImp implements MmaInterface {
> +    @Override public void init(Application application) {
> +

Is this the actual implementation other than the Stub?

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

The idea of this bug was to just do a review of the existing leanplum code to make sure that it doesn't contain anything that would block us from releasing it. But I guess we can do the integration here too. :)

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

https://reviewboard.mozilla.org/r/138964/#review142888

::: mobile/android/thirdparty/build.gradle:8
(Diff revision 4)
>  apply plugin: 'com.android.library'
>  
>  android {
>      compileSdkVersion 23
>      buildToolsVersion mozconfig.substs.ANDROID_BUILD_TOOLS_VERSION
> -
> +    useLibrary 'org.apache.http.legacy'

I wonder how we get this to compile in non-gradle builds (as far as I know this line doesn't actually add the library but just makes it known at compile time) - nalexander might know.

::: mobile/android/thirdparty/build.gradle:43
(Diff revision 4)
> +    compile "com.android.support:appcompat-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
> +    compile "com.google.android.gms:play-services-gcm:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
> +    compile "com.google.android.gms:play-services-location:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
> +    compile "com.google.android.gms:play-services-ads:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
> +    compile "com.android.support:support-annotations:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"

This should be behind build flags to be only included if really needed.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

https://reviewboard.mozilla.org/r/139082/#review142884

::: mobile/android/base/AndroidManifest.xml.in:488
(Diff revision 2)
> +        <service android:name="com.leanplum.LeanplumPushListenerService" android:exported="false"
> +            android:enabled="false">
> +            <intent-filter>
> +                <action android:name="com.google.android.c2dm.intent.RECEIVE" />
> +            </intent-filter>
> +        </service>

Are you sure this is correct? We already define a receiver for this if MOZ_GCM is enabled. I had the impression that we would need a custom receiver that will route the received messages either to leanplum or our web push implementation. However I can't find this in the leanplum docs.

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8867535 [details]
Bug 1351585 - Part 5. Add build flag to inject mma implementation.

https://reviewboard.mozilla.org/r/139088/#review142892

::: mobile/android/base/moz.build:1344
(Diff revision 2)
>  <span class="indent">>>>></span>    CONFIG['ANDROID_PLAY_SERVICES_GCM_AAR_LIB'],
>  <span class="indent">>>>></span>    CONFIG['ANDROID_SUPPORT_ANNOTATIONS_JAR_LIB'],
>  <span class="indent">>>>></span>    CONFIG['ANDROID_SUPPORT_V4_AAR_LIB'],
>  <span class="indent">>>>></span>    CONFIG['ANDROID_SUPPORT_V4_AAR_INTERNAL_LIB'],
>  <span class="indent">>>>></span>    CONFIG['ANDROID_APPCOMPAT_V7_AAR_LIB'],
>  <span class="indent">>>>></span>    CONFIG['ANDROID_PLAY_SERVICES_ADS_AAR_LIB'],

this one doesn't seem to use at least play-services-location like the gradle build?

Comment 27

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review commit: https://reviewboard.mozilla.org/r/139544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139544/

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1351585 - Part 2. Change our existing build system to reference leanplum

***
Bug 1351585 - Part 5. Add build flag to inject mma implementation. r?maliu,sebastian,nalexander

Review commit: https://reviewboard.mozilla.org/r/139546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139546/

Comment 29

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1351585 - Part 3. Add proguard for leanplum.

Review commit: https://reviewboard.mozilla.org/r/139548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139548/

Comment 30

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1351585 - Part 4. Use Switchboard to limit Leanplum startup/init

Review commit: https://reviewboard.mozilla.org/r/139550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139550/

Comment 31

[Nick Alexander :nalexander [he/him]]

Attachment: fix ddce5b17 o draft Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review commit: https://reviewboard.mozilla.org/r/139552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139552/

Comment 32

[Nick Alexander :nalexander [he/him]]

Attachment: fix 1b3f2cf0 o draft Bug 1351585 - Part 2. Change our existing build system to reference leanplum

Review commit: https://reviewboard.mozilla.org/r/139554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139554/

Comment 33

[Nick Alexander :nalexander [he/him]]

nechen: I've pushed back a version of your sequence with your pieces folded and re-ordered, and some changes on top.  Both moz.build and Gradle build for me, but I haven't really thought through everything, and I haven't tried to launch it or anything.  It should unblock you for the rest of the day, though.  Please make your changes and trigger try builds (android-api-15 and android-api-15-gradle, please).

(There's a commit for Bug 1365089 hidden underneath that you'll need to pull in to make this actually work.)

Comment 34

[Max Liu [:maliu]]

Comment on attachment 8868000 [details]
fix 1b3f2cf0 o draft Bug 1351585 - Part 2. Change our existing build system to reference leanplum

https://reviewboard.mozilla.org/r/139554/#review143114

::: mobile/android/base/moz.build:133
(Diff revision 1)
>  x.script = 'generate_build_config.py:generate_java'
>  x.inputs += ['AdjustConstants.java.in']
>  y = GENERATED_FILES['generated/preprocessed/org/mozilla/gecko/AppConstants.java']
>  y.script = 'generate_build_config.py:generate_java'
>  y.inputs += ['AppConstants.java.in']
> -
> +y = GENERATED_FILES['generated/preprocessed/org/mozilla/gecko/MmaConstants.java']

y object is replaced here. How do the w,x,y,z things work? Is it doesn't matter whether w,x,y,z are been replaced? If not, how about replace them all with a same var to avoid misunderstanding?

Comment 35

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8868000 [details]
fix 1b3f2cf0 o draft Bug 1351585 - Part 2. Change our existing build system to reference leanplum

https://reviewboard.mozilla.org/r/139554/#review143114

> y object is replaced here. How do the w,x,y,z things work? Is it doesn't matter whether w,x,y,z are been replaced? If not, how about replace them all with a same var to avoid misunderstanding?

Correct: `y` is just a reference to the `GeneratedFile` instance in that collection.  I thought about calling them `x1, ..., x5`, or making them all `x`.  If you care, we can do that in this patch.  Numbering or labeling them isn't great for alphabetical ordering; having them all the same looks a little odd.

Comment 36

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/4-5/

Comment 37

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139082/diff/2-3/

Comment 38

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139084/diff/2-3/

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

https://reviewboard.mozilla.org/r/139082/#review143654

A couple of addition comments, but otherwise this looks good to me.  Please do any additional Leanplum integration work in a separate patch or a separate ticket.

::: mobile/android/base/MmaConstants.java.in:26
(Diff revision 3)
> +    null;
> +//#endif
> +
> +public static final String MOZ_LEANPLUM_APP_ID =
> +//#ifdef MOZ_LEANPLUM_APP_ID
> +    "@MOZ_LEANPLUM_APP_ID@";

This will be `MOZ_LEANPLUM_SDK_CLIENTID` after the patches in https://reviewboard.mozilla.org/r/139456/diff/2#index_header land, so can you rebase onto that?

::: mobile/android/base/generate_build_config.py:48
(Diff revision 3)
>                  'MOZ_ANDROID_DOWNLOADS_INTEGRATION',
>                  'MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE',
>                  'MOZ_ANDROID_EXCLUDE_FONTS',
>                  'MOZ_ANDROID_GCM',
>                  'MOZ_ANDROID_MLS_STUMBLER',
> +                'MOZ_ANDROID_MMA',

You'll need to add the `MOZ_LEANPLUM_*` stuff here too.

Comment 40

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

https://reviewboard.mozilla.org/r/139082/#review143660

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:26
(Diff revision 3)
> +    public void init(Application application) {
> +        Leanplum.setApplicationContext(application);
> +        Parser.parseVariables(application);
> +        LeanplumActivityHelper.enableLifecycleCallbacks(application);
> +
> +        Leanplum.setAppIdForDevelopmentMode(MmaConstants.MOZ_LEANPLUM_APP_ID, MmaConstants.MOZ_LEANPLUM_SDK_KEY);

This should be guarded by `AppConstants.MOZILLA_OFFICIAL`.  You want to set production for official builds.

See https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in?q=path%3AAppConstants%2Ajava%2A&redirect_type=single#263 and usages in the tree.

Comment 41

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/5-6/

Comment 42

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139082/diff/3-4/

Comment 43

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139084/diff/3-4/

Comment 44

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351585 - Part 4. Add pref to turn on/off leanplum.

Review commit: https://reviewboard.mozilla.org/r/140656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140656/

Comment 45

[Max Liu [:maliu]]

Comment on attachment 8868979 [details]
Bug 1351585 - Part 4. Add pref to turn on/off leanplum.

https://reviewboard.mozilla.org/r/140656/#review144248

::: mobile/android/app/mobile.js:422
(Diff revision 1)
> +#if MOZ_UPDATE_CHANNEL == nightly
> +pref("mma.enabled", true);
> +#elif MOZ_UPDATE_CHANNEL == beta
> +pref("mma.enabled", true);
> +#else
> +pref("mma.enabled", true);

All true in these scenario?

Comment 46

[Max Liu [:maliu]]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

https://reviewboard.mozilla.org/r/138964/#review144250

Comment 47

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8868979 [details]
Bug 1351585 - Part 4. Add pref to turn on/off leanplum.

https://reviewboard.mozilla.org/r/140656/#review144270

I don't think using a Gecko pref for this makes sense.  First, can you _disable_ Leanplum while Gecko is running?  What does that mean?  Does the browser stop receiving events?  Will we turn off any options that we have had Leanplum toggle remotely for us?

As written, the Prefs stuff is invoked from http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#637.  That's well before Gecko is running.  Are you confident your prefs listener makes sense at that point?  I'm not!

You should have Jim Chen (:darchons) review this patch.

Comment 48

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138964/diff/6-7/

Comment 49

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867532 [details]
Bug 1351585 - Part 2. Add build flag and change our existing build system to inject leanplum implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139082/diff/4-5/

Comment 50

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139084/diff/4-5/

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01942414ab89
Part 1. Add Leanplum SDK source code to thirdparty module r=maliu
https://hg.mozilla.org/integration/autoland/rev/a1e493554517
Part 2. Add build flag and change our existing build system to inject leanplum implementation r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2f4c52ef4a4b
Part 3. Add proguard for leanplum. r=maliu

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01942414ab89
https://hg.mozilla.org/mozilla-central/rev/a1e493554517
https://hg.mozilla.org/mozilla-central/rev/2f4c52ef4a4b

Comment 53

[Nick Alexander :nalexander [he/him]]

This was not ready to land: it depends on Bug 1365089, which needs additional work.

nechen: the only reason this doesn't break the build is that everything is opt-in, and without Bug 1365089, nothing is setting any of the build flags.  So I guess this can stay in and wait until we enable everything, but that's not what I expected.  Is that what you were trying to achieve?

Comment 54

[Nevin Chen(Not active on Bugzilla)]

Yes! Exactly! Thanks for your full support on this. Deeply appreciated!

Comment 55

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

Confused. This already landed but there are still review flags? Clearing..

Comment 59

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

This has landed.

Comment 60

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

This has landed.

Comment 61

[Nick Alexander :nalexander [he/him]]

maliu: you flagged a bunch of patches for re-review.  Accident?