Set drive as the default download folder

chrome/browser/download/download_dir_policy_handler.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_dir_policy_handler.h"
 
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_value_map.h"
 #include "base/values.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/policy/policy_path_parser.h"
 #include "chrome/common/pref_names.h"
+#include "components/policy/core/browser/configuration_policy_handler_parameters.h"
 #include "components/policy/core/common/policy_map.h"
+#include "components/policy/core/common/policy_types.h"
 #include "policy/policy_constants.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/drive/file_system_util.h"
+#endif
+
+#if defined(OS_CHROMEOS)
+const char* kDriveNamePolicyVarName = "${drive_name}";

[bartfab (slow), 2014/03/13 13:20:00]

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.

+const char* kPathRelativeToDriveRoot = "/root/";

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

nit: char => base::FilePath::CharType (and FILE_PATH_LITERAL("/root/")

[bartfab (slow), 2014/03/13 13:20:00]

Document what this is.

+#endif
+
 DownloadDirPolicyHandler::DownloadDirPolicyHandler()
     : TypeCheckingPolicyHandler(policy::key::kDownloadDirectory,
                                 base::Value::TYPE_STRING) {}
 
 DownloadDirPolicyHandler::~DownloadDirPolicyHandler() {}
 
+bool DownloadDirPolicyHandler::CheckPolicySettings(
+    const policy::PolicyMap& policies,
+    policy::PolicyErrorMap* errors) {
+  const base::Value* value = NULL;
+  bool check_status = CheckAndGetValue(policies, errors, &value);

[bartfab (slow), 2014/03/13 13:20:00]

Nit: Instead of introducing a boolean, do an early return:

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

+
+#if defined(OS_CHROMEOS)
+  if (check_status && value != NULL &&

[bartfab (slow), 2014/03/13 13:20:00]

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.

+      policies.Get(policy_name())->scope != policy::POLICY_SCOPE_USER)
+    check_status = false;
+#endif
+
+  return check_status;
+}
+
 void DownloadDirPolicyHandler::ApplyPolicySettings(
     const policy::PolicyMap& policies,
     PrefValueMap* prefs) {
+  NOTREACHED() << "This policy handler should not be called";

[bartfab (slow), 2014/03/13 13:20:00]

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

+}
+
+void DownloadDirPolicyHandler::ApplyPolicySettingsWithPars(
+    const policy::PolicyMap& policies,
+    const policy::PolicyHandlerParameters& pars,
+    PrefValueMap* prefs) {
   const base::Value* value = policies.GetValue(policy_name());
   base::FilePath::StringType string_value;
   if (!value || !value->GetAsString(&string_value))
     return;
 
   base::FilePath::StringType expanded_value =
       policy::path_parser::ExpandPathVariables(string_value);

[bartfab (slow), 2014/03/13 13:20:00]

There is a policy_path_parser_linux.cc. Why not add a
policy_path_parser_chromeos.cc that handles the drive variable substitution?

+#if defined(OS_CHROMEOS)
+  if (!string_value.compare(kDriveNamePolicyVarName))

[bartfab (slow), 2014/03/13 13:20:00]

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

+    expanded_value = pars.active_username_hash.empty()
+                         ? ""
+                         : drive::util::GetDriveMountPointPathUsingUserHash(
+                               pars.active_username_hash).value() +
+                               kPathRelativeToDriveRoot;
+#endif
   // Make sure the path isn't empty, since that will point to an undefined
   // location; the default location is used instead in that case.
   // This is checked after path expansion because a non-empty policy value can
   // lead to an empty path value after expansion (e.g. "\"\"").
   if (expanded_value.empty())
     expanded_value = DownloadPrefs::GetDefaultDownloadDirectory().value();
   prefs->SetValue(prefs::kDownloadDefaultDirectory,
                   base::Value::CreateStringValue(expanded_value));
-  prefs->SetValue(prefs::kPromptForDownload,
-                  base::Value::CreateBooleanValue(false));
+  if (policies.Get(policy_name())->level == policy::POLICY_LEVEL_MANDATORY)

[bartfab (slow), 2014/03/13 13:20:00]

Nit: Multi-line conditionals require curly braces.

+    prefs->SetValue(prefs::kPromptForDownload,
+                    base::Value::CreateBooleanValue(false));
 }