Re-enable child account detection, now behind a flag.

Re-enable child account detection, now behind a flag.

Was disabled previously in https://codereview.chromium.org/988493002.

BUG=464271, 465662
Committed: https://crrev.com/5b73268f800bf9901db7746ee855f4612f8b41b2
Cr-Commit-Position: refs/heads/master@{#319877}

[Marc Treib, 2015-03-09 10:44:53]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-03-09 11:08:53]

Is there going to be a Finch flag for this as well?

https://codereview.chromium.org/986303002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6372: +        Enable detection of child
accounts on sign-in, as well as periodically.
Nit: "sign-in and startup"?

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2199: kOsMac | kOsWin | kOsLinux | kOsCrOS,
For consistency, we should probably add the flag on Android as well.

[Marc Treib, 2015-03-09 11:20:12]

Yup, the plan is to have a finch flag for this. I was gonna do it in a separate
CL, but I guess there's no reason to. So, added.

https://codereview.chromium.org/986303002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6372: +        Enable detection of child
accounts on sign-in, as well as periodically.
On 2015/03/09 11:08:53, Bernhard (slow until Mar 9) wrote:
> Nit: "sign-in and startup"?

Done.

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2199: kOsMac | kOsWin | kOsLinux | kOsCrOS,
On 2015/03/09 11:08:53, Bernhard (slow until Mar 9) wrote:
> For consistency, we should probably add the flag on Android as well.

But for now, it won't do anything on Android (right?)
Can you create a downstream CL to honor the flag an Android as well, or should I
look into it?

[Bernhard Bauer, 2015-03-09 14:02:51]

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2199: kOsMac | kOsWin | kOsLinux | kOsCrOS,
On 2015/03/09 11:20:12, Marc Treib wrote:
> On 2015/03/09 11:08:53, Bernhard (slow until Mar 9) wrote:
> > For consistency, we should probably add the flag on Android as well.
> 
> But for now, it won't do anything on Android (right?)
> Can you create a downstream CL to honor the flag an Android as well, or should
I
> look into it?

That code is upstreamed now! :) The logic is in Java code though. If you want to
implement that, go ahead, if not, I can do it as well.

[Marc Treib, 2015-03-09 14:48:02]

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/986303002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2199: kOsMac | kOsWin | kOsLinux | kOsCrOS,
On 2015/03/09 14:02:51, Bernhard Bauer wrote:
> On 2015/03/09 11:20:12, Marc Treib wrote:
> > On 2015/03/09 11:08:53, Bernhard (slow until Mar 9) wrote:
> > > For consistency, we should probably add the flag on Android as well.
> > 
> > But for now, it won't do anything on Android (right?)
> > Can you create a downstream CL to honor the flag an Android as well, or
should
> I
> > look into it?
> 
> That code is upstreamed now! :) The logic is in Java code though. If you want
to
> implement that, go ahead, if not, I can do it as well.

Right, awesome :)
Done!

[Bernhard Bauer, 2015-03-09 14:54:35]

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:61:
if (!isChildAccountDetectionEnabled())
I think this could be problematic if this code is called before we have loaded
the native library (yeah, that can happen). Maybe we should put it into
checkHasChildAccount instead?

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:62:
mHasChildAccount = false;
Android style is to put everything on the same line if it fits, otherwise wrap
in braces.

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:222:
private native boolean nativeIsChildAccountDetectionEnabled();
Move all native methods together?

[Marc Treib, 2015-03-09 15:06:27]

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:61:
if (!isChildAccountDetectionEnabled())
On 2015/03/09 14:54:35, Bernhard Bauer wrote:
> I think this could be problematic if this code is called before we have loaded
> the native library (yeah, that can happen). Maybe we should put it into
> checkHasChildAccount instead?

Ugh. Done.

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:62:
mHasChildAccount = false;
On 2015/03/09 14:54:35, Bernhard Bauer wrote:
> Android style is to put everything on the same line if it fits, otherwise wrap
> in braces.

Done.

https://codereview.chromium.org/986303002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:222:
private native boolean nativeIsChildAccountDetectionEnabled();
On 2015/03/09 14:54:35, Bernhard Bauer wrote:
> Move all native methods together?

Done.

[Bernhard Bauer, 2015-03-09 15:14:34]

LGTM with a suggestion:

https://codereview.chromium.org/986303002/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:95:
if (!isChildAccountDetectionEnabled()) mHasChildAccount = false;
Another possibility would be to move this down and combine it with the check for
ChromeSwitches.CHILD_ACCOUNT and shouldForceChildAccount into a single method
that returns a (possibly null) Boolean.

[Marc Treib, 2015-03-09 15:32:26]

https://codereview.chromium.org/986303002/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:95:
if (!isChildAccountDetectionEnabled()) mHasChildAccount = false;
On 2015/03/09 15:14:33, Bernhard Bauer wrote:
> Another possibility would be to move this down and combine it with the check
for
> ChromeSwitches.CHILD_ACCOUNT and shouldForceChildAccount into a single method
> that returns a (possibly null) Boolean.

Done.

[Marc Treib, 2015-03-09 16:04:29]

treib@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Marc Treib, 2015-03-09 16:04:29]

+asvitkine for histograms. PTAL!

[Alexei Svitkine (slow), 2015-03-09 16:51:20]

https://codereview.chromium.org/986303002/diff/100001/chrome/browser/supervis...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/986303002/diff/100001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:89: // If
any arg is specified, disable the field trial.
Can you instead follow the best practices described at go/finch-and-flags?

[Marc Treib, 2015-03-09 17:32:18]

https://codereview.chromium.org/986303002/diff/100001/chrome/browser/supervis...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/986303002/diff/100001/chrome/browser/supervis...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:89: // If
any arg is specified, disable the field trial.
On 2015/03/09 16:51:20, Alexei Svitkine wrote:
> Can you instead follow the best practices described at go/finch-and-flags?

Done.

[Alexei Svitkine (slow), 2015-03-09 18:16:44]

lgtm

[Marc Treib, 2015-03-10 09:02:16]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-10 09:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/986303002/#ps120001
(title: "asvitkine review")

[commit-bot: I haz the power, 2015-03-10 09:02:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/986303002/120001

[commit-bot: I haz the power, 2015-03-10 10:53:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-10 10:53:44]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[Bernhard Bauer, 2015-03-10 12:31:21]

https://codereview.chromium.org/986303002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:33:
private enum ForceChildAccount { FORCE_ON, FORCE_OFF, DONT_FORCE }
I think we actually use ints for enums, because we run into limits with the
number of classes if we use enum classes.

(Welcome to Android!)

[Marc Treib, 2015-03-10 13:22:15]

https://codereview.chromium.org/986303002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java
(right):

https://codereview.chromium.org/986303002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:33:
private enum ForceChildAccount { FORCE_ON, FORCE_OFF, DONT_FORCE }
On 2015/03/10 12:31:20, Bernhard Bauer wrote:
> I think we actually use ints for enums, because we run into limits with the
> number of classes if we use enum classes.
> 
> (Welcome to Android!)

Done.

[Bernhard Bauer, 2015-03-10 13:24:26]

LGTM!

[Marc Treib, 2015-03-10 13:25:00]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-10 13:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/986303002/#ps160001
(title: "enum -> int")

[commit-bot: I haz the power, 2015-03-10 13:25:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/986303002/160001

[commit-bot: I haz the power, 2015-03-10 14:19:41]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-10 14:20:35]

Patchset 9 (id:??) landed as
https://crrev.com/5b73268f800bf9901db7746ee855f4612f8b41b2
Cr-Commit-Position: refs/heads/master@{#319877}