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 "base/values.h"
 #include "chrome/browser/download/download_dir_policy_handler.h"
 #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:
   virtual void SetUp() OVERRIDE {
     handler_list_.AddHandler(
         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) {
   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);
 }
+
+#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