1269734 - Include adjust campaign ID with core ping

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Metrics, defect)

Description

[:Margaret Leibovic]

This would allow us to understand how marketing campaigns correlate with retention (among other things).

mfinkle said iOS already does this. bnicholson, do you have some code (or Adjust docs) you can point us to to show us how you did this?

Comment 1

[Brian Nicholson (:bnicholson)]

iOS isn't doing this yet :(

Comment 2

[:Margaret Leibovic]

13:51 <•mfinkle> bnicholson, margaret: st3fan had said that ios is saving the "attribution" from adjust in a pref
13:51 <•mfinkle> and that the data would be used to post in core ping, when the time came

Let's try to save the "attribution" in a shared pref, and include this in the core ping.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Margaret, will anyone be looking at the telemetry data in relation to the adjust campaign ID? Is there someone else we also need to confirm with?

Comment 4

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #3)
> Margaret, will anyone be looking at the telemetry data in relation to the
> adjust campaign ID? Is there someone else we also need to confirm with?

Asking Barbara and Alex. If neither of them are going to look at this, then I don't think it's a high priority.

Comment 5

[Barbara Bermes [:barbara] - NI please!]

I will want to look at this, I really think for all telemetry related remaining stuff, it's better to get it in before the eng resource shift.

Comment 6

[Alex Davis [:adavis]]

I believe we will use it and ultimately, it might just be the start of getting more of our Adjust data into our own telemetry via their callbacks. (https://docs.adjust.com/en/callbacks/)

Comment 7

[Jonathan Almeida [:jonalmeida]]

Talked to mcomella, I'm going to take a swing at this instead.

Comment 8

[:Margaret Leibovic]

Jonathan, have you made any progress on this?

Comment 9

[Jonathan Almeida [:jonalmeida]]

I just had some testing with my patch when I last left it before work week. Pushing it for review to sebastian when he gets a chance.

Comment 10

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1269734 - Include adjust campaign ID with core ping

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

Comment 11

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/59838/#review56798

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:308
(Diff revision 1)
>      // race by determining if the web content should be hidden at the animation's end.
>      private boolean mHideWebContentOnAnimationEnd;
>  
>      private final DynamicToolbar mDynamicToolbar = new DynamicToolbar();
>  
> +    // Holding a reference here so that we can pass it to the AdjustHelper.

I pass the reference because I've added my listener here. The alternative would be to pass hold on to the Context passed to the AdjustHelper and using that to save the attribution to prefs there. 

I wasn't sure what sort of memory issue this may cause, so I chose the less ideal looking, but safer approach. I wouldn't mind an alternative solution if there might be one.

Comment 12

[Jonathan Almeida [:jonalmeida]]

s/to pass hold/to hold/

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

https://reviewboard.mozilla.org/r/59838/#review57386

This looks good to me. But I do not know much about adjust. If you are not in a hurry maybe it makes sense to let mcomella have a look next week too.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:63
(Diff revision 1)
>          new AdjustReferrerReceiver().onReceive(context, intent);
>      }
> +
> +    @Override
> +    public void onAttributionChanged(AdjustAttribution attribution) {
> +        if (attributionListener == null || attribution == null) return;

nit: We always use braces even for single statement if blocks.

Comment 14

[Jonathan Almeida [:jonalmeida]]

Yes, it can wait since bug 1277407 has been backed out. Will fix the nit till then.

Comment 15

[:Margaret Leibovic]

Jonathan and Mike, are you planning on finishing this? We should let Barbara know the status.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #15)
> Jonathan and Mike, are you planning on finishing this? We should let Barbara
> know the status.

Spoke with Jonathan – this is ready to land via Sebastian's review but I'm going to take a look at it first.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

https://reviewboard.mozilla.org/r/59838/#review60462

This looks like it should work for the main use case but:
 1) It's not following the design of the existing code – see my comment in CorePingBuilder. If the design wasn't clear before, I'd be curious if you have suggestions on how we can make it so!
 2) I'm not sure it works in the case that the attribution callback hasn't been called yet.

Plus a few nits.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:812
(Diff revision 1)
>  
>      private static void initTelemetryUploader(final boolean isInAutomation) {
>          TelemetryUploadService.setDisabled(isInAutomation);
>      }
>  
> -    private static void initAdjustSDK(final Context context, final boolean isInAutomation) {
> +    private static void initAdjustSDK(final Context context, final boolean isInAutomation, TelemetryCorePingDelegate delegate) {

nit: You can take the AttributionListener here.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:63
(Diff revision 1)
>          new AdjustReferrerReceiver().onReceive(context, intent);
>      }
> +
> +    @Override
> +    public void onAttributionChanged(AdjustAttribution attribution) {
> +        if (attributionListener == null || attribution == null) return;

As Sebastian says.

I'd expect running `./mach gradle checkstyle` to catch this.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AttributionHelperListener.java:9
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.adjust;
> +
> +/**
> + * A callback Used along with the AdjustHelper for notifying when an Adjust Attribution

nit: explain why this needs to exist if the `OnAttributionChangedListener` already exists.

::: mobile/android/base/java/org/mozilla/gecko/adjust/StubAdjustHelper.java:11
(Diff revision 1)
> -public class StubAdjustHelper implements AdjustHelperInterface {
> -    public void onCreate(final Context context, final String appToken) {
> +import com.adjust.sdk.AdjustAttribution;
> +import com.adjust.sdk.OnAttributionChangedListener;

Did you build with mach? When we use `StubAdjustHelper`, there are supposed to be no traces of Adjust but including these imports gets around that – I'm suprised the restriction is not enforced by the build system.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryConstants.java:16
(Diff revision 1)
>      public static final boolean UPLOAD_ENABLED = AppConstants.MOZILLA_OFFICIAL; // Disabled for developer builds.
>  
>      public static final String USER_AGENT =
>              "Firefox-Android-Telemetry/" + AppConstants.MOZ_APP_VERSION + " (" + AppConstants.MOZ_APP_UA_NAME + ")";
>  
> +    public static final String PREF_SERVER_URL = "telemetry-serverUrl";

nit: unused

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryConstants.java:17
(Diff revision 1)
>  
>      public static final String USER_AGENT =
>              "Firefox-Android-Telemetry/" + AppConstants.MOZ_APP_VERSION + " (" + AppConstants.MOZ_APP_UA_NAME + ")";
>  
> +    public static final String PREF_SERVER_URL = "telemetry-serverUrl";
> +    public static final String PREF_CAMPAIGN_ID = "campaignId";

nit: These constants are specific to the core ping (for now) and should be in the core ping classes (or the new `Measurements` class if you agree with my approach).

Large `*Constants` classes are generally bad practice because once they grow to a certain size, it's hard to find anything relevant in them. I've been doing my best to reduce the size of this class.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java:157
(Diff revision 1)
>                          .setDefaultSearchEngine(TelemetryCorePingBuilder.getEngineIdentifier(engine))
>                          .setProfileCreationDate(TelemetryCorePingBuilder.getProfileCreationDate(activity, profile))
>                          .setSequenceNumber(TelemetryCorePingBuilder.getAndIncrementSequenceNumber(sharedPrefs))
>                          .setSessionCount(sessionMeasurementsContainer.sessionCount)
> -                        .setSessionDuration(sessionMeasurementsContainer.elapsedSeconds);
> +                        .setSessionDuration(sessionMeasurementsContainer.elapsedSeconds)
> +                        .setOptCampaignId(activity);

This should occur in the `maybeSetOptionalMeasurements` method.

I separated them for visual & cognitive consistency.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:162
(Diff revision 1)
> +        final String campaignId = GeckoSharedPrefs.forProfile(context)
> +                .getString(TelemetryConstants.PREF_CAMPAIGN_ID, null);

nit: The `set*` methods in this class do exactly that – they set their arguments (which they must validate first). This method breaks that convention – it also performs an action to get the value.

This may seem nit-picky but it's my opinion it helps good design – it's the single-responsibility principle. Essentially, each component of the software should do one thing and one thing only. This class sets fields in a telemetry ping, while other components manage and access those values. That way when something goes wrong, you generally have some idea of which components won't be involved (e.g. I won't have to look in the Builder – it's pretty dumb!).

The single responsibility principle can encourage you to rethink your design – e.g. in this case, you access the value in `TelemetryCorePingBuilder` and set it via a listener in `CorePingDelegate`. What if we were to make a separate class that managed both setting and getting the value? Would that be clearer? Then, this class can actually act as the listener you passed in and you remove the dependency we currently have on `CorePingDelegate`, which contains a lot of unrelated code to what we're trying to do here.

A sample of this approach would be `SearchCountMeasurements` & `SessionMeasurements`. Try to identify the complexity if the logic in these classes was dumped into separate locations, e.g. `CorePingDelegate` & the builder.

To summarize my ideal solution (but not necessarily the best if you feel otherwise): we add a `telemetry.measurements.CampaignAttributionMeasurement` which is the attribution listener (and thus the setter). It also includes the getter, which  `CorePingDelegate` accesses to set the value in setOptCampaignId.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:164
(Diff revision 1)
>      }
>  
> +    public TelemetryCorePingBuilder setOptCampaignId(final Context context) {
> +        final String campaignId = GeckoSharedPrefs.forProfile(context)
> +                .getString(TelemetryConstants.PREF_CAMPAIGN_ID, null);
> +        Log.d(LOGTAG, "Got adjust campaignId from prefs: " + campaignId);

nit: I get the feeling this is private data – we shouldn't log it.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:165
(Diff revision 1)
>  
> +    public TelemetryCorePingBuilder setOptCampaignId(final Context context) {
> +        final String campaignId = GeckoSharedPrefs.forProfile(context)
> +                .getString(TelemetryConstants.PREF_CAMPAIGN_ID, null);
> +        Log.d(LOGTAG, "Got adjust campaignId from prefs: " + campaignId);
> +        if (campaignId == null) {

Would TextUtils.isEmpty be more appropriate here? Do we want to accept "" too?

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:166
(Diff revision 1)
> +    public TelemetryCorePingBuilder setOptCampaignId(final Context context) {
> +        final String campaignId = GeckoSharedPrefs.forProfile(context)
> +                .getString(TelemetryConstants.PREF_CAMPAIGN_ID, null);
> +        Log.d(LOGTAG, "Got adjust campaignId from prefs: " + campaignId);
> +        if (campaignId == null) {
> +            throw new IllegalStateException("Received empty string.");

What happens if the listener hasn't received the callback yet? Will we crash? What is the expected value in that case? We should include some comments explaining that case.

Comment 18

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/59838/#review60462

> Would TextUtils.isEmpty be more appropriate here? Do we want to accept "" too?

Yes, an empty string would be fine as long as it's non-null.

> What happens if the listener hasn't received the callback yet? Will we crash? What is the expected value in that case? We should include some comments explaining that case.

I've changed this to check for a non-null campaign ID instead.

Comment 19

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1269734 - Changes based on previous review for campaign ID retrieval

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

Comment 20

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

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

Comment 21

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

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

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

https://reviewboard.mozilla.org/r/59838/#review64558

Almost there! We should make sure we're not adding a memory leak + a few nits.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:828
(Diff revision 3)
>          final boolean enabled = !isInAutomation &&
>                  prefs.getBoolean(GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true);
>          adjustHelper.setEnabled(enabled);
>      }
>  
> +    private AttributionHelperListener addAttributionHelperListener() {

nit: We're not adding an attribution helper listener here – we're creating one and returning it.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:829
(Diff revision 3)
>                  prefs.getBoolean(GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true);
>          adjustHelper.setEnabled(enabled);
>      }
>  
> +    private AttributionHelperListener addAttributionHelperListener() {
> +        return new AttributionHelperListener() {

This creates an anonymous reference to `this`, which is BrowserApp, and will cause a memory leak.

Did you mean to use the TelemetryCorePingDelegate instance instead? This update camapign ID pref code is actually duplicated there.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AttributionHelperListener.java:9
(Diff revision 3)
> + * We use this listener to notify when to store the campaign ID when we receive
> + * the attribution from the OnAttributionListener in the Adjust SDK. We can't directly access
> + * the attribution since we don't always compile against the Adjust SDK and use a stub
> + * in those cases.

nit: This is the javadoc for the interface, but you're describing an implementation of the interface: `to notify when to store the campaign ID`.

I think what's important to highlight in this javadoc is that we can't reference Adjust's `OnAttributionChangedListener` outside of `AdjustHelper`, due to build module dependencies. Additionally, any code in `AdjustHelper` can't access fennec. Instead, we pass this to the `AdjustHelper` so any code we'd write in Adjust's listener, we write in here instead.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java:166
(Diff revision 3)
>              }
>          });
>      }
>  
> -    private void maybeSetOptionalMeasurements(final SharedPreferences sharedPrefs, final TelemetryCorePingBuilder pingBuilder) {
> +    private void maybeSetOptionalMeasurements(final SharedPreferences sharedPrefs, final TelemetryCorePingBuilder pingBuilder,
> +                                              final BrowserApp activity) {

nit: This only requires a `Context`, not `BrowserApp`, right?

Also, I like to put the most significant arguments first in the argument list and, because Context is a god object, it always goes first.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/CampaignIdMeasurements.java:21
(Diff revision 3)
> +/**
> + * A class to retrieve and store the campaign Id pref that is used when the Adjust SDK gives us
> + * new attribution from the {@link AttributionHelperListener}.
> + */
> +public class CampaignIdMeasurements {
> +    private static final String PREF_CAMPAIGN_ID = "campaignId";

nit: I'd make this more specific for a lower chance of collisions – e.g. `measurements-campaignId`. You don't know who might be storing data later!

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/CampaignIdMeasurements.java:23
(Diff revision 3)
> + * new attribution from the {@link AttributionHelperListener}.
> + */
> +public class CampaignIdMeasurements {
> +    private static final String PREF_CAMPAIGN_ID = "campaignId";
> +
> +    public static String getCampaignIdFromPrefs(@NonNull final BrowserApp activity) {

nit: You only need `Context` here right? Here and below.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:20
(Diff revision 3)
>  import android.text.TextUtils;
>  
>  import android.util.Log;
>  import org.mozilla.gecko.AppConstants;
>  import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.GeckoSharedPrefs;

nit: unused import

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java:47
(Diff revision 3)
>  public class TelemetryCorePingBuilder extends TelemetryPingBuilder {
>      private static final String LOGTAG = StringUtils.safeSubstring(TelemetryCorePingBuilder.class.getSimpleName(), 0, 23);
>  
>      // For legacy reasons, this preference key is not namespaced with "core".
>      private static final String PREF_SEQ_COUNT = "telemetry-seqCount";
> +    private static final String PREF_CAMPAIGN_ID = "campaignId";

nit: This isn't a pref – this should be down with the other attribute names below.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Jonathan, you said you have an updated patch to upload, right? Do you think I should take over this bug or were you planning to complete it? Again, no pressure. :)

NI self to make sure this gets landed soon.

Comment 24

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

https://reviewboard.mozilla.org/r/59838/#review64558

> This creates an anonymous reference to `this`, which is BrowserApp, and will cause a memory leak.
> 
> Did you mean to use the TelemetryCorePingDelegate instance instead? This update camapign ID pref code is actually duplicated there.

Yes, this isn't what I intended initially. I've gone back to passing a reference of the TelemetryCorePingDelegate again, although looking at how the other deledates are used this doesn't seem like the ideal way to use it.

Comment 25

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

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

Comment 26

[Jonathan Almeida [:jonalmeida]]

Sorry about the slow progress on this bug, I haven't been as confident of my changes to this patch. I'm curious to see if this works as I expect it to with the limited testing I'm able to do with it.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #26)
> Sorry about the slow progress on this bug, I haven't been as confident of my
> changes to this patch.

No worries – do you know which version Barbara wanted this to go in?

> I'm curious to see if this works as I expect it to
> with the limited testing I'm able to do with it.

Can you elaborate a little bit? Do you mean you weren't able to test this version of the patch as thoroughly as the last version? Or that Adjust makes it difficult to test this patch and it's hard for you to know for sure if it will work without running it in with a real attribution?

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8763690 [details]
Bug 1269734 - Include adjust campaign ID with core ping

https://reviewboard.mozilla.org/r/59838/#review69980

Cool beans – looks good to me, provided our testing story is reasonable.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:64
(Diff revision 4)
>      }
> +
> +    @Override
> +    public void onAttributionChanged(AdjustAttribution attribution) {
> +        if (attributionListener == null || attribution == null) {
> +            return;

nit: We never expect `attributionListener` to be null, right? Best to fail early and throw if that's the case then.

Also, it could be good to log if the attribution is null. That way if we have trouble with attributions in prod, we at least have some debug ability.

Be sure not to log the content of the attribution variable because it may contain private data.

::: mobile/android/base/java/org/mozilla/gecko/adjust/AttributionHelperListener.java:9
(Diff revision 4)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.adjust;
> +
> +/**
> + * Because of how our build module dependencies is structured, we aren't able to use

nit: -> `are structured`

::: mobile/android/base/java/org/mozilla/gecko/adjust/AttributionHelperListener.java:11
(Diff revision 4)
> + * If the Adjust SDK is enabled, this listener will be notified when {@link com.adjust.sdk.OnAttributionChangedListener}
> + * is fired (i.e. this listener would be daisy-chained to the Adjust one)

nit: -> `should be notified`

-> `is expected to be daisy-chained`

The document is less misleading if someone changes the code later and this documentation gets out of date.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java:166
(Diff revision 4)
> -    private void maybeSetOptionalMeasurements(final SharedPreferences sharedPrefs, final TelemetryCorePingBuilder pingBuilder) {
> +    private void maybeSetOptionalMeasurements(final Context context,
> +                                              final SharedPreferences sharedPrefs,
> +                                              final TelemetryCorePingBuilder pingBuilder) {

nit: we generally don't use this formatting – we wrap on each line.

imo, it's not worth taking the time to change now (unless checkstyle barfs) but just fyi for future reference.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/CampaignIdMeasurements.java:17
(Diff revision 4)
> + * A class to retrieve and store the campaign Id pref that is used when the Adjust SDK gives us
> + * new attribution from the {@link AttributionHelperListener}.

nit: mention this is thread safe (which it is b/c sharedprefs is thread safe). We don't know when campaign IDs will come in in relation to when we pass it to the telemetry ping so it needs to be thread safe.

Comment 29

[Jonathan Almeida [:jonalmeida]]

(In reply to Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] from comment #27)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #26)
> > Sorry about the slow progress on this bug, I haven't been as confident of my
> > changes to this patch.
> 
> No worries – do you know which version Barbara wanted this to go in?

Barbara wanted this for 50 but I prefer it to be in 51 if possible to see if anything breaks, and if so there'll be time to fix it.

> > I'm curious to see if this works as I expect it to
> > with the limited testing I'm able to do with it.
> 
> Can you elaborate a little bit? Do you mean you weren't able to test this
> version of the patch as thoroughly as the last version? Or that Adjust makes
> it difficult to test this patch and it's hard for you to know for sure if it
> will work without running it in with a real attribution?

The latter really. I've tested it from a dev build side, but I'm not sure if the campaign ID will show up in the analytic side for it's actual use case as expected.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ed793a550dd6
Include adjust campaign ID with core ping r=mcomella

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed793a550dd6

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks for pushing this over the finish line, Jonathan.

Comment 33

[Ioana Chiorean]

Will this be pushed in Aurora too? It appears under version 50 in AHA Fennec board but I see it here under Milestone 51.

Comment 34

[Barbara Bermes [:barbara] - NI please!]

I moved the Aha card to 51 to reflect current approach/status