Closed Bug 976232 Opened 6 years ago Closed 6 years ago

Banner shown in Guest Browsing

Categories

(Firefox for Android :: General, defect)

30 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: Margaret)

Details

Attachments

(1 file, 3 obsolete files)

Currently we show a banner in a guest browsing session. I assume this is undesirable (especially for Sync setup).
tracking-fennec: ? → 29+
Assignee: nobody → margaret.leibovic
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 :)
Attachment #8387007 - Flags: review?(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.
Follows bnicholson's suggestion.
Attachment #8387007 - Attachment is obsolete: true
Attachment #8387007 - Flags: review?(bnicholson)
Attachment #8389492 - Flags: review?(wjohnston)
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
Attachment #8389492 - Flags: review?(wjohnston) → review+
(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 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
Attachment #8389492 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7c458c70cd5c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8389492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Note to sheriffs: this may have caused a talos regression, so let's hold off on uplifting to aurora for now.
Backed out for suspected talos regression:
https://hg.mozilla.org/integration/fx-team/rev/80fda301471e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8389492 - Attachment is obsolete: true
Attachment #8390771 - Flags: review?(wjohnston)
(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.
Oops too many bugs, wrong patch.
Attachment #8390771 - Attachment is obsolete: true
Attachment #8390771 - Flags: review?(wjohnston)
Attachment #8390886 - Flags: review?(wjohnston)
This isn't really very different from the original patch, just landed it:
https://hg.mozilla.org/integration/fx-team/rev/26e1df0e1ee1
Attachment #8390886 - Flags: review?(wjohnston)
https://hg.mozilla.org/mozilla-central/rev/26e1df0e1ee1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.