Closed Bug 1222381 Opened 9 years ago Closed 9 years ago

Remove "Parental Controls Theme" for restricted profiles

Categories

(Firefox for Android Graveyard :: Family Friendly Browsing, defect)

All
Android
defect
Not set
normal

Tracking

(firefox45 verified)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- verified

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(1 file)

Currently we automatically install a greenish "Parental Controls Theme" (Lightweight theme) for restricted profiles. In bug 1220251 we want to introduce a theme picker. Let's remove our code for this override. There's no reason to switch from the "default" theme to a greenish theme and then let the user pick a theme.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Bug 1222381 - Remove default theme for restricted profiles. r?ally

Instead we will introduce a theme picker in bug 1220251.
Attachment #8686004 - Flags: review?(ally)
Attachment #8686004 - Flags: review?(ally)
Comment on attachment 8686004 [details]
MozReview Request: Bug 1222381 - Remove default theme for restricted profiles. r?ally

https://reviewboard.mozilla.org/r/24939/#review22529

The patch is good, but before we actually ship it, we should settle the issue of programmatically determining guest browsing from FFB. r+ once we settle that.

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -      // We are using the DEFAULT_THEME restriction to differentiate between restricted profiles & guest mode - Bug 1199596

So.. this comment begs the question, if we get rid of the DISALLOW_DEFAULT_THEME restriction....how do we know the difference between guest & FBB anymore?

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -  _installParentalControlsTheme: function() {

I'm using this, but I can pull it into my patch once you've landed this. Or we could just leave it. Up to you.
(In reply to Allison Naaktgeboren :ally from comment #2)
> The patch is good, but before we actually ship it, we should settle the
> issue of programmatically determining guest browsing from FFB. r+ once we
> settle that.

The thing is we currently do not need it on JS side. On Java side we have RestrictedProfile.isRestrictedProfile(). I can add it (as part of an other bug) if we need it - so far we only check some restrictions but not the state of the profile.


> So.. this comment begs the question, if we get rid of the
> DISALLOW_DEFAULT_THEME restriction....how do we know the difference between
> guest & FBB anymore?

This has only been some very ugly crutch for getting the default theme done. As mentioned above: Currently we do not need to distinguish (on JS side) because we just query whether a feature is allowed or not.


> I'm using this, but I can pull it into my patch once you've landed this. Or
> we could just leave it. Up to you.

I assume for the panel you'll want to send an event to the JS side and call themeChanged() with a new theme configuration. There's probably no need to install it as a built-in (not removable) theme?


I'd like to land this here and if we need to distinguish between guests and restricted profile (Do you have a use-case / bug that needs it currently?) then let's file a new bug and add this to nsIParentalControlsService.idl - the right way this time. :)
Flags: needinfo?(ally)
(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> (In reply to Allison Naaktgeboren :ally from comment #2)
> > The patch is good, but before we actually ship it, we should settle the
> > issue of programmatically determining guest browsing from FFB. r+ once we
> > settle that.
> 
> The thing is we currently do not need it on JS side. On Java side we have
> RestrictedProfile.isRestrictedProfile(). I can add it (as part of an other
> bug) if we need it - so far we only check some restrictions but not the
> state of the profile.

I fear this will come back to bit us, and me in particular on bug 1220251. Depends on where the mocks end up exactly.

> 
> > I'm using this, but I can pull it into my patch once you've landed this. Or
> > we could just leave it. Up to you.
> 
> I assume for the panel you'll want to send an event to the JS side and call
> themeChanged() with a new theme configuration. There's probably no need to
> install it as a built-in (not removable) theme?

If they're bundled with fennec, they're not removable anyway. The theme manager lets me switch between built in themes easily.  
> 
> I'd like to land this here and if we need to distinguish between guests and
> restricted profile (Do you have a use-case / bug that needs it currently?)
> then let's file a new bug and add this to nsIParentalControlsService.idl -
> the right way this time. :)

That's a plan I can roll with for the js side.

You can haz r+
Flags: needinfo?(ally)
https://hg.mozilla.org/mozilla-central/rev/40e3ec351137
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Verified as fixed on latest Nightly (45.0a1 2015-11-22). Tested on Sony Xperia Z2 - Android 5.0.2
Barbara, if we're not going forward with the theme picker during first run (bug 1220251), should we back out this change?
Flags: needinfo?(bbermes)
(In reply to :Margaret Leibovic from comment #8)
> Barbara, if we're not going forward with the theme picker during first run
> (bug 1220251), should we back out this change?

All the changes around restrictions (Reorganizing, Renaming, ..) will make this super painful to back out.
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> (In reply to :Margaret Leibovic from comment #8)
> > Barbara, if we're not going forward with the theme picker during first run
> > (bug 1220251), should we back out this change?
> 
> All the changes around restrictions (Reorganizing, Renaming, ..) will make
> this super painful to back out.

I actually feel like we should just forget about this and leave it alone, but I want it to be a conscious decision, not one we accidentally stumble into.
I'll provide more feedback once I talked to Karen re theme picker, however is it correct to assume if we don't do anything with this bug, the parental theme will remain the same and in the code?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(bbermes)
(In reply to Barbara Bermes [:barbara] from comment #11)
> I'll provide more feedback once I talked to Karen re theme picker, however
> is it correct to assume if we don't do anything with this bug, the parental
> theme will remain the same and in the code?

We removed the built-in theme in this bug (anticipating we would have a theme chooser), so if we don't take more action, there won't be a custom theme applied to the restricted profiles.
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(s.kaspari)
Blocks: 1234238
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.