Set drive as the default download folder

Set drive as the default download folder

This CL modifies download directory policy handler to enable drive to be set as the default download folder in chromebooks.

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

[kaliamoorthi, 2014-03-12 16:10:53]

PTAL

[kinaba, 2014-03-13 03:56:50]

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_dir_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:25: const char*
kPathRelativeToDriveRoot = "/root/";
nit: char => base::FilePath::CharType (and FILE_PATH_LITERAL("/root/")

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:68: #if 0  //
defined(OS_CHROMEOS)
could this test be enabled?

[dconnelly, 2014-03-13 08:06:29]

lgtm

https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:1042: 'supported_on':
['chrome.*:11-', 'chrome_os:12-'],
why 12? has this actually been supported since cros 12?

[dconnelly, 2014-03-13 08:08:14]

On 2014/03/13 08:06:29, dconnelly wrote:
> lgtm

To be more clear: policy_templates.json lgtm. Didn't look at the rest.

> 
>
https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
> File components/policy/resources/policy_templates.json (right):
> 
>
https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
> components/policy/resources/policy_templates.json:1042: 'supported_on':
> ['chrome.*:11-', 'chrome_os:12-'],
> why 12? has this actually been supported since cros 12?

[bartfab (slow), 2014-03-13 13:19:59]

Please extend PolicyTest.DownloadDirectory to cover this policy on Chrome OS as
well.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system_util.h (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system_util.h:66: //
"/special/drive-<username_hash>" when provided with the <username_hash>
Nit 1: Add comma before "when".
Nit 2: Add full stop at the end.
Nit 3: Surround symbols in comments with ||, not with <>.
Nit 4: Specifying the format of the mount path in the comment makes no sense to
me. The whole point of this function is to hide this detail from anyone wishing
to access Drive.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system_util.h:67: base::FilePath
GetDriveMountPointPathUsingUserHash(std::string username_hash);
Nit 1: After an unfortunate proliferation of many different names for the same
thing, we eventually settled on |user_id|. Thus, s/username_hash/user_id_hash/.
Nit 2: Make the argument a const reference.
Nit 3: How about a method name like GetDriveMountPointForUser() instead?

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_dir_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:24: const char*
kDriveNamePolicyVarName = "${drive_name}";
1: Why the "name" in "drive_name"? How about something like ${google_drive}
instead?
2: Nit: Avoid abbreviations: s/Var/Variable/
3: Put constants in an anonymous namespace.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:25: const char*
kPathRelativeToDriveRoot = "/root/";
Document what this is.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:38: bool check_status =
CheckAndGetValue(policies, errors, &value);
Nit: Instead of introducing a boolean, do an early return:

if (!CheckAndGetValue(policies, errors, &value))
  return false;

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:41: if (check_status &&
value != NULL &&
1: When the check result is |false|, you should add an error message to
|errors|.
2: Nit: s/value != NULL/value/
3: Nit: Multi-line ifs require curly braces.
4: Nit: Document why this check is needed.
5: Nit: Instead of setting a boolean, do an early return false.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:52: NOTREACHED() << "This
policy handler should not be called";
Nit 1: #include "base/logging.h"
Nit 2: No need to add a message. NOTREACHED() means this place in the code
should never be reached :).

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:65:
policy::path_parser::ExpandPathVariables(string_value);
There is a policy_path_parser_linux.cc. Why not add a
policy_path_parser_chromeos.cc that handles the drive variable substitution?

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:67: if
(!string_value.compare(kDriveNamePolicyVarName))
You should do a substring match and substitution instead. Otherwise, the policy
cannot be used to set the download directory to e.g. "${google_drive}/Some
Subdirectory".

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.cc:82: if
(policies.Get(policy_name())->level == policy::POLICY_LEVEL_MANDATORY)
Nit: Multi-line conditionals require curly braces.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_dir_policy_handler.h (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.h:22: //
ConfigurationPolicyHandler methods:
Nit: Override and implement the methods in the same order as they are declared
in ConfigurationPolicyHandler.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler.h:26: PrefValueMap* prefs)
OVERRIDE;
Nit: #include "base/compiler_specific.h"

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:29: bool
GetBooleanFromStore(const std::string& key) {
Nit: #include <string>

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:31: bool
return_flag;
Nit: How about "boolean_value" instead?

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:33:
EXPECT_TRUE(value != NULL);
Nit: No need for "!= NULL". EXPECT_TRUE(value) is sufficient.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:38: bool
StringValueEquals(const std::string& key, const std::string& expected) {
Nit 1: Better implement a GetStringFromStore() and then compare the result in
the test body. The reason for this is that a failed EXPECT will print its line
number in the logs. A line number that points to the place in the test where
things went wrong is more useful than a line number that points inside a helper
function.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:42:
EXPECT_TRUE(value != NULL);
Nit: No need for "!= NULL". EXPECT_TRUE(value) is sufficient.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:44: return
string_value.compare(expected) == 0;
Nit: std::string provides an operator==(), so you can just do:

return string_value == expected;

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:75:
policy::POLICY_LEVEL_MANDATORY,
Now that the policy supports both mandatory and recommended, both should be
tested.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:76:
policy::POLICY_SCOPE_MACHINE,
I see no reason to test this with machine and user scopes separately. You are
feeding the policy to the same policy provider anyway, so it will be processed
by the user policy handlers no matter what.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:77:
base::Value::CreateStringValue(kDriveNamePolicyVarName),
Nit: CreateStringValue() is deprecated. Use "new base::StringValue()" instead.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:95:
chromeos::ScopedUserManagerEnabler user_manager_enabler(user_manager);
Nit: #include "chrome/browser/chromeos/login/user_manager.h"

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:96: const char
test_user_email[] = "test_user@example.com";
Nit 1: s/email/user_id/
Nit 2: Place constants in an anonymous namespace at the top of the file.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/download/...
chrome/browser/download/download_dir_policy_handler_unittest.cc:99: }
This test looks incomplete. It logs in as a test user but then does not verify
that the policy works correctly for the user.

Since this is a unit test, I recommend you do not set up a full fake UserManager
stack. Instead, just use a mock version of PopulatePolicyHandlerParameters()
that returns a user_id_hash.

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_handler_list_factory.h (right):

https://codereview.chromium.org/197013007/diff/20001/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_handler_list_factory.h:16: void
PopulatePolicyHandlerParameters(PolicyHandlerParameters* parameters);
Instead of exposing this function via the header, I think it would be better for
tests to implement their own mock version. That way, they can actually set a
user id hash if the want to. This function will always leave the hash blank in
unit tests.

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
File components/policy/core/browser/configuration_policy_handler.h (right):

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler.h:57: // This
function takes an additional input that could be used to supply
Nit: "that could be used to supply" makes no sense. The additional input *is*
used to supply additional parameters. You cannot call this method without
supplying parameters.

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler.h:58: // parameters
in |pars| to the policy handlers.
Nit: s/handlers/handler/

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler.h:59: virtual void
ApplyPolicySettingsWithPars(const PolicyMap& policies,
Nit: Avoid abbreviations: s/Pars/Parameters/

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler.h:60: const
PolicyHandlerParameters& pars,
Nit: Avoid abbreviations: s/pars/parameters/

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
File components/policy/core/browser/configuration_policy_handler_list.h (right):

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler_list.h:27: typedef
base::Callback<void(PolicyHandlerParameters*)>
Nit: Move the definitions inside ConfigurationPolicyHandlerList.

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
File components/policy/core/browser/configuration_policy_handler_parameters.h
(right):

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler_parameters.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Nit 1: s/2012/2014/
Nit 2: There are two variants of this header, one with (c) and one without. Last
I heard, the version without (c) was preferred for some reason.

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler_parameters.h:9: 
Nit: Document what this struct is for.

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler_parameters.h:10:
struct POLICY_EXPORT PolicyHandlerParameters {
Nit: #include "components/policy/policy_export.h"

https://codereview.chromium.org/197013007/diff/20001/components/policy/core/b...
components/policy/core/browser/configuration_policy_handler_parameters.h:11:
std::string active_username_hash;
1: Remove the "active_" prefix. When the ConfigurationPolicyHandlers are
running, they are processing policies for a particular user. The fact that this
happens to be the currently active user is a Chrome OS implementation detail.
2: Nit: #include <string>
3: Nit: s/username/user_id/

https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/197013007/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:1042: 'supported_on':
['chrome.*:11-', 'chrome_os:12-'],
Please update policy_test_cases.json so that this policy is tested on Chrome OS
as well.

[kaliamoorthi, 2014-03-13 19:53:21]

Hi Kazuhiro,

Please give me a lgtm for file system util changes. Please let me know if you
have some comments on it. This CL has to land tomorrow. Others here in Munich
are reviewing other files.

Hi Daniel,

Please also look at download_dir_policy_handler related files, since you are
listed as an owner.

Hi all,

This version fixes many nits. PTAL. A few related to allowing admin to choose a
subfolder in drive mount path are still pending.

[kinaba, 2014-03-13 22:31:40]

c/b/cros/drive lgtm

[dconnelly, 2014-03-14 12:52:05]

On 2014/03/13 22:31:40, kinaba wrote:
> c/b/cros/drive lgtm

Assuming Bartosz thinks the download_dir_policy_handler changes are reasonable,
then LGTM. I'm not familiar enough with the way CrOS interacts with Drive.

[dconnelly, 2014-03-14 12:53:17]

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:113: #if 0
?

[bartfab (slow), 2014-03-14 13:43:44]

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_system_util.h (right):

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util.h:63: //
"/special/drive-<username_hash>", when provided with the |userid_hash|.
Nit: s/userid/user_id/

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util.h:64: base::FilePath
GetDriveMountPointPathUsingUserHash(std::string userid_hash);
Nit 1: s/userid/user_id/
Nit 2: How about s/UsingUserHash/ForUser/? It better describes what you are
trying to get - a particular user's mount point path. The fact that this is
derived from the user's ID hash is secondary.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:29: // The relative path
of the drive download folder with respect to the path
Is this really the path to the *download* folder? Is it not the path to the root
of your drive?

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:50: // On chromeos,
setting Download directory is supported for the current user,
Nit 1: s/chromeos/Chrome OS/
Nit 2: s/Download/the download/
3: You are right that this cannot be set for the whole device. But that won't
happen on Chrome OS anyway because you cannot set the download directory through
device policy. What the check here really prevents is someone setting the
download directory via *platform policy*, which happens to be picked up by
chromeos=1 builds on Linux.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:72: #if
defined(OS_CHROMEOS)
Nit: File a bug and add a TODO to fold this Chrome OS-specific code into
policy::path_parser.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:74: if (position !=
std::string::npos) {
Nit: s/std::string/base::FilePath::StringType/

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:75:
base::FilePath::StringType drive_mount;
Nit: What you store in this variable is not the path to the mounting point but
the path to the root directory of the mounted drive. How about
s/drive_mount/google_drive_root/?

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:76: if
(!parameters.user_id_hash.empty())
Nit: Multi-line conditionals require curly braces.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:81: position,
strlen(kDriveNamePolicyVariableName), drive_mount);
Once you go for base::FilePath::CharType (which can be an 8-bit or 16-bit
character, depending on platform), you cannot use strlen(), which handles 8-bit
characters only.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:94: #if
!defined(OS_CHROMEOS)
The policy is documented as:

"If this policy is left not set the default download directory will be used and
the user will be able to change it."

With this #ifdef, that is not true on Chrome OS.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler.h (right):

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.h:29: const
policy::PolicyHandlerParameters& pars,
Nit: s/pars/parameters/

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.h:33:
DISALLOW_COPY_AND_ASSIGN(DownloadDirPolicyHandler);
Nit: #include "base/basictypes.h" (It's not your fault that this was not
included so far, but you actually need basictypes.h yourself now, for size_t
used in the implementation file)

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:4: #include
<string>
Nit: Add blank line before.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:20: const char*
kUserName = "dummy@downloaddirpolicyhandlertest.com";
Nit 1: s/downloaddirpolicyhandlertest.com/example.com/ (example.com was
specifically set aside for testing and documentation in the standard)
Nit 2: s/kUserName/kTestUserID/

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:36: virtual void
PopulatePolicyHandlerParameters(
Nit: Add blank line before.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:37:
policy::PolicyHandlerParameters* perf) OVERRIDE {
Nit: s/perf/parameters/

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:38:
perf->user_id_hash = kUserName;
You are setting the user ID hash to a user ID. The actual value is totally
irrelevant in this test of course but setting an ID hash to an ID is really
misleading for anyone who wants to understand the code. Instead of an ID that
looks like an e-mail address, just use a constant like kUserIDHash = "deadbeef".

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:76: const
base::Value* value = NULL;
Nit: No need for this. You could just use NULL instead of |value| because you
never inspect |value|.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:80:
policy.Set(policy::key::kDownloadDirectory,
What is this testing? The policy is not supported at machine scope.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:90:
EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
Nit: You just checked the value of |prefs::kPromptForDownload| three lines
above.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:91:
policy.Set(policy::key::kDownloadDirectory,
What is this testing? The policy cannot be recommended and is not supported at
machine scope.

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:111:
EXPECT_TRUE(download_directory.find(kUserName) != std::string::npos);
1: Why not use drive::util::GetDriveMountPointPathUsingUserHash() to construct
the actual expected string and then compare the whole thing?
2: Nit: EXPECT_EQ

https://codereview.chromium.org/197013007/diff/140001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:113: #if 0
Either extend the policy to support being recommended or remove this incomplete
test.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler.cc:54: NOTREACHED();
Nit: #include "base/logging.h"

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_handler.h (right):

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler.h:25: struct
PolicyHandlerParameters;
Nit: Arrange in alphabetic order.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler.h:57: // This
function takes an additional input that is used to supply
Nit: You can simplify this sentence, especially "supply parameters in
|parameters|" is unnecessarily verbose. How about:

This method forwards additional |parameters| to the policy handler.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler.h:59: virtual void
ApplyPolicySettingsWithParameters(
Since PolicyHandlerList always invokes ApplyPolicySettingsWithParameters(), this
is now the default entry point. You should document this. Instead of saying that
ApplyPolicySettingsWithParameters() takes additional |parameters|, it would be
better to document ApplyPolicySettings() as a convenience version of
ApplyPolicySettingsWithParameters() that leaves out the parameters, to be used
by those not interested in |parameters|. I would then swap the order of the two
methods so that the default entry point ApplyPolicySettingsWithParameters()
appears first.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_handler_list.h (right):

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler_list.h:25: struct
PolicyHandlerParameters;
Nit: Arrange in alphabetic order.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler_list.h:55:
PopulatePolicyHandlerParametersCallback parameters_callback_;
Nit: const

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_test.cc
(right):

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_test.cc:26:
base::Unretained(this)),
Nit: #include "base/bind_helpers.h"

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_test.cc:40:
PolicyHandlerParameters* perf) {}
Nit: s/perf/parameters/

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_test.h
(right):

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_test.h:22: struct
PolicyHandlerParameters;
Nit: Arrange in alphabetic order.

https://codereview.chromium.org/197013007/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_test.h:29:
virtual void PopulatePolicyHandlerParameters(PolicyHandlerParameters* perf);
Nit 1: Move this down a few lines so that instead of being lumped into this
block of methods, it stands on its own, and document that this can be overridden
to pass specific parameters to handlers.

Nit 2: s/perf/parameters/

https://codereview.chromium.org/197013007/diff/140001/components/policy_strin...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/197013007/diff/140001/components/policy_strin...
components/policy_strings.grdp:200: <message name="IDS_POLICY_SCOPE_ERROR"
desc="Text displayed in the status column when an incorrect value is set for the
policy scope.">
Nit: s/an incorrect value is set for the policy scope/a policy is set in an
unsupported scope/

[bartfab (slow), 2014-03-14 16:16:45]

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:72: //
TODO(kaliamoorthi): cleanup policy::path_parser and fold this code
Nit: s/cleanup/Clean up/

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:78: google_drive_root =
drive::util::GetDriveMountPointPathForUserIdHash(
Nit: A better way to do this is
GetDriveMountPointPathForUserIdHash(parameters.user_id_hash).Append(kRootRelativeToDriveMount).value()

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:98: if
(policies.Get(policy_name())->level == policy::POLICY_LEVEL_MANDATORY)
Nit 1: Multi-line conditionals require curly braces.
Nit 2: Please add a comment explaining why this is done for mandatory level
only.

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:25: const
base::FilePath::CharType* kUserIDHash = "deadbeef";
Nit: This one should be a char*.

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:28: const
base::FilePath::CharType* kDriveNamePolicyVariableName =
Nit: This one should be a char*.

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:30: const
base::FilePath::CharType* kRootRelativeToDriveMount =
Nit: #include "base/files/file_path.h"

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:45:
policy::POLICY_LEVEL_RECOMMENDED);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:57: std::string
GetDownloadFolder() {
Nit 1: This does not access any members. It can be a function in an anonymous
namespace instead.
Optional nit 2: How about GetExpectedDownloadDirectory() instead?

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:58: return
drive::util::GetDriveMountPointPathForUserIdHash(kUserIDHash)
Nit: A better way to do this is
GetDriveMountPointPathForUserIdHash(kUserIDHash).Append(kRootRelativeToDriveMount).value()

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:65:
scoped_refptr<policy::ConfigurationPolicyPrefStore> store2_;
Nit 1: #include "base/memory/ref_counted.h"
Nit 2: s/store2/recommended_store/

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:93: const
base::Value* value = NULL;
Nit: Define variables when you actually use them, not all at the top of the
method.

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:109:
EXPECT_EQ(prompt_for_download, false);
Nit 1: The arguments go the other way around: Expected first, actual second.
Nit 2: This can be simplified to EXPECT_FALSE(prompt_for_download);

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:114:
EXPECT_EQ(download_directory, GetDownloadFolder());
Nit: The arguments go the other way around: Expected first, actual second.

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:121:
kRelativeToDriveRoot),
Why are you appending kRelativeToDriveRoot here?

https://codereview.chromium.org/197013007/diff/160001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:131:
GetDownloadFolder() + base::FilePath::StringType(kRelativeToDriveRoot));
Why are you appending kRelativeToDriveRoot here?

https://codereview.chromium.org/197013007/diff/160001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_test.h
(right):

https://codereview.chromium.org/197013007/diff/160001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_test.h:31: // A
unit test could override this method to populate the policy handler
Nit: s/could/can/

[bartfab (slow), 2014-03-14 16:55:40]

Just a couple more nits. LGTM - ship it!

https://codereview.chromium.org/197013007/diff/180001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler.cc (right):

https://codereview.chromium.org/197013007/diff/180001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler.cc:100: // If the policy is
mandatory prompt for download should be disabled, if not
Nit 1: s/mandatory/mandatory, /
Nit 2: s/, if not/. Otherwise,/

https://codereview.chromium.org/197013007/diff/180001/chrome/browser/download...
File chrome/browser/download/download_dir_policy_handler_unittest.cc (right):

https://codereview.chromium.org/197013007/diff/180001/chrome/browser/download...
chrome/browser/download/download_dir_policy_handler_unittest.cc:64:
scoped_refptr<policy::ConfigurationPolicyPrefStore> recommended_store;
Nit: Add trailing underscore.

[kaliamoorthi, 2014-03-14 17:27:45]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/197013007/20...

[kaliamoorthi, 2014-03-14 17:41:48]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:45:57]

Failed to trigger a try job on mac_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-03-14 17:46:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/197013007/22...

[kaliamoorthi, 2014-03-14 17:46:28]

The CQ bit was unchecked by kaliamoorthi@chromium.org

[kaliamoorthi, 2014-03-14 17:46:40]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:48:11]

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

[commit-bot: I haz the power, 2014-03-14 17:48:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[bartfab (slow), 2014-03-14 17:50:57]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:51:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/197013007/22...

[commit-bot: I haz the power, 2014-03-14 19:28:34]

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

[commit-bot: I haz the power, 2014-03-14 19:28:36]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[bartfab (slow), 2014-03-14 19:35:23]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-03-14 19:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/197013007/22...

[commit-bot: I haz the power, 2014-03-14 21:27:01]

Change committed as 257206