1189336 - Find better naming for RestrictedProfiles class and methods

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Picking up some concerns from bug 1125293 and related bugs:

* RestrictedProfiles: The name of the class can be confusing because it handles guest profiles and restricted profiles. We might even query it from a normal profile. Find a more descriptive name. Maybe just Restrictions? E.g. Restrictions.isAllowed(..)

* We are querying the RestrictedProfiles class to see whether the user is allowed to use specific features (isAllowed) but inside the class we check whether some restrictions are enforced. This leads to booleans being flipped here and there (isAllowed = !restricted). Maybe we can use the same logical concept inside and outside of the class.

* In addition to the previous point: We currently check features using something like RestrictedProfiles.isAllowed(Restriction.DISALLOW_FEATURE). isAllowed(DISALLOW_*) is confusing.

* In addition to the previous point: The device admin can configure what restrictions are enforced for a specific restricted profile. They can switch things like "Disallow clearing history" to ON/OFF. This might be easier to understand if we get rid of the negation and talk about features that can be enabled or disabled: "Clearing history ON/OFF"

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1189336 - (Part 1) Rename RestrictedProfiles to Restrictions. r?ally

Bug 1189336 - (Part 1) Rename RestrictedProfiles to Restrictions. r?ally

RestrictedProfiles: The name of the class can be confusing because it handles
guest profiles and restricted profiles. We might even query it from a normal
profile.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1189336 - (Part 2) Rename Restriction class to Restrictable. r?ally

Bug 1189336 - (Part 2) Rename Restriction class to Restrictable. r?ally

This is the first step to move from a list of DISALLOW_* things to a list of
features that can be enabled or disabled for a profile. At a later stage we
will check whether a "restrictable" feature is allowed or not.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1189336 - (Part 3) Move from a list of disallowed things to a list of restrictable features. r?ally

Bug 1189336 - (Part 3) Move from a list of disallowed things to a list of restrictable features. r?ally

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1189336 - (Part 4) Migrate old restrictions if needed. r?ally

Bug 1189336 - (Part 4) Migrate old restrictions if needed. r?ally

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

This (maybe scary) series of patches addresses the issues mentioned in comment 0:

* Instead of RestrictedProfiles.isAllowed(Restriction.DISALLOW_PRIVATE_BROWSING) we now use: Restrictions.isAllowed(Restrictable.PRIVATE_BROWSING)
* We now use the same logic (see above) like we JS and native code use (checking whether features are allowed)
* The user facing UI now is much easier to understand: Instead of confusing "Disallow Private Browsing ON/OFF" it is now "Private Browsing ON/OFF".

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688441 [details]
MozReview Request: Bug 1189336 - (Part 1) Rename RestrictedProfiles to Restrictions. r?ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25317/diff/1-2/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688442 [details]
MozReview Request: Bug 1189336 - (Part 2) Rename Restriction class to Restrictable. r?ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25319/diff/1-2/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688443 [details]
MozReview Request: Bug 1189336 - (Part 3) Move from a list of disallowed things to a list of restrictable features. r?ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25321/diff/1-2/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688444 [details]
MozReview Request: Bug 1189336 - (Part 4) Migrate old restrictions if needed. r?ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25323/diff/1-2/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

I rebased the patches to include some recent changes.

Comment 11

[Allison Naaktgeboren :ally]

Comment on attachment 8688441 [details]
MozReview Request: Bug 1189336 - (Part 1) Rename RestrictedProfiles to Restrictions. r?ally

https://reviewboard.mozilla.org/r/25317/#review22921

::: mobile/android/base/db/LocalBrowserDB.java
(Diff revision 2)
> -import org.mozilla.gecko.AppConstants;

I imagine that you are removing these for general cleanup while your're renaming the Restrictions import? They don't otherwise appear to relevant to this patch...

::: toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp:6
(Diff revision 2)
>  #include "nsParentalControlsService.h"

I entertain the notion of renaming the ParentalControlsService to something like ApplicationControlsService as we move toward a more general model.

That would be outside the scope of this bug, and for another time.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 8688442 [details]
MozReview Request: Bug 1189336 - (Part 2) Rename Restriction class to Restrictable. r?ally

https://reviewboard.mozilla.org/r/25319/#review22923

looks good other than the comment below.

::: mobile/android/base/restrictions/RestrictionProvider.java
(Diff revision 2)
> -import org.mozilla.gecko.restrictions.Restriction;

Don't we still need this import for the Restriction objects used later in the patch?

Comment 13

[Allison Naaktgeboren :ally]

Comment on attachment 8688443 [details]
MozReview Request: Bug 1189336 - (Part 3) Move from a list of disallowed things to a list of restrictable features. r?ally

https://reviewboard.mozilla.org/r/25321/#review22925

r+, with questions addressed and a checkin with Axel before landing.

::: mobile/android/base/home/TopSitesPanel.java:361
(Diff revision 2)
>  

much more readable indeed. :)

::: mobile/android/base/locales/en-US/android_strings.dtd:727
(Diff revision 2)
> -<!ENTITY restriction_disallow_addons_title2 "Disable add-on installation">
> +<!ENTITY restrictable_feature_addons_installation "Add-on installation">

The ENTITY localization system doesn't play well with changes. We'll need to check in with Axel.

Since the strings themselves have changed with the removal of the word 'disable', I suspect this will be fine.

::: mobile/android/chrome/content/browser.js:4503
(Diff revision 2)
> -    if (!ParentalControls.isAllowed(ParentalControls.VISIT_FILE_URLS, fixedURI)) {
> +    if (!ParentalControls.isAllowed(ParentalControls.BROWSE, fixedURI)) {

So I didn't see anything in the bug comments about unifying these two restrictions. Is this intentional? Is there some context I'm missing?

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8688444 [details]
MozReview Request: Bug 1189336 - (Part 4) Migrate old restrictions if needed. r?ally

https://reviewboard.mozilla.org/r/25323/#review22927

I have concerns about this patch, mostly around part of the 'why'.

Why don't we have a quick chat on irc tomorrow around the mobile meeting?

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java:105
(Diff revision 2)
> +    public static void migrateRestrictionsIfNeeded(Bundle bundle) {

Do we have a bug on file to remove this migration code? We won't need it forever.

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java:106
(Diff revision 2)
> +        if (!bundle.containsKey(Restrictable.INSTALL_EXTENSION.name) && bundle.containsKey("no_install_extensions")) {

So I'm a little lost why these of the 18 or so need migration but the rest don't. Could you help me understand your thinking here?

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Allison Naaktgeboren :ally from comment #11)
> I imagine that you are removing these for general cleanup while your're
> renaming the Restrictions import? They don't otherwise appear to relevant to
> this patch...

Yes, in this case actually IntelliJ did this after I let it auto-import the new classes.


(In reply to Allison Naaktgeboren :ally from comment #11)
> I entertain the notion of renaming the ParentalControlsService to something
> like ApplicationControlsService as we move toward a more general model.
> 
> That would be outside the scope of this bug, and for another time.

This is a good point and does make sense. I tried to tinker not too much with this interface because it's shared between mobile and desktop.


(In reply to Allison Naaktgeboren :ally from comment #12)
> Don't we still need this import for the Restriction objects used later in
> the patch?

Both classes are in the same package, so an import is not needed. This has been the case before this refactoring too, so I guess this is some artifact from previous changes.

(In reply to Allison Naaktgeboren :ally from comment #13)
> ::: mobile/android/base/locales/en-US/android_strings.dtd:727
> (Diff revision 2)
> > -<!ENTITY restriction_disallow_addons_title2 "Disable add-on installation">
> > +<!ENTITY restrictable_feature_addons_installation "Add-on installation">
> 
> The ENTITY localization system doesn't play well with changes. We'll need to
> check in with Axel.

I created a completely new entity here - is this problematic too? I thought only changing existing entities can or will screw up the localization system?


(In reply to Allison Naaktgeboren :ally from comment #13)
> ::: mobile/android/chrome/content/browser.js:4503
> (Diff revision 2)
> > -    if (!ParentalControls.isAllowed(ParentalControls.VISIT_FILE_URLS, fixedURI)) {
> > +    if (!ParentalControls.isAllowed(ParentalControls.BROWSE, fixedURI)) {
> 
> So I didn't see anything in the bug comments about unifying these two
> restrictions. Is this intentional? Is there some context I'm missing?

Oh, sorry. I should have made this more clear here and even moved to a separate commit: In the beginning this restriction was used to restrict accessing file:// URLs. However this has been changed and we actually restrict things like about:config or about:addons here to. So I thought I'll rename it while cleaning things up here. I can mention this side effect in the commit message if this is helpful.


(In reply to Allison Naaktgeboren :ally from comment #14)
> Comment on attachment 8688444 [details]
> I have concerns about this patch, mostly around part of the 'why'.

I wanted to avoid that - after the update to 45 - the restrictions are reset to the defaults - because they have new names/keys. So this migration should make sure that if a parent has, for example, set "Disable Private Browsing OFF" that this is going to be "Private Browsing ON" after the update.

> Why don't we have a quick chat on irc tomorrow around the mobile meeting?

Sure! I hope my comments here will be helpful for that.


(In reply to Allison Naaktgeboren :ally from comment #14)
> ::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java:105
> (Diff revision 2)
> > +    public static void migrateRestrictionsIfNeeded(Bundle bundle) {
> 
> Do we have a bug on file to remove this migration code? We won't need it
> forever.

Agreed. I don't know what the best time to remove this migration code would be though. As this is migrating restrictions from Firefox 42-44 to the new ones in Firefox 45+, the best moment would be as soon as Firefox < 44 is not actively used anymore in the wild? Do we have guidelines for that? Maybe from desktop? But yeah, I did not file a bug yet.


(In reply to Allison Naaktgeboren :ally from comment #14)
> ::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java:106
> (Diff revision 2)
> > +        if (!bundle.containsKey(Restrictable.INSTALL_EXTENSION.name) && bundle.containsKey("no_install_extensions")) {
> 
> So I'm a little lost why these of the 18 or so need migration but the rest
> don't. Could you help me understand your thinking here?

This should migrate all restrictions that have been configurable in a restricted profile in Firefox 42-44. All others are either only used in guest browsing or haven't been configurable anyways.

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> (In reply to Allison Naaktgeboren :ally from comment #11)
> > I entertain the notion of renaming the ParentalControlsService to something
> > like ApplicationControlsService as we move toward a more general model.
> > 
> > That would be outside the scope of this bug, and for another time.
> 
> This is a good point and does make sense. I tried to tinker not too much
> with this interface because it's shared between mobile and desktop.

Afaik desktop does nothing with this, so you could probably change it. File a bug for renaming so that it doesn't dissappear into the ether.

> (In reply to Allison Naaktgeboren :ally from comment #13)
> > ::: mobile/android/base/locales/en-US/android_strings.dtd:727
> > (Diff revision 2)
> > > -<!ENTITY restriction_disallow_addons_title2 "Disable add-on installation">
> > > +<!ENTITY restrictable_feature_addons_installation "Add-on installation">
> > 
> > The ENTITY localization system doesn't play well with changes. We'll need to
> > check in with Axel.
> 
> I created a completely new entity here - is this problematic too? I thought
> only changing existing entities can or will screw up the localization system?

So this is not problematic in will-it-break-their-tools, but in the human aspect. That they look like entirely new strings causes churn for localizers who are doing unnecessary re-translating, but the existing system doesn't accommodate that migration that well. Which is why we check in with Axel. It may be that he'll just let the localizers know to manually migrate the strings. 
> 
> 
> (In reply to Allison Naaktgeboren :ally from comment #13)
> > ::: mobile/android/chrome/content/browser.js:4503
> > (Diff revision 2)
> > > -    if (!ParentalControls.isAllowed(ParentalControls.VISIT_FILE_URLS, fixedURI)) {
> > > +    if (!ParentalControls.isAllowed(ParentalControls.BROWSE, fixedURI)) {
> > 
> > So I didn't see anything in the bug comments about unifying these two
> > restrictions. Is this intentional? Is there some context I'm missing?
> 
> Oh, sorry. I should have made this more clear here and even moved to a
> separate commit: In the beginning this restriction was used to restrict
> accessing file:// URLs. However this has been changed and we actually
> restrict things like about:config or about:addons here to. So I thought I'll
> rename it while cleaning things up here. I can mention this side effect in
> the commit message if this is helpful.

That would be nice.

> (In reply to Allison Naaktgeboren :ally from comment #14)
> > ::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java:105
> > (Diff revision 2)
> > > +    public static void migrateRestrictionsIfNeeded(Bundle bundle) {
> > 
> > Do we have a bug on file to remove this migration code? We won't need it
> > forever.
> 
> Agreed. I don't know what the best time to remove this migration code would
> be though. As this is migrating restrictions from Firefox 42-44 to the new
> ones in Firefox 45+, the best moment would be as soon as Firefox < 44 is not
> actively used anymore in the wild? Do we have guidelines for that? Maybe
> from desktop? But yeah, I did not file a bug yet.

Migration removal is handled on a case by case basis. Please file the bug so we can start sorting out when the best time to remove it will be. 

r+ for the last patch with bugs filed and Axel notified.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/c52cc48aea1ea85a331012dfecf7f9cc837b7136
Bug 1189336 - (Part 1) Rename RestrictedProfiles to Restrictions. r=ally

https://hg.mozilla.org/integration/fx-team/rev/82cd040777655c08b79aca030a9bd885c8fd9d88
Bug 1189336 - (Part 2) Rename Restriction class to Restrictable. r=ally

https://hg.mozilla.org/integration/fx-team/rev/6d38b37d27a44859fcda808e8c7f17980da88376
Bug 1189336 - (Part 3) Move from a list of disallowed things to a list of restrictable features. r=ally

https://hg.mozilla.org/integration/fx-team/rev/2a711953a62c00ee3ddc753592ccffac7876c966
Bug 1189336 - (Part 4) Migrate old restrictions if needed. r=ally

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c52cc48aea1e
https://hg.mozilla.org/mozilla-central/rev/82cd04077765
https://hg.mozilla.org/mozilla-central/rev/6d38b37d27a4
https://hg.mozilla.org/mozilla-central/rev/2a711953a62c