Closed
Bug 896121
Opened 12 years ago
Closed 11 years ago
[guest] Disable 'share' menus in guest mode
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox24 unaffected, firefox25 verified, firefox26 verified)
VERIFIED
FIXED
Firefox 26
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | --- | verified |
firefox26 | --- | verified |
People
(Reporter: mfinkle, Assigned: wesj)
References
Details
Attachments
(3 files, 8 obsolete files)
4.10 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
16.49 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.73 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
We do not want a guest to be able to share data using the owner's share apps accounts. We should disable the menus. We should even add a fail-safe in the core sharing code to make sure it doesn't slip through.
https://wiki.mozilla.org/Mobile/Projects/Guest_mode
Reporter | ||
Updated•12 years ago
|
Blocks: guest-mode
Assignee | ||
Comment 1•12 years ago
|
||
Again, depends on the patch in bug 871863....
Attachment #779479 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 2•12 years ago
|
||
Comment on attachment 779479 [details] [diff] [review]
patch
I don't think this handles the other share menus, right?
Attachment #779479 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 3•12 years ago
|
||
This disables the share menus everywhere.
We don't actually support disabling contextmenu items (that aren't html5), so this adds the ability to return a string "disabled" from your selector (rather than the normal true or false). I like adding the ability, and I like showing these disabled rather than just hiding them if we're in guest mode, but it does feel a bit like a hack.
Attachment #779479 -
Attachment is obsolete: true
Attachment #783550 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 783550 [details] [diff] [review]
Patch
You said you would re-do this patch to be less complex and just hide menus. Other feedback:
* Since we have "docShell.usePrivateBrowsing" and we use "isPrivate" in browser.js a lot, let use BrowserApp.isGuest
* In BrowserCLH, let's drop the "mode" and just use "guest". That includes the passed param, rename "guest-mode" to "guest".
If we used the long form more often (usePrivateBrowsing) I would suggest using isGuestBrowsing.
Assignee | ||
Comment 5•12 years ago
|
||
This is the basic feature. Disables sharing and saving. For JS context menu items, the items are hidden entirely. The native UI items are just disabled.
Attachment #783550 -
Attachment is obsolete: true
Attachment #783550 -
Flags: review?(mark.finkle)
Attachment #784838 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 6•12 years ago
|
||
This adds the disabled piece back for js context menu items.
Attachment #784839 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 784838 [details] [diff] [review]
Patch 1/2
>diff --git a/mobile/android/base/awesomebar/AwesomeBarTab.java b/mobile/android/base/awesomebar/AwesomeBarTab.java
> menu.findItem(R.id.open_new_tab).setVisible(!isPrivate);
> menu.findItem(R.id.open_private_tab).setVisible(isPrivate);
>+ menu.findItem(R.id.share).setEnabled(GeckoProfile.get(mContext).inGuestMode());
I think you want (!GeckoProfile.get(mContext).inGuestMode());
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.shareLink"),
>- NativeWindow.contextmenus.linkShareableContext,
>+ NativeWindow.contextmenus._disableInGuestMode(NativeWindow.contextmenus.linkShareableContext),
I wonder how complicated this will become when we have 2 or 3 more boolean states
>+ _disableInGuestMode: function _disableInGuestMode(selector) {
_disableInGuestMode -> disableInGuest or disableInGuestBrowsing
>+ return {
>+ matches: function _disableInGuestModeMatches(aElement, aX, aY) {
>+ let result = selector.matches(aElement, aX, aY);
>+ if (result && BrowserApp.isGuest)
Maybe you should do the isGuest check first and return early
>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js
>+ guest = aCmdLine.handleFlag("guest-mode", false);
Where is this passed to Gecko? Also "guest-mode" -> "guest"
Attachment #784838 -
Flags: review?(mark.finkle) → review-
Reporter | ||
Comment 8•12 years ago
|
||
I checked in with Ian about showing diabled "Share" menus or just hiding "Share" menus:
<mfinkle> ibarlow, for guest browsing, do we want the show disabled "Share" menus everywhere
or just hide them?
we typically don't show disabled context menus
<ibarlow> mfinkle: probably hide them yeah
So when you update Part 1, remember to hide the Share menus, not just disable. We need Part 2 at this time.
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #7)
> I think you want (!GeckoProfile.get(mContext).inGuestMode());
Doh!
> Maybe you should do the isGuest check first and return early
Good idea. This was just fallout from my disable idea...
> Where is this passed to Gecko? Also "guest-mode" -> "guest"
This is the same arg from here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1816
I'll update those as well....
Assignee | ||
Comment 10•12 years ago
|
||
Updated to hide the menu items. Had to move the GuestProfile check in GeckoProfile.get because the Awesomescreen is not a GeckoApp.
Attachment #784838 -
Attachment is obsolete: true
Attachment #785096 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 11•12 years ago
|
||
Comment on attachment 785096 [details] [diff] [review]
Patch
>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>+ private static final String GUEST_MODE_ARGUMENT = "--guest";
GUEST_MODE_ARGUMENT -> GUEST_BROWSING_ARG
> // Disable save as PDF for about:home and xul pages
> saveAsPDF.setEnabled(!(tab.getURL().equals("about:home") ||
> tab.getContentType().equals("application/vnd.mozilla.xul+xml")));
>+ saveAsPDF.setVisible(!GeckoProfile.get(this).inGuestMode());
Do we want to turn off Save as PDF ?
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>- NativeWindow.contextmenus.linkShareableContext,
>+ NativeWindow.contextmenus._disableInGuestMode(NativeWindow.contextmenus.linkShareableContext),
_disableInGuestMode -> disableInGuest or disableInGuestBrowsing
>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js
>+ try {
>+ guest = aCmdLine.handleFlag("guest", false);
>+ } catch (e) { /* Optional */ }
You are assuming that the "guest" arg is passed to GeckoApp every time Fennec starts up. This only happens the first time you switch to a guest session. If there is an OOM or the user swipes close via recent apps, and then starts again, we are still in Guest Browsing, but the "guest" arg will not be sent.
I think you could add "guest" here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#104
Can't r+ without making sure we handle all the "starting in guest session" cases.
Attachment #785096 -
Flags: review?(mark.finkle) → review-
Reporter | ||
Comment 12•12 years ago
|
||
Comment on attachment 784839 [details] [diff] [review]
Patch 2/2
Not needed just yet
Attachment #784839 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 13•12 years ago
|
||
Good catch. This passes the guest argument in GeckoThread.
I also realized I made a last minute mistake in the session restore patch. I moved the getGuestSession check from GeckoProfile.get outside of the if (context instanceof GeckoApp) check for some real reason, but that means that we aren't hitting the guest section part anymore. I can't really think of any time when we're not in a GeckoApp that we care about guest mode, so this new one does:
if we're in GeckoApp
if we have cached session, return it
if we're in BrowserApp and in guest mode, return it (don't do this for webapps!)
return the default session for this app
else return GeckoProfile's default.
Attachment #784839 -
Attachment is obsolete: true
Attachment #785096 -
Attachment is obsolete: true
Attachment #786421 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 14•12 years ago
|
||
Also, I left in hiding "Save as PDF", but I'm not sure if we want it or not. It doesn't really affect "accounts", so maybe we do.
Flags: needinfo?(ibarlow)
Comment 15•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #14)
> Also, I left in hiding "Save as PDF", but I'm not sure if we want it or not.
> It doesn't really affect "accounts", so maybe we do.
We can probably keep it. Taking it out would mean we would probably want to disallow saving images and so on as well, which I'm not sure we want to get into.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 16•11 years ago
|
||
I ran into another problem while updating this. Namely, on the awesomescreen calling GeckoProfile.get(context) uses a non-GeckoApp context (until tomorrow whent this activity dies). Since we want to uplift this, this just makes the profile manager more aware of AwesomeBar and that we can be in guestmode while the awesomebar is open, but not for WebApps.
As a longer term fix, I've got a WIP I'm gonna throw up in Bug 902060.
Attachment #786421 -
Attachment is obsolete: true
Attachment #786421 -
Flags: review?(mark.finkle)
Attachment #792387 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 17•11 years ago
|
||
This is the real patch again, with the "Save" menu items re-added.
Attachment #792389 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 18•11 years ago
|
||
Comment on attachment 792387 [details] [diff] [review]
Patch 0/1
>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java
> public static GeckoProfile get(Context context) {
> if (context instanceof GeckoApp) {
>+ if (context instanceof BrowserApp || context instanceof AwesomeBar) {
>+ if (context instanceof GeckoApp) {
This looks a little odd. Did you want the first and last to be GeckoApp checks? In general, I am OK with this even though I dislike it. I'm OK because I know we are working to remove these ugly dependencies.
Attachment #792387 -
Flags: review?(mark.finkle) → feedback+
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 792389 [details] [diff] [review]
Patch 1/1
>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java
> public static GeckoProfile get(Context context) {
> if (context instanceof GeckoApp) {
> // Check for a cached profile on this context already
> // TODO: We should not be caching profile information on the Activity context
> if (((GeckoApp)context).mProfile != null) {
> return ((GeckoApp)context).mProfile;
> }
>-
>- GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>- // if the guest profile exists and is locked, return it
>- if (guest != null && guest.locked()) {
>- return guest;
>+ if (context instanceof BrowserApp) {
>+ GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>+ // if the guest profile exists and is locked, return it
>+ if (guest != null && guest.locked()) {
>+ return guest;
>+ }
> }
>-
> // Otherwise, get the default profile for the Activity
> return get(context, ((GeckoApp)context).getDefaultProfileName());
> }
This looks different than part 0/1. Do you need to rebase this patch?
Will r+ after we chat about the GeckoProfile.get questions
Attachment #792389 -
Flags: review?(mark.finkle) → feedback+
Assignee | ||
Comment 20•11 years ago
|
||
This is the idea we talked about a bit. I kinda like the other patch better to be honest, but this ships the "guestmode" flag over to the awesomebar activity in the intent.
Two more patches coming to do this same thing with the new awesomebar, and to handle the rest of our share endpoints.
Attachment #792387 -
Attachment is obsolete: true
Attachment #794307 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #792389 -
Attachment is obsolete: true
Attachment #794314 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #794315 -
Flags: review?(mark.finkle)
Reporter | ||
Updated•11 years ago
|
Attachment #794315 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 23•11 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #20)
> Created attachment 794307 [details] [diff] [review]
> Aurora awesomebar patch
>
> This is the idea we talked about a bit. I kinda like the other patch better
> to be honest, but this ships the "guestmode" flag over to the awesomebar
> activity in the intent.
I like this one. Less noise in GeckoProfile and less use of a singleton in the awesomebar.
Reporter | ||
Comment 24•11 years ago
|
||
Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch
># HG changeset patch
># Parent 2d1848f275f663d3e3ffb60ec3a7ae94ad283dc0
># User Wes Johnston <wjohnston@mozilla.com>
>
>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>--- a/mobile/android/base/AwesomeBar.java
>+++ b/mobile/android/base/AwesomeBar.java
>@@ -59,18 +59,21 @@ public class AwesomeBar extends GeckoAct
> public static final String URL_KEY = "url";
> public static final String TAB_KEY = "tab";
> public static final String CURRENT_URL_KEY = "currenturl";
> public static final String TARGET_KEY = "target";
> public static final String SEARCH_KEY = "search";
> public static final String TITLE_KEY = "title";
> public static final String USER_ENTERED_KEY = "user_entered";
> public static final String READING_LIST_KEY = "reading_list";
>+ public static final String GUEST_MODE_KEY = "guest_mode_enabled";
MODE? 1 MILLION YEARS DUNGEON!
"guest"
>+ public boolean inGuestMode = false;
isGuest
Attachment #794307 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #794314 -
Flags: review?(mark.finkle) → review+
Updated•11 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 794314 [details] [diff] [review]
Patch for BrowserApp and JS
[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: Guest mode shows "Share" options directly linking into the device owners account
Testing completed (on m-c, etc.): Landed on mc 8/26
Risk to taking this patch (and alternatives if risky): Low risk. Guest mode isn't a normal way to run. Just removing some context menu items. Alternative would probably be to back the feature out or just ship without this polish in it for v1?
String or IDL/UUID changes made by this patch: None.
Attachment #794314 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: Guest mode shows "Share" options directly linking into the device owners account
Testing completed (on m-c, etc.): Because of huge architectural changes, this patch is specially made for Aurora and is completely different from the mc one. Tested locally on Aurora.
Risk to taking this patch (and alternatives if risky): Low risk. Guest mode isn't a normal way to run. Just removing some context menu items, sending a new extra to the awesomebar. Alternative would probably be to back the feature out or just ship without this polish in it for v1?
String or IDL/UUID changes made by this patch: None.
Attachment #794307 -
Flags: approval-mozilla-aurora?
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8870bca1edd3
https://hg.mozilla.org/mozilla-central/rev/b46cafdfc8ba
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
status-firefox26:
--- → verified
Comment 29•11 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #27)
> Risk to taking this patch (and alternatives if risky): Low risk. Guest mode
> isn't a normal way to run. Just removing some context menu items, sending a
> new extra to the awesomebar. Alternative would probably be to back the
> feature out or just ship without this polish in it for v1?
Not a critical issue (not worth tracking), but also a very simple patch. Wouldn't recommend backing out over this.
Comment 30•11 years ago
|
||
Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch
Forgot to approve apparently.
Attachment #794307 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #794314 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 31•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c619a15e9e2
https://hg.mozilla.org/releases/mozilla-aurora/rev/15344ea24f5b
status-firefox25:
--- → fixed
Comment 32•11 years ago
|
||
Verified fixed on:
Build: Firefox for Android 25.0a2 (2013-09-12)
Device: LG Nexus 4
OS: Android 4.2.2
Updated•11 years ago
|
tracking-fennec: ? → ---
status-firefox24:
--- → unaffected
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•