815682 - (fennec-lockscreen) Lock screen widget/shortcut for Guest Sessions on Android 4.2 and above

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Android 4.2 introduces lock screen widgets (http://developer.android.com/about/versions/android-4.2.html#Lockscreen) where we could possibly make it easier for people to jump right into a search or their favorite content

Comment 1

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

Needs design?

Comment 2

[Ian Barlow (:ibarlow)]

I wonder if this would be an opportunity to explore some of Lucas's just-in-time search thinking?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Guest sessions aren't too useful without being implemented as a lock screen widget, as guests can still navigate the rest of the phone as well as exit their guest session.

Doing something like this would allow people to pass their phone without any worries about losing privacy.

Comment 5

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Guest sessions aren't too useful without being implemented as a lock screen
> widget, as guests can still navigate the rest of the phone as well as exit
> their guest session.
> 
> Doing something like this would allow people to pass their phone without any
> worries about losing privacy.

I like this idea. No idea if it is possible though.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > Guest sessions aren't too useful without being implemented as a lock screen
> > widget, as guests can still navigate the rest of the phone as well as exit
> > their guest session.
> > 
> > Doing something like this would allow people to pass their phone without any
> > worries about losing privacy.
> 
> I like this idea. No idea if it is possible though.

Yeah, I'm curious if we can do this with a widget, or if we'd need to write an entire custom lockscreen.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I don't see why we can't do this as a lockscreen widget. The "CBS Sports" app has a lockscreen widget that shows sports scores for today's games, a refresh button, etc.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: CBS Sports lockscreen widget

We may not be able to consume 100% of the screen real estate though. This is a screenshot of the CBS Sports lockscreen widget.

Comment 9

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

widgets are kinda limited in what they can do. For instance you can use these layouts:


    FrameLayout
    LinearLayout
    RelativeLayout
    GridLayout

and these widgets:

    AnalogClock
    Button
    Chronometer
    ImageButton
    ImageView
    ProgressBar
    TextView
    ViewFlipper
    ListView
    GridView
    StackView
    AdapterViewFlipper


reference: 
http://developer.android.com/guide/topics/appwidgets/index.html#CreatingLayout

So we can't implement a full browser here (we'd need a SurfaceView or similar)

Comment 10

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

I thought the Guest Browsing idea is just adding a simple button (icon) widget that will launch Firefox in Guest Mode.

Comment 11

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

In fact, I don't think we should be trying to make a browser anything on the lock screen.

Comment 12

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

sticking with the core idea though, on my Nexus 5 there is a camera icon, which if I drag launches the camera. But if I exit the camera it sends me back to the lock screen. I think that would acomplish what Jared is looking for. I just have no idea how they do that.

Comment 13

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> I thought the Guest Browsing idea is just adding a simple button (icon)
> widget that will launch Firefox in Guest Mode.

Well, the idea is to do that without unlocking the rest of phone, which is the part that would need some exploration.

Comment 14

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

I just added the little Google Keep bar to my lockscreen. Tapping one of the icons opens Keep. Closing the activity using the UI (Done) or the Back button or the Home button, return me to the lock screen.

I think it might be built into the system.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Cool, so an icon that launches a guest session may be enough, if it can exit back to the lock screen.

Comment 16

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> I thought the Guest Browsing idea is just adding a simple button (icon)
> widget that will launch Firefox in Guest Mode.

That was my understanding as well. 

> In fact, I don't think we should be trying to make a browser anything on the 
> lock screen.

Agreed. 


(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> Cool, so an icon that launches a guest session may be enough, if it can exit
> back to the lock screen.

Seems like a good place to start. It could satisfy the case of someone looking something up (in the browser at least) without having access to the rest of the device. 


---

As an aside, I still really wonder what other use cases a "guest mode" should support on a mobile device. We've been thinking about this very specifically in terms of a browser, which makes sense given that, well, that's the product we're shipping. But given the conversations happening around what else we might explore on mobile, it's worth asking what other kinds of things a guest -- one who I don't trust enough to let them see my data -- would need on a device to complete a particular task. Is it really just a browser?

It could be, but I'm also imagining what other kinds of things a guest without a phone might need. Make a phone call? Get directions for something? Take a picture of a thing to email themselves? So might a more useful Guest feature be a limited set of other core device features like a Phone with my contacts hidden, or Maps, or something else? 

Anyway this is obviously not for this bug, but I wanted to call it out as we explore other product areas.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I got frustrated that the GuestMode code is... confusing, so I pulled it out into something I could understand. I'll split this up now so that we can see the difference better. Based on top of the code in bug 1035642.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch

This still builds on top of the simple widget. Doesn't move the world around. Still creates a GuestMode class though. I'll put the move the world patches in a separate bug.

It does remove code that automatically removes the old Guest profile if we start with the --guest args. The new patch can start in guest mode through a variety of means, and we don't want to do file i/o on each of those startups.

It also includes some changes so that if you send in an intent with a --guest arg, we'll try and restart in guest mode (without prompting, since the prompts may not appear if the keyguard is up).

It still leaves the "Exit guest mode" menu options when the keyguard is up. Hitting it prompts, then restarts in Guest mode again, which is dumb. Follow up for some of this polish? Next cycle of course :) since this depends on the other widget I think that's a given.

Comment 19

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

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


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

>+import android.app.KeyguardManager;

Might not be needed in this file?

>+    // Returns true if the user is using a secure keyguard, and its currently locked.
>+    public boolean isKeyguardLocked() {
>+        KeyguardManager manager = (KeyguardManager) getSystemService(Context.KEYGUARD_SERVICE);
>+        return manager.isKeyguardLocked();
>+    }

Not used in this file?

>     protected void onNewIntent(Intent intent) {

>+        final String args = intent.getStringExtra("args");
>+        // If an external intent tries to start Fennec in guest mode, and its not already
>+        // in guest mode, this will change modes before opening the url.
>+        // This is useful if we're launching from the keyguard.

Add a blank line before the comments

>+        final boolean enableGuestMode = GuestMode.shouldUse(this, args);
>+        final boolean inGuestMode = GeckoProfile.get(this).inGuestMode();
>+        if (enableGuestMode != inGuestMode) {
>+            doRestart(GUEST_BROWSING_ARG);
>+            GeckoAppShell.systemExit();
>+            return;
>+        }

So we will restart into guest browsing even if Fennec is already running but we get a --guest intent. Makes sense I guess.

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

>+    public static boolean shouldUse(final GeckoApp context, final String args) {
>+        // Did the command line args requested guest mode?
>+        if (args != null && args.contains(BrowserApp.GUEST_BROWSING_ARG)) {

Shouldn't GuestMode hold GUEST_BROWSING_ARG now? I thought we worked hard to use GuestBrowsing.java as the "name" for this too. Or GuestSession.java, but I see some code that uses inGuestMode too. Sigh.

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

>+<!ENTITY guest_session_button "Guest Session">

Not used

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

>+  <string name="guest_session_button">&guest_session_button;</string>

Not used

You might want Brian to take a quick feedback look at this too

Comment 20

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

I think this should have some UX love before landing too.

+1 for this landing at the beginning of the Fx34 cycle.

(In reply to Wesley Johnston (:wesj) from comment #18)

> It still leaves the "Exit guest mode" menu options when the keyguard is up.
> Hitting it prompts, then restarts in Guest mode again, which is dumb. Follow
> up for some of this polish? Next cycle of course :) since this depends on
> the other widget I think that's a given.

Follow ups for these is fine, but let's make sure we nail them quickly in the next cycle (Fx34) as soon as we can.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sorry for the re-review, but I think this is a bit better. Since we have to know if we should show guest mode or not when you tap the widget, things get a little complex. I know ian has talked about changing this behavior, but I want to at least save my place before I see whats involved there.

For instance, if we just set the FLAG_DISMISS_KEYGUARD flags on all startups, then if you start Fennec in normal mode, hit the power button, and then hit the power button again, we'll just keep showing Fennec instead of showing your lock screen. Note, in this case we don't even get an onNewIntent or onCreate call. We probably hit onResume(), but I wanted to make the decision to flip on Guest mode earlier than that (and not have to kill Gecko mid startup).

HOWEVER, if you have a non-secure keyguard, we do want to support your normal profile, so in that case we set FLAG_DISMISS_KEYGUARD regardless of whether you're in guest mode or not. I have no idea how to detect if your keyguard changes mid-session, so we may have to ignore that edge case.

As a counter, if you start Fennec in Guest mode from the lockscreen, then close your lockscreen and start fennec normally, you'd expect to be back in your normal session. I did that here by explicitly NOT locking the guest profile if the lockscreen is up. An unlocked profile will be deleted at next startup. NOTE: we don't unlock it from the lockscreen (we just don't explicitly lock it) so if your normal Fennec is already in guest mode, we'll continue to use that guest profile until you explicitly select Exit Guest Session and do unlock it.

I have no idea how to write tests for this stuff... Thinking about it.

This also disables the Exit Guest Session buttons, and makes the Settings page work in Guest mode. I filed bugs about disabling webapps, downloads, or extensions in guest mode.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: WIP Config

Here's a rough WIP for this Configuration bit. A build is at:

http://people.mozilla.org/~wjohnston/widget.apk

This shows a dialog when you add the widget. You can pick between "New Tab", "Private Tab", and "Guest session" right now. I'll throw up screenshots for some feedback. I changed the text labels based on the mode you selected, but I actually think it might be nicer to change the icon i.e.

 Plus Icon   - New Tab
 Mask Icon   - Private Tab
 Person Icon - Guest Mode

Unfortunately, RemoteViews don't let you setCompoundDrawables very easily, so that may require separate layouts for eat widget option...

I still need to rejigger the internals here, since you can basically "force" us into GuestMode rather than my old method of looking at the keyguard status.

A couple odd things, these states aren't quite orthogonal. i.e. You can use a private tab in guest mode. Do we want to support them like that? If you have a widget that opens a "New Tab", but you last used Firefox in a Guest Session, should we open that New Tab in the guest session or switch you back to a normal one?

Secondly, if we allow configuring this "New tab" button, do we also want to allow configuring the "Search..." button to do something different? It feels kinda crappy if we don't, but that's a lot of configuration to ask of the user.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Configure screen

Here's what the config dialog looks like right now. This appears as soon as you add the widget on the home or lock screen. If you hit back to cancel, it (should) just cancel adding the widget alltogether (there's a bug there I just noticed :) )

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Home screen widgets

Here's all three of these options on home. Obviously the text/icons need some love.

Comment 25

[Ian Barlow (:ibarlow)]

Nice, this is coming together Wes. I'll let Anthony comment on visual design, but I have a few writing / interaction comments. 

I like the config dialog, but perhaps we can tweak the wording a bit? What do you think of something like this:


-------------------------------------------------------
| # How should Firefox open from this widget?         |
-------------------------------------------------------
| New tab                                           O |
-------------------------------------------------------
| New private tab                                   O |
-------------------------------------------------------
| Guest session                                     O |
| (recommended for password-protected lock screens)   |
-------------------------------------------------------


Also, now that we have choices of how to open the browser, it is making me wonder if we should actually expose those terms in the widget or not. Showing text that reads New Tab / Private Tab / Guest Session feels a little odd somehow now. 

Perhaps a better way of looking at this is that the widget offers two basic entry points: Search and Browse. And when you set this Normal/Private/Guest pref and it applies to *all entry points in the widget*. 

So you might imagine the widget looking like this: 

-----------------------------------------------------
  [] Search   |    [FENNEC HEAD]    |   [] Browse
-----------------------------------------------------

If I were to set the widget to "open everything in guest sessions", then tapping Browse would open Firefox in a guest session. Tapping Search would launch the search activity, and then *launch any webpages you like to in a Firefox Guest session*. Same for private browsing, and so on. 

What do you think of that?

Comment 26

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

(In reply to Ian Barlow (:ibarlow) from comment #25)
> Nice, this is coming together Wes. I'll let Anthony comment on visual
> design, but I have a few writing / interaction comments. 
> 
> I like the config dialog, but perhaps we can tweak the wording a bit? What
> do you think of something like this:
> 
> 
> -------------------------------------------------------
> | # How should Firefox open from this widget?         |
> -------------------------------------------------------
> | New tab                                           O |
> -------------------------------------------------------
> | New private tab                                   O |
> -------------------------------------------------------
> | Guest session                                     O |
> | (recommended for password-protected lock screens)   |
> -------------------------------------------------------

Is it "New Tab" or "Guest Session" explicitly? Or is it "Use guest session from lockscreen" ?

> Perhaps a better way of looking at this is that the widget offers two basic
> entry points: Search and Browse. And when you set this Normal/Private/Guest
> pref and it applies to *all entry points in the widget*. 
> 
> So you might imagine the widget looking like this: 
> 
> -----------------------------------------------------
>   [] Search   |    [FENNEC HEAD]    |   [] Browse
> -----------------------------------------------------

I agree with this. It's "Search" or "Browse"

Comment 27

[Wesley Johnston (:wesj)]

I talked to mark about this this morning a bit. I think that presenting the user with a choice between "Guest" and "Private" is probably too confusing of a choice, especially since they're orthogonal choices. You can have a Guest+Private browsing session. Its also confusing even what a generic "New Tab" would do as well, since if you start a Guest session from Fennec, we keep you in that until you exit it. Does 'New tab' continue in that guest session or kill it and restart your normal session?

As a simpler approach, we talked about restricting the guest stuff to the lockscreen widget only. From there, we'd present you with a "simpler" question. Something like a checkbox/slider with "Allow access to my history and bookmarks from the lock screen?"

I hate getting in the users way with a confusing question like that. We could offer a "settings" button on the widget itself, or hide this setting in Fennec's setting. Neither sounds like a great option, so for now maybe using this "Configure when its installed" approach is our best bet (I wish Android offered UI for opening the configuration activity later..)

Comment 28

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

Wes and I also talked about just creating two versions of the widget:
1. Normal branding with "search" and "browse" actions. This one has access to all your data.
2. Locked branding with "search" and "browse" actions. This one will lock down into Guest Browsing when launched from the lockscreen. The name of this version could have "secure" in it as well.

Comment 29

[Ian Barlow (:ibarlow)]

What about a third option, where we just "do the right thing" depending on the way your device is configured. On the widget config screen, we provide two options:

-------------------------------------------------------
| # How should Firefox open from this widget?         |
-------------------------------------------------------
| New tab                                           O |
-------------------------------------------------------
| New private tab                                   O |
-------------------------------------------------------

And then the behaviour adapts to the widget's context, if necessary:

If users selects new (normal) tab:
* [WIDGET ON DESKTOP] --> Open normal tab
* [WIDGET ON UNSECURE LOCK SCREEN] --> Open normal tab
* [WIDGET ON SECURED LOCK SCREEN] --> *Open Guest browsing*

If users selects private tab:
* [WIDGET ON DESKTOP] --> Open private tab
* [WIDGET ON UNSECURE LOCK SCREEN] --> Open private tab
* [WIDGET ON SECURED LOCK SCREEN] --> *Open Guest browsing* (I think for the purposes of doing a quick lookup and then exiting the session, this behaves pretty much the same as private browsing)

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Comment 31

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Whoops. Hit enter too soon. This implements this second design on top of the first patch. Its a lot more like my first design, so it wasn't that bad. This is a screenshot of this on your homescreen. I made a new/reused the private browsing icon here, since "New private tab" doesn't really fit in the widget.

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Lock screen

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Configure screen

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Forgot to add the images.

Comment 35

[Wesley Johnston (:wesj)]

[Tracking Requested - why for this release]: This is on track for 34 I think, but has some dependencies that should hold it up if they don't make 34. It doesn't make sense to track the dependencies if this isn't tracking...

Comment 36

[Brian Nicholson (:bnicholson)]

Comment on attachment 8468739 [details] [diff] [review]
Patch 2/2

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

Have some questions as always with a patch this size, but this looks pretty good overall.

::: mobile/android/base/BrowserApp.java
@@ +2766,5 @@
>          final String args = intent.getStringExtra("args");
>  
>          // If an external intent tries to start Fennec in guest mode, and its not already
>          // in guest mode, this will change modes before opening the url.
> +        boolean enableGuestSession = GuestSession.shouldUse(this, args);

Why remove 'final'?

@@ +2789,5 @@
>          }
>  
>          super.onNewIntent(intent);
>  
> +        if (ACTION_LOAD.equals(action)) {

Why is this chunk being moved from GeckoApp -> BrowserApp? Will this have any effect on webapps?

::: mobile/android/base/LaunchMode.java
@@ +10,5 @@
> +import android.content.SharedPreferences;
> +import android.util.Log;
> +
> +/* This class is an enum containing information about how we launch Fennec. This
> + * data can be passed as an Extra to Fennec by intents. */

Nit: JavaDoc commenting

@@ +13,5 @@
> +/* This class is an enum containing information about how we launch Fennec. This
> + * data can be passed as an Extra to Fennec by intents. */
> +public enum LaunchMode {
> +    NEW_TAB(R.string.launch_mode_new_tab_label, R.drawable.ic_widget_new_tab),
> +    NEW_PRIVATE_TAB(R.string.launch_mode_new_private_tab_label, R.drawable.ic_widget_new_tab_pb),

What is ic_widget_new_tab_pb? Did you forget to include it?

::: mobile/android/base/resources/layout/launch_widget_keyguard.xml
@@ +6,5 @@
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              android:layout_width="match_parent"
> +              android:layout_height="@dimen/widget_header_height"
> +              android:orientation="horizontal"
> +              android:background="@drawable/widget_bg">

Our search activity depends on Fennec, so it seems wrong that Fennec would have resource dependencies on search activity. Any resources that are currently in m/a/search should probably be moved to m/a/base if they're referenced in Fennec to avoid circular dependencies.

::: mobile/android/search/java/org/mozilla/search/SearchWidget.java
@@ +71,5 @@
> +
> +            try {
> +                LaunchMode mode = LaunchMode.getForWidget(id, context);
> +                redirect.putExtra(LaunchMode.EXTRA, mode.toString());
> +            } catch(IllegalArgumentException ex) {

What's this try/catch for? If possible, it's better to check for invalid arguments before passing them to avoid throwing in the first place. If that's not possible, please add a comment describing the conditions when this exception will be thrown.

@@ +121,5 @@
> +            }
> +        } else {
> +            views = new RemoteViews(context.getPackageName(), R.layout.search_widget);
> +            isKeyguard = false;
> +        }

This block could be simplified a bit if it was written like this:

boolean isKeyguard = false;

if (options != null) {
    final int category = ...;
    isKeyguard = ...;
}

final int resource = isKeyguard ? R.layout.launch_widget_keyguard : R.layout.search_widget;
views = new RemoteViews(context.getPackageName(), resource);

@@ +125,5 @@
> +        }
> +
> +        // Setup intents for when buttons are clicked
> +        if (!isKeyguard) {
> +            // The search button isn't shown on the keygaurd (yet)

Nit: keygaurd -> keyguard

::: mobile/android/search/java/org/mozilla/search/SearchWidgetConfigure.java
@@ +26,5 @@
> +import android.widget.RemoteViews;
> +import android.util.Log;
> +
> +/* A configuration activity for the search widget. This shows a "fake" dialog
> + * so the user can configure what mode Fennec launches in. This also provide som*/

Finish comment. Also, change to JavaDoc formatting style.

@@ +52,5 @@
> +        builder.setTitle(res.getString(R.string.search_widget_configure_dialog_title))
> +               .setSingleChoiceItems(new String[] {
> +                    LaunchMode.NEW_TAB.getName(res),
> +                    LaunchMode.NEW_PRIVATE_TAB.getName(res),
> +                }, -1, new DialogInterface.OnClickListener() {

This formatting is kind of hard to read. I'd turn these listeners into class variables that you pass in here to keep onCreate more concise.

@@ +59,5 @@
> +                        dialog.dismiss();
> +
> +                        final LaunchMode mode = which == 0 ? LaunchMode.NEW_TAB :
> +                                                which == 1 ? LaunchMode.NEW_PRIVATE_TAB :
> +                                                             LaunchMode.UNDEFINED;

Isn't onClick only fired if one of the two given options is clicked? If so, how is UNDEFINED possible?

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Enable configuring widgets

Comment 38

[Wesley Johnston (:wesj)]

Comment on attachment 8465670 [details] [diff] [review]
Patch v2

Brian, you want to review all of this?

Comment 39

[Brian Nicholson (:bnicholson)]

I still have several of the same questions -- can you answer them?

(In reply to Brian Nicholson (:bnicholson) from comment #36)
> @@ +2789,5 @@
> >          }
> >  
> >          super.onNewIntent(intent);
> >  
> > +        if (ACTION_LOAD.equals(action)) {
> 
> Why is this chunk being moved from GeckoApp -> BrowserApp? Will this have
> any effect on webapps?

> ::: mobile/android/base/resources/layout/launch_widget_keyguard.xml
> @@ +6,5 @@
> > +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> > +              android:layout_width="match_parent"
> > +              android:layout_height="@dimen/widget_header_height"
> > +              android:orientation="horizontal"
> > +              android:background="@drawable/widget_bg">
> 
> Our search activity depends on Fennec, so it seems wrong that Fennec would
> have resource dependencies on search activity. Any resources that are
> currently in m/a/search should probably be moved to m/a/base if they're
> referenced in Fennec to avoid circular dependencies.

To be more clear, these resources are all coming from m/a/search/res, but referenced in Fennec resources:
    @drawable/widget_bg
    @drawable/widget_button_left
    @drawable/widget_button_right
    @drawable/ic_widget_new_tab
    @dimen/widget_header_height
    @dimen/widget_drawable_padding
    @dimen/widget_text_size

If you disable the MOZ_ANDROID_SEARCH_ACTIVITY flag, the build will fail since the search resources won't be included. To fix that, I think these should be moved to m/a/b/resources.

> @@ +59,5 @@
> > +                        dialog.dismiss();
> > +
> > +                        final LaunchMode mode = which == 0 ? LaunchMode.NEW_TAB :
> > +                                                which == 1 ? LaunchMode.NEW_PRIVATE_TAB :
> > +                                                             LaunchMode.UNDEFINED;
> 
> Isn't onClick only fired if one of the two given options is clicked? If so,
> how is UNDEFINED possible?

Comment 40

[Wesley Johnston (:wesj)]

> I still have several of the same questions -- can you answer them?
Oops. Apologies. 
 
> > Why is this chunk being moved from GeckoApp -> BrowserApp? Will this have
> > any effect on webapps?

I don't think webapps should reply to any intents except Notification Callbacks or things like them. They don't register for any in their manifests, so they only things that should get through are dynamically registered handlers. Looking at this I see I actually moved those intents, so I'll move them back.

> To be more clear, these resources are all coming from m/a/search/res, but
> referenced in Fennec resources:
>     @drawable/widget_bg
>     @drawable/widget_button_left
>     @drawable/widget_button_right
>     @drawable/ic_widget_new_tab
>     @dimen/widget_header_height
>     @dimen/widget_drawable_padding
>     @dimen/widget_text_size

Err. Yeah. I moved some but not all here. Gah, I hate that I tied this across the search boundary.

> > Isn't onClick only fired if one of the two given options is clicked? If so,
> > how is UNDEFINED possible?

Its not. I'm just playing it safe. I can not play it safe. We probably don't need UNDEFINED TBH. Its a leftover from iterating through these designs/ideas. But at this point, this patch has zero to do with this bug anyway, I should probably move it to a separate one.

Comment 41

[Brian Nicholson (:bnicholson)]

Comment on attachment 8465670 [details] [diff] [review]
Patch v2

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

This patch is a bit scary, and I have a few questions I'd like answered before I can give r+. I wish my phone support lock screen widgets so I could test this!

For one, do we have some kind of clear UI indicating that the user is going to go into guest mode when the screen is locked? If not, people will likely end up in guest mode without expecting to, and their profiles will appear to be gone. This could be especially bad with our silent hiding of the "Exit guest mode" button (see comments below).

::: mobile/android/base/BrowserApp.java
@@ +466,5 @@
>          final Intent intent = getIntent();
> +        final String args = intent.getStringExtra("args");
> +
> +        if (GuestSession.shouldUse(this, args)) {
> +            // In guest mode we allow the Fennec to show over the keyguard.

the Fennec -> Fennec

@@ +2536,5 @@
>  
> +        if (mProfile.inGuestMode()) {
> +            // If the keyguard is locked, guest sessions are forced. Don't let users exit.
> +            if (!GuestSession.isSecureKeyguardLocked(this)) {
> +                exitGuestMode.setVisible(true);

Will users understand what's going on when their "Exit guest mode" menu option is gone? Maybe we should keep the button in the menu, but have it show a toast/dialog telling the user why they can't leave.

@@ +2774,5 @@
>          final boolean isBookmarkAction = GeckoApp.ACTION_HOMESCREEN_SHORTCUT.equals(action);
>  
> +        final String args = intent.getStringExtra("args");
> +
> +        // If an external intent tries to start Fennec in guest mode, and its not already

its -> it's

::: mobile/android/base/GeckoProfile.java
@@ +88,5 @@
>              }
>          }
>  
>          // If the guest profile exists and is locked, return it
> +        if (GuestSession.shouldUse(context, "")) {

I'm scared that this will end up returning true in situations we don't want. What happens when a CP calls GeckoProfile.get() and the screen is locked?

@@ +196,5 @@
>              getGuestDir(context).mkdir();
>              GeckoProfile profile = getGuestProfile(context);
> +
> +            if (!GuestSession.isSecureKeyguardLocked(context)) {
> +                profile.lock();

Can you explain why profile.lock() depends on this condition?

@@ +226,5 @@
>          }
>          return sGuestDir;
>      }
>  
> +    static GeckoProfile getGuestProfile(Context context) {

I think we're generally trying to avoid package-level scope, so this should be public.

::: mobile/android/base/GuestSession.java
@@ +25,5 @@
> +
> +// Utility methods for entering/exiting guest mode.
> +public final class GuestSession {
> +    private static final String LOGTAG = "GeckoGuestSession";
> +    // Returns true if the user is using a secure keyguard, and its currently locked.

Nit: newline before comment

@@ +34,5 @@
> +
> +    // Returns true if you should be in guest mode. This can be because a secure keyguard
> +    // is locked, or because the user has explicitly started guest mode via a dialog. If the
> +    // user has explicitly started Fennec in guest mode, this will return true until they
> +    // explicitly exit it.

Use JavaDocs

@@ +36,5 @@
> +    // is locked, or because the user has explicitly started guest mode via a dialog. If the
> +    // user has explicitly started Fennec in guest mode, this will return true until they
> +    // explicitly exit it.
> +    public static boolean shouldUse(final Context context, final String args) {
> +        // Did the command line args requested guest mode?

requested -> request

Comment 42

[Brian Nicholson (:bnicholson)]

Comment on attachment 8472100 [details] [diff] [review]
Patch 2/2 - Enable configuring widgets

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

I think this mostly looks OK, but the search resources need to be moved into Fennec resources as discussed above.

::: mobile/android/base/BrowserApp.java
@@ +2827,5 @@
> +                flags |= Tabs.LOADURL_PRIVATE;
> +            }
> +
> +            Tabs.getInstance().loadUrl(uri, flags);
> +        } else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {

So we get ACTION_WEBAPP_PREFIX in BrowserApp? Does this functionality even exist still, or is this just here for legacy reasons?

::: mobile/android/search/java/org/mozilla/search/SearchWidget.java
@@ -27,5 @@
>  
>      final public static String ACTION_LAUNCH_BROWSER = "org.mozilla.widget.LAUNCH_BROWSER";
>      final public static String ACTION_LAUNCH_SEARCH = "org.mozilla.widget.LAUNCH_SEARCH";
>  
> -    @SuppressLint("NewApi")

Why remove this?

@@ +33,5 @@
>      @Override
>      public void onUpdate(final Context context, final AppWidgetManager manager, final int[] ids) {
>          for (int id : ids) {
>              final Bundle bundle;
> +            if (Build.VERSION.SDK_INT >= 16) {

Why did you change this from "AppConstants.Versions.feature16Plus"?

@@ -43,5 @@
>  
>          super.onUpdate(context, manager, ids);
>      }
>  
> -    @SuppressLint("NewApi")

Why remove this?

Comment 43

[Wesley Johnston (:wesj)]

r.e. Sync, rnewman, do you know if sync is always explicit about the profile to save data into? With this patch asking for the "current" profile (via. GeckoProfile.get(context)); will return Guest if the lock screen is up (and password protected). If Sync isn't passing "default" along with its requests, it'll end up reading/writing to the guest profile, even if you've never used it.

I feel like we've fixed this bug, but wanted to make sure the solution will work here.

Comment 44

[Richard Newman [:rnewman]]

After Bug 957131, I think it's always explicit.

Sync doesn't use GeckoProfile, so the only implicitness would be inside the CPs. If there's a bug, it would currently apply to active guest mode browsing, too.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Patch

Just moved everything into the search widget for now. If that is missing the next train but we want this, we can pull it apart then.

> For one, do we have some kind of clear UI indicating that the user is going
> to go into guest mode when the screen is locked?

The indicator in bug 897711 should help. We could also use the banner or something (the banner is disabled in guest mode right now). No plans there yet though.

> Will users understand what's going on when their "Exit guest mode" menu
> option is gone? Maybe we should keep the button in the menu, but have it
> show a toast/dialog telling the user why they can't leave.

I put this back in. It doesn't restart Fennec anymore, just clears the guest profile and exits. Seems better.

> I'm scared that this will end up returning true in situations we don't want.
> What happens when a CP calls GeckoProfile.get() and the screen is locked?

I agree there some scariness here. Looks like Sync is ok, and we can't really force Content Providers to write into anything. I think we'll land this early and have to watch closely for regressions.... I wish I had a better solution than that.

> Can you explain why profile.lock() depends on this condition?
So the goal I had here was

1.) If the user explicitly starts guest mode, they stay in guest mode until they explicitly exit it. In this case, we lock the profile
2.) If the user is put into guest mode because the phone is locked, they will will leave it if they run Fennec unlocked. We do that by NOT locking the profile.

locking the profile is basically our cue as to whether Fennec should use guest or not when the phone is unlocked (when the phone is locked, we always use guest).

Comment 46

[Brian Nicholson (:bnicholson)]

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

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

This patch still scares me, and I have this nagging feeling that it will introduce subtle profile-based regressions. But I can't think of what those might be, and sadly, I can't test this on my phone, so r+ from me with a few questions.

Also adding r? to rnewman since I think he has a fair amount of experience with GeckoProfile. rnewman, if you're able to give this a trial run and could try hitting on some sync/CP edge cases, that would very useful!

::: mobile/android/base/BrowserApp.java
@@ +2849,5 @@
> +        // in guest mode, this will change modes before opening the url.
> +        final boolean enableGuestSession = GuestSession.shouldUse(this, args);
> +        final boolean inGuestSession = GeckoProfile.get(this).inGuestMode();
> +        if (enableGuestSession != inGuestSession) {
> +            doRestart(enableGuestSession ? GUEST_BROWSING_ARG : "");

So does the intent just get dropped here? If not, how does it survive the restart? AFAICT, doRestart() launches a new intent of its own and doesn't try to forward this one.

::: mobile/android/base/GeckoProfile.java
@@ +199,5 @@
>              GeckoProfile profile = getGuestProfile(context);
> +
> +            // If we're creating this guest session over the keyguard, don't lock it.
> +            // This will force the guest session to exit if the user unlocks their phone
> +            // and starts Fennec.

So if I understand this correctly, we're relying on the new args intent above to switch us out of guest mode. Does that mean if they use the Android task switcher to get back to Fennec, they'll still be in guest mode?

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +290,5 @@
>      protected void onCreate(Bundle savedInstanceState) {
> +        if (GeckoProfile.get(this).inGuestMode()) {
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_DISMISS_KEYGUARD);
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_SHOW_WHEN_LOCKED);
> +        }

I wish we didn't have to do this. But it makes me wonder: why do guests need access to settings at all? Guest profiles are temporary, so it seems like we should be able to strip out bookmarks, add-ons, apps, settings, etc.... But for a different bug, of course.

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Follow up

(In reply to Brian Nicholson (:bnicholson) from comment #46)
> So does the intent just get dropped here? If not, how does it survive the
> restart? AFAICT, doRestart() launches a new intent of its own and doesn't
> try to forward this one.

Yeah, probably. I guess you might hit this if you launched Fennec from the lock screen. Then unlocked your phone and clicked a link from email. I fixed this here by letting you pass an intent to the restarter that it will automatically use.

> So if I understand this correctly, we're relying on the new args intent
> above to switch us out of guest mode. Does that mean if they use the Android
> task switcher to get back to Fennec, they'll still be in guest mode?

Yeah, good catch. We don't depend entirely on args, but the task switcher was an entrance point I forgot about. I moved this check to onResume()... Not sure of any better way to catch us starting from the recent apps list.

Comment 48

[Brian Nicholson (:bnicholson)]

Comment on attachment 8485254 [details] [diff] [review]
Follow up

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

Pretty ugly, but I suppose this works. Flagging rnewman for a second look.

Comment 49

[Richard Newman [:rnewman]]

Comment on attachment 8485254 [details] [diff] [review]
Follow up

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

::: mobile/android/base/BrowserApp.java
@@ +705,5 @@
>          super.onBackPressed();
>      }
>  
>      @Override
>      public void onResume() {

What happens if you enter Guest Mode in this way, then hit the power button (pausing the app), then wake it up again? Does onResume run again, causing us to restart the app?

@@ +710,5 @@
>          super.onResume();
> +
> +        final String args = getIntent().getStringExtra("args");
> +        // If an external intent tries to start Fennec in guest mode, and it's not already
> +        // in guest mode, this will change modes before opening the url.

This seems like it could be user-hostile, no?

You have your browser open on your tablet playing music. The screen is locked. Your friend grabs it and fires up Guest Mode. Your music stops playing.

(And your session ends, maybe you lose your tabs, clear history runs…)

Are there alternatives?

Can we define a new activity (alias?) in a different process, and use that to run Guest Mode?

Can we at least show a warning/confirmation -- "Entering guest mode will end your current session" -- if the user has unzombified tabs and doesn't have Restore Previous Session turned on?

If the answer is "not really", then fine, but I want to make sure we haven't excluded either mitigations or better approaches before we put this in the "done" pile.

Comment 50

[Richard Newman [:rnewman]]

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

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

::: mobile/android/base/BrowserApp.java
@@ +475,5 @@
> +
> +        if (GuestSession.shouldUse(this, args)) {
> +            // In guest sessions we allow showing over the keyguard.
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_DISMISS_KEYGUARD);
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_SHOW_WHEN_LOCKED);

Maybe bundle these into GuestSession.configureWindow()?

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +289,5 @@
>      @Override
>      protected void onCreate(Bundle savedInstanceState) {
> +        if (GeckoProfile.get(this).inGuestMode()) {
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_DISMISS_KEYGUARD);
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_SHOW_WHEN_LOCKED);

… code reuse ftw!

@@ +290,5 @@
>      protected void onCreate(Bundle savedInstanceState) {
> +        if (GeckoProfile.get(this).inGuestMode()) {
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_DISMISS_KEYGUARD);
> +            getWindow().addFlags(WindowManager.LayoutParams.FLAG_SHOW_WHEN_LOCKED);
> +        }

Re Brian's point about guest access to settings: 

* Maybe folks use Guest Mode as a quick throwaway profile for experimentation? We don't have a profile switcher, after all.
* Font size.
* Language.
* Etc.

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Prompt on start Patch

(In reply to Richard Newman [:rnewman] from comment #49)
> (And your session ends, maybe you lose your tabs, clear history runs…)
> 
> Are there alternatives?

This mostly works, but needs some polishing. i.e. the guest mode dialog shows twice. I have no idea why. I only see show() called once. It also has some strange races with content providers. I'd really like to just do this in a follow up if we want it.

The interactions get tricky. We have to

1.) set flags on normal Fennec's windows to allow them to show over the keyguard,
2.) show the dialog (which may leak private info to the guest user, so really we need a whole new dialog).
2a.) If they hit cancel, we have to turn off the keyguard mode.
2b.) If they hit "Restart" we restart.
3.) A little flashing about as the keyguard shows then disappears, and things "work".

Thats the simple steps, but there are edge cases in here. For instance, I try not to show the dialog if you move from lock screen session to non-lockscreen session (i.e. don't get in your way), so onResume has to be special cased for that.

Or we also try not to restart when you hit "Exit guest session" while the keyguard is up. That means the the dialog result has to know not to restart sometimes.

Throw in some strange flashing as you move between states and some angry content providers. I would much rather land this as is and deal with this edge case in a follow up (or never). I'd like to pull this into a "GuestSession.shouldShowDialog()" method or something and then change the callers to just GuestSession.start() or leave(), so that we don't have so if-elses scattered in BrowserApp. I'd like to pull apart the Dialog listeners so that we don't have one for both dialogs. But I'd like to do it in a separate bug.

This feature isn't too hard to disable now. The widget is trivial to remove. We can comment out configureWindow() to make sure we never allow Fennec showing over the keyguard. Should be easy to hold back if we need to.

Comment 52

[Wesley Johnston (:wesj)]

Try seemed happy. Landed:
https://hg.mozilla.org/integration/fx-team/rev/3de21c283716

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android S4 failures.
https://hg.mozilla.org/integration/fx-team/rev/3b61d93b670d

https://tbpl.mozilla.org/php/getParsedLog.php?id=47835049&tree=Fx-Team

Comment 54

[Wesley Johnston (:wesj)]

Tracked this down to some strangeness on the build machines. They're running Android 4.2, but don't seem to have a KeyguardManager Service? I added a null check to bail on them.

https://hg.mozilla.org/integration/fx-team/rev/f1a36cb2bba6

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1a36cb2bba6

Comment 56

[Lawrence Mandel [:lmandel] (use needinfo)]

I reviewed this with mfinkle. There's no need to uplift this feature. I have removed the tracking flag.

Comment 57

[Jared Wein [:jaws] (please needinfo? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature and way of advertising guest browsing, great for sharing phones
[Suggested wording]: Open Firefox from your lock screen without sharing any of your personal information.
[Links (documentation, blog post, etc)]: Someone needs to blog about it! :)

Comment 58

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Open Firefox from your lock screen without sharing any of your personal information" as wording.

Comment 59

[Aaron Train [:aaronmt]]

Is this first iteration shipping? There was talk of going back to the drawing board because the current widget doesn't really act nor feel like a traditional lockscreen widget

Comment 60

[Richard Newman [:rnewman]]

Wes: which of these patches still need review/feedback? Did they get spun out into follow-ups?

Comment 61

[Wesley Johnston (:wesj)]

sure. Only one needs review still:
Bug 1074496 - Disable import from Android in Guest mode. Needs review. I flipped it to you richard.

The rest for the curious:
Bug 1078182 - crash in java.lang.NullPointerException: at org.mozilla.search.SearchWidget.addView(SearchWidget.java). Ready to land. Will do.
Bug 1046941 - Disable downloads and extension installs in guest mode. Follow up needs to land.

Bug 1073569 - [Breakdown] Investigate ways to advertise/teach about the lock screen widget from within Fennec . Not blocking. No patch or UX yet.
Bug 1073567 - Add a clock to the lock screen widget. Needs UX.
Bug 1073135 - External app URL bar button shows when in lockscreen guest mode. I thought this was basically WONTFX.
Bug 1072832 - Lock screen flashes between different settings menus if Fennec is launched from lock screen. I have no idea if this is even fixable.
Bug 1054036 - Block downloads in GuestMode inside HelperAppDialog.js. I tried this, but not all downloads actually go through here (for instance, Save Image doesn't). I decided against redudant checks for now.