Code review of open-source Leanplum SDK

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: sebastian, Assigned: cnevinchen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [LP_M1])

Attachments

(9 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
(Reporter)

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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!
Flags: needinfo?(nalexander)
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,
Flags: needinfo?(max)

Comment 16

2 years ago
(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?
Flags: needinfo?(cnevinchen)

Comment 17

2 years ago
mozreview-review
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?
(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.
Flags: needinfo?(cnevinchen)

Comment 19

2 years ago
mozreview-review
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

2 years ago
mozreview-review
Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

https://reviewboard.mozilla.org/r/139084/#review142818
Attachment #8867533 - Flags: review?(max) → review+

Comment 21

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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?
(Reporter)

Comment 23

2 years ago
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. :)
(Reporter)

Comment 24

2 years ago
mozreview-review
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.
(Reporter)

Comment 25

2 years ago
mozreview-review
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.
(Reporter)

Comment 26

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.)
Flags: needinfo?(nalexander)
Attachment #8867424 - Flags: review?(nalexander)
Attachment #8867532 - Flags: review?(nalexander)
Attachment #8867533 - Flags: review?(nalexander)
Attachment #8867535 - Flags: review?(nalexander)
Attachment #8867995 - Flags: review?(nalexander)
Attachment #8867996 - Flags: review?(nalexander)
Attachment #8867997 - Flags: review?(nalexander)
Attachment #8867999 - Flags: review?(nalexander)
Attachment #8868000 - Flags: review?(nalexander)

Comment 34

2 years ago
mozreview-review
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

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8867534 - Attachment is obsolete: true
Attachment #8867534 - Flags: review?(s.kaspari)
Attachment #8867534 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867535 - Attachment is obsolete: true
Attachment #8867535 - Flags: review?(s.kaspari)
Attachment #8867535 - Flags: review?(max)

Comment 39

2 years ago
mozreview-review
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.
Attachment #8867532 - Flags: review?(nalexander) → review+

Comment 40

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8868000 - Attachment is obsolete: true
Attachment #8868000 - Flags: review?(s.kaspari)
Attachment #8868000 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867999 - Attachment is obsolete: true
Attachment #8867999 - Flags: review?(s.kaspari)
Attachment #8867999 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867998 - Attachment is obsolete: true
Attachment #8867998 - Flags: review?(s.kaspari)
Attachment #8867998 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867997 - Attachment is obsolete: true
Attachment #8867997 - Flags: review?(s.kaspari)
Attachment #8867997 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867996 - Attachment is obsolete: true
Attachment #8867996 - Flags: review?(s.kaspari)
Attachment #8867996 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Attachment #8867995 - Attachment is obsolete: true
Attachment #8867995 - Flags: review?(s.kaspari)
Attachment #8867995 - Flags: review?(max)
Comment hidden (mozreview-request)

Comment 45

2 years ago
mozreview-review
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

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

https://reviewboard.mozilla.org/r/138964/#review144250
Attachment #8867424 - Flags: review?(max) → review+

Comment 47

2 years ago
mozreview-review
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.
Attachment #8868979 - Flags: review-
(Assignee)

Updated

2 years ago
Attachment #8868979 - Attachment is obsolete: true
Attachment #8868979 - Flags: review?(s.kaspari)
Attachment #8868979 - Flags: review?(max)
(Assignee)

Updated

2 years ago
Assignee: nobody → cnevinchen
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

2 years ago
Pushed by nechen@mozilla.com:
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
Keywords: checkin-needed

Comment 52

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01942414ab89
https://hg.mozilla.org/mozilla-central/rev/a1e493554517
https://hg.mozilla.org/mozilla-central/rev/2f4c52ef4a4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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?
Flags: needinfo?(cnevinchen)
Yes! Exactly! Thanks for your full support on this. Deeply appreciated!
Flags: needinfo?(cnevinchen)
(Reporter)

Comment 55

2 years ago
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..
Attachment #8867424 - Flags: review?(s.kaspari)
(Reporter)

Updated

2 years ago
Attachment #8867532 - Flags: review?(s.kaspari)
(Reporter)

Updated

2 years ago
Attachment #8867533 - Flags: review?(s.kaspari)
Duplicate of this bug: 1351573
Duplicate of this bug: 1351583
Duplicate of this bug: 1351581
Priority: -- → P1

Updated

2 years ago
Attachment #8867532 - Flags: review?(max)
Comment on attachment 8867533 [details]
Bug 1351585 - Part 3. Add proguard for leanplum.

This has landed.
Attachment #8867533 - Flags: review?(nalexander)
Comment on attachment 8867424 [details]
Bug 1351585 - Part 1. Add Leanplum SDK source code to thirdparty module

This has landed.
Attachment #8867424 - Flags: review?(nalexander)

Updated

2 years ago
Attachment #8867995 - Attachment is obsolete: false
Attachment #8867996 - Attachment is obsolete: false
Attachment #8867997 - Attachment is obsolete: false
Attachment #8867998 - Attachment is obsolete: false
Attachment #8867999 - Attachment is obsolete: false
Attachment #8868000 - Attachment is obsolete: false
Attachment #8867995 - Flags: review?(s.kaspari)
Attachment #8867995 - Flags: review?(nalexander)
Attachment #8867996 - Flags: review?(s.kaspari)
Attachment #8867996 - Flags: review?(nalexander)
Attachment #8867997 - Flags: review?(s.kaspari)
Attachment #8867997 - Flags: review?(nalexander)
Attachment #8867998 - Flags: review?(s.kaspari)
Attachment #8867999 - Flags: review?(s.kaspari)
Attachment #8867999 - Flags: review?(nalexander)
Attachment #8868000 - Flags: review?(s.kaspari)
Attachment #8868000 - Flags: review?(nalexander)
maliu: you flagged a bunch of patches for re-review.  Accident?
Attachment #8867995 - Flags: review?(s.kaspari)
Attachment #8867995 - Flags: review?(nalexander)
Attachment #8867996 - Flags: review?(s.kaspari)
Attachment #8867996 - Flags: review?(nalexander)
Attachment #8867997 - Flags: review?(s.kaspari)
Attachment #8867997 - Flags: review?(nalexander)
Attachment #8867998 - Flags: review?(s.kaspari)
Attachment #8867999 - Flags: review?(s.kaspari)
Attachment #8867999 - Flags: review?(nalexander)
Attachment #8868000 - Flags: review?(s.kaspari)
Attachment #8868000 - Flags: review?(nalexander)

Updated

a year ago
Whiteboard: [LP_M2]

Updated

a year ago
Whiteboard: [LP_M2] → [LP_M1]

Updated

11 months ago
Flags: needinfo?(max)
You need to log in before you can comment on or make changes to this bug.