Add convenience Get method into base::PathService

Add a new CheckedGet() method that is equivalent to
base::PathService::Get(), but returns the result as a return value (and
CHECKs that the operation succeeded).

This is a pure convenience addition that allows to simplify the code
that anyway cannot correctly handle the path service being unavailable.
(In the existing codebase, there are already >100 callsites that do
CHECK(PathService::Get(...)), and also many callsites in tests that just
take the path returned by PathService::Get() without checking its return
value.)

Bug: none
Change-Id: I38535138ea08f0fd71e0eaf1979a1bdc99c504ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2279813Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786413}

base/path_service.cc

@@ -228,6 +228,12 @@ bool PathService::Get(int key, FilePath* result) {
   return true;
 }
 
+FilePath PathService::CheckedGet(int key) {
+  FilePath path;
+  CHECK(Get(key, &path));
+  return path;
+}
+
 // static
 bool PathService::Override(int key, const FilePath& path) {
   // Just call the full function with true for the value of |create|, and

base/path_service.h

@@ -27,6 +27,9 @@ class BASE_EXPORT PathService {
   // |path| will not be changed.
   static bool Get(int key, FilePath* path);
 
+  // Returns the corresponding path; CHECKs that the operation succeeds.
+  static FilePath CheckedGet(int key);
+
   // Overrides the path to a special directory or file.  This cannot be used to
   // change the value of DIR_CURRENT, but that should be obvious.  Also, if the
   // path specifies a directory that does not exist, the directory will be

base/path_service_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
+#include "base/test/gtest_util.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest-spi.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -141,6 +142,26 @@ TEST_F(PathServiceTest, Get) {
 #endif
 }
 
+// Tests that CheckedGet returns the same path as Get.
+TEST_F(PathServiceTest, CheckedGet) {
+  constexpr int kKey = DIR_CURRENT;
+  FilePath path;
+  ASSERT_TRUE(PathService::Get(kKey, &path));
+  EXPECT_EQ(path, PathService::CheckedGet(kKey));
+}
+
+#if defined(GTEST_HAS_DEATH_TEST)
+
+// Tests that CheckedGet CHECKs on failure.
+TEST_F(PathServiceTest, CheckedGetFailure) {
+  constexpr int kBadKey = PATH_END;
+  FilePath path;
+  EXPECT_FALSE(PathService::Get(kBadKey, &path));
+  EXPECT_CHECK_DEATH(PathService::CheckedGet(kBadKey));
+}
+
+#endif  // GTEST_HAS_DEATH_TEST
+
 // Test that all versions of the Override function of PathService do what they
 // are supposed to do.
 TEST_F(PathServiceTest, Override) {

base/process/process_util_unittest.cc

@@ -536,8 +536,7 @@ MULTIPROCESS_TEST_MAIN(CheckCwdProcess) {
   event.Wait();
 
   // Get a new cwd for the process.
-  FilePath home_dir;
-  CHECK(PathService::Get(DIR_HOME, &home_dir));
+  const FilePath home_dir = PathService::CheckedGet(DIR_HOME);
 
   // Change the cwd on the secondary thread. IgnoreResult is used when setting
   // because it is checked immediately after.

chrome/browser/chromeos/extensions/signin_screen_extensions_external_loader.cc

@@ -27,13 +27,6 @@ namespace chromeos {
 
 namespace {
 
-base::FilePath GetExtensionsCachePath() {
-  base::FilePath extensions_cache_path;
-  CHECK(base::PathService::Get(DIR_SIGNIN_PROFILE_EXTENSIONS,
-                               &extensions_cache_path));
-  return extensions_cache_path;
-}
-
 base::Value GetForceInstalledExtensionsFromPrefs(const PrefService* prefs) {
   const PrefService::Preference* const login_screen_extensions_pref =
       prefs->FindPreference(extensions::pref_names::kLoginScreenExtensions);
@@ -58,14 +51,15 @@ base::Value GetForceInstalledExtensionsFromPrefs(const PrefService* prefs) {
 SigninScreenExtensionsExternalLoader::SigninScreenExtensionsExternalLoader(
     Profile* profile)
     : profile_(profile),
-      external_cache_(GetExtensionsCachePath(),
-                      g_browser_process->shared_url_loader_factory(),
-                      base::ThreadPool::CreateSequencedTaskRunner(
-                          {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
-                           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}),
-                      this,
-                      /*always_check_updates=*/true,
-                      /*wait_for_cache_initialization=*/false) {
+      external_cache_(
+          base::PathService::CheckedGet(DIR_SIGNIN_PROFILE_EXTENSIONS),
+          g_browser_process->shared_url_loader_factory(),
+          base::ThreadPool::CreateSequencedTaskRunner(
+              {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+               base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}),
+          this,
+          /*always_check_updates=*/true,
+          /*wait_for_cache_initialization=*/false) {
   DCHECK(ProfileHelper::IsSigninProfile(profile));
 }
 

chrome/browser/devtools/devtools_sanity_browsertest.cc

@@ -434,11 +434,11 @@ void TimeoutCallback(const std::string& timeout_message) {
 class DevToolsExtensionTest : public DevToolsSanityTest,
                               public content::NotificationObserver {
  public:
-  DevToolsExtensionTest() : DevToolsSanityTest() {
-    base::PathService::Get(chrome::DIR_TEST_DATA, &test_extensions_dir_);
-    test_extensions_dir_ = test_extensions_dir_.AppendASCII("devtools");
-    test_extensions_dir_ = test_extensions_dir_.AppendASCII("extensions");
-  }
+  DevToolsExtensionTest()
+      : test_extensions_dir_(
+            base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
+                .AppendASCII("devtools")
+                .AppendASCII("extensions")) {}
 
  protected:
   // Load an extension from test\data\devtools\extensions\<extension_name>
@@ -606,7 +606,7 @@ class DevToolsExtensionTest : public DevToolsSanityTest,
 
   std::vector<std::unique_ptr<extensions::TestExtensionDir>>
       test_extension_dirs_;
-  base::FilePath test_extensions_dir_;
+  const base::FilePath test_extensions_dir_;
 };
 
 class DevToolsExperimentalExtensionTest : public DevToolsExtensionTest {
@@ -2344,13 +2344,11 @@ IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, TestOpenInNewTabFilter) {
 }
 
 IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, LoadNetworkResourceForFrontend) {
-  base::FilePath root_dir;
-  CHECK(base::PathService::Get(base::DIR_SOURCE_ROOT, &root_dir));
   std::string file_url =
-      "file://" +
-      root_dir.AppendASCII("content/test/data/devtools/navigation.html")
-          .NormalizePathSeparatorsTo('/')
-          .AsUTF8Unsafe();
+      "file://" + base::PathService::CheckedGet(base::DIR_SOURCE_ROOT)
+                      .AppendASCII("content/test/data/devtools/navigation.html")
+                      .NormalizePathSeparatorsTo('/')
+                      .AsUTF8Unsafe();
 
   embedded_test_server()->ServeFilesFromSourceDirectory("content/test/data");
   ASSERT_TRUE(embedded_test_server()->Start());

chrome/browser/extensions/updater/chromeos_extension_cache_delegate.cc

@@ -13,10 +13,9 @@
 
 namespace extensions {
 
-ChromeOSExtensionCacheDelegate::ChromeOSExtensionCacheDelegate() {
-  CHECK(base::PathService::Get(chromeos::DIR_DEVICE_EXTENSION_LOCAL_CACHE,
-                               &cache_dir_));
-}
+ChromeOSExtensionCacheDelegate::ChromeOSExtensionCacheDelegate()
+    : cache_dir_(base::PathService::CheckedGet(
+          chromeos::DIR_DEVICE_EXTENSION_LOCAL_CACHE)) {}
 
 ChromeOSExtensionCacheDelegate::ChromeOSExtensionCacheDelegate(
     const base::FilePath& cache_dir)

chrome/browser/extensions/updater/chromeos_extension_cache_delegate.h

@@ -26,7 +26,7 @@ class ChromeOSExtensionCacheDelegate {
   base::TimeDelta GetMaximumCacheAge() const;
 
  private:
-  base::FilePath cache_dir_;
+  const base::FilePath cache_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeOSExtensionCacheDelegate);
 };

chrome/browser/task_manager/sampling/shared_sampler_win_unittest.cc

@@ -170,9 +170,8 @@ TEST_F(SharedSamplerTest, MultipleRefreshTypes) {
 static int ReturnZeroThreadProcessInformation(unsigned char* buffer,
                                               int buffer_size) {
   // Calculate the number of bytes required for the structure, and ImageName.
-  base::FilePath current_exe;
-  CHECK(base::PathService::Get(base::FILE_EXE, &current_exe));
-  base::string16 image_name = current_exe.BaseName().value();
+  base::string16 image_name =
+      base::PathService::CheckedGet(base::FILE_EXE).BaseName().value();
 
   const int kImageNameBytes = image_name.length() * sizeof(base::char16);
   const int kRequiredBytes = sizeof(SYSTEM_PROCESS_INFORMATION) +

chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc

@@ -39,10 +39,9 @@ using extensions::TestManagementPolicyProvider;
 ExtensionSettingsUIBrowserTest::ExtensionSettingsUIBrowserTest()
     : policy_provider_(TestManagementPolicyProvider::PROHIBIT_MODIFY_STATUS |
                        TestManagementPolicyProvider::MUST_REMAIN_ENABLED |
-                       TestManagementPolicyProvider::MUST_REMAIN_INSTALLED) {
-  CHECK(base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
-  test_data_dir_ = test_data_dir_.AppendASCII("extensions");
-}
+                       TestManagementPolicyProvider::MUST_REMAIN_INSTALLED),
+      test_data_dir_(base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
+                         .AppendASCII("extensions")) {}
 
 ExtensionSettingsUIBrowserTest::~ExtensionSettingsUIBrowserTest() {}
 

chrome/browser/ui/webui/extensions/extension_settings_browsertest.h

@@ -63,7 +63,7 @@ class ExtensionSettingsUIBrowserTest : public WebUIBrowserTest {
   // Used to simulate managed extensions (by being registered as a provider).
   extensions::TestManagementPolicyProvider policy_provider_;
 
-  base::FilePath test_data_dir_;
+  const base::FilePath test_data_dir_;
 
   // Disable extension content verification.
   extensions::ScopedIgnoreContentVerifierForTest ignore_content_verification_;

chrome/browser/ui/webui/management_a11y_browsertest.cc

@@ -10,10 +10,9 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_paths.h"
 
-ManagementA11yUIBrowserTest::ManagementA11yUIBrowserTest() {
-  CHECK(base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
-  test_data_dir_ = test_data_dir_.AppendASCII("extensions");
-}
+ManagementA11yUIBrowserTest::ManagementA11yUIBrowserTest()
+    : test_data_dir_(base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
+                         .AppendASCII("extensions")) {}
 
 ManagementA11yUIBrowserTest::~ManagementA11yUIBrowserTest() {}
 

chrome/browser/ui/webui/management_a11y_browsertest.h

@@ -17,7 +17,7 @@ class ManagementA11yUIBrowserTest : public WebUIBrowserTest {
 
  protected:
   void InstallPowerfulPolicyEnforcedExtension();
-  base::FilePath test_data_dir_;
+  const base::FilePath test_data_dir_;
 };
 
 #endif  // CHROME_BROWSER_UI_WEBUI_MANAGEMENT_A11Y_BROWSERTEST_H_

chrome/chrome_cleaner/os/pre_fetched_paths_unittest.cc

@@ -38,36 +38,34 @@ TEST(PreFetchedPathsTest, CheckExpectedPaths) {
   PreFetchedPaths* pre_fetched_paths = PreFetchedPaths::GetInstance();
   // Note: pre_fetched_paths->Initialized() is already called in test_main.cc.
 
-  auto GetExpectedPath = [](int key) -> base::FilePath {
-    base::FilePath expected_path;
-    CHECK(base::PathService::Get(key, &expected_path));
-    return expected_path;
-  };
-
-  EXPECT_EQ(GetExpectedPath(base::FILE_EXE),
+  EXPECT_EQ(base::PathService::CheckedGet(base::FILE_EXE),
             pre_fetched_paths->GetExecutablePath());
-  EXPECT_EQ(GetExpectedPath(base::DIR_PROGRAM_FILES),
+  EXPECT_EQ(base::PathService::CheckedGet(base::DIR_PROGRAM_FILES),
             pre_fetched_paths->GetProgramFilesFolder());
-  EXPECT_EQ(GetExpectedPath(base::DIR_WINDOWS),
+  EXPECT_EQ(base::PathService::CheckedGet(base::DIR_WINDOWS),
             pre_fetched_paths->GetWindowsFolder());
-  EXPECT_EQ(GetExpectedPath(base::DIR_COMMON_APP_DATA),
+  EXPECT_EQ(base::PathService::CheckedGet(base::DIR_COMMON_APP_DATA),
             pre_fetched_paths->GetCommonAppDataFolder());
-  EXPECT_EQ(GetExpectedPath(base::DIR_LOCAL_APP_DATA),
+  EXPECT_EQ(base::PathService::CheckedGet(base::DIR_LOCAL_APP_DATA),
             pre_fetched_paths->GetLocalAppDataFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_PROGRAM_FILES)),
-            pre_fetched_paths->GetCsidlProgramFilesFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_PROGRAM_FILESX86)),
+  EXPECT_EQ(
+      base::PathService::CheckedGet(CsidlToPathServiceKey(CSIDL_PROGRAM_FILES)),
+      pre_fetched_paths->GetCsidlProgramFilesFolder());
+  EXPECT_EQ(base::PathService::CheckedGet(
+                CsidlToPathServiceKey(CSIDL_PROGRAM_FILESX86)),
             pre_fetched_paths->GetCsidlProgramFilesX86Folder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_WINDOWS)),
+  EXPECT_EQ(base::PathService::CheckedGet(CsidlToPathServiceKey(CSIDL_WINDOWS)),
             pre_fetched_paths->GetCsidlWindowsFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_STARTUP)),
+  EXPECT_EQ(base::PathService::CheckedGet(CsidlToPathServiceKey(CSIDL_STARTUP)),
             pre_fetched_paths->GetCsidlStartupFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_SYSTEM)),
+  EXPECT_EQ(base::PathService::CheckedGet(CsidlToPathServiceKey(CSIDL_SYSTEM)),
             pre_fetched_paths->GetCsidlSystemFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_COMMON_APPDATA)),
+  EXPECT_EQ(base::PathService::CheckedGet(
+                CsidlToPathServiceKey(CSIDL_COMMON_APPDATA)),
             pre_fetched_paths->GetCsidlCommonAppDataFolder());
-  EXPECT_EQ(GetExpectedPath(CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA)),
-            pre_fetched_paths->GetCsidlLocalAppDataFolder());
+  EXPECT_EQ(
+      base::PathService::CheckedGet(CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA)),
+      pre_fetched_paths->GetCsidlLocalAppDataFolder());
 }
 
 }  // namespace

chrome/test/media_router/media_router_integration_browsertest.cc

@@ -252,12 +252,10 @@ void MediaRouterIntegrationBrowserTest::SetTestData(
 
 base::FilePath MediaRouterIntegrationBrowserTest::GetResourceFile(
     base::FilePath::StringPieceType relative_path) const {
-  base::FilePath base_dir;
-  // ASSERT_TRUE can only be used in void returning functions.
-  // Use CHECK instead in non-void returning functions.
-  CHECK(base::PathService::Get(base::DIR_MODULE, &base_dir));
-  base::FilePath full_path =
-      base_dir.Append(kResourcePath).Append(relative_path);
+  const base::FilePath full_path =
+      base::PathService::CheckedGet(base::DIR_MODULE)
+          .Append(kResourcePath)
+          .Append(relative_path);
   {
     // crbug.com/724573
     base::ScopedAllowBlockingForTesting allow_blocking;

chrome/test/media_router/media_router_one_ua_integration_browsertest.cc

@@ -31,10 +31,9 @@ class MediaRouterIntegrationOneUABrowserTest
 
     // Set up embedded test server to serve offscreen presentation with relative
     // URL "presentation_receiver.html".
-    base::FilePath base_dir;
-    CHECK(base::PathService::Get(base::DIR_MODULE, &base_dir));
-    base::FilePath resource_dir = base_dir.Append(
-        FILE_PATH_LITERAL("media_router/browser_test_resources/"));
+    base::FilePath resource_dir =
+        base::PathService::CheckedGet(base::DIR_MODULE)
+            .Append(FILE_PATH_LITERAL("media_router/browser_test_resources/"));
     embedded_test_server()->ServeFilesFromDirectory(resource_dir);
     ASSERT_TRUE(embedded_test_server()->Start());
   }

chromeos/tpm/install_attributes.cc

@@ -77,10 +77,8 @@ void InstallAttributes::Initialize() {
 
   DCHECK(!g_install_attributes);
   g_install_attributes = new InstallAttributes(CryptohomeClient::Get());
-  base::FilePath install_attrs_file;
-  CHECK(base::PathService::Get(dbus_paths::FILE_INSTALL_ATTRIBUTES,
-                               &install_attrs_file));
-  g_install_attributes->Init(install_attrs_file);
+  g_install_attributes->Init(
+      base::PathService::CheckedGet(dbus_paths::FILE_INSTALL_ATTRIBUTES));
 }
 
 // static