Closed Bug 815682 (fennec-lockscreen) Opened 7 years ago Closed 5 years ago

Lock screen widget/shortcut for Guest Sessions on Android 4.2 and above

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
relnote-firefox --- 35+
fennec - ---

People

(Reporter: blassey, Assigned: wesj, Mentored)

References

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

Details

(Whiteboard: [lang=java])

Attachments

(4 files, 12 obsolete files)

1.04 MB, image/png
Details
23.97 KB, patch
bnicholson
: review+
rnewman
: review+
Details | Diff | Splinter Review
6.23 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
6.01 KB, patch
Details | Diff | Splinter Review
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
tracking-fennec: --- → ?
tracking-fennec: ? → 21+
Needs design?
Flags: needinfo?(ibarlow)
Keywords: uiwanted
I wonder if this would be an opportunity to explore some of Lucas's just-in-time search thinking?
Flags: needinfo?(ibarlow)
Assignee: nobody → ibarlow
Keywords: uiwanted
tracking-fennec: 21+ → -
Assignee: ibarlow → nobody
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.
(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.
(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.
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.
We may not be able to consume 100% of the screen real estate though. This is a screenshot of the CBS Sports lockscreen widget.
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)
I thought the Guest Browsing idea is just adding a simple button (icon) widget that will launch Firefox in Guest Mode.
In fact, I don't think we should be trying to make a browser anything on the lock screen.
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.
(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.
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.
Cool, so an icon that launches a guest session may be enough, if it can exit back to the lock screen.
(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.
Summary: lock screen widget for Android 4.2 → Lock screen widget/shortcut for Guest Sessions on Android 4.2 and above
Whiteboard: [mentor=margaret][lang=java]
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=java] → [lang=java]
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8455965 - Attachment is obsolete: true
Attachment #8456546 - Flags: review?(mark.finkle)
Depends on: widget
Blocks: 1039037
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
Attachment #8456546 - Flags: review?(mark.finkle) → review+
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8456546 - Attachment is obsolete: true
Attachment #8465670 - Flags: feedback?(mark.finkle)
Attached patch WIP Config (obsolete) — Splinter Review
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.
Attached image Configure screen (obsolete) —
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 :) )
Attached image Home screen widgets (obsolete) —
Here's all three of these options on home. Obviously the text/icons need some love.
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?
(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"
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..)
Flags: needinfo?(ibarlow)
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.
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)
Flags: needinfo?(ibarlow)
Attached patch Patch 2/2 (obsolete) — Splinter Review
Attachment #8468706 - Flags: review?(bnicholson)
Attached image Screenshot (obsolete) —
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.
Attached image Lock screen (obsolete) —
Attachment #8465816 - Attachment is obsolete: true
Attachment #8465818 - Attachment is obsolete: true
Attachment #8465820 - Attachment is obsolete: true
Attached image Configure screen (obsolete) —
Attached patch Patch 2/2 (obsolete) — Splinter Review
Forgot to add the images.
Attachment #8468706 - Attachment is obsolete: true
Attachment #8468706 - Flags: review?(bnicholson)
Attachment #8468739 - Flags: review?(bnicholson)
[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...
Blocks: 1004715
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?
Attachment #8468739 - Flags: review?(bnicholson) → review-
Depends on: 1046941
Depends on: 1052254
Depends on: 1046885
Attachment #8468739 - Attachment is obsolete: true
Attachment #8472100 - Flags: review?(bnicholson)
Comment on attachment 8465670 [details] [diff] [review]
Patch v2

Brian, you want to review all of this?
Attachment #8465670 - Flags: feedback?(mark.finkle) → review?(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?
Flags: needinfo?(wjohnston)
> 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.
Flags: needinfo?(wjohnston)
Depends on: 1054035
Depends on: 1054036
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
Attachment #8465670 - Flags: review?(bnicholson) → review-
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?
Attachment #8472100 - Flags: review?(bnicholson) → feedback+
Blocks: 1058149
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.
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Attached patch PatchSplinter Review
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).
Attachment #8465670 - Attachment is obsolete: true
Attachment #8468708 - Attachment is obsolete: true
Attachment #8468709 - Attachment is obsolete: true
Attachment #8468710 - Attachment is obsolete: true
Attachment #8472100 - Attachment is obsolete: true
Attachment #8481602 - Flags: review?(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.
Attachment #8481602 - Flags: review?(rnewman)
Attachment #8481602 - Flags: review?(bnicholson)
Attachment #8481602 - Flags: review+
Attached patch Follow upSplinter Review
(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.
Attachment #8485254 - Flags: review?(bnicholson)
Depends on: 1058150
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.
Attachment #8485254 - Flags: review?(rnewman)
Attachment #8485254 - Flags: review?(bnicholson)
Attachment #8485254 - Flags: review+
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.
Flags: needinfo?(wjohnston)
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.
Attachment #8481602 - Flags: review?(rnewman) → review+
(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.
Attachment #8486655 - Flags: feedback?(rnewman)
Flags: needinfo?(wjohnston)
Blocks: 1065752
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
https://hg.mozilla.org/mozilla-central/rev/f1a36cb2bba6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
I reviewed this with mfinkle. There's no need to uplift this feature. I have removed the tracking flag.
Depends on: 1072376
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! :)
relnote-firefox: --- → ?
Alias: lockscreen
Depends on: 1072832
Depends on: 1073135
Depends on: 1074496
Added to the release notes with "Open Firefox from your lock screen without sharing any of your personal information" as wording.
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
Depends on: 1078182
Wes: which of these patches still need review/feedback? Did they get spun out into follow-ups?
Flags: needinfo?(wjohnston)
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.
Flags: needinfo?(wjohnston)
Depends on: 1082201
Alias: lockscreen → fennec-lockscreen
Depends on: 1072091
Depends on: 1109165
Attachment #8485254 - Flags: review?(rnewman)
Attachment #8486655 - Flags: feedback?(rnewman)
You need to log in before you can comment on or make changes to this bug.