Set drive as the default download folder

chrome/browser/download/download_dir_policy_handler_unittest.cc

Index: chrome/browser/download/download_dir_policy_handler_unittest.cc
diff --git a/chrome/browser/download/download_dir_policy_handler_unittest.cc b/chrome/browser/download/download_dir_policy_handler_unittest.cc
index 1b5e8b7a9a617fb12dcb0b5a904aa1a93a2a6d74..a00e0af550e492f78526e04ca7e9a23bc8597ed1 100644
--- a/chrome/browser/download/download_dir_policy_handler_unittest.cc
+++ b/chrome/browser/download/download_dir_policy_handler_unittest.cc
@@ -1,16 +1,30 @@
 // 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>

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

Nit: Add blank line before.

 
+#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 "chrome/test/base/scoped_testing_local_state.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_map.h"
 #include "policy/policy_constants.h"
 
+namespace {
+
+const char* kUserName = "dummy@downloaddirpolicyhandlertest.com";

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

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/

+
+#if defined(OS_CHROMEOS)
+const char* kDriveNamePolicyVariableName = "${google_drive}";
+#endif
+
+}  // namespace
+
 class DownloadDirPolicyHandlerTest
     : public policy::ConfigurationPolicyPrefStoreTest {
  public:
@@ -19,8 +33,22 @@ class DownloadDirPolicyHandlerTest
         make_scoped_ptr<policy::ConfigurationPolicyHandler>(
             new DownloadDirPolicyHandler));
   }
+  virtual void PopulatePolicyHandlerParameters(

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

Nit: Add blank line before.

+      policy::PolicyHandlerParameters* perf) OVERRIDE {

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

Nit: s/perf/parameters/

+    perf->user_id_hash = kUserName;

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

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

+  }
+
+  std::string GetStringFromStore(const std::string& key) {
+    const base::Value* value = NULL;
+    std::string string_value;
+    EXPECT_TRUE(store_->GetValue(key, &value));
+    EXPECT_TRUE(value);
+    EXPECT_TRUE(value->GetAsString(&string_value));
+    return string_value;
+  }
 };
 
+#if !defined(OS_CHROMEOS)
 TEST_F(DownloadDirPolicyHandlerTest, SetDownloadDirectory) {
   policy::PolicyMap policy;
   EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
@@ -40,3 +68,61 @@ TEST_F(DownloadDirPolicyHandlerTest, SetDownloadDirectory) {
   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 13:43:45]

Nit: No need for this. You could just use NULL instead of |value| because you
never inspect |value|.

+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
+
+  policy.Set(policy::key::kDownloadDirectory,

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

What is this testing? The policy is not supported at machine scope.

+             policy::POLICY_LEVEL_MANDATORY,
+             policy::POLICY_SCOPE_MACHINE,
+             new base::StringValue(kDriveNamePolicyVariableName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, &value));
+  EXPECT_FALSE(store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));

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

Nit: You just checked the value of |prefs::kPromptForDownload| three lines
above.

+  policy.Set(policy::key::kDownloadDirectory,

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

What is this testing? The policy cannot be recommended and is not supported at
machine scope.

+             policy::POLICY_LEVEL_RECOMMENDED,
+             policy::POLICY_SCOPE_MACHINE,
+             new base::StringValue(kDriveNamePolicyVariableName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, &value));
+  EXPECT_FALSE(store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
+
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_MANDATORY,
+             policy::POLICY_SCOPE_USER,
+             new base::StringValue(kDriveNamePolicyVariableName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, &value));
+  std::string download_directory =
+      GetStringFromStore(prefs::kDownloadDefaultDirectory);
+  EXPECT_TRUE(download_directory.find(kUserName) != std::string::npos);

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

1: Why not use drive::util::GetDriveMountPointPathUsingUserHash() to construct
the actual expected string and then compare the whole thing?
2: Nit: EXPECT_EQ

+
+#if 0

[dconnelly, 2014/03/14 12:53:18]

?

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

Either extend the policy to support being recommended or remove this incomplete
test.

+  policy.Clear();
+  UpdateProviderPolicy(policy);
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_RECOMMENDED,
+             policy::POLICY_SCOPE_USER,
+             new base::StringValue(kDriveNamePolicyVariableName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, &value));
+  download_directory = GetStringFromStore(prefs::kDownloadDefaultDirectory);
+  EXPECT_TRUE(download_directory.find(kUserName) != std::string::npos);
+#endif
+}
+#endif