Set drive as the default download folder

chrome/browser/download/download_dir_policy_handler_unittest.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 <string>
+
+#include "base/compiler_specific.h"
 #include "base/values.h"
 #include "chrome/browser/download/download_dir_policy_handler.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/common/pref_names.h"
+#include "components/policy/core/browser/configuration_policy_handler_parameters.h"
 #include "components/policy/core/browser/configuration_policy_pref_store.h"
 #include "components/policy/core/browser/configuration_policy_pref_store_test.h"
+#include "components/policy/core/common/policy_details.h"
 #include "components/policy/core/common/policy_map.h"
 #include "policy/policy_constants.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/drive/file_system_util.h"
+#endif
+
+namespace {
+
+const base::FilePath::CharType* kUserIDHash = "deadbeef";

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

Nit: This one should be a char*.

+
+#if defined(OS_CHROMEOS)
+const base::FilePath::CharType* kDriveNamePolicyVariableName =

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

Nit: This one should be a char*.

+    "${google_drive}";
+const base::FilePath::CharType* kRootRelativeToDriveMount =

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

Nit: #include "base/files/file_path.h"

+    FILE_PATH_LITERAL("/root/");
+const base::FilePath::CharType* kRelativeToDriveRoot =
+    FILE_PATH_LITERAL("/home/");
+#endif
+
+}  // namespace
+
 class DownloadDirPolicyHandlerTest
     : public policy::ConfigurationPolicyPrefStoreTest {
  public:
   virtual void SetUp() OVERRIDE {
+    store2_ = new policy::ConfigurationPolicyPrefStore(
+        policy_service_.get(),
+        &handler_list_,
+        policy::POLICY_LEVEL_RECOMMENDED);

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

Nit: #include "components/policy/core/common/policy_types.h"

     handler_list_.AddHandler(
         make_scoped_ptr<policy::ConfigurationPolicyHandler>(
             new DownloadDirPolicyHandler));
   }
+
+  virtual void PopulatePolicyHandlerParameters(
+      policy::PolicyHandlerParameters* parameters) OVERRIDE {
+    parameters->user_id_hash = kUserIDHash;
+  }
+
+#if defined(OS_CHROMEOS)
+  std::string GetDownloadFolder() {

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

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?

+    return drive::util::GetDriveMountPointPathForUserIdHash(kUserIDHash)

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

Nit: A better way to do this is
GetDriveMountPointPathForUserIdHash(kUserIDHash).Append(kRootRelativeToDriveMount).value()

+               .value() +
+           base::FilePath::StringType(kRootRelativeToDriveMount);
+  }
+#endif
+
+ protected:
+  scoped_refptr<policy::ConfigurationPolicyPrefStore> store2_;

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

Nit 1: #include "base/memory/ref_counted.h"
Nit 2: s/store2/recommended_store/

 };
 
+#if !defined(OS_CHROMEOS)
 TEST_F(DownloadDirPolicyHandlerTest, SetDownloadDirectory) {
   policy::PolicyMap policy;
   EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
   policy.Set(policy::key::kDownloadDirectory,
              policy::POLICY_LEVEL_MANDATORY,
              policy::POLICY_SCOPE_USER,
              base::Value::CreateStringValue(std::string()),
              NULL);
   UpdateProviderPolicy(policy);
 
   // Setting a DownloadDirectory should disable the PromptForDownload pref.
   const base::Value* value = NULL;
   EXPECT_TRUE(store_->GetValue(prefs::kPromptForDownload, &value));
   ASSERT_TRUE(value);
   bool prompt_for_download = true;
   bool result = value->GetAsBoolean(&prompt_for_download);
   ASSERT_TRUE(result);
   EXPECT_FALSE(prompt_for_download);
 }
+#endif
+
+#if defined(OS_CHROMEOS)
+TEST_F(DownloadDirPolicyHandlerTest, SetDownloadToDrive) {
+  policy::PolicyMap policy;
+  const base::Value* value = NULL;

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

Nit: Define variables when you actually use them, not all at the top of the
method.

+  std::string download_directory;
+  bool prompt_for_download;
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
+
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_MANDATORY,
+             policy::POLICY_SCOPE_USER,
+             new base::StringValue(kDriveNamePolicyVariableName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_TRUE(store_->GetValue(prefs::kPromptForDownload, &value));
+  EXPECT_TRUE(value);
+  EXPECT_TRUE(value->GetAsBoolean(&prompt_for_download));
+  EXPECT_EQ(prompt_for_download, false);

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

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

+
+  EXPECT_TRUE(store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
+  EXPECT_TRUE(value);
+  EXPECT_TRUE(value->GetAsString(&download_directory));
+  EXPECT_EQ(download_directory, GetDownloadFolder());

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

Nit: The arguments go the other way around: Expected first, actual second.

+
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_RECOMMENDED,
+             policy::POLICY_SCOPE_USER,
+             new base::StringValue(
+                 base::FilePath::StringType(kDriveNamePolicyVariableName) +
+                 kRelativeToDriveRoot),

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

Why are you appending kRelativeToDriveRoot here?

+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(store2_->GetValue(prefs::kPromptForDownload, &value));
+  EXPECT_TRUE(store2_->GetValue(prefs::kDownloadDefaultDirectory, &value));
+  EXPECT_TRUE(value);
+  EXPECT_TRUE(value->GetAsString(&download_directory));
+  EXPECT_EQ(
+      download_directory,
+      GetDownloadFolder() + base::FilePath::StringType(kRelativeToDriveRoot));

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

Why are you appending kRelativeToDriveRoot here?

+}
+#endif