Add a path parsing policy handler for the RoamingProfileLocation policy.

Add a path parsing policy handler for the RoamingProfileLocation policy.

This policy should like most other FilePath policies parse variables
in its content before storing its value in its backing pref.

BUG=726007
TEST=unit_tests

Review-Url: https://codereview.chromium.org/2912353003
Cr-Commit-Position: refs/heads/master@{#476272}
Committed: https://chromium.googlesource.com/chromium/src/+/f281a6c87fbb09457c03cdcbf99d33ba59c3a6b0

[pastarmovj, 2017-06-01 09:31:55]

pastarmovj@chromium.org changed reviewers:
+ ljusten@chromium.org

[pastarmovj, 2017-06-01 09:31:56]

Please review this CL for me.

Thanks,
Julian

[pastarmovj, 2017-06-01 09:32:05]

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

[commit-bot: I haz the power, 2017-06-01 09:32:12]

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-06-01 10:02:44]

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

[commit-bot: I haz the power, 2017-06-01 10:02:45]

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

[ljusten (tachyonic), 2017-06-01 10:05:54]

LGTM modulo minor stuff (but I'm not a committer)

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler.cc (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler.h (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler_unittest.cc (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:28:
EXPECT_FALSE(prefs_.GetValue(syncer::prefs::kLocalSyncBackendDir, NULL));
Nit: nullptr

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:32: //
RoamingProfileLocation policy expects a string; give it a boolean.
Nit: I'd suggest to move this comment above TEST_F because it describes the
general idea of the test.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:35:
base::MakeUnique<base::Value>(false), nullptr);
Nit: nullptr /* external_data_fetcher */

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:37:
EXPECT_FALSE(prefs_.GetValue(syncer::prefs::kLocalSyncBackendDir, NULL));
Nit: nullptr

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:41: // Use a
variable in the value. It should be expanded by the handler.
Same here.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:45:
base::MakeUnique<base::Value>(in), nullptr);
Nit: nullptr /* external_data_fetcher */

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:53:
EXPECT_EQ(std::string::npos, out.find("${user_name}"));
Is it possible to check whether ${user_name} was replaced by the actual user
name?

[pastarmovj, 2017-06-01 10:39:19]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-06-01 10:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ljusten@chromium.org
Link to the patchset: https://codereview.chromium.org/2912353003/#ps20001
(title: "Address comments and android compatibility.")

[commit-bot: I haz the power, 2017-06-01 10:39:26]

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-06-01 10:39:27]

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

[commit-bot: I haz the power, 2017-06-01 10:39:28]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[bartfab (slow), 2017-06-01 11:03:54]

bartfab@chromium.org changed reviewers:
+ bartfab@chromium.org

[bartfab (slow), 2017-06-01 11:03:55]

Committer stamp: LGTM

[pastarmovj, 2017-06-01 11:04:57]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-06-01 11:05:07]

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler.cc (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> 2017

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler.h (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> 2017

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
File chrome/browser/policy/local_sync_policy_handler_unittest.cc (right):

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> 2017

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:28:
EXPECT_FALSE(prefs_.GetValue(syncer::prefs::kLocalSyncBackendDir, NULL));
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:32: //
RoamingProfileLocation policy expects a string; give it a boolean.
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Nit: I'd suggest to move this comment above TEST_F because it describes the
> general idea of the test.

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:35:
base::MakeUnique<base::Value>(false), nullptr);
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Nit: nullptr /* external_data_fetcher */

I don't think this is particularly useful comment.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:37:
EXPECT_FALSE(prefs_.GetValue(syncer::prefs::kLocalSyncBackendDir, NULL));
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:41: // Use a
variable in the value. It should be expanded by the handler.
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Same here.

Done.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:45:
base::MakeUnique<base::Value>(in), nullptr);
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Nit: nullptr /* external_data_fetcher */

ditto.

https://codereview.chromium.org/2912353003/diff/1/chrome/browser/policy/local...
chrome/browser/policy/local_sync_policy_handler_unittest.cc:53:
EXPECT_EQ(std::string::npos, out.find("${user_name}"));
On 2017/06/01 10:05:53, ljusten (instantaneous) wrote:
> Is it possible to check whether ${user_name} was replaced by the actual user
> name?

Not really in a platform independent way. It is important here that substitution
is happening and we do have complete tests of the substitution logic in its own
unit tests.

[commit-bot: I haz the power, 2017-06-01 11:05:08]

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-06-01 11:33:00]

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

[commit-bot: I haz the power, 2017-06-01 11:33:01]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pastarmovj, 2017-06-01 12:29:41]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-06-01 12:29:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, ljusten@chromium.org
Link to the patchset: https://codereview.chromium.org/2912353003/#ps40001
(title: "Make all code not compile on Android at all.")

[commit-bot: I haz the power, 2017-06-01 12:29:48]

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-06-01 13:57:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496320181018250,
"parent_rev": "ff30a0e48df5a0de78c693aa3c377d760a4dfdf7", "commit_rev":
"f281a6c87fbb09457c03cdcbf99d33ba59c3a6b0"}

[commit-bot: I haz the power, 2017-06-01 13:57:38]

Description was changed from

==========
Add a path parsing policy handler for the RoamingProfileLocation policy.

This policy should like most other FilePath policies parse variables
in its content before storing its value in its backing pref.

BUG=726007
TEST=unit_tests
==========

to

==========
Add a path parsing policy handler for the RoamingProfileLocation policy.

This policy should like most other FilePath policies parse variables
in its content before storing its value in its backing pref.

BUG=726007
TEST=unit_tests

Review-Url: https://codereview.chromium.org/2912353003
Cr-Commit-Position: refs/heads/master@{#476272}
Committed:
https://chromium.googlesource.com/chromium/src/+/f281a6c87fbb09457c03cdcbf99d...
==========

[commit-bot: I haz the power, 2017-06-01 13:57:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f281a6c87fbb09457c03cdcbf99d...