1372091 - Turn off Leanplum on Beta and release

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: Metrics, defect)

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Per discussion with releng.

Comment 2

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1372091 - Turn off Leanplum on Beta and release.

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

Comment 3

[Max Liu [:maliu]]

Comment on attachment 8876611 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.

https://reviewboard.mozilla.org/r/147930/#review152272

::: mobile/android/app/mobile.js:417
(Diff revision 1)
>  pref("ui.bookmark.mobilefolder.enabled", true);
>  #else
>  pref("ui.bookmark.mobilefolder.enabled", false);
>  #endif
>  
> -#if MOZ_UPDATE_CHANNEL == nightly
> +#ifndef RELEASE_OR_BETA

if"""n"""def !?

::: mobile/android/app/mobile.js:417
(Diff revision 1)
>  pref("ui.bookmark.mobilefolder.enabled", true);
>  #else
>  pref("ui.bookmark.mobilefolder.enabled", false);
>  #endif
>  
> -#if MOZ_UPDATE_CHANNEL == nightly
> +#ifndef RELEASE_OR_BETA

if"""n"""def !?

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Hi Tom.
Could you please help review this?
Since today is merge day, could you please help?

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8876611 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.

https://reviewboard.mozilla.org/r/147930/#review152288

I agree with Max that the mobile.js change looks wrong (why not simplify to a if channel == nightly: true pref, otherwise false pref ?). I'm not a legit reviewer for these changes, so I'm going to clear the request. You'll have to request uplift to beta as the merge process has already begun.

Comment 6

[Carsten Book [:Tomcat]]

looks fine for me but i'm not a expert in this area so i cannot give r+ or so in this case

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8876611 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.

https://reviewboard.mozilla.org/r/147930/#review152288

I've fixed the error.
I want to make bebug build also have this pref on, so I add the extra check.

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8876611 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.

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

Comment 9

[Gerry Chang [:gchang]]

Mark 55 as blocking as we have to turn off the Leanplum on beta/release and this has to be in 55.0b1.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Nevin, Snorp, I was under the assumption that LeanPlum and other bits of code for MMA SDK were behind a pref/switchboard flag and therefore not riding to Beta55. Is that incorrect? We are currently blocking on gtb beta1 until this is confirmed. Thanks!

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8876611 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.

https://reviewboard.mozilla.org/r/147930/#review152508

This looks correct to me.

::: mobile/android/app/mobile.js:417
(Diff revision 2)
>  pref("ui.bookmark.mobilefolder.enabled", true);
>  #else
>  pref("ui.bookmark.mobilefolder.enabled", false);
>  #endif
>  
> -#if MOZ_UPDATE_CHANNEL == nightly
> +#ifdef RELEASE_OR_BETA

It's so complicated to differentiate these channels, but my reading suggests this is correct.

::: mobile/android/config/mozconfigs/common:71
(Diff revision 2)
>  fi
>  
>  # MOZ_ANDROID_MMA depends on --with-leanplum-sdk-keyfile, and there's no default
>  # keyfile set, so if we misconfigure beta or release, the builds will fail (at
>  # configure time).
> -if test "$MOZ_UPDATE_CHANNEL" = "release" ; then
> +if test "$MOZ_UPDATE_CHANNEL" = "nightly" ; then

It's always safe to set the Leanplum SDK keyfile, and it serves to document what files are expected to be where, so don't remove this block.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1372091 - Turn off Leanplum on Beta and release. a=ritu

This disables the compiled (Java) and interpreted (JavaScript) code
for mozilla-beta.

I cannot at this time determine a /nightly mozconfig that enables the
compiled (Java) code in Nightly and developer builds while disabling
it in Release and Beta.

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

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #12)
> Created attachment 8876864 [details]
> Bug 1372091 - Turn off Leanplum on Beta and release.  a=ritu
> 
> This disables the compiled (Java) and interpreted (JavaScript) code
> for mozilla-beta.
> 
> I cannot at this time determine a /nightly mozconfig that enables the
> compiled (Java) code in Nightly and developer builds while disabling
> it in Release and Beta.
> 
> Review commit: https://reviewboard.mozilla.org/r/148188/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/148188/

ritu: RyanVM: this applies to mozilla-beta; I can't determine a cross-tree solution (see commit comment).

This disables both the JS and the Java sides, which seems like the right way to turn the feature off (if we're really trying to turn it off).  I don't know why Nevin didn't do this in his approach.

NI only to ritu, who can work with RyanVM to land on his schedule.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ddde9a807752

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8876864 [details]
Bug 1372091 - Turn off Leanplum on Beta and release.  a=ritu

I reviewed the initial patch, and produced the follow-up, which has landed.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Nick for helping with this one so promptly!

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Hi Nick!
Thanks for your prompt support as always!

Hi Ritu
You are right. Switchboard can turn off Leanplum.
And this bug is to turn off Leanplum using a Gecko preferene even before Switchboard initialization.(Double safety)

The original idea is to let Leanplum still works at Nightly.
I’m not sure it will still be the case after Nick’s patch.
If so, I’d rather just use switchboard to turn off Leanplum on Beta cause we still want it on Nightly.

Comment 18

[Nick Alexander :nalexander [he/him]]

> The original idea is to let Leanplum still works at Nightly.
> I’m not sure it will still be the case after Nick’s patch.
> If so, I’d rather just use switchboard to turn off Leanplum on Beta cause we
> still want it on Nightly.

The patch has landed on mozilla-beta only, so Nightly should work as it does now.  Of course, we'll need to do this dance again for the _next_ uplift, but hopefully we can improve the situation in the meantime.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

As we discussed on IRC yesterday, I'm still hoping we can come up with a better option than having to manually land a patch on Beta post-uplift at the beginning of each cycle :)

Comment 20

[Carsten Book [:Tomcat]]

Hi Max,
i can't review this change since i don't know the code.

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Leanplum should go to beta now. I think this bug could be closed.

BTW, I can't find Leanplum code in Beta APK (even the newly-56 added package org.mozilla.gecko.mma ), do you know whom I should speak to?

Thank you!

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Oh.. My problem was Beta was still on 55 yesterday. Everything is normal now.

Comment 23

[Nevin Chen(Not active on Bugzilla)]

Leanplum is on beta 56 so we don't need this bug anymore.