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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally

Bug 1125312 - Restriced profiles: Hide "Tools" menu item. r=ally

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d39e2c795f59

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1125313 - Hide "Report Site Issue" menu item for restricted profiles. r=ally

Bug 1125313 - Hide "Report Site Issue" menu item for restricted profiles. r=ally

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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

[Allison Naaktgeboren :ally]

(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

[Allison Naaktgeboren :ally]

Sebastian, in which order do you have these patches applied? I would like to take this code for a spin before completing the review.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 9

[Allison Naaktgeboren :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 10

[Allison Naaktgeboren :ally]

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.

Comment 11

[Allison Naaktgeboren :ally]

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!

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

I updated the patch and pushed as a single commit.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64d240b53e52

Comment 15

[:Margaret Leibovic]

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

[Dylan Roeh (:droeh) (he/him)]

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

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

I think Sebastian has an Android M device and can help look at the issue.

Comment 19

[Dylan Roeh (:droeh) (he/him)]

Attachment: Updated patch

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).

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

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).

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

(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

[:Margaret Leibovic]

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.

Comment 24

[u549602]

Verified as fixed on latest Aurora.