Closed Bug 896281 Opened 9 years ago Closed 9 years ago

[guest] - Synced tabbed carry over from owner

Categories

(Firefox for Android Graveyard :: General, defect)

25 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox25+ verified, firefox26+ verified, firefox27 verified, fennec25+)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox25 + verified
firefox26 + verified
firefox27 --- verified
fennec 25+ ---

People

(Reporter: aaronmt, Assigned: bnicholson)

References

Details

Attachments

(2 files, 1 obsolete file)

Should entering guest mode hide synced tabs (both in the tab menu and on about:home)?
Flags: needinfo?(ibarlow)
I hadn't thought of that but yes, yes it should.
Flags: needinfo?(ibarlow)
Hide the banner on about:home too? (spaghetti-code?)
Yup
Duplicate of this bug: 907149
tracking-fennec: --- → ?
Assignee: nobody → bnicholson
tracking-fennec: ? → 25+
I assume we also want to remove the Sync pref from the settings menu?
Flags: needinfo?(ibarlow)
For now, yes
Flags: needinfo?(ibarlow)
Attachment #803217 - Flags: review?(sriram) → review+
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.
Attachment #803221 - Flags: review?(wjohnston)
Minor cleanup.
Attachment #803221 - Attachment is obsolete: true
Attachment #803221 - Flags: review?(wjohnston)
Attachment #803224 - Flags: review?(wjohnston)
Attachment #803217 - Attachment description: Disable synced tab for guest session → Part 1: Disable synced tab for guest session
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!
Attachment #803224 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/6450475d597d
https://hg.mozilla.org/mozilla-central/rev/15904709259c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Status: RESOLVED → VERIFIED
Noticed that this is missing in 25 (beta #1) but tracking 25+.
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
Attachment #803217 - Flags: approval-mozilla-beta?
Attachment #803217 - Flags: approval-mozilla-aurora?
Attachment #803224 - Flags: approval-mozilla-beta?
Attachment #803224 - Flags: approval-mozilla-aurora?
(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.
Attachment #803217 - Flags: approval-mozilla-beta?
Attachment #803217 - Flags: approval-mozilla-beta+
Attachment #803217 - Flags: approval-mozilla-aurora?
Attachment #803217 - Flags: approval-mozilla-aurora+
Attachment #803224 - Flags: approval-mozilla-beta?
Attachment #803224 - Flags: approval-mozilla-beta+
Attachment #803224 - Flags: approval-mozilla-aurora?
Attachment #803224 - Flags: approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.