1187260 - Simplify RestrictedProfiles class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

RestrictedProfiles is getting more and more complex with it handling restrictions for guest profiles and restricted profiles. Especially multiple versions of isAllowed() that call each other make it hard to follow different code paths.

Some ideas to simplify this class:

* Move inner-class "Restriction" to it's own file
* Remove seemingly unused methods, e.g. getRestrictions()
* Move actual implementations for guest profiles and restricted profiles to dedicated classes and just delegate from RestrictedProfiles
* Determine if we are guest / restricted / normal profile only once and then delegate (see point above)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

And:

* There's a comment in the code mentioning that initWithProfile() can be removed after bug 1077590 lands. This has happened, so let's see if we can remove that too.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1187260-simplify-restricted-profiles-WIP.patch

WIP version. Looking for feedback about the general approach before investing time to finalize the patch.

The basic idea is to simplify the RestrictedProfiles class by moving the functionality for guest / restricted / normal profiles out to classes implementing the new RestrictionConfiguration interface.

RestrictedProfiles determines once whether this is currently a guest / restricted / normal profile and then instantiates DefaultConfiguration, GuestProfileConfiguration or RestrictedProfileConfiguration. Calls to isAllowed() or canLoadUrl() will then be delegated to the instance.

RestrictedProfiles is simpler because it just delegates and doesn't have to handle all cases and make all the decisions. The classes implementing DefaultConfiguration are simple because they just need to handle one case.

Apart from that the patch includes some of the simplifications mentioned in comment #0.

Comment 3

[Allison Naaktgeboren :ally]

Comment on attachment 8638656 [details] [diff] [review]
1187260-simplify-restricted-profiles-WIP.patch

Review of attachment 8638656 [details] [diff] [review]:
-----------------------------------------------------------------

In general I like this approach, even if the refactor is a little gnarly in places. However, this work needs to be tabled until after the v1 bugs are all in.

Comment 4

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #3)

> In general I like this approach, even if the refactor is a little gnarly in
> places. However, this work needs to be tabled until after the v1 bugs are
> all in.

Looks like at least 3 other kidfox v1 patches are based on this work. If the refactor doesn't break existing code (how are tests?) then it might be faster to move ahead now.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I'd like to convince you to get this into v1. It has simplified the upcoming patches and because of the separation I do not need to fear that adding a feature to restricted profiles will break guest mode or 'normal' mode. I'm really not keen to hack more functionality into the currently shared RestrictedProfiles class (Especially since I already accidentally introduced regressions in the past because of that).

Regarding tests: I have been developing and using this since Friday without noticeable issues. In bug 1184094 I'd like to write at least a test that verifies that all restrictions are not enforced for 'normal' profiles. This should be a fast and easy test. Maybe we can even test guest mode pretty fast depending on whether we already have tests for guest mode. Restricted profiles being a system feature are more hard to test and probably are not going to be automated in v1. However they are getting a good coverage during development right now.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1187260-simplify-restricted-profiles.patch

Final version of the patch for review.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1187260-simplify-restricted-profiles-v2.patch

The previous patch did not include the auto-generated JNI wrappers.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

New try run along with the other kidfox patches in review: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea116c0fa36b

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8639881 [details] [diff] [review]
1187260-simplify-restricted-profiles-v2.patch

Trying to get some more eyes looking on this. :)

Comment 11

[:Margaret Leibovic]

Comment on attachment 8639881 [details] [diff] [review]
1187260-simplify-restricted-profiles-v2.patch

Review of attachment 8639881 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/RestrictedProfiles.java
@@ -42,5 @@
> -    /**
> -     * This is a hack to allow non-GeckoApp activities to safely call into
> -     * RestrictedProfiles without reworking this class or GeckoProfile.
> -     *
> -     * It can be removed after Bug 1077590 lands.

I guess this bug was fixed and nobody followed up on this? So that's why we can just remove this now?

::: mobile/android/base/restrictions/RestrictionConfiguration.java
@@ +7,5 @@
> +
> +/**
> + * Interface for classes that RestrictedProfiles will delegate to for making decisions.
> + */
> +public interface RestrictionConfiguration {

This interface is nice.

::: mobile/android/base/restrictions/RestrictionProvider.java
@@ +23,5 @@
> +/**
> + * Broadcast receiver providing supported restrictions to the system.
> + */
> +@TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR2)
> +public class RestrictionProvider extends BroadcastReceiver {

In the future, using hg mv instead of removing/adding files can help make diffs easier to review :)

::: widget/android/GeneratedJNIWrappers.cpp
@@ -759,5 @@
> -
> -mozilla::jni::String::LocalRef RestrictedProfiles::GetUserRestrictions()
> -{
> -    return mozilla::jni::Method<GetUserRestrictions_t>::Call(nullptr, nullptr);
> -}

Why can we get rid of this?

I see that we do use JNI to access this method in a test here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testRestrictedProfiles.js#42

But I don't think we need to generate JNI wrappers for that, since I think JNI.jsm should take care of that for us. If you're not sure about this change, I would ask mfinkle or wesj to take a look here.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 8639881 [details] [diff] [review]
1187260-simplify-restricted-profiles-v2.patch

Review of attachment 8639881 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good. Looks like there's some driveby feedback to address and martyn has a f? .

I think you're close enough to get a 'r+ with nits' assuming martyn spots nothing show shopping.

::: mobile/android/base/RestrictedProfiles.java
@@ +29,3 @@
>  
> +    private static RestrictionConfiguration getConfiguration(Context context) {
> +        if (configuration == null) {

can this not be if (!configuration) ?

@@ -39,5 @@
>  
> -    private static final String ABOUT_ADDONS = "about:addons";
> -
> -    /**
> -     * This is a hack to allow non-GeckoApp activities to safely call into

Oh? ...what non geckoapp calls into restricted profiles?

::: mobile/android/base/restrictions/GuestProfileConfiguration.java
@@ +1,1 @@
> +package org.mozilla.gecko.restrictions;

Needs a license block here

@@ +43,5 @@
> +
> +    @Override
> +    public boolean canLoadUrl(String url) {
> +        // Null URLs are always permitted.
> +        if (url == null) {

same nit about above about value == null vs !value for null checks

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java
@@ +1,1 @@
> +package org.mozilla.gecko.restrictions;

license block needed

@@ +35,5 @@
> +    public boolean canLoadUrl(String url) {
> +        if (!isAllowed(Restriction.DISALLOW_INSTALL_EXTENSION) && url.toLowerCase().startsWith(ABOUT_ADDONS)) {
> +            return false;
> +        }
> +

nit: this newline seems unnecessary here.

::: mobile/android/base/restrictions/Restriction.java
@@ +82,5 @@
> +    public String getTitle(Context context) {
> +        if (titleResource == 0) {
> +            return toString();
> +        }
> +

nit - this new line seems unnecessary here. I can see the use of one when there are multiple if blocks and such, but for a 5 line function, it seems like it doesn't add anything.

@@ +90,5 @@
> +    public String getDescription(Context context) {
> +        if (descriptionResource == 0) {
> +            return name;
> +        }
> +

same

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #11)
> ::: mobile/android/base/RestrictedProfiles.java
> @@ -42,5 @@
> > -    /**
> > -     * This is a hack to allow non-GeckoApp activities to safely call into
> > -     * RestrictedProfiles without reworking this class or GeckoProfile.
> > -     *
> > -     * It can be removed after Bug 1077590 lands.
> 
> I guess this bug was fixed and nobody followed up on this? So that's why we
> can just remove this now?

Correct. I was about to file a bug for this but then I just removed it and tested it. GeckoPreferences was the only class using it and it seems to do fine without it.


(In reply to :Margaret Leibovic from comment #11)
> In the future, using hg mv instead of removing/adding files can help make
> diffs easier to review :)

Ah, thanks for the hint. I'm still used to git. It was able to transform rm/add to a move automatically. :)


(In reply to :Margaret Leibovic from comment #11)
> ::: widget/android/GeneratedJNIWrappers.cpp
> @@ -759,5 @@
> > -
> > -mozilla::jni::String::LocalRef RestrictedProfiles::GetUserRestrictions()
> > -{
> > -    return mozilla::jni::Method<GetUserRestrictions_t>::Call(nullptr, nullptr);
> > -}
> 
> Why can we get rid of this?

I removed the Java method RestrictedProfiles.getUserRestrictions() because there was no one using it and the code seemed to be outdated too. The changes in the JNI interfaces were automatically generated by the build process because the method was annotated with @WrapElementForJNI. 

> I see that we do use JNI to access this method in a test here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/
> robocop/testRestrictedProfiles.js#42

Interestingly this test does not fail. But I guess we can remove this task now. No need to keep the method around just for a test case, right?


(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> > +    private static RestrictionConfiguration getConfiguration(Context context) {
> > +        if (configuration == null) {
> 
> can this not be if (!configuration) ?

That would be nice! Java is too strict for that and only allows that for booleans or boolean expressions.


(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> ::: mobile/android/base/restrictions/GuestProfileConfiguration.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.restrictions;
> 
> Needs a license block here

Oh! I went back and added it to a couple of new files in this patch but it seems like I still missed some files.


(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> ::: mobile/android/base/restrictions/Restriction.java
> @@ +82,5 @@
> > +    public String getTitle(Context context) {
> > +        if (titleResource == 0) {
> > +            return toString();
> > +        }
> > +
> 
> nit - this new line seems unnecessary here. I can see the use of one when
> there are multiple if blocks and such, but for a 5 line function, it seems
> like it doesn't add anything.

I feel like we are in the realm of personal taste here. :) For me the "return true" is not related to the early return above so I separated it. Usually I often separate such distinct blocks of code with a new line. But this might be a bit verbose at times.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1187260-simplify-restricted-profiles-v3.patch

Updated patch to address the nits.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8640979 [details] [diff] [review]
1187260-simplify-restricted-profiles-v3.patch

Woops.

Comment 16

[Martyn Haigh (:mhaigh)]

Comment on attachment 8640979 [details] [diff] [review]
1187260-simplify-restricted-profiles-v3.patch

Review of attachment 8640979 [details] [diff] [review]:
-----------------------------------------------------------------

Nice bit of refactoring.  I'm lamenting that you didn't call it a RestrictedProfileFactory ;)  Architecture looks good and with the fixes from the other reviews I can't spot any other issues. 

Ship it!

Comment 17

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #13)

> > I see that we do use JNI to access this method in a test here:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/
> > robocop/testRestrictedProfiles.js#42
> 
> Interestingly this test does not fail. But I guess we can remove this task
> now. No need to keep the method around just for a test case, right?

That's weird, I wonder how that test is not failing... but yeah, let's get rid of this task.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/8fd046b77ea3777e01afc2e4f1de9f9cb54a86bc
changeset:  8fd046b77ea3777e01afc2e4f1de9f9cb54a86bc
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Jul 24 19:47:31 2015 +0200
description:
Bug 1187260 - Simplify RestrictedProfiles class. r=ally

This patch transforms RestrictedProfiles to delegate isAllowed() and
canLoadUrl() calls to an object implementing the RestrictionConfiguration
interface.

DefaultConfiguration, GuestProfileConfiguration and
RestrictedProfileConfiguration are implementing RestrictionConfiguration
and will take care of handling the restrictions for the different types
of profiles.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8fd046b77ea3