1039037 - Pull GuestMode code out of GeckoProfile and BrowserApp

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Neither of these classes needs to know much about GuestMode. It feels nice to put all this knowledge/machinery in one place.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This pulls everything related to Guest mode out of BrowserApp and GeckoProfile and puts it in the GuestMode file I started in bug 815682.

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8456547 [details] [diff] [review]
Patch

Review of attachment 8456547 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is looking OK overall, but I suggest splitting the profile logic in GuestMode into a separate GuestProfile class. GuestProfile would contain the standard profile-specific code (add, remove, get), and GuestMode would handle actual guest mode actions (toggling the profile, prompting, etc). Note how there are a number of methods that exist with guest profile equivalents outlined below, so some kind of Profile interface/base class would make a lot of sense here.

createGuestProfile | createProfileDir   => createProfile
removeProfile      | removeGuestProfile => removeProfile
getDir             | getGuestDir        => getDir

Also notice how you had to copy over the lock/unlock methods from GeckoProfile, which is another indicator that this could use some OO.

::: mobile/android/base/GuestMode.java
@@ +33,5 @@
> +    private static File sGuestDir = null;
> +    private static GeckoProfile sGuestProfile = null;
> +    private static GuestMode sInstance = null;
> +
> +    // The types of guest mode dialogs we show.

Nit: This comment is redundant, so you could just remove it.

@@ +40,5 @@
> +        LEAVING
> +    }
> +
> +    // Flags for use by some methods.
> +    public static final int NO_FLAGS = 0;

Nit: new line after this

@@ +41,5 @@
> +    }
> +
> +    // Flags for use by some methods.
> +    public static final int NO_FLAGS = 0;
> +    // Pass to getProfile to force creation of the profile if it doesn't currently exists.

s/exists/exist/

@@ +43,5 @@
> +    // Flags for use by some methods.
> +    public static final int NO_FLAGS = 0;
> +    // Pass to getProfile to force creation of the profile if it doesn't currently exists.
> +    // This will cause file i/o. Avoid it if you can!
> +    public static final int FORCE_CREATE = 0x01;

JavaDocs for this comment instead of "//".

@@ +48,5 @@
> +
> +    // Passed to the enable or toggle methods to control whether the user will be prompted
> +    // before switching modes. If the user was prompted, they won't leave guest mode until they
> +    // explicitly exit it from the menu.
> +    public static final int SHOW_PROMPT = 0x01;

JavaDocs for this comment instead of "//".

@@ +82,5 @@
>          return profile.locked();
>      }
> +
> +    // Set guest mode enabled or disabled. No op if the browser is already in the requested mode.
> +    // This defaults to prompting the user before entering guest mode.

JavaDocs (here and all methods below).

@@ +100,5 @@
> +        return false;
> +    }
> +
> +    // Returns true if the browser is currently in guest mode.
> +    public static boolean enabled(final GeckoApp app) {

All of these methods accepting GeckoApp are pretty ugly. I agree with the motive to remove guest mode logic from the activity, but we should also be careful about dependencies in the reverse direction. Conceptually, Activity is the concept of a UI for the user to perform a task, so it just doesn't make sense to have to give an Activity instance to GuestMode#enabled to determine whether guest mode is active.

For this method, you could just get away with accepting a GeckoProfile instead of a GeckoApp, but I know several of the other methods here aren't so straightforward. I could try tackling this in a follow-up if you don't want to, but maybe you have some ideas on how to fix this?

@@ +184,5 @@
> +            Log.e(LOGTAG, "Error creating guest profile", ex);
> +        }
> +    }
> +
> +    // Lock the current profile, if it exists

Nit: add trailing . for consistency

@@ +194,5 @@
> +
> +        return false;
> +    }
> +
> +    // Unlocks the guest mode profile (if it exists at all). It will be delted the next time we start.

s/delted/deleted/

@@ +204,5 @@
> +
> +        return false;
> +    }
> +
> +    // Get the guest mode dir

Nit: add trailing . for consistency

@@ +213,5 @@
> +
> +        return sGuestDir;
> +    }
> +
> +    // If the guest mode dir exists, deltes it and clears all guest mode preferences.

s/deltes/deletes/

Comment 3

[Wesley Johnston (:wesj)]

Seems like guest mode is on its way out, so I'm going to WONTFX this for now.

Comment 4

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

(In reply to Wesley Johnston (:wesj) from comment #3)
> Seems like guest mode is on its way out, so I'm going to WONTFX this for now.

Unassigning is fine, but let's not bury Guest Browsing too quickly.

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.