Disable incognito if parental controls are enabled on android.

Disable incognito if parental controls are enabled on android.

Provides a hook to the Android application to specify whether parental
controls are enabled and incognito should be disabled.

TEST=build/android/test_runner.py gtest -s unit_tests
BUG=329276
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245390

[Ted C, 2014-01-14 02:55:19]

PTAL, probably worth both of you looking over all the files as they are minimal
changes.

[Yaron, 2014-01-14 03:14:42]

On 2014/01/14 02:55:19, Ted C wrote:
> PTAL, probably worth both of you looking over all the files as they are
minimal
> changes.

lgtm

[Bernhard Bauer, 2014-01-14 09:59:07]

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:17: #elif defined(OS_ANDROID)
Nit: I'd rather keep these #ifdefs separate (it reduces merge conflicts).

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:43: if
(base::win::IsParentalControlActivityLoggingOn()) {
Hm, would it make sense to make this into a single method that returns whether
platform-specific parental controls are enabled, instead of adding one for every
platform?

[Ted C, 2014-01-14 20:22:58]

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:43: if
(base::win::IsParentalControlActivityLoggingOn()) {
On 2014/01/14 09:59:07, Bernhard Bauer wrote:
> Hm, would it make sense to make this into a single method that returns whether
> platform-specific parental controls are enabled, instead of adding one for
every
> platform?

Thoughts on where such a thing should live?

Since we have the dependency on chrome, it couldn't live in base w/ the existing
metro.h.

I could create a define a function in incognito_mode_prefs.cc:
bool ArePlatformParentalControlsEnabled();

Then have incognito_mode_prefs_android.cc, incognito_mode_prefs_win[posix]
etc... that define the method and return the platform implementation.

Or are you thinking there is a more generic place?  I tried poking around and
nothing jumped out as a "platform feature" helper or anything.

[Bernhard Bauer, 2014-01-14 23:26:26]

LGTM with the comment below.

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:43: if
(base::win::IsParentalControlActivityLoggingOn()) {
On 2014/01/14 20:22:59, Ted C wrote:
> On 2014/01/14 09:59:07, Bernhard Bauer wrote:
> > Hm, would it make sense to make this into a single method that returns
whether
> > platform-specific parental controls are enabled, instead of adding one for
> every
> > platform?
> 
> Thoughts on where such a thing should live?
> 
> Since we have the dependency on chrome, it couldn't live in base w/ the
existing
> metro.h.
> 
> I could create a define a function in incognito_mode_prefs.cc:
> bool ArePlatformParentalControlsEnabled();
> 
> Then have incognito_mode_prefs_android.cc, incognito_mode_prefs_win[posix]
> etc... that define the method and return the platform implementation.
> 
> Or are you thinking there is a more generic place?  I tried poking around and
> nothing jumped out as a "platform feature" helper or anything.

Hmm... good question. We could probably add some hooks for a base/ embedder, to
get rid of the chrome dependency, but that seems a bit complicated. So does
moving the implementations to platform-specific files (if that's the only thing
in them).

I guess I'd be okay with pulling this out into a method like you suggested, and
then keeping it here for now, keeping in mind to move it to its own file if we
start adding more things like it. think in general organized things by feature
instead of by platform scales better.

[Ted C, 2014-01-15 00:15:49]

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:17: #elif defined(OS_ANDROID)
On 2014/01/14 09:59:07, Bernhard Bauer wrote:
> Nit: I'd rather keep these #ifdefs separate (it reduces merge conflicts).

Done.

https://codereview.chromium.org/136693011/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:43: if
(base::win::IsParentalControlActivityLoggingOn()) {
On 2014/01/14 23:26:26, Bernhard Bauer wrote:
> On 2014/01/14 20:22:59, Ted C wrote:
> > On 2014/01/14 09:59:07, Bernhard Bauer wrote:
> > > Hm, would it make sense to make this into a single method that returns
> whether
> > > platform-specific parental controls are enabled, instead of adding one for
> > every
> > > platform?
> > 
> > Thoughts on where such a thing should live?
> > 
> > Since we have the dependency on chrome, it couldn't live in base w/ the
> existing
> > metro.h.
> > 
> > I could create a define a function in incognito_mode_prefs.cc:
> > bool ArePlatformParentalControlsEnabled();
> > 
> > Then have incognito_mode_prefs_android.cc, incognito_mode_prefs_win[posix]
> > etc... that define the method and return the platform implementation.
> > 
> > Or are you thinking there is a more generic place?  I tried poking around
and
> > nothing jumped out as a "platform feature" helper or anything.
> 
> Hmm... good question. We could probably add some hooks for a base/ embedder,
to
> get rid of the chrome dependency, but that seems a bit complicated. So does
> moving the implementations to platform-specific files (if that's the only
thing
> in them).
> 
> I guess I'd be okay with pulling this out into a method like you suggested,
and
> then keeping it here for now, keeping in mind to move it to its own file if we
> start adding more things like it. think in general organized things by feature
> instead of by platform scales better.

I split it out a separate function.

[commit-bot: I haz the power, 2014-01-15 17:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/136693011/120001

[commit-bot: I haz the power, 2014-01-15 19:32:43]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ted C, 2014-01-16 04:26:18]

On 2014/01/15 19:32:43, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Turns out I needed to update the chrome unit_tests_apk to have
a slightly different test configuration with a stub ChromiumApplication
(where the normal gtests apks use the BaseChromiumApplication).

This needed a couple tweaks to the .gypi files and a new 
AndroidManifest.xml and native test specific application, so any
chance you both could take another quick look and see if it seems
reasonable?

[Yaron, 2014-01-16 18:38:19]

https://codereview.chromium.org/136693011/diff/400001/build/apk_test.gypi
File build/apk_test.gypi (left):

https://codereview.chromium.org/136693011/diff/400001/build/apk_test.gypi#old...
build/apk_test.gypi:34: 'android_manifest_path':
'<(DEPTH)/testing/android/AndroidManifest.xml',
I'm not sure how this works. I would think that this breaks all non-unit_test
targets (e.g. base_unittests) since they longer have an android manifest.

[Yaron, 2014-01-16 18:47:22]

lgtm

Thanks for clarification off-thread. I missed the new AndroidManifest.xml. Feel
free to ignore nit if you disagree strongly

https://codereview.chromium.org/136693011/diff/400001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/136693011/diff/400001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2804: 'java_in_dir':
'test/android/native_test_apk',
nit: I think I like "unit_tests" throughout instead of "native_tests" as it's
more consistent

[Ted C, 2014-01-16 19:22:44]

https://codereview.chromium.org/136693011/diff/400001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/136693011/diff/400001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2804: 'java_in_dir':
'test/android/native_test_apk',
On 2014/01/16 18:47:23, Yaron wrote:
> nit: I think I like "unit_tests" throughout instead of "native_tests" as it's
> more consistent

Done.

[commit-bot: I haz the power, 2014-01-16 19:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/136693011/580001

[commit-bot: I haz the power, 2014-01-16 21:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/136693011/840001

[commit-bot: I haz the power, 2014-01-17 00:18:15]

Change committed as 245390