Disable incognito link if parental control is on

Disable incognito link if parental control is on

The CL disables the incognito link on the gaia page if parental control is on. It adds a cached version for windows parental control check so that it may be accessed from IO thread. A similar CL for thread-safe android parental control check is under review at https://chrome-internal-review.googlesource.com/#/c/166265/.

BUG=383682
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278848

[guohui, 2014-06-16 22:58:34]

Hey,

could you please review the CL?

cpu@ for base/win/...
bauserb@ for chrome/browser/prefs/...
rogerta@ for chrome/browser/signin/...

Thanks,

Hui

[guohui, 2014-06-16 23:16:40]

moved the parental control check to profile_io_data in patch 2.

+mmenke@ for owner review of profile_io_data.

[mmenke, 2014-06-16 23:25:12]

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:474:
IncognitoModePrefs::GetAvailability(profile->GetPrefs());
Is there a reason we can't use a PrefMember to keep this information up to date?

[Bernhard Bauer, 2014-06-17 09:15:45]

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.cc
File base/win/metro.cc (right):

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.cc#newco...
base/win/metro.cc:14: bool parental_control_logging_required = false;
Global (mutable) variables are usually named starting with g_.

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:131: #if defined(OS_ANDROID)
Move this block before OS_WIN, so the #ifdef conditions are in alphabetic order?

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:474:
IncognitoModePrefs::GetAvailability(profile->GetPrefs());
On 2014/06/16 23:25:11, mmenke wrote:
> Is there a reason we can't use a PrefMember to keep this information up to
date?

The value isn't always coming from PrefService (if the platform enables parental
controls), but PrefMember relies on the preference change notification mechanism
to keep up to date.

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:763: bool
ProfileIOData::ArePlatformParentalControlsEnabledCached() const {
So... above we initialize the parental control setting via IncognitoModePrefs,
which abstracts away where the setting is coming from, but here we fetch it
directly again? We should do either one or the other, but not both.

[cpu_(ooo_6.6-7.5), 2014-06-17 16:35:51]

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.h
File base/win/metro.h (right):

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.h#newcode91
base/win/metro.h:91: BASE_EXPORT bool
IsParentalControlActivityLoggingOnCached();
Most functions in this file are not used and slated to be removed. Are you
actually hitting this codepath?

[guohui, 2014-06-17 18:45:04]

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.cc
File base/win/metro.cc (right):

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.cc#newco...
base/win/metro.cc:14: bool parental_control_logging_required = false;
On 2014/06/17 09:15:45, Bernhard Bauer wrote:
> Global (mutable) variables are usually named starting with g_.

Done.

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.h
File base/win/metro.h (right):

https://codereview.chromium.org/336213005/diff/100001/base/win/metro.h#newcode91
base/win/metro.h:91: BASE_EXPORT bool
IsParentalControlActivityLoggingOnCached();
On 2014/06/17 16:35:51, cpu wrote:
> Most functions in this file are not used and slated to be removed. Are you
> actually hitting this codepath?

we hit this codepatch in this CL. We need to check for parental control on
IOThread, and the existing method IsParentalControlActivityLoggingOn "should
ideally be called on the UI thread", thus i added a cached version safe to
access from any thread. However after second thought, i think it is better to
cache the value in ProfileIOData and keep this file unchanged. Please see patch
3.

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:131: #if defined(OS_ANDROID)
On 2014/06/17 09:15:45, Bernhard Bauer wrote:
> Move this block before OS_WIN, so the #ifdef conditions are in alphabetic
order?

Done.

https://codereview.chromium.org/336213005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:763: bool
ProfileIOData::ArePlatformParentalControlsEnabledCached() const {
On 2014/06/17 09:15:45, Bernhard Bauer wrote:
> So... above we initialize the parental control setting via IncognitoModePrefs,
> which abstracts away where the setting is coming from, but here we fetch it
> directly again? We should do either one or the other, but not both.

Done.

[mmenke, 2014-06-17 20:41:44]

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:141: bool g_parental_control_on =
false;
Why not make this a member of ProfileIOData?

[mmenke, 2014-06-17 20:44:14]

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:141: bool g_parental_control_on =
false;
On 2014/06/17 20:41:44, mmenke wrote:
> Why not make this a member of ProfileIOData?

Alternatively, put it in some other file (metro.h?  I don't know), make sure
it's initialized on startup, and make a static accessor for it, that can be
called on any thread.  It does seem kind of odd to have a non-per-profile piece
of information in ProfileIOData, just so it can be accessed on the IO thread.

[cpu_(ooo_6.6-7.5), 2014-06-18 17:52:37]

I have no file to review thusly I graciously exit the stage.

[guohui, 2014-06-18 19:36:38]

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/336213005/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:141: bool g_parental_control_on =
false;
On 2014/06/17 20:44:13, mmenke wrote:
> On 2014/06/17 20:41:44, mmenke wrote:
> > Why not make this a member of ProfileIOData?
> 
> Alternatively, put it in some other file (metro.h?  I don't know), make sure
> it's initialized on startup, and make a static accessor for it, that can be
> called on any thread.  It does seem kind of odd to have a non-per-profile
piece
> of information in ProfileIOData, just so it can be accessed on the IO thread.

Done.

[guohui, 2014-06-18 19:37:03]

+sky for owner review of chrome/browser/chrome_browser_main.cc

[sky, 2014-06-19 14:56:15]

https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1537:
IncognitoModePrefs::InitializePlatformParentalControls();
Can this be moved to chrome_browser_main_win?

[mmenke, 2014-06-19 15:03:58]

I no longer own any of the files being modified, so defer to other reviewers on
the rest of this review and am removing myself from this review.  Feel free to
rope me back in if you end up going back to modifying ProfileIOData.

[Roger Tawa OOO till Jul 10th, 2014-06-19 17:35:13]

lgtm

[guohui, 2014-06-19 17:51:41]

https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1537:
IncognitoModePrefs::InitializePlatformParentalControls();
On 2014/06/19 14:56:15, sky wrote:
> Can this be moved to chrome_browser_main_win?

Done.

[guohui, 2014-06-20 02:02:36]

On 2014/06/19 17:51:41, guohui wrote:
>
https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/336213005/diff/160001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_main.cc:1537:
> IncognitoModePrefs::InitializePlatformParentalControls();
> On 2014/06/19 14:56:15, sky wrote:
> > Can this be moved to chrome_browser_main_win?
> 
> Done.

Ping?

[Bernhard Bauer, 2014-06-20 08:08:34]

lgtm

[guohui, 2014-06-20 17:44:29]

On 2014/06/20 08:08:34, Bernhard Bauer wrote:
> lgtm

ping sky@? this is a release blocker for m37 so i am hoping to land it by EoD.
Thanks!

[sky, 2014-06-20 19:24:42]

LGTM

[guohui, 2014-06-20 19:24:57]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:28:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/guohui@chromium.org/336213005/180001

[commit-bot: I haz the power, 2014-06-20 22:46:40]

Change committed as 278848