896281 - [guest] - Synced tabbed carry over from owner

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Should entering guest mode hide synced tabs (both in the tab menu and on about:home)?

Comment 1

[Ian Barlow (:ibarlow)]

I hadn't thought of that but yes, yes it should.

Comment 2

[Aaron Train [:aaronmt]]

Hide the banner on about:home too? (spaghetti-code?)

Comment 3

[Ian Barlow (:ibarlow)]

Yup

Comment 5

[Brian Nicholson (:bnicholson)]

I assume we also want to remove the Sync pref from the settings menu?

Comment 6

[Ian Barlow (:ibarlow)]

For now, yes

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Disable synced tab for guest session

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Remove Sync pref for guest session

The context in GeckoPreferences is not a GeckoApp, but we still need to know whether the active profile is a guest one or not. Determining that doesn't require a GeckoApp context, so I did a bit of code shifting in GeckoProfile to get what we want. GeckoProfile gets uglier every day, but this should work for now while those GeckoProfile refactor patches are in progress.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Remove Sync pref for guest session, v2

Minor cleanup.

Comment 10

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

Comment on attachment 803224 [details] [diff] [review]
Part 2: Remove Sync pref for guest session, v2

>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java

>     public static GeckoProfile get(Context context) {
>         if (context instanceof GeckoApp) {
>             // Check for a cached profile on this context already
>             // TODO: We should not be caching profile information on the Activity context
>             if (((GeckoApp)context).mProfile != null) {
>                 return ((GeckoApp)context).mProfile;
>             }
>+        }
> 
>-            GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>-            // if the guest profile exists and is locked, return it
>-            if (guest != null && guest.locked()) {
>-                return guest;
>-            }
>+        // If the guest profile exists and is locked, return it
>+        GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>+        if (guest != null && guest.locked()) {
>+            return guest;
>+        }
> 
>+        if (context instanceof GeckoApp) {
>             // Otherwise, get the default profile for the Activity
>             return get(context, ((GeckoApp)context).getDefaultProfileName());
>         }
> 
>         return get(context, "");
>     }
> 

Why do I think Wes had this same patch for an old Awesomescreen fix? I think I forced him to pass a boolean to the Activity, back when the Awesomescreen was a separate activity. If you need this bandaid, fine. But by God it's time to clean this code up. Things I care about in GeckoProfile:
1. GeckoProfile should not care about Activities
2. If GeckoProfile needs to fetch a default profile name from someone, use an interface!
3. If GeckoProfile needs to know the root folder for profiles, pass it in!

Comment 11

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/6450475d597d
https://hg.mozilla.org/integration/fx-team/rev/15904709259c

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6450475d597d
https://hg.mozilla.org/mozilla-central/rev/15904709259c

Comment 13

[Aaron Train [:aaronmt]]

Noticed that this is missing in 25 (beta #1) but tracking 25+.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 803217 [details] [diff] [review]
Part 1: Disable synced tab for guest session

[Approval Request Comment]
Bug caused by (feature/regressing bug #): guest-mode
User impact if declined: sync UI is visible using guest mode
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Medium-low. This potentially changes which profile is returned and could break things in our ContentProvider code if it doesn't handle profiles correctly. I've tested this, however, and haven't seen any issues.
String or IDL/UUID changes made by this patch: none

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Aaron Train [:aaronmt] from comment #13)
> Noticed that this is missing in 25 (beta #1) but tracking 25+.

This will require extra QA testing, given the medium-low risk evaluation above.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for the beta uplift.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac0639bf267
https://hg.mozilla.org/releases/mozilla-aurora/rev/8de790b4863a

Comment 17

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2674701bc7b
https://hg.mozilla.org/releases/mozilla-beta/rev/ac6031737273