975239 - Home banner will only show one message per app lifetime (the one that was added first)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Josh Dover [:jdover]]

After changes in bug 960359, the HomeBanner will only poll for a new message once per app launch, meaning that the banner that shows is just a race of which banner is posted first by addons and snippets.

We should poll for a new banner at a sensible time, such as on a regular timer or every nth time the user visits the awesomescreen?

Comment 1

[:Margaret Leibovic]

Updating the summary to reflect the problem, rather than prescribe a solution, since I'm not sure what a good solution would be.

I'd like to get ibarlow's opinion on this... so, we allow consumers of the Home.banner API to add multiple messages to the banner (e.g. a set of active snippets), and presumably we'll rotate through those messages. In practice, when should we actually do that? On desktop Firefox, I believe they change the snippet every time the user re-opens about:home, but we show about:home a lot more frequently than desktop.

We could implement a solution that rotates messages while the app is alive, but we could also implement a solution that randomizes which of these messages we decide to show, and only show one per app lifetime. The problem with this approach is that it screws us over if anything decides to add a message after we've already requested a message from Gecko (e.g. updating the snippets while the browser is running), and this could be a problem on high-end devices where the app doesn't actually die in the background very often.

Comment 2

[:Margaret Leibovic]

I can own this.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

If polling is still a possible solution, we could try to do it when Fennec goes into the background. Not every time Fennec goes into the background, but after some predefined period of time.

Comment 4

[Richard Newman [:rnewman]]

A suggestion: 

* Pick a random snippet each time we cold-launch.
* Keep an in-memory timestamp for when we last picked one. (No prefs write.)
* Each onResume, check to see if it's been a while. If so...
* After some interval, fade in another snippet. (All background work.)

If the user stays in the app for ages and ages, yeah, we won't show you a fresh snippet. But next time you switch to a different app, you'll see something new when you come back to Firefox.

Comment 5

[Ian Barlow (:ibarlow)]

I think I agree with Richard, pretty much. What I was going to suggest is,

As long as a user *doesn't hit the close button* on the banner, we rotate the message they see once per day, or when Firefox is quit/killed and restarted, whichever comes first.

If they *do* hit close, then we hide the banner for a longer period of time (a few days or a week or something), and then resume with the above behaviour again once it reappears.

Comment 6

[:Margaret Leibovic]

Attachment: (Part 1) Choose a banner message at random instead of rotating through a queue

Instead of rotating through a queue of messages, this patch makes us just choose one to show at random.

However, this does not fix the problem. The root issue is that we send a "HomeBanner:Get" message when the HomeBanner view is created, which happens before gecko is up and running, and as soon as Gecko is running and the first banner is added, we send a "HomeBanner:Data" message with that first banner message. I started working on another patch to address this issue, which I'll post separately.

Comment 7

[:Margaret Leibovic]

Attachment: (Part 2 - WIP) Request a home banner message when the HomePager loads

Instead of requesting a message once when creating the HomeBanner view (which now only gets created once per app lifetime), this patch requests a new message every time we load the HomePager. If we think this now changes the message too frequently, we could have store some in-memory timestamp like rnewman suggested, and only send this message periodically.

I also experimented with getting rid of the _handleGet call in Home.banner.add, since that is what is causing the first-message-added race. However, without that call, the home banner won't appear when the HomePager is loaded before Gecko is running (i.e. startup), but maybe we're okay with that.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8385571 [details] [diff] [review]
(Part 2 - WIP) Request a home banner message when the HomePager loads

Review of attachment 8385571 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense to me; I think it's better to err on the side of changing the banner too frequently as opposed to not changing it often enough. This is especially true now that the selected message is random as messages could be unlucky.

Comment 9

[:Margaret Leibovic]

I accidentally started a conversation about this bug in bug 977155. Here's what ibarlow had to say about my concerns.

(In reply to :Margaret Leibovic from comment #16)
> Ian, how do you feel about the message rotating every time we create the
> home page? This actually matches the original banner behavior, although that
> didn't get much use in the wild, so maybe that design wasn't intentional :)

This part works for me. 

> However, we'll also want to think about how this will interact with the
> close button. With the patch I posted, if the user hits the close button,
> we'll remove that message from the rotation, but if there are other messages
> in the rotation, we'd end up showing one of those the next time the user
> opens about:home.

Yeah, we want to make sure that we are respecting the close button action here. If they hit close, it should mark the message as read *and* put a pause on other banners for some period of time.

Comment 10

[:Margaret Leibovic]

Attachment: (Part 2) Request a home banner message when the HomePager loads

I moved the message a bit higher in HomePager.load. I realized there is a bit of a race here that the HomePager could load with the old banner contents before the new banner contents get swapped in, but I'm not seeing that in practice. Not sure how much I should try to see if I can make that race happen.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 3) Refactor HomeBanner.handleMessage to handle being called multiple times

The way handleMessage is currently written, it isn't really designed to be called multiple times to update the state of the banner. For example, if we run into an exception parsing the JSON to get the title, we will have already set the id with a different banner message. Also, the current logic assumes the banner is hidden when handleMessage is called, but that's not necessarily the case now.

This patch refactors this logic to move all the UI updating inside one runnable.

I also wonder if we should file a follow-up to use Picasso here for the image loading. Although that would break the drawable:// URI I'm using for the sync promo banner.

Comment 12

[:Margaret Leibovic]

Attachment: (Part 4) Don't show the banner again for the rest of the app lifetime after the user taps the close button

This addresses the second part of ibarlow's feedback. With this patch, we won't show the banner again for the rest of the app lifetime after the user hits the close button.

I was trying to think of the least invasive way to do this, and I came up with removing the listener, but maybe you have a better idea.

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386396 [details] [diff] [review]
(Part 2) Request a home banner message when the HomePager loads

Review of attachment 8386396 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomePager.java
@@ +157,5 @@
>          mLoaded = true;
>          mInitialPanelId = panelId;
>  
> +        // Update the home banner message each time the HomePager is loaded.
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Get", null));

Rather than splitting the HomeBanner:Get event and the HomeBanner:Data listener between two classes, can you create an update method on HomeBanner that sends this internally?

Comment 14

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> Comment on attachment 8386396 [details] [diff] [review]
> (Part 2) Request a home banner message when the HomePager loads
> 
> Review of attachment 8386396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +157,5 @@
> >          mLoaded = true;
> >          mInitialPanelId = panelId;
> >  
> > +        // Update the home banner message each time the HomePager is loaded.
> > +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Get", null));
> 
> Rather than splitting the HomeBanner:Get event and the HomeBanner:Data
> listener between two classes, can you create an update method on HomeBanner
> that sends this internally?

Yes, that does sound like a better idea.

Comment 15

[:Margaret Leibovic]

Attachment: (Part 2 - v2) Request a home banner message when the HomePager loads

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386400 [details] [diff] [review]
(Part 3) Refactor HomeBanner.handleMessage to handle being called multiple times

Review of attachment 8386400 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this also addresses my setTag-off-the-UI-thread concerns from https://bugzilla.mozilla.org/show_bug.cgi?id=977155#c4.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386402 [details] [diff] [review]
(Part 4) Don't show the banner again for the rest of the app lifetime after the user taps the close button

Review of attachment 8386402 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, this goes back to the lifecycle issues from https://bugzilla.mozilla.org/show_bug.cgi?id=961523#c6: namely, the "app lifetime" on Android is pretty loosely defined. On older phones, we get killed pretty frequently whenever we're put into the background, so the closed banner may not last long.

The technique I described there -- storing the state in the bundle and restoring it when we're recreated -- would be one way to handle this. Having the banner last for the app lifecycle might be good enough for now, but I'd file a follow-up to save this state to the bundle.

Even if you don't want to address the bundle now, I'd still create the OnBannerClosed callback so that the close message gets propagated to HomePager. Then in HomePager, you can do:

    removeView(mHomeBanner);
    mHomeBanner = null;

which will not only unregister the Data listener for you, but it will remove the unused HomeBanner altogether (saving a bit of memory), and it will prevent unnecessary mHomeBanner.update() calls whenever HomePager is reloaded.

Comment 18

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #17)

> Hmm, this goes back to the lifecycle issues from
> https://bugzilla.mozilla.org/show_bug.cgi?id=961523#c6: namely, the "app
> lifetime" on Android is pretty loosely defined. On older phones, we get
> killed pretty frequently whenever we're put into the background, so the
> closed banner may not last long.

Yeah, this really raises a bigger question of how much we want to explicitly control the timing of when the banner comes back. I think this is a fine solution for now (especially if we're looking for something to uplift to aurora before the merge), but to really control it we may want to store a pref with a timestamp.

> The technique I described there -- storing the state in the bundle and
> restoring it when we're recreated -- would be one way to handle this. Having
> the banner last for the app lifecycle might be good enough for now, but I'd
> file a follow-up to save this state to the bundle.

Wouldn't this make the banner go away forever?

> Even if you don't want to address the bundle now, I'd still create the
> OnBannerClosed callback so that the close message gets propagated to
> HomePager. Then in HomePager, you can do:
> 
>     removeView(mHomeBanner);
>     mHomeBanner = null;
> 
> which will not only unregister the Data listener for you, but it will remove
> the unused HomeBanner altogether (saving a bit of memory), and it will
> prevent unnecessary mHomeBanner.update() calls whenever HomePager is
> reloaded.

Sounds good, updating my patch now!

(Also, thanks for all these banner-related reviews)

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #18)
> > The technique I described there -- storing the state in the bundle and
> > restoring it when we're recreated -- would be one way to handle this. Having
> > the banner last for the app lifecycle might be good enough for now, but I'd
> > file a follow-up to save this state to the bundle.
> 
> Wouldn't this make the banner go away forever?

Not forever -- state Bundles only last as long as the user perceived app lifetime. Once the user force kills the app, swipes it from recent apps, or the app crashes, all the saved state stored in that Bundle gets lost. In a sense, the perceived app lifetime on Android is similar to the perceived app lifetime on a desktop, where explicitly closing the desktop window ends the application.

Also note that the state doesn't need to be a boolean; you could store a timestamp in the Bundle just as you would a pref. When you restore from the Bundle, you could check how much time has elapsed to expire it.

Comment 20

[:Margaret Leibovic]

Attachment: (Part 4 - v2) Don't show the banner again for the rest of the app lifetime after the user taps the close button

This patch implements the idea to remove the banner view once the close button is hit (good idea to just kill the view if we're never going to show it again). I don't really like adding more logic to BrowserApp, but this seemed like the most straightforward way to do this.

I decided to go with "dismiss" rather than "close", since that's the term we're already using on the JS side of things.

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 8387754 [details] [diff] [review]
(Part 4 - v2) Don't show the banner again for the rest of the app lifetime after the user taps the close button

Review of attachment 8387754 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +1700,5 @@
> +            homeBanner.setOnDismissListener(new HomeBanner.OnDismissListener() {
> +                @Override
> +                public void onDismiss() {
> +                    mHomePager.setBanner(null);
> +                    mHomePagerContainer.removeView(homeBanner);

If you don't want to hold a reference to mHomePagerContainer, I think you could also do `((ViewGroup) homeBanner.getParent()).removeView(homeBanner)`. That would also allow you to keep this in HomePager if you wanted, though I think it makes more sense here anyway since this is where the HomeBanner gets set.

Comment 22

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #21)

> ::: mobile/android/base/BrowserApp.java
> @@ +1700,5 @@
> > +            homeBanner.setOnDismissListener(new HomeBanner.OnDismissListener() {
> > +                @Override
> > +                public void onDismiss() {
> > +                    mHomePager.setBanner(null);
> > +                    mHomePagerContainer.removeView(homeBanner);
> 
> If you don't want to hold a reference to mHomePagerContainer, I think you
> could also do `((ViewGroup) homeBanner.getParent()).removeView(homeBanner)`.
> That would also allow you to keep this in HomePager if you wanted, though I
> think it makes more sense here anyway since this is where the HomeBanner
> gets set.

We already hold a reference to mHomePagerContainer, and I like being explicit, as a reminder of where the view actually lives in the hierarchy.

https://hg.mozilla.org/integration/fx-team/rev/f94c24ba1496
https://hg.mozilla.org/integration/fx-team/rev/109d0ec8dc27
https://hg.mozilla.org/integration/fx-team/rev/be4a906fa5fa
https://hg.mozilla.org/integration/fx-team/rev/b940770772d2

Comment 23

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #22)
> We already hold a reference to mHomePagerContainer

Oops, missed that. And I agree it's better to be more explicit -- not sure why I even brought that up!

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f94c24ba1496
https://hg.mozilla.org/mozilla-central/rev/109d0ec8dc27
https://hg.mozilla.org/mozilla-central/rev/be4a906fa5fa
https://hg.mozilla.org/mozilla-central/rev/b940770772d2

Comment 25

[:Margaret Leibovic]

Attachment: folded patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960359
User impact if declined: only one message will show in the banner per app lifetime (and it's a race for which one is added first)
Testing completed (on m-c, etc.): landed on m-c 3/7, tested locally
Risk to taking this patch (and alternatives if risky): should be low-risk, but I would like QA to verify on Nightly before we uplift, since this logic can be bug-prone
String or IDL/UUID changes made by this patch: none

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/924e983b2dea