Closed
Bug 1125312
Opened 10 years ago
Closed 10 years ago
KidFox: Restricted profiles - Hide other non-essential menu items - All “Tool” menu items
Categories
(Firefox for Android Graveyard :: Profile Handling, defect)
Tracking
(firefox42 verified)
VERIFIED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | verified |
People
(Reporter: jchaulk, Assigned: sebastian)
References
Details
Attachments
(3 files)
No description provided.
Updated•10 years ago
|
OS: Windows 8.1 → Android
Hardware: x86_64 → All
Version: unspecified → Trunk
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally
Attachment #8629933 -
Flags: review?(ally)
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Bug 1125313 - Hide "Report Site Issue" menu item for restricted profiles. r=ally
Attachment #8630003 -
Flags: review?(ally)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
(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.
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8629933 -
Flags: review?(ally)
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
I updated the patch and pushed as a single commit.
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 15•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
I think Sebastian has an Android M device and can help look at the issue.
Flags: needinfo?(s.kaspari)
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
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)
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
•