Closed Bug 871863 (guest-mode) Opened 7 years ago Closed 7 years ago

Guest mode for browsing

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox25 --- verified
firefox26 --- verified
relnote-firefox --- 25+

People

(Reporter: liuche, Assigned: blassey)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: feature)

Attachments

(1 file, 1 obsolete file)

Bug to track guest mode for the new setting UI rework.
OS: Mac OS X → Android
Hardware: x86 → ARM
Attached patch patch (obsolete) — Splinter Review
Attachment #769878 - Flags: review?(mark.finkle)
Assignee: nobody → blassey.bugs
I thought we frowned on doing System.exit(0), since it doesn't let us do things like shut down content providers correctly. I do see we currently use it in other places, so maybe that shouldn't stop this patch from landing, but I'm cc'ing rnewman because I know he's expressed an opinion about this in the past.
Yeah, we shouldn't exit anywhere in any Android code (except, perhaps, on upgrade, but then the system does it for us). That'll kill our process, including all background services, which are all designed to be profile-agnostic. 

Theoretically this should be safe, but you're relying on database recovery mechanisms, successful Sync retries, and praying not to hit consistency errors. 

You probably want to finish your activity and perhaps kill the Gecko thread, nothing more. 

Phrased differently: how would you implement this in GeckoWebView, given that you can't kill the app's process? Do it that way.
Looking over the patch...

Food for thought: You can ask Gecko to create new profiles and switch to that profile during a restart. I have some code here that can do it:
https://addons.mozilla.org/en-us/mobile/files/browse/108656/file/bootstrap.js#L69

Not sure  if it's better to do it all in Java or not. Mulling it over.
(In reply to :Margaret Leibovic from comment #2)
> I thought we frowned on doing System.exit(0), since it doesn't let us do
> things like shut down content providers correctly. I do see we currently use
> it in other places, so maybe that shouldn't stop this patch from landing,
> but I'm cc'ing rnewman because I know he's expressed an opinion about this
> in the past.

System.exit(0) just gets us restarted quicker (presumably because we're not doing the work that rnewmans wants us to do on shutdown). Its not needed, but makes the experience nicer.

I'd also say that we should handle shutting down quickly better.
Comment on attachment 769878 [details] [diff] [review]
patch

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

>+    private boolean mInGuestMode = false;

I think this should be moved to GeckoProfile with a getter

>+                File guestDir = getDir("guests", MODE_PRIVATE);
>+                final File[] profilesToCleanup = guestDir.listFiles();
>+                ThreadUtils.postToBackgroundThread(new Runnable() {
>+                    @Override
>+                    public void run() {
>+                        for (File p : profilesToCleanup){
>+                            try {
>+                                p.delete();                        
>+                            } catch (Exception e) { Log.i(LOGTAG, "failed to delete", e); }
>+                        }
>+                    }
>+                });
>+                File tempDir = File.createTempFile("guest", "profile", guestDir);
>+                mProfile = mProfile = GeckoProfile.get(this, null, tempDir.getCanonicalPath());
>+                mInGuestMode = true;

Similarly, I think this could be pushed into GeckoProfile too. Maybe add a method to create a guest profile and call it here.

>+        MenuItem enterGuestMode = aMenu.findItem(R.id.guest_mode);
>+        MenuItem exitGuestMode = aMenu.findItem(R.id.exit_guest_mode);

I wonder if we really need two menu items, or if one menu could do both jobs. I guess two is fine for now, but use symmetric ID names:
R.id.enter_guest_mode
R.id.exit_guest_mode

>+        if (mInGuestMode)
>+            exitGuestMode.setVisible(true);
>+        else
>+            enterGuestMode.setVisible(true);

Use the GeckoProfile.inGuestMode() getter

>+            case R.id.guest_mode:
>+                doRestart("--guest-mode");
>+                System.exit(0);
>+                return true;
>+            case R.id.exit_guest_mode:
>+                doRestart();
>+                System.exit(0);
>+                return true;

The System.exit(0) does worry me. I wish we had a safer way to shutdown.

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

>-        String profileName = null;
>-        String profilePath = null;
>-        if (args != null) {
>-            if (args.contains("-P")) {
>-                Pattern p = Pattern.compile("(?:-P\\s*)(\\w*)(\\s*)");
>-                Matcher m = p.matcher(args);
>-                if (m.find()) {
>-                    profileName = m.group(1);
>+        if (mProfile == null) {
>+            String profileName = null;
>+            String profilePath = null;
>+            if (args != null) {
>+                if (args.contains("-P")) {
>+                    Pattern p = Pattern.compile("(?:-P\\s*)(\\w*)(\\s*)");
>+                    Matcher m = p.matcher(args);
>+                    if (m.find()) {
>+                        profileName = m.group(1);
>+                    }
>                 }
>-            }
> 
>-            if (args.contains("-profile")) {
>-                Pattern p = Pattern.compile("(?:-profile\\s*)(\\S*)(\\s*)");
>-                Matcher m = p.matcher(args);
>-                if (m.find()) {
>-                    profilePath =  m.group(1);
>+                if (args.contains("-profile")) {
>+                    Pattern p = Pattern.compile("(?:-profile\\s*)(\\S*)(\\s*)");
>+                    Matcher m = p.matcher(args);
>+                    if (m.find()) {
>+                        profilePath =  m.group(1);
>+                    }
>+                    if (profileName == null) {
>+                        profileName = getDefaultProfileName();
>+                        if (profileName == null)
>+                            profileName = "default";
>+                    }
>+                    GeckoApp.sIsUsingCustomProfile = true;
>                 }
>-                if (profileName == null) {
>-                    profileName = getDefaultProfileName();
>-                    if (profileName == null)
>-                        profileName = "default";
>+
>+                if (profileName != null || profilePath != null) {
>+                    mProfile = GeckoProfile.get(this, profileName, profilePath);
>                 }
>-                GeckoApp.sIsUsingCustomProfile = true;
>-            }
>-
>-            if (profileName != null || profilePath != null) {
>-                mProfile = GeckoProfile.get(this, profileName, profilePath);

Make sure this doesn't blow up our talos and testing code. It's a bit more fragile than we'd like it to be.

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!-- Guest mode -->
>+<!ENTITY guest_mode "Guest mode">
>+<!ENTITY exit_guest_mode "Exit guest mode">

Our current menus use title case so "Guest Mode" and "Exit Guest Mode" would be consistent. Also, let's use "enter_guest_mode" here as well.

Let's get some Ian love on the strings themselves.

>diff --git a/mobile/android/base/strings.xml.in b/mobile/android/base/strings.xml.in

>+  <!-- Guest mode -->
>+  <string name="guest_mode">&guest_mode;</string>

"enter_guest_mode" here too
Attachment #769878 - Flags: review?(mark.finkle) → review-
Just noticed:

>+                mProfile = mProfile = GeckoProfile.get(this, null, tempDir.getCanonicalPath());


Looks like a typo - got an extra 'mProfile =' in there.
Attached patch patchSplinter Review
Attachment #769878 - Attachment is obsolete: true
Attachment #773746 - Flags: review?(mark.finkle)
Comment on attachment 773746 [details] [diff] [review]
patch

First pass: Looks pretty good.
* I need to double check the GeckoProfile changes. Maybe get Wes as a second reviewer on those.
* A nice followup would be to move GeckoApp.sIsUsingCustomProfile to GeckoProfile.isCustomProfile
Comment on attachment 773746 [details] [diff] [review]
patch

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

>+    public static GeckoProfile get(Context context, String profileName, File dir) {

Can you change "dir" to "profileDir" ?

>+        if (TextUtils.isEmpty(profileName) && dir == null) {

>+                profile = new GeckoProfile(context, profileName, dir);

>+                profile.setDir(dir);

Same

>+    public static GeckoProfile createGuestProfile(Context context) {

We might want to create a util function to delete the guestDir and use it anytime we use a profile. Right now, we only cleanup a guest profile when making a new guest profile. It would be nice to clean that folder up ASAP. Good followup bug. Can you file it?

>+            Log.i(LOGTAG, "we're in guest mode");

Let's remove this

>+    private GeckoProfile(Context context, String profileName, File dir) {

Rname "dir" to "profileDir"

>+        setDir(dir);

Same

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

>+        String profile = "";
>+        if (GeckoAppShell.getGeckoInterface() == null || 
>+            GeckoApp.sIsUsingCustomProfile) {

Shouldn't this test for getGeckoInterface != null ?

>+            profile = " -P " + GeckoAppShell.getGeckoInterface().getProfile().getName();
>+        } else if (GeckoAppShell.getGeckoInterface().getProfile().inGuestMode()) {

Since both the "if" and "else if" blocks depend on getGeckoInterface not being null perhaps you could use an outer block for that check, or return early.

r+, but:
* Make a Try run?
* Resolve the getGeckoInterface null check
* File a bug for moving GeckoApp.sIsUsingCustomProfile to GeckoProfile.isCustomProfile ?
* File a bug for adding guest profile cleanup ASAP

We might want to move the Guest Mode menu to the Tools menu, but let's see what it feels like in Nightly.
Attachment #773746 - Flags: review?(mark.finkle) → review+
I'd add two requirements to Mark's list:

* File a follow-up for clean shutdown (no exit(0)), so we don't kill the process.
* Test that syncing does the right thing. For example:

  1. Set up Sync in a profile. Wait for syncing to finish.
  2. Open Android Settings > Accounts & sync > Firefox Sync.
  3. Open Firefox. Switch to guest mode.
  4. Add a bookmark on desktop.
  5. Back on the phone, app-switch back to settings, leaving Firefox running. Menu > Sync Now. Wait until the spinner stops.
  6. Verify that the new bookmark doesn't appear in the guest profile.
  7. Verify that the new bookmark has appeared in the default profile.
(In reply to Mark Finkle (:mfinkle) from comment #10)

> r+, but:
> * Make a Try run?
https://tbpl.mozilla.org/?tree=Try&rev=2a7c5b0b16eb
> * Resolve the getGeckoInterface null check
done
> * File a bug for moving GeckoApp.sIsUsingCustomProfile to
> GeckoProfile.isCustomProfile ?
bug 895707
> * File a bug for adding guest profile cleanup ASAP
bug 895709


(In reply to Richard Newman [:rnewman] from comment #11)
> I'd add two requirements to Mark's list:
> 
> * File a follow-up for clean shutdown (no exit(0)), so we don't kill the
> process.
895710
https://hg.mozilla.org/mozilla-central/rev/20ccaa1b374a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Blocks: 896067
Blocks: 896068
Alias: guest-mode
Depends on: 896092
Depends on: 896281
Seeing a build error that might be due to these changes:

src/mobile/android/base/GeckoProfile.java:119: warning: [static] static variable should be qualified by type name, Context, instead of by an expression
            File guestDir = context.getDir("guest", context.MODE_PRIVATE);
                                                           ^
error: warnings found and -Werror specified
(In reply to Douglas Crosher [:dougc] from comment #14)
> Seeing a build error that might be due to these changes:
> 
> src/mobile/android/base/GeckoProfile.java:119: warning: [static] static
> variable should be qualified by type name, Context, instead of by an
> expression
>             File guestDir = context.getDir("guest", context.MODE_PRIVATE);
>                                                            ^
> error: warnings found and -Werror specified

Should be fixed by bug 896031
Depends on: 896031
No longer blocks: 896067, 896068
Depends on: 896067, 896068
Blocks: 896505
Blocks: 896507
Blocks: 896509
Depends on: 896513
Depends on: 897702
Depends on: 898583
Depends on: 906030
Just tried latest nightly, and if I press the menu button I see the option "leave guest mode" (I never entered guest mode!). After I press it and confirm the action, pressing the menu button reveals the same option again.
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #16)
> Just tried latest nightly, and if I press the menu button I see the option
> "leave guest mode" (I never entered guest mode!). After I press it and
> confirm the action, pressing the menu button reveals the same option again.

Fixed in bug 906030.
Status: RESOLVED → VERIFIED
Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.
Keywords: feature
For zh-tw, New guest session is currently translated to 開新訪客瀏覽階段 which means "Start a new guest browsing stage" which sounds just as irrelevant in English as it does in Chinese.

It would be better translated as "啟動訪客模式" and "關閉訪客模式" which means "activate guest mode" and "exit guest mode".  It sounds much more user-friendly and less technical.  (This also brings it in line with the default wording for flight mode activation/de-activation on Android.)

(Note the noun for "browsing session" doesn't exist in Chinese, at least not one which is comprehensible by normal people)
Please file a bug for the zh-tw localization community in the respective component.
Depends on: 917538
Depends on: 917986
Depends on: 906039
Depends on: 1030758
Depends on: 1030770
You need to log in before you can comment on or make changes to this bug.