Add feature flag MyFilesVolume and change path_util to use it

Add feature flag MyFilesVolume that when it's enabled the MyFiles folder
will be an actual volume and Downloads will be just a plain folder
inside it.

Change GetDownloadsFolderForProfile to return
<cryptohome>/MyFiles/Downloads when the flag is active.

Add a new function GetMyFilesFolderForProfile which returns the correct
folder to be used as volume root, when the flag is disable it returns
the current value, by calling GetDownloadsFolderForProfile, which
returns <cryptohome>/Downloads. When the flag is active it returns
<cryptohome>/MyFiles.

Change GetPathDisplayTextForSettings to recognize the
<cryptohome>/MyFiles/Downloads pattern, for now it will return just
Downloads.

This is an initial work, follow up CLs wil change the rest of code base
to use either GetDownloadsFolderForProfile (when the code wants the
actual downloads folder) or GetMyFilesFolderForProfile when the code
wants the volume root, previously these 2 situations were the same path;
when the new flag is enabled they're different and depending on the use
case we might have to change to GetMyFilesFolderForProfile.

Bug: 873539

Change-Id: I7d01bc5370072ba574a6761d4a54d43d4b62ff6e
Reviewed-on: https://chromium-review.googlesource.com/c/1298824
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602979}

chrome/browser/chromeos/file_manager/path_util.cc

@@ -40,6 +40,7 @@ namespace util {
 namespace {
 
 constexpr char kDownloadsFolderName[] = "Downloads";
+constexpr char kMyFilesFolderName[] = "MyFiles";
 constexpr char kGoogleDriveDisplayName[] = "Google Drive";
 constexpr char kMyDriveDisplayName[] = "My Drive";
 constexpr char kTeamDrivesDisplayName[] = "Team Drives";
@@ -82,6 +83,22 @@ void OnAllContentUrlsResolved(ConvertToContentUrlsCallback callback,
   std::move(callback).Run(*urls);
 }
 
+// On non-ChromeOS system (test+development), the primary profile uses
+// $HOME/Downloads for ease access to local files for debugging.
+bool ShouldMountPrimaryUserDownloads(Profile* profile) {
+  if (!base::SysInfo::IsRunningOnChromeOS() &&
+      user_manager::UserManager::IsInitialized()) {
+    const user_manager::User* const user =
+        chromeos::ProfileHelper::Get()->GetUserByProfile(
+            profile->GetOriginalProfile());
+    const user_manager::User* const primary_user =
+        user_manager::UserManager::Get()->GetPrimaryUser();
+    return user == primary_user;
+  }
+
+  return false;
+}
+
 }  // namespace
 
 const base::FilePath::CharType kRemovableMediaPath[] =
@@ -100,23 +117,44 @@ base::FilePath GetDownloadsFolderForProfile(Profile* profile) {
   if (mount_points->GetRegisteredPath(mount_point_name, &path))
     return path;
 
-  // On non-ChromeOS system (test+development), the primary profile uses
-  // $HOME/Downloads for ease for accessing local files for debugging.
-  if (!base::SysInfo::IsRunningOnChromeOS() &&
-      user_manager::UserManager::IsInitialized()) {
-    const user_manager::User* const user =
-        chromeos::ProfileHelper::Get()->GetUserByProfile(
-            profile->GetOriginalProfile());
-    const user_manager::User* const primary_user =
-        user_manager::UserManager::Get()->GetPrimaryUser();
-    if (user == primary_user)
-      return DownloadPrefs::GetDefaultDownloadDirectory();
+  // Return $HOME/Downloads as Download folder.
+  if (ShouldMountPrimaryUserDownloads(profile))
+    return DownloadPrefs::GetDefaultDownloadDirectory();
+
+  // Return <cryptohome>/MyFiles/Downloads if it feature is enabled.
+  if (base::FeatureList::IsEnabled(chromeos::features::kMyFilesVolume)) {
+    return profile->GetPath()
+        .AppendASCII(kMyFilesFolderName)
+        .AppendASCII(kDownloadsFolderName);
   }
 
   // Return <cryptohome>/Downloads.
   return profile->GetPath().AppendASCII(kDownloadsFolderName);
 }
 
+base::FilePath GetMyFilesFolderForProfile(Profile* profile) {
+  // When MyFilesVolume feature is disabled this should behave just like
+  // GetDownloadsFolderForProfile.
+  if (!base::FeatureList::IsEnabled(chromeos::features::kMyFilesVolume))
+    return GetDownloadsFolderForProfile(profile);
+
+  // Check if FilesApp has a registered path already. This happens for tests.
+  const std::string mount_point_name =
+      util::GetDownloadsMountPointName(profile);
+  storage::ExternalMountPoints* const mount_points =
+      storage::ExternalMountPoints::GetSystemInstance();
+  base::FilePath path;
+  if (mount_points->GetRegisteredPath(mount_point_name, &path))
+    return path;
+
+  // Return $HOME/Downloads as MyFiles folder.
+  if (ShouldMountPrimaryUserDownloads(profile))
+    return DownloadPrefs::GetDefaultDownloadDirectory();
+
+  // Return <cryptohome>/MyFiles.
+  return profile->GetPath().AppendASCII(kMyFilesFolderName);
+}
+
 bool MigratePathFromOldFormat(Profile* profile,
                               const base::FilePath& old_path,
                               base::FilePath* new_path) {
@@ -366,6 +404,15 @@ std::string GetPathDisplayTextForSettings(Profile* profile,
                                profile->GetPath().BaseName().value() +
                                "/Downloads",
                            kDownloadsFolderName)) {
+  } else if (ReplacePrefix(&result,
+                           std::string("/home/chronos/user/") +
+                               kMyFilesFolderName + "/" + kDownloadsFolderName,
+                           kDownloadsFolderName)) {
+  } else if (ReplacePrefix(&result,
+                           "/home/chronos/" +
+                               profile->GetPath().BaseName().value() + "/" +
+                               kMyFilesFolderName + "/" + kDownloadsFolderName,
+                           kDownloadsFolderName)) {
   } else if (drive_integration_service &&
              ReplacePrefix(&result,
                            drive_integration_service->GetMountPointPath()

chrome/browser/chromeos/file_manager/path_util.h

@@ -27,6 +27,9 @@ extern const base::FilePath::CharType kAndroidFilesPath[];
 // Gets the absolute path for the 'Downloads' folder for the |profile|.
 base::FilePath GetDownloadsFolderForProfile(Profile* profile);
 
+// Gets the absolute path for the 'MyFiles' folder for the |profile|.
+base::FilePath GetMyFilesFolderForProfile(Profile* profile);
+
 // Converts |old_path| to |new_path| and returns true, if the old path points
 // to an old location of user folders (in "Downloads" or "Google Drive").
 // The |profile| argument is used for determining the location of the

chrome/browser/chromeos/file_manager/path_util_unittest.cc

@@ -47,6 +47,66 @@ const char kLsbRelease[] =
     "CHROMEOS_RELEASE_NAME=Chrome OS\n"
     "CHROMEOS_RELEASE_VERSION=1.2.3.4\n";
 
+TEST(FileManagerPathUtilTest, GetDownloadsFolderForProfile) {
+  content::TestBrowserThreadBundle thread_bundle;
+  TestingProfile profile(base::FilePath("/home/chronos/u-0123456789abcdef"));
+  std::string mount_point_name = GetDownloadsMountPointName(&profile);
+  EXPECT_EQ("Downloads", mount_point_name);
+}
+
+TEST(FileManagerPathUtilTest, GetMyFilesFolderForProfile) {
+  content::TestBrowserThreadBundle thread_bundle;
+
+  base::FilePath profile_path =
+      base::FilePath("/home/chronos/u-0123456789abcdef");
+  TestingProfile profile(profile_path);
+
+  // When running outside ChromeOS, it should return $HOME/Downloads for both
+  // MyFiles and Downloads.
+  EXPECT_EQ(DownloadPrefs::GetDefaultDownloadDirectory(),
+            GetMyFilesFolderForProfile(&profile));
+  EXPECT_EQ(DownloadPrefs::GetDefaultDownloadDirectory(),
+            GetDownloadsFolderForProfile(&profile));
+
+  // When running inside ChromeOS, it should return /home/u-{hash}/MyFiles.
+  chromeos::ScopedSetRunningOnChromeOSForTesting fake_release(kLsbRelease,
+                                                              base::Time());
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndDisableFeature(chromeos::features::kMyFilesVolume);
+    // When MyFilesVolume feature is disabled it will return the same as
+    // Downloads.
+    EXPECT_EQ(GetDownloadsFolderForProfile(&profile),
+              GetMyFilesFolderForProfile(&profile));
+    EXPECT_EQ("/home/chronos/u-0123456789abcdef/Downloads",
+              GetDownloadsFolderForProfile(&profile).value());
+  }
+  {
+    // When MyFilesVolume feature is enabled Downloads path is inside MyFiles.
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(chromeos::features::kMyFilesVolume);
+
+    base::FilePath myfiles_path = profile_path.AppendASCII("MyFiles");
+    base::FilePath myfiles_downloads_path =
+        myfiles_path.AppendASCII("Downloads");
+
+    EXPECT_EQ("/home/chronos/u-0123456789abcdef/MyFiles",
+              GetMyFilesFolderForProfile(&profile).value());
+    EXPECT_EQ("/home/chronos/u-0123456789abcdef/MyFiles/Downloads",
+              GetDownloadsFolderForProfile(&profile).value());
+
+    // Mount the volume to test the return from mount_points.
+    storage::ExternalMountPoints::GetSystemInstance()->RegisterFileSystem(
+        GetDownloadsMountPointName(&profile),
+        storage::kFileSystemTypeNativeLocal, storage::FileSystemMountOption(),
+        profile_path.Append("MyFiles"));
+
+    // Still the same: /home/u-{hash}/MyFiles.
+    EXPECT_EQ("/home/chronos/u-0123456789abcdef/MyFiles",
+              GetMyFilesFolderForProfile(&profile).value());
+  }
+}
+
 TEST(FileManagerPathUtilTest, GetPathDisplayTextForSettings) {
   content::TestBrowserThreadBundle thread_bundle;
   content::TestServiceManagerContext service_manager_context;
@@ -58,6 +118,14 @@ TEST(FileManagerPathUtilTest, GetPathDisplayTextForSettings) {
   EXPECT_EQ("Downloads",
             GetPathDisplayTextForSettings(
                 &profile, "/home/chronos/u-0123456789abcdef/Downloads"));
+
+  EXPECT_EQ("Downloads", GetPathDisplayTextForSettings(
+                             &profile, "/home/chronos/user/MyFiles/Downloads"));
+  EXPECT_EQ(
+      "Downloads",
+      GetPathDisplayTextForSettings(
+          &profile, "/home/chronos/u-0123456789abcdef/MyFiles/Downloads"));
+
   EXPECT_EQ("Play files \u203a foo \u203a bar",
             GetPathDisplayTextForSettings(
                 &profile, "/run/arc/sdcard/write/emulated/0/foo/bar"));

chromeos/chromeos_features.cc

@@ -23,6 +23,11 @@ const base::Feature kChromeVoxArcSupport{"ChromeVoxArcSupport",
 // If enabled, DriveFS will be used for Drive sync.
 const base::Feature kDriveFs{"DriveFS", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// If enabled, MyFiles will be a root/volume and user can create other
+// sub-folders and files in addition to the Downloads folder inside MyFiles.
+const base::Feature kMyFilesVolume{"MyFilesVolume",
+                                   base::FEATURE_DISABLED_BY_DEFAULT};
+
 // If enabled, the Chrome OS Settings UI will include a menu for the unified
 // MultiDevice settings.
 const base::Feature kEnableUnifiedMultiDeviceSettings{

chromeos/chromeos_features.h

@@ -19,6 +19,7 @@ CHROMEOS_EXPORT extern const base::Feature kAndroidMessagesIntegration;
 CHROMEOS_EXPORT extern const base::Feature kAndroidMessagesProdEndpoint;
 CHROMEOS_EXPORT extern const base::Feature kChromeVoxArcSupport;
 CHROMEOS_EXPORT extern const base::Feature kDriveFs;
+CHROMEOS_EXPORT extern const base::Feature kMyFilesVolume;
 CHROMEOS_EXPORT extern const base::Feature kEnableUnifiedMultiDeviceSettings;
 CHROMEOS_EXPORT extern const base::Feature kEnableUnifiedMultiDeviceSetup;
 CHROMEOS_EXPORT extern const base::Feature kImeServiceConnectable;