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 49e6ae2531ab1b96ced126797d820fd31105e576..3e22cfbfb598c04c2710982a3b3a766d935c756a 100644
--- a/chrome/browser/download/download_dir_policy_handler_unittest.cc
+++ b/chrome/browser/download/download_dir_policy_handler_unittest.cc
@@ -7,10 +7,16 @@
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/policy/configuration_policy_pref_store_test.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/test/base/scoped_testing_local_state.h"
 #include "components/policy/core/browser/configuration_policy_pref_store.h"
 #include "components/policy/core/common/policy_map.h"
 #include "policy/policy_constants.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/login/fake_user_manager.h"
+#include "chrome/browser/chromeos/login/user.h"
+#endif
+
 class DownloadDirPolicyHandlerTest
     : public policy::ConfigurationPolicyPrefStoreTest {
  public:
@@ -19,6 +25,24 @@ class DownloadDirPolicyHandlerTest
         make_scoped_ptr<policy::ConfigurationPolicyHandler>(
             new DownloadDirPolicyHandler));
   }
+
+  bool GetBooleanFromStore(const std::string& key) {

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

Nit: #include <string>

+    const base::Value* value = NULL;
+    bool return_flag;

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

Nit: How about "boolean_value" instead?

+    EXPECT_TRUE(store_->GetValue(key, &value));
+    EXPECT_TRUE(value != NULL);

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

Nit: No need for "!= NULL". EXPECT_TRUE(value) is sufficient.

+    EXPECT_TRUE(value->GetAsBoolean(&return_flag));
+    return return_flag;
+  }
+
+  bool StringValueEquals(const std::string& key, const std::string& expected) {

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

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.

+    const base::Value* value = NULL;
+    std::string string_value;
+    EXPECT_TRUE(store_->GetValue(key, &value));
+    EXPECT_TRUE(value != NULL);

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

Nit: No need for "!= NULL". EXPECT_TRUE(value) is sufficient.

+    EXPECT_TRUE(value->GetAsString(&string_value));
+    return string_value.compare(expected) == 0;

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

Nit: std::string provides an operator==(), so you can just do:

return string_value == expected;

+  }
 };
 
 TEST_F(DownloadDirPolicyHandlerTest, SetDownloadDirectory) {
@@ -40,3 +64,37 @@ TEST_F(DownloadDirPolicyHandlerTest, SetDownloadDirectory) {
   ASSERT_TRUE(result);
   EXPECT_FALSE(prompt_for_download);
 }
+
+#if 0  // defined(OS_CHROMEOS)

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

could this test be enabled?

+TEST_F(DownloadDirPolicyHandlerTest, SetDownloadToDrive) {
+  const char* kDriveNamePolicyVarName = "${drive_name}";
+
+  policy::PolicyMap policy;
+  EXPECT_FALSE(store_->GetValue(prefs::kPromptForDownload, NULL));
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_MANDATORY,

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

Now that the policy supports both mandatory and recommended, both should be
tested.

+             policy::POLICY_SCOPE_MACHINE,

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

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.

+             base::Value::CreateStringValue(kDriveNamePolicyVarName),

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

Nit: CreateStringValue() is deprecated. Use "new base::StringValue()" instead.

+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_FALSE(GetBooleanFromStore(prefs::kPromptForDownload));
+  EXPECT_TRUE(StringValueEquals(prefs::kDownloadDefaultDirectory, ""));
+
+  policy.Set(policy::key::kDownloadDirectory,
+             policy::POLICY_LEVEL_MANDATORY,
+             policy::POLICY_SCOPE_USER,
+             base::Value::CreateStringValue(kDriveNamePolicyVarName),
+             NULL);
+  UpdateProviderPolicy(policy);
+
+  EXPECT_TRUE(GetBooleanFromStore(prefs::kPromptForDownload));
+  EXPECT_TRUE(StringValueEquals(prefs::kDownloadDefaultDirectory, ""));
+
+  chromeos::FakeUserManager* user_manager = new chromeos::FakeUserManager();
+  chromeos::ScopedUserManagerEnabler user_manager_enabler(user_manager);

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

Nit: #include "chrome/browser/chromeos/login/user_manager.h"

+  const char test_user_email[] = "test_user@example.com";

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

Nit 1: s/email/user_id/
Nit 2: Place constants in an anonymous namespace at the top of the file.

+  user_manager->AddUser(test_user_email);
+  user_manager->LoginUser(test_user_email);
+}

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

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.

+#endif