KidFox: Restricted profiles - Hide other non-essential menu items - All “Tool” menu items

VERIFIED FIXED in Firefox 42

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jchaulk, Assigned: sebastian)

Tracking

Trunk
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 verified)

Details

Attachments

(3 attachments)

Comment hidden (empty)
Blocks: FFB
OS: Windows 8.1 → Android
Hardware: x86_64 → All
Version: unspecified → Trunk
(Reporter)

Updated

4 years ago
Blocks: kidfox-v1
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally
Attachment #8629933 - Flags: review?(ally)
(In reply to :Sebastian Kaspari from comment #1)
> Created attachment 8629933 [details]
> MozReview Request: Bug 1125312 - Restriced profiles: Hide "Tools" menu item.
> r=ally

This patch hides the "tools" menu entry by hard-coding the restriction for restricted profiles. Instead we probably want to make these restrictions configurable and read them from Android's user manager. I filed bug 1180653 for this.
Bug 1125313 - Hide "Report Site Issue" menu item for restricted profiles. r=ally
Attachment #8630003 - Flags: review?(ally)
Mh, I wanted the review board entry to span two bugs (because the patches are intermingled) but it seems like this is not possible. Both changesets are attached here.
(In reply to :Sebastian Kaspari from comment #5)
> Mh, I wanted the review board entry to span two bugs (because the patches
> are intermingled) but it seems like this is not possible. Both changesets
> are attached here.

You'll probably need to file a mozRB bug for that feature if you think it would be useful in general.
Sebastian, in which order do you have these patches applied? I would like to take this code for a spin before completing the review.
Flags: needinfo?(s.kaspari)
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #7)
> Sebastian, in which order do you have these patches applied? I would like to
> take this code for a spin before completing the review.

First fa7db3ce0e9e ("tools" menu item) then c5bd074907b8 ("report site issue" menu item). Maybe I should squash them to one commit.

Some background:
* I use the already existing class RestricedProfiles and added the new restrictions for the menu items

fa7db3ce0e9e
* The "tools" menu item is hidden in Java code (onPrepareOptionsMenu)  based on the restriction. 

c5bd074907b8
* The "report site issue" item is added from JavaScript (WebcompatReporter.js), so I read the restrictions on the JavaScript side and set the item to invisible.
* In this patch I also refactored RestrictedProfiles a bit to separate restrictions for guest profiles (the class is also used for these) from restrictions for restricted profiles.
Flags: needinfo?(s.kaspari)
Attachment #8629933 - Flags: review?(ally)
Comment on attachment 8629933 [details]
MozReview Request: Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally

https://reviewboard.mozilla.org/r/12657/#review11359

Thanks for providing context about the refactor. You should probably land these as one big commit, so that there's no funny business if it has to be backed out for whatever reason.

I think we should file some bugs for getting tests up for kidfox. We might need to talk to a-team (automation & tools) about getting automated restricted profiles, but let's get bugs on file for our side.

::: toolkit/components/parentalcontrols/nsIParentalControlsService.idl:36
(Diff revision 1)
>     */ 

Not your nit, but could you remove this trailing whitespace while we're in this code.

::: mobile/android/base/RestrictedProfiles.java:176
(Diff revision 1)
> +            // Hardcoded restriction. Make restriction configurable and read from UserManager (Bug 1180653)

thanks for documenting
Comment on attachment 8630003 [details]
MozReview Request: Bug 1125313 - Hide "Report Site Issue" menu item for restricted profiles. r=ally

https://reviewboard.mozilla.org/r/12667/#review11361

Thanks!

::: mobile/android/base/RestrictedProfiles.java:91
(Diff revision 1)
> +    private static List<Restriction> restrictionsOfGuestProfile = Arrays.asList(

I like this. :)

When we dicussed the implementation after the meeting with Product, I was concerned about pitfalls from conflating guest mode with kidfox.

::: mobile/android/chrome/content/WebcompatReporter.js:23
(Diff revision 1)
> +    if ("@mozilla.org/parental-controls-service;1" in Cc) {

I look forward to making these more configurable. :)

::: mobile/android/chrome/content/WebcompatReporter.js:21
(Diff revision 1)
> +    // Menu item can be hidden when restricted profile is in use (Bug 1125313)

As much as I love commented code, I don't think this adds much.
Attachment #8630003 - Flags: review?(ally) → review+
Comment on attachment 8629933 [details]
MozReview Request: Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally

https://reviewboard.mozilla.org/r/12657/#review11363

Ship It!
Attachment #8629933 - Flags: review+
I updated the patch and pushed as a single commit.
https://hg.mozilla.org/mozilla-central/rev/64d240b53e52
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
https://reviewboard.mozilla.org/r/12667/#review11447

::: mobile/android/chrome/content/WebcompatReporter.js:28
(Diff revision 1)
> +    this.addMenuItem(visible);

I'm late to see this, but I don't think we actually need to worry about hiding this, since this menu item is only available for Nightly users, and I don't think we have any plan to ever expose it more broadly.

Updated

4 years ago
Duplicate of this bug: 1125313
The patch for this seems to cause serious problems on the Android M Preview; every page fails to load (reporting "The address wasn't understood"), backing out the changeset fixes it. I'm looking into it further.
I think Sebastian has an Android M device and can help look at the issue.
Flags: needinfo?(s.kaspari)
Posted patch Updated patchSplinter Review
Updates isUserRestricted to check for any restrictions that are true, rather than for any restrictions at all. This fixes a problem in Android M preview that was causing all pageloads to fail (as in comment 17).
Attachment #8632297 - Flags: review?(margaret.leibovic)
This is a mix of two issues.

1) For some reason Android M users always have a restriction in their profile: bug 1181660 and so our code thinks we are on a restricted profile. Patch with pending review is attached to the bug.

2) We accidentally applied all restrictions to restricted profiles. This is fixed with the patch waiting for review in bug 1181660.

So this patch here is actually a duplicate of 1).
Flags: needinfo?(s.kaspari)
(In reply to :Sebastian Kaspari from comment #20)
> 2) We accidentally applied all restrictions to restricted profiles. This is
> fixed with the patch waiting for review in bug 1181660.

Whoops, This should be bug 1125316.
Depends on: 1181660, 1125316
Comment on attachment 8632297 [details] [diff] [review]
Updated patch

I'm going to clear review, because this looks like the patch that Sebastian wrote for bug 1181660. Sorry for the redundant work!

As a meta point about process, it's generally always best to file a new bug for follow-up issues, since that makes it much easier to track the status of changes, especially when and where they landed.
Attachment #8632297 - Flags: review?(margaret.leibovic)
Duplicate of this bug: 1125294

Comment 24

4 years ago
Verified as fixed on latest Aurora.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.