871863 - (guest-mode) Guest mode for browsing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Bug to track guest mode for the new setting UI rework.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 2

[:Margaret Leibovic]

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.

Comment 3

[Richard Newman [:rnewman]]

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.

Comment 4

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

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.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(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 6

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

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

Comment 7

[Chris Kitching [:ckitching]]

Just noticed:

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


Looks like a typo - got an extra 'mProfile =' in there.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 9

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

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 10

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

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.

Comment 11

[Richard Newman [:rnewman]]

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.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20ccaa1b374a

Comment 14

[Douglas Crosher [:dougc]]

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

Comment 15

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

(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

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

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.

Comment 17

[Aaron Train [:aaronmt]]

(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.

Comment 18

[Alex Keybl [:akeybl]]

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.

Comment 19

[henryfhchan]

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)

Comment 20

[Aaron Train [:aaronmt]]

Please file a bug for the zh-tw localization community in the respective component.