976232 - Banner shown in Guest Browsing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently we show a banner in a guest browsing session. I assume this is undesirable (especially for Sync setup).

Comment 1

[:Margaret Leibovic]

Attachment: Don't show banner messages in guest mode

Sorry, Brian, you're the lucky reviewer for all these.

This is a really harsh approach that makes us just avoid showing any banner when you're in guest mode, but I feel like guests don't need to see promotions. I also feel like I just want a solution that gets the job done, with the least risk of regressing anything :)

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8387007 [details] [diff] [review]
Don't show banner messages in guest mode

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

Rather than doing this in JS, could you just add a mProfile.inGuestMode() check around around the setBanner code at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1696 ? That should prevent the banner from getting set in the first place. We already have several guest mode checks in BrowserApp, so it seems better to do it here vs. introducing the BrowserApp dependency in Home.jsm.

Comment 3

[:Margaret Leibovic]

Attachment: (v2) Disable home banner in guest mode

Follows bnicholson's suggestion.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8389492 [details] [diff] [review]
(v2) Disable home banner in guest mode

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

I really want to move more things into a common code place to make this less hacky, but for now this is what we do...

::: mobile/android/base/BrowserApp.java
@@ +1696,5 @@
>              final HomeBanner homeBanner = (HomeBanner) findViewById(R.id.home_banner);
> +
> +            // Never show the home banner in guest mode.
> +            if (GeckoProfile.get(this).inGuestMode()) {
> +                mHomePagerContainer.removeView(homeBanner);

I see one case in HomePager where mHomeBanner is used without checking for null.  I think we'll need to update that?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#259

Comment 5

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 8389492 [details] [diff] [review]
> (v2) Disable home banner in guest mode
> 
> Review of attachment 8389492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really want to move more things into a common code place to make this less
> hacky, but for now this is what we do...
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1696,5 @@
> >              final HomeBanner homeBanner = (HomeBanner) findViewById(R.id.home_banner);
> > +
> > +            // Never show the home banner in guest mode.
> > +            if (GeckoProfile.get(this).inGuestMode()) {
> > +                mHomePagerContainer.removeView(homeBanner);
> 
> I see one case in HomePager where mHomeBanner is used without checking for
> null.  I think we'll need to update that?
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomePager.java#259

Lucas just landed a patch for that in bug 981614 :)

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/7c458c70cd5c

Comment 7

[:Margaret Leibovic]

Comment on attachment 8389492 [details] [diff] [review]
(v2) Disable home banner in guest mode

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling sync promo banner and snippets
User impact if declined: banner will be shown while in guest mode
Testing completed (on m-c, etc.): tested locally, just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, just disables the banner in guest mode
String or IDL/UUID changes made by this patch: none

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/7c458c70cd5c

Comment 9

[:Margaret Leibovic]

Note to sheriffs: this may have caused a talos regression, so let's hold off on uplifting to aurora for now.

Comment 10

[:Margaret Leibovic]

Backed out for suspected talos regression:
https://hg.mozilla.org/integration/fx-team/rev/80fda301471e

Comment 11

[:Margaret Leibovic]

Attachment: (v3) Disable home banner in guest mode

We should confirm that the previous patch here was indeed responsible for the talos regression, but this should make that better if that was the case. With my patch for bug 968640 removing the view should become unnecessary, so I left that out.

Comment 12

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #11)
> Created attachment 8390771 [details] [diff] [review]
> (v3) Disable home banner in guest mode
> 
> We should confirm that the previous patch here was indeed responsible for
> the talos regression, but this should make that better if that was the case.
> With my patch for bug 968640 removing the view should become unnecessary, so
> I left that out.

FYI, looks like the talos regressions were caused by bug 979101.

Comment 13

[:Margaret Leibovic]

Attachment: (v3) Disable home banner in guest mode

Oops too many bugs, wrong patch.

Comment 14

[:Margaret Leibovic]

This isn't really very different from the original patch, just landed it:
https://hg.mozilla.org/integration/fx-team/rev/26e1df0e1ee1

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/26e1df0e1ee1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1f617d5c5ea

Comment 17

[Flaviu Cos]

Verified as fixed in builds:
- 29 beta 2; 
- 30.0a2 (2014-03-25);
- 31.0a1 (2014-03-25);
Device: Google Nexus 10 (Android 4.4.2).