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>

[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:
   virtual void SetUp() OVERRIDE {
     handler_list_.AddHandler(
         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));
   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 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