Remove "Parental Controls Theme" for restricted profiles

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
Family Friendly Browsing
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8686004 [details]
MozReview Request: Bug 1222381 - Remove default theme for restricted profiles. r?ally

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

Comment 3

2 years ago
(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)
(Assignee)

Comment 5

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/40e3ec35113722d706a62898082dfb0adeb9867c
Bug 1222381 - Remove default theme for restricted profiles. r=ally

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40e3ec351137
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Comment 7

2 years ago
Verified as fixed on latest Nightly (45.0a1 2015-11-22). Tested on Sony Xperia Z2 - Android 5.0.2
status-firefox45: fixed → verified

Comment 8

2 years ago
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)
(Assignee)

Comment 9

2 years ago
(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.

Comment 10

2 years ago
(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)

Comment 12

2 years ago
(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)

Updated

2 years ago
Flags: needinfo?(s.kaspari)

Updated

2 years ago
Blocks: 1234238
You need to log in before you can comment on or make changes to this bug.