Skip ARC initial screen when everything is set up by policy

Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Original-Commit-Position: refs/heads/master@{#451340}
Committed: https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8031fc13c8dad
Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451437}
Committed: https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e9c562929c7cf

[emaxx, 2017-02-08 19:07:55]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 19:09:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-02-08 19:45:47]

Description was changed from

==========
Skip ARC Terms of Service when policies are set up

BUG=
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs
==========

[emaxx, 2017-02-08 19:46:08]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs
==========

[emaxx, 2017-02-08 19:48:14]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

[emaxx, 2017-02-08 19:50:51]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

[commit-bot: I haz the power, 2017-02-08 19:54:13]

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

[commit-bot: I haz the power, 2017-02-08 19:54:13]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-08 20:00:45]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:01:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-02-08 20:02:06]

emaxx@chromium.org changed reviewers:
+ yusukes@chromium.org

[emaxx, 2017-02-08 20:02:07]

Yusuke, could you PTAL?

[Yusuke Sato, 2017-02-08 20:37:18]

yusukes@chromium.org changed reviewers:
+ hidehiko@chromium.org

[Yusuke Sato, 2017-02-08 20:37:18]

+hidehiko who would be the best reviewer for the files.

[commit-bot: I haz the power, 2017-02-08 20:47:13]

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

[commit-bot: I haz the power, 2017-02-08 20:47:14]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-08 21:17:43]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen when there is nothing to be requested
from the user on that screen, i.e. when the following conditions
are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

[emaxx, 2017-02-08 21:18:05]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=new unit test;
     manual test: enable ARC by policy, set ArcBackupRestoreEnabled
         and ArcLocationServiceEnabled policies to some values, start
         a fresh profile, check that ARC eventually gets provisioned
         without showing any dialogs
==========

to

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

[emaxx, 2017-02-09 15:02:18]

Description was changed from

==========
Skip ARC initial screen when policies are set up

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

[hidehiko, 2017-02-09 15:28:04]

Posted a question on the bug.

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:530: if
(!g_disable_ui_for_testing && prefs->HasPrefPath(prefs::kArcEnabled)) {
Style: Could you elide the brace for one-line if-body case?

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:566:
prefs->SetBoolean(prefs::kArcTermsAccepted, true);
How about:

// Skip to show UI asking users to enable/disable their preference for
backup-restore and location-service, if both are managed by the admin policy.
// Note that, in managed case, ToS agreement is already taken in ${where it is},
so it is ok to mark arc.terms.accepted.

BTW, is there any case that arc.enabled pref is changed from managed to
unmanaged at some point while ARC is running? If yes, what's the expected
behavior in the case?

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:610: if (support_host_ &&
Good catch! Thank you for fix.

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448:
TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) {
Could you use parameterized test so that what failed is easily identified.

[emaxx, 2017-02-10 14:50:10]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 14:50:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 15:33:17]

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

[commit-bot: I haz the power, 2017-02-10 15:33:17]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-10 15:40:16]

On 2017/02/09 15:28:04, hidehiko wrote:
> Posted a question on the bug.

So I implemented a different approach that allows to return to the "disabled"
state once the managed values go away and there was no explicit user decision
yet.

However, this doesn't work fully now, due to the way how the settings
synchronization with ARC is implemented (the ArcSettingsServiceImpl class; see
also b/35043055). I'm investigating now whether that issue can be fixed
separately, so that the implementation from this CL may function correctly...

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:530: if
(!g_disable_ui_for_testing && prefs->HasPrefPath(prefs::kArcEnabled)) {
On 2017/02/09 15:28:04, hidehiko wrote:
> Style: Could you elide the brace for one-line if-body case?

Done.

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:566:
prefs->SetBoolean(prefs::kArcTermsAccepted, true);
On 2017/02/09 15:28:04, hidehiko wrote:
> How about:
> 
> // Skip to show UI asking users to enable/disable their preference for
> backup-restore and location-service, if both are managed by the admin policy.
> // Note that, in managed case, ToS agreement is already taken in ${where it
is},
> so it is ok to mark arc.terms.accepted.
> 
> BTW, is there any case that arc.enabled pref is changed from managed to
> unmanaged at some point while ARC is running? If yes, what's the expected
> behavior in the case?

Done, with a slight modification of the second sentence, because the actual ToS
text is already hidden in the managed ARC case regardless of this CL.
PTAL.

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:610: if (support_host_ &&
On 2017/02/09 15:28:04, hidehiko wrote:
> Good catch! Thank you for fix.

Acknowledged.

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448:
TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) {
On 2017/02/09 15:28:04, hidehiko wrote:
> Could you use parameterized test so that what failed is easily identified.

Quick question: do you think this will be better, given that the debug
information is currently printed by the test code too? (Note the last EXPECT_EQ
in this function - all other EXPECT* statements are actually assertions which
should hold regardless of the tested pref values.)
I'm just concerned a little bit about bloating the test code.

[emaxx, 2017-02-10 17:22:18]

Please hold on reviewing. After some offline discussion, an idea for how to deal
with the corner cases emerged.

[emaxx, 2017-02-10 17:50:17]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 17:50:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 18:44:38]

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

[commit-bot: I haz the power, 2017-02-10 18:44:39]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-13 17:13:02]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 17:13:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-02-13 17:18:54]

emaxx@chromium.org changed reviewers:
+ khmel@chromium.org

[emaxx, 2017-02-13 17:18:55]

I've updated the CL to support the "managed pref"=>"unmanaged pref" transition.
PTAL.


+khmel: Yury, as per our offline discussion, the CL now contains the pref
manipulation that allows to support the "managed pref"=>"unmanaged" transition.
As for another idea - assign the prefs to "true" when the ToS screen is shown -
then I decided against it, as if the user closes the ToS screen without pressing
"Agree", then the prefs would contain wrong values.

[emaxx, 2017-02-13 17:21:38]

Description was changed from

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

[emaxx, 2017-02-13 17:58:19]

Description was changed from

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit test;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

[commit-bot: I haz the power, 2017-02-13 18:03:36]

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

[commit-bot: I haz the power, 2017-02-13 18:03:36]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-14 11:51:36]

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448:
TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) {
On 2017/02/10 15:40:16, emaxx wrote:
> On 2017/02/09 15:28:04, hidehiko wrote:
> > Could you use parameterized test so that what failed is easily identified.
> 
> Quick question: do you think this will be better, given that the debug
> information is currently printed by the test code too? (Note the last
EXPECT_EQ
> in this function - all other EXPECT* statements are actually assertions which
> should hold regardless of the tested pref values.)
> I'm just concerned a little bit about bloating the test code.

I do not think using WithParamInterface bloats the code. It'd be something like;

class ArcSessionManagerPolicyTest : public ArcSessionManagerTest, public
testing::WithParamInterface<std::tuple<base::Value, base::Value>> {

  const base::Value& GetBackupRestorePref() {
    return std::get<0>(GetParam());
  }
  const base::Value& GetLocationServicePref() {
    return std::get<1>(GetParam());
  }
};

INSTANTIATE_TEST_CASE_P(
    ArcSessionManagerPolicyTest,
    ArcSessionManagerPolicyTest,
    testing::Combine(
        testing::Values(base::Value(), base::Value(false), base::Value(true)),
        testing::Values(base::Value(), base::Value(false), base::Value(true))));

So that you can flatten all the contents in the nested for stmt.
Additional benefit to the logs is, all inflight test state is reset for each
case, so even if some are failed, others won't be affected.
WDYT?

Note: you may want/need to rebase on top of
https://codereview.chromium.org/2691213002/, to avoid name conflicting.

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:334:
prefs->ClearPref(pref);
In general, side effect in a function to check the condition (e.g. IsSomething,
HasSomething, CanSomething, ShouldSomething etc. in Chrome) is practically
confusing. Actually, it is also marked as "const".

Could you avoid them?
Maybe renaming somehow?

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc
(right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84:
const bool checkbox_checked = enabled || !managed;
For unmanaged users, even if backup-and-restore is set to false, ArcSupport will
receive "true", which sounds unexpected behavior.

Could you kindly explain what you'd like to do here?

Ditto for below.

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:29:
protected:
nit: could you keep public?

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:128:
base::MakeUnique<base::Value>(false).release());
nit: new base::Value(false)

BTW, according to the comments of SetManagedPref, it takes the ownership of the
Value. Can it be changed to std::unique_ptr<base::Value> (maybe in a separate
CL?).

[emaxx, 2017-02-14 17:33:18]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 17:33:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 18:18:54]

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

[commit-bot: I haz the power, 2017-02-14 18:18:55]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-15 01:21:36]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 01:22:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 03:29:56]

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

[commit-bot: I haz the power, 2017-02-15 03:29:57]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[emaxx, 2017-02-15 03:40:43]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[emaxx, 2017-02-15 03:40:59]

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2682833003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:448:
TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) {
On 2017/02/14 11:51:36, hidehiko wrote:
> On 2017/02/10 15:40:16, emaxx wrote:
> > On 2017/02/09 15:28:04, hidehiko wrote:
> > > Could you use parameterized test so that what failed is easily identified.
> > 
> > Quick question: do you think this will be better, given that the debug
> > information is currently printed by the test code too? (Note the last
> EXPECT_EQ
> > in this function - all other EXPECT* statements are actually assertions
which
> > should hold regardless of the tested pref values.)
> > I'm just concerned a little bit about bloating the test code.
> 
> I do not think using WithParamInterface bloats the code. It'd be something
like;
> 
> class ArcSessionManagerPolicyTest : public ArcSessionManagerTest, public
> testing::WithParamInterface<std::tuple<base::Value, base::Value>> {
> 
>   const base::Value& GetBackupRestorePref() {
>     return std::get<0>(GetParam());
>   }
>   const base::Value& GetLocationServicePref() {
>     return std::get<1>(GetParam());
>   }
> };
> 
> INSTANTIATE_TEST_CASE_P(
>     ArcSessionManagerPolicyTest,
>     ArcSessionManagerPolicyTest,
>     testing::Combine(
>         testing::Values(base::Value(), base::Value(false), base::Value(true)),
>         testing::Values(base::Value(), base::Value(false),
base::Value(true))));
> 
> So that you can flatten all the contents in the nested for stmt.
> Additional benefit to the logs is, all inflight test state is reset for each
> case, so even if some are failed, others won't be affected.
> WDYT?
> 
> Note: you may want/need to rebase on top of
> https://codereview.chromium.org/2691213002/, to avoid name conflicting.

Done. Thanks for the detailed comment!

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:334:
prefs->ClearPref(pref);
On 2017/02/14 11:51:36, hidehiko wrote:
> In general, side effect in a function to check the condition (e.g.
IsSomething,
> HasSomething, CanSomething, ShouldSomething etc. in Chrome) is practically
> confusing. Actually, it is also marked as "const".
> 
> Could you avoid them?
> Maybe renaming somehow?

Done. I left only non-modifying operations in the Should* methods, and moved
modifications into the Sync* methods.
(This required to move away from sharing this bit of code, but the duplication
probably made the code even clearer.)

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc
(right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84:
const bool checkbox_checked = enabled || !managed;
On 2017/02/14 11:51:36, hidehiko wrote:
> For unmanaged users, even if backup-and-restore is set to false, ArcSupport
will
> receive "true", which sounds unexpected behavior.
> 
> Could you kindly explain what you'd like to do here?
> 
> Ditto for below.

That's exactly what I was trying to achieve: the pref has its default ("false")
value
until the ToS screen completes, while the checkbox displayed to the user in the
unmanaged case should be "on" by default.
Added a comment on that.

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:29:
protected:
On 2017/02/14 11:51:36, hidehiko wrote:
> nit: could you keep public?

Done. (Although I thought it's more common in the Chrome code base to use
"protected" in such cases.)

https://codereview.chromium.org/2682833003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc:128:
base::MakeUnique<base::Value>(false).release());
On 2017/02/14 11:51:36, hidehiko wrote:
> nit: new base::Value(false)
> 
> BTW, according to the comments of SetManagedPref, it takes the ownership of
the
> Value. Can it be changed to std::unique_ptr<base::Value> (maybe in a separate
> CL?).

Done.
Created crbug.com/692121 to track that refactoring.

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right):

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:120: void
SyncInitialSettings() const;
Moved this from the group of other Sync* methods, as I think this method is
conceptually separate from them.

[commit-bot: I haz the power, 2017-02-15 03:41:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 04:30:26]

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

[commit-bot: I haz the power, 2017-02-15 04:30:27]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-15 04:59:16]

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc
(right):

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84:
// Tick the checkbox by default in the unmanaged case, regardless of the pref
Ok, then, could you move them to arc_optin_preference_handler.cc instead?
Here, callbacks are called on initialization *or update*.
So, anytime even the preference is explicitly set to false, the checkbox is
set to true (= checked), if it is for unmanaged user.

You can see if the value is explicitly set or not by HasPref() in
ArcOptinPreferenceHandler.

[emaxx, 2017-02-15 05:12:31]

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc
(right):

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84:
// Tick the checkbox by default in the unmanaged case, regardless of the pref
On 2017/02/15 04:59:16, hidehiko wrote:
> Ok, then, could you move them to arc_optin_preference_handler.cc instead?
> Here, callbacks are called on initialization *or update*.
> So, anytime even the preference is explicitly set to false, the checkbox is
> set to true (= checked), if it is for unmanaged user.
> 
> You can see if the value is explicitly set or not by HasPref() in
> ArcOptinPreferenceHandler.
> 

Hmm, but would it be more appropriate in ArcOptinPreferenceHandler? The
interface of this class seems to talk about actual prefs, and so is
ArcOptInPreferenceHandlerObserver. And it's probably not clean to "patch" the
passed pref value for the sole goal of having the desired UI effect (having the
checkbox checked by default).

Another option is to modify the ArcOptInPreferenceHandlerObserver interface to
include the HasPref bit of information, so that the observers may choose how to
react...

[hidehiko, 2017-02-15 14:38:43]

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc
(right):

https://codereview.chromium.org/2682833003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:84:
// Tick the checkbox by default in the unmanaged case, regardless of the pref
On 2017/02/15 05:12:31, emaxx wrote:
> On 2017/02/15 04:59:16, hidehiko wrote:
> > Ok, then, could you move them to arc_optin_preference_handler.cc instead?
> > Here, callbacks are called on initialization *or update*.
> > So, anytime even the preference is explicitly set to false, the checkbox is
> > set to true (= checked), if it is for unmanaged user.
> > 
> > You can see if the value is explicitly set or not by HasPref() in
> > ArcOptinPreferenceHandler.
> > 
> 
> Hmm, but would it be more appropriate in ArcOptinPreferenceHandler? The
> interface of this class seems to talk about actual prefs,

I think it is ok to modify ArcOptinPreferenceHandler directly.
Quote from the comment: "This helper encapsulates access to preferences"...
I think what you're trying to do is changing default value of the preference
based on managed state, which is not supported by Preference framework by
default.

> and so is
> ArcOptInPreferenceHandlerObserver. And it's probably not clean to "patch" the
> passed pref value for the sole goal of having the desired UI effect (having
the
> checkbox checked by default).

So, in the above sense, it is not only for changing checkbox default value, but
also
changing default value for the preference (assuming all access to the preference
should pass through the class).

What I'm imaging is something like;
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/optin/arc_op...

bool is_managed = pref_service->IsManagedPreference(...);

// If the preference is not set, switch the default value based on whether
// it is managed preference or not: "true" if unmanaged, otherwise "false".
bool value =
    pref_service_->HasPrefPath(kArcBackupRestoreEnabled) ?
    pref_service_->GetBoolean(...) :
    !is_managed;
observer_->OnBackupAndRestoreModeChanged(value, is_managed);

WDYT?

> 
> Another option is to modify the ArcOptInPreferenceHandlerObserver interface to
> include the HasPref bit of information, so that the observers may choose how
to
> react...

[emaxx, 2017-02-15 15:15:26]

Hidehiko, I see now.
But wouldn't this be a bit fragile in the sense that some consumers
that read the pref value directly would get the contradicting
information comparing to the consumers of ArcOptinPreferenceHandler?
There is already one such consumer - ArcSettingsServiceImpl; it's
fine as it works only after optin, but there may be more added in
the future.

[khmel, 2017-02-15 16:06:06]

High level comment.

We should handle OOBE case too. Please see 

chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc
chrome/browser/chromeos/login/wizard_controller.cc
(ShowArcTermsOfServiceScreen)

[emaxx, 2017-02-16 15:59:05]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 15:59:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 16:02:54]

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

[commit-bot: I haz the power, 2017-02-16 16:02:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2017-02-16 16:07:46]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 16:08:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-02-16 16:27:49]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 16:28:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-02-16 16:57:45]

As per offline discussion with Hidehiko, I've updated the CL to use the simpler
approach with putting the logic around default values into
ArcOptInPreferenceHandler. This allows to reuse the same logic for both UIs:
in-session (ArcTermsOfServiceDefaultNegotiator) and OOBE
(ArcTermsOfServiceScreenHandler).
Also placed a comment to clarify that ArcOptInPreferenceHandler should only be
used for OptIn flow.

On 2017/02/15 16:06:06, khmel wrote:
> High level comment.
> 
> We should handle OOBE case too.

Thanks, this is fixed now. The checkboxes will receive the correct values (true
by default, and the managed value if there's one).
Note that the ARC OOBE screen won't be skipped even when all policies are set -
this should be tackled in a separate task.

[emaxx, 2017-02-16 17:03:03]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 17:03:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 17:51:00]

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

[commit-bot: I haz the power, 2017-02-16 17:51:01]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-02-16 19:57:21]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 19:58:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 20:05:05]

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

[commit-bot: I haz the power, 2017-02-16 20:05:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[emaxx, 2017-02-16 20:36:08]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 20:37:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 21:58:27]

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

[commit-bot: I haz the power, 2017-02-16 21:58:29]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-17 09:03:01]

lgtm

https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h (right):

https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h:24: // Note
that the preferences and metrics mode passed by this class should only
Nice comment!

https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
(right):

https://codereview.chromium.org/2682833003/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h:13: //
Notifies that the metrics mode has been changed.
Thank you for clean up!

[emaxx, 2017-02-17 16:25:47]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2017-02-17 16:26:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 16:36:36]

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

[commit-bot: I haz the power, 2017-02-17 16:36:39]

Failed to apply patch for
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h:10
  error:
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h: patch
does not apply

Patch:      
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
Index: chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
diff --git
a/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
b/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
index
c1518c2b6e67799b943736fc6b14c3425e714d14..f41f8726c47b195fd5ada78135bc02ab48892688
100644
--- a/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
+++ b/chrome/browser/chromeos/arc/optin/arc_optin_preference_handler_observer.h
@@ -10,11 +10,11 @@ namespace arc {
 // Notifies about changes in Arc related preferences and metrics mode.
 class ArcOptInPreferenceHandlerObserver {
  public:
-  // Notifies metrics mode has been changed.
+  // Notifies that the metrics mode has been changed.
   virtual void OnMetricsModeChanged(bool enabled, bool managed) = 0;
-  // Notifies use backup and restore preference has been changed.
+  // Notifies that the backup and restore mode has been changed.
   virtual void OnBackupAndRestoreModeChanged(bool enabled, bool managed) = 0;
-  // Notifies location service consent preference has been changed.
+  // Notifies that the location service mode has been changed.
   virtual void OnLocationServicesModeChanged(bool enabled, bool managed) = 0;
 
   virtual ~ArcOptInPreferenceHandlerObserver() = default;

[emaxx, 2017-02-17 16:49:31]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2017-02-17 16:49:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2682833003/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-17 16:50:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 18:16:23]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487350171215170,
"parent_rev": "54e8d335c0e4bebe49fbb2cacc5e49c06f29dc6e", "commit_rev":
"1f0e3460633a614baa787c46a2c8031fc13c8dad"}

[commit-bot: I haz the power, 2017-02-17 18:17:03]

Description was changed from

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451340}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
==========

[commit-bot: I haz the power, 2017-02-17 18:17:05]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...

[alph, 2017-02-18 02:08:23]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2702893002/ by alph@chromium.org.

The reason for reverting is: Tentative revert for breaking
RestoreOnStartupTestChromeOS.PRE_LogInAndVerify

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....

[alph, 2017-02-18 04:32:53]

Description was changed from

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451340}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451340}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
==========

[alph, 2017-02-18 04:33:02]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2017-02-18 04:33:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 07:01:31]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487392382829600,
"parent_rev": "7c0ecf945d1514a474eb184587112cd7f50b7e9d", "commit_rev":
"f392d837d227d79a9b8dec1d298e9c562929c7cf"}

[commit-bot: I haz the power, 2017-02-18 07:04:59]

Description was changed from

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451340}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
==========

to

==========
Skip ARC initial screen when everything is set up by policy

Skip the ARC initial screen during provision when there is nothing
to be requested from the user on that screen, i.e. when the following
conditions are all true:
1. ArcEnabled policy is set to true;
2. ArcBackupRestoreEnabled policy is set to some value;
3. ArcLocationServiceEnabled policy is set to some value.

Note that the subsequent screen with the progress spinner will
also be hidden. This is the intended behavior - the UI feedback for
the user, if we decide to add it, will have to be implemented in a
different, less noisy for the user way.

If some of the {ArcBackupRestoreEnabled;ArcLocationServiceEnabled}
policies become unset later, then the corresponding preferences will
change to the "false" state (which means that the user would have to
enable these features manually if they want to).

BUG=690121
BUG=b/34285262
TEST=New unit and browser tests;
     Manual test: enable ARC by policy, set ArcBackupRestoreEnabled
     and ArcLocationServiceEnabled policies to some values, start
     a fresh profile, check that ARC eventually gets provisioned
     without showing any dialogs.

Review-Url: https://codereview.chromium.org/2682833003
Cr-Original-Commit-Position: refs/heads/master@{#451340}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f0e3460633a614baa787c46a2c8...
Review-Url: https://codereview.chromium.org/2682833003
Cr-Commit-Position: refs/heads/master@{#451437}
Committed:
https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e...
==========

[commit-bot: I haz the power, 2017-02-18 07:05:01]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/f392d837d227d79a9b8dec1d298e...