WebApp: Change a directory structure, how we store files under profile dir.

The directory structure before:

<ProfileDir>
    WebApps
        Sync Data
            LevelDB
        Temp
            <temp_name>
        <app_id>
    Web Applications
        _crx_<app_id>

After:

<ProfileDir>
    Web Applications
        _crx_<app_id>
        Sync Data
            LevelDB
        Temp
            <temp_name>
        Manifest Resources
            <app_id>

This is breaking change, all local BMO profile icons will be
clean. We should land it now to avoid file migrations in the
future.

Note: "Sync Data/LevelDB" directory is created only if
kDesktopPWAsSharedStoreService feature is off.
We use shared <ProfileDir>/Sync Data/LevelDB/ otherwise.

This code is disabled behind kDesktopPWAsWithoutExtensions feature.
It introduces no behavior changes.

Bug: 877898
Change-Id: I96af88386f3ce6031881d713b330eff2e32f33e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2084215
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746682}

chrome/browser/web_applications/components/web_app_shortcut.cc

@@ -92,15 +92,13 @@ std::string GenerateApplicationNameFromInfo(const ShortcutInfo& shortcut_info) {
 }
 
 base::FilePath GetWebAppDataDirectory(const base::FilePath& profile_path,
-                                      const std::string& extension_id,
+                                      const std::string& app_id,
                                       const GURL& url) {
   DCHECK(!profile_path.empty());
   base::FilePath app_data_dir(profile_path.Append(chrome::kWebAppDirname));
 
-  if (!extension_id.empty()) {
-    return app_data_dir.AppendASCII(
-        GenerateApplicationNameFromAppId(extension_id));
-  }
+  if (!app_id.empty())
+    return app_data_dir.AppendASCII(GenerateApplicationNameFromAppId(app_id));
 
   std::string host(url.host());
   std::string scheme(url.has_scheme() ? url.scheme() : "http");

chrome/browser/web_applications/components/web_app_shortcut.h

@@ -99,11 +99,16 @@ enum ShortcutCreationReason {
 // Compute a deterministic name based on data in the shortcut_info.
 std::string GenerateApplicationNameFromInfo(const ShortcutInfo& shortcut_info);
 
-// Gets the user data directory for given web app. The path for the directory is
-// based on |extension_id|. If |extension_id| is empty then |url| is used
-// to construct a unique ID.
+// Returns a per-app directory for OS-specific web app data to handle OS
+// registration and unregistration. To store manifest resources, use
+// GetManifestResourcesDirectoryForApp() declared in web_app_utils.h.
+//
+// The path for the directory is based on |app_id|. If |app_id| is empty then
+// |url| is used to construct a unique ID.
+// TODO(crbug.com/877898): Rename this function to
+// GetOsIntegrationDataDirectoryForApp().
 base::FilePath GetWebAppDataDirectory(const base::FilePath& profile_path,
-                                      const std::string& extension_id,
+                                      const std::string& app_id,
                                       const GURL& url);
 
 // Callback made when CreateShortcuts has finished trying to create the

chrome/browser/web_applications/components/web_app_utils.cc

@@ -6,6 +6,7 @@
 
 #include "base/files/file_path.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_constants.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -14,8 +15,11 @@
 
 namespace web_app {
 
-constexpr base::FilePath::CharType kWebAppsDirectoryName[] =
-    FILE_PATH_LITERAL("WebApps");
+constexpr base::FilePath::CharType kManifestResourcesDirectoryName[] =
+    FILE_PATH_LITERAL("Manifest Resources");
+
+constexpr base::FilePath::CharType kTempDirectoryName[] =
+    FILE_PATH_LITERAL("Temp");
 
 bool AreWebAppsEnabled(Profile* profile) {
   if (!profile)
@@ -67,8 +71,29 @@ content::BrowserContext* GetBrowserContextForWebAppMetrics(
   return is_web_app_metrics_enabled ? original_profile : nullptr;
 }
 
-base::FilePath GetWebAppsDirectory(Profile* profile) {
-  return profile->GetPath().Append(base::FilePath(kWebAppsDirectoryName));
+base::FilePath GetWebAppsRootDirectory(Profile* profile) {
+  return profile->GetPath().Append(chrome::kWebAppDirname);
+}
+
+base::FilePath GetManifestResourcesDirectory(
+    const base::FilePath& web_apps_root_directory) {
+  return web_apps_root_directory.Append(kManifestResourcesDirectoryName);
+}
+
+base::FilePath GetManifestResourcesDirectory(Profile* profile) {
+  return GetManifestResourcesDirectory(GetWebAppsRootDirectory(profile));
+}
+
+base::FilePath GetManifestResourcesDirectoryForApp(
+    const base::FilePath& web_apps_root_directory,
+    const AppId& app_id) {
+  return GetManifestResourcesDirectory(web_apps_root_directory)
+      .AppendASCII(app_id);
+}
+
+base::FilePath GetWebAppsTempDirectory(
+    const base::FilePath& web_apps_root_directory) {
+  return web_apps_root_directory.Append(kTempDirectoryName);
 }
 
 std::string GetProfileCategoryForLogging(Profile* profile) {

chrome/browser/web_applications/components/web_app_utils.h

@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "chrome/browser/web_applications/components/web_app_id.h"
+
 class Profile;
 
 namespace base {
@@ -35,7 +37,29 @@ content::BrowserContext* GetBrowserContextForWebApps(
 content::BrowserContext* GetBrowserContextForWebAppMetrics(
     content::BrowserContext* context);
 
-base::FilePath GetWebAppsDirectory(Profile* profile);
+// Returns a root directory for all Web Apps themed data.
+//
+// All the related directory getters always require |web_apps_root_directory| as
+// a first argument to avoid directory confusions.
+base::FilePath GetWebAppsRootDirectory(Profile* profile);
+
+// Returns a directory to store local cached manifest resources in
+// OS-independent manner. Use GetManifestResourcesDirectoryForApp function to
+// get per-app manifest resources directory.
+//
+// To store OS-specific integration data, use GetWebAppDataDirectory declared in
+// web_app_shortcut.h.
+base::FilePath GetManifestResourcesDirectory(
+    const base::FilePath& web_apps_root_directory);
+base::FilePath GetManifestResourcesDirectory(Profile* profile);
+
+// Returns per-app directory name to store manifest resources.
+base::FilePath GetManifestResourcesDirectoryForApp(
+    const base::FilePath& web_apps_root_directory,
+    const AppId& app_id);
+
+base::FilePath GetWebAppsTempDirectory(
+    const base::FilePath& web_apps_root_directory);
 
 // The return value (profile categories) are used to report metrics. They are
 // persisted to logs and should not be renamed. If new names are added, update

chrome/browser/web_applications/test/web_app_icon_test_utils.cc

@@ -59,8 +59,9 @@ bool AreColorsEqual(SkColor expected_color,
 }
 
 base::FilePath GetAppIconsDir(Profile* profile, const AppId& app_id) {
-  base::FilePath web_apps_dir = GetWebAppsDirectory(profile);
-  base::FilePath app_dir = web_apps_dir.AppendASCII(app_id);
+  base::FilePath web_apps_root_directory = GetWebAppsRootDirectory(profile);
+  base::FilePath app_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_root_directory, app_id);
   base::FilePath icons_dir = app_dir.AppendASCII("Icons");
   return icons_dir;
 }

chrome/browser/web_applications/web_app_database_factory.cc

@@ -17,7 +17,7 @@ WebAppDatabaseFactory::WebAppDatabaseFactory(Profile* profile)
   if (!base::FeatureList::IsEnabled(features::kDesktopPWAsSharedStoreService)) {
     model_type_store_service_ =
         std::make_unique<syncer::ModelTypeStoreServiceImpl>(
-            GetWebAppsDirectory(profile));
+            GetWebAppsRootDirectory(profile));
   }
 }
 

chrome/browser/web_applications/web_app_icon_manager.cc

@@ -28,42 +28,37 @@ namespace web_app {
 
 namespace {
 
-constexpr base::FilePath::CharType kTempDirectoryName[] =
-    FILE_PATH_LITERAL("Temp");
-
-constexpr base::FilePath::CharType kIconsDirectoryName[] =
-    FILE_PATH_LITERAL("Icons");
-
-base::FilePath GetAppDirectory(const base::FilePath& web_apps_directory,
-                               const AppId& app_id) {
-  return web_apps_directory.AppendASCII(app_id);
-}
-
-base::FilePath GetTempDir(FileUtilsWrapper* utils,
-                          const base::FilePath& web_apps_dir) {
-  // Create the temp directory as a sub-directory of the WebApps directory.
-  // This guarantees it is on the same file system as the WebApp's eventual
-  // install target.
-  base::FilePath temp_path = web_apps_dir.Append(kTempDirectoryName);
-  if (utils->PathExists(temp_path)) {
-    if (!utils->DirectoryExists(temp_path)) {
-      LOG(ERROR) << "Not a directory: " << temp_path.value();
-      return base::FilePath();
+// Returns false if directory doesn't exist or it is not writable.
+bool CreateDirectoryIfNotExists(FileUtilsWrapper* utils,
+                                const base::FilePath& path) {
+  if (utils->PathExists(path)) {
+    if (!utils->DirectoryExists(path)) {
+      LOG(ERROR) << "Not a directory: " << path.value();
+      return false;
     }
-    if (!utils->PathIsWritable(temp_path)) {
-      LOG(ERROR) << "Can't write to path: " << temp_path.value();
-      return base::FilePath();
+    if (!utils->PathIsWritable(path)) {
+      LOG(ERROR) << "Can't write to path: " << path.value();
+      return false;
     }
     // This is a directory we can write to.
-    return temp_path;
+    return true;
   }
 
   // Directory doesn't exist, so create it.
-  if (!utils->CreateDirectory(temp_path)) {
-    LOG(ERROR) << "Could not create directory: " << temp_path.value();
-    return base::FilePath();
+  if (!utils->CreateDirectory(path)) {
+    LOG(ERROR) << "Could not create directory: " << path.value();
+    return false;
   }
-  return temp_path;
+  return true;
+}
+
+// This is a private implementation detail of WebAppIconManager, where and how
+// to store icon files.
+base::FilePath GetAppIconsDirectory(
+    const base::FilePath& app_manifest_resources_directory) {
+  constexpr base::FilePath::CharType kIconsDirectoryName[] =
+      FILE_PATH_LITERAL("Icons");
+  return app_manifest_resources_directory.Append(kIconsDirectoryName);
 }
 
 bool WriteIcon(FileUtilsWrapper* utils,
@@ -95,7 +90,7 @@ bool WriteIcon(FileUtilsWrapper* utils,
 bool WriteIcons(FileUtilsWrapper* utils,
                 const base::FilePath& app_dir,
                 const std::map<SquareSizePx, SkBitmap>& icon_bitmaps) {
-  const base::FilePath icons_dir = app_dir.Append(kIconsDirectoryName);
+  const base::FilePath icons_dir = GetAppIconsDirectory(app_dir);
   if (!utils->CreateDirectory(icons_dir)) {
     LOG(ERROR) << "Could not create icons directory.";
     return false;
@@ -116,10 +111,13 @@ bool WriteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
                        const base::FilePath& web_apps_directory,
                        const AppId& app_id,
                        const std::map<SquareSizePx, SkBitmap>& icons) {
-  const base::FilePath temp_dir = GetTempDir(utils.get(), web_apps_directory);
-  if (temp_dir.empty()) {
-    LOG(ERROR)
-        << "Could not get path to WebApps temporary directory in profile.";
+  // Create the temp directory under the web apps root.
+  // This guarantees it is on the same file system as the WebApp's eventual
+  // install target.
+  base::FilePath temp_dir = GetWebAppsTempDirectory(web_apps_directory);
+  if (!CreateDirectoryIfNotExists(utils.get(), temp_dir)) {
+    LOG(ERROR) << "Could not create or write to WebApps temporary directory in "
+                  "profile.";
     return false;
   }
 
@@ -132,12 +130,20 @@ bool WriteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
   if (!WriteIcons(utils.get(), app_temp_dir.GetPath(), icons))
     return false;
 
-  // Commit: move whole app data dir to final destination in one mv operation.
-  const base::FilePath app_dir = GetAppDirectory(web_apps_directory, app_id);
+  base::FilePath manifest_resources_directory =
+      GetManifestResourcesDirectory(web_apps_directory);
+  if (!CreateDirectoryIfNotExists(utils.get(), manifest_resources_directory)) {
+    LOG(ERROR) << "Could not create Manifest Resources directory.";
+    return false;
+  }
+
+  base::FilePath app_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_directory, app_id);
 
-  // Try to delete the destination. Needed for update.
+  // Try to delete the destination. Needed for update. Ignore the result.
   utils->DeleteFileRecursively(app_dir);
 
+  // Commit: move whole app data dir to final destination in one mv operation.
   if (!utils->Move(app_temp_dir.GetPath(), app_dir)) {
     LOG(ERROR) << "Could not move temp WebApp directory to final destination.";
     return false;
@@ -152,15 +158,18 @@ bool WriteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
 bool DeleteDataBlocking(const std::unique_ptr<FileUtilsWrapper>& utils,
                         const base::FilePath& web_apps_directory,
                         const AppId& app_id) {
-  const base::FilePath app_dir = GetAppDirectory(web_apps_directory, app_id);
+  base::FilePath app_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_directory, app_id);
+
   return utils->DeleteFileRecursively(app_dir);
 }
 
 base::FilePath GetIconFileName(const base::FilePath& web_apps_directory,
                                const AppId& app_id,
                                int icon_size_px) {
-  const base::FilePath app_dir = GetAppDirectory(web_apps_directory, app_id);
-  const base::FilePath icons_dir = app_dir.Append(kIconsDirectoryName);
+  base::FilePath app_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_directory, app_id);
+  base::FilePath icons_dir = GetAppIconsDirectory(app_dir);
 
   return icons_dir.AppendASCII(base::StringPrintf("%i.png", icon_size_px));
 }
@@ -269,7 +278,7 @@ WebAppIconManager::WebAppIconManager(Profile* profile,
                                      WebAppRegistrar& registrar,
                                      std::unique_ptr<FileUtilsWrapper> utils)
     : registrar_(registrar), utils_(std::move(utils)) {
-  web_apps_directory_ = GetWebAppsDirectory(profile);
+  web_apps_directory_ = GetWebAppsRootDirectory(profile);
 }
 
 WebAppIconManager::~WebAppIconManager() = default;

chrome/browser/web_applications/web_app_icon_manager_unittest.cc

@@ -389,9 +389,12 @@ TEST_F(WebAppIconManagerTest, DeleteData_Success) {
   WriteIcons(app1_id, sizes_px, colors);
   WriteIcons(app2_id, sizes_px, colors);
 
-  const base::FilePath web_apps_directory = GetWebAppsDirectory(profile());
-  const base::FilePath app1_dir = web_apps_directory.AppendASCII(app1_id);
-  const base::FilePath app2_dir = web_apps_directory.AppendASCII(app2_id);
+  const base::FilePath web_apps_root_directory =
+      GetWebAppsRootDirectory(profile());
+  const base::FilePath app1_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_root_directory, app1_id);
+  const base::FilePath app2_dir =
+      GetManifestResourcesDirectoryForApp(web_apps_root_directory, app2_id);
 
   EXPECT_TRUE(file_utils().DirectoryExists(app1_dir));
   EXPECT_FALSE(file_utils().IsDirectoryEmpty(app1_dir));
@@ -407,7 +410,9 @@ TEST_F(WebAppIconManagerTest, DeleteData_Success) {
                             }));
   run_loop.Run();
 
-  EXPECT_TRUE(file_utils().DirectoryExists(web_apps_directory));
+  base::FilePath manifest_resources_directory =
+      GetManifestResourcesDirectory(web_apps_root_directory);
+  EXPECT_TRUE(file_utils().DirectoryExists(manifest_resources_directory));
 
   EXPECT_TRUE(file_utils().DirectoryExists(app1_dir));
   EXPECT_FALSE(file_utils().IsDirectoryEmpty(app1_dir));

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -643,9 +643,10 @@ TEST_F(WebAppInstallTaskTest, WriteDataToDisk) {
 
   // TestingProfile creates temp directory if TestingProfile::path_ is empty
   // (i.e. if TestingProfile::Builder::SetPath was not called by a test fixture)
-  const base::FilePath profile_dir = profile()->GetPath();
-  const base::FilePath web_apps_dir = profile_dir.AppendASCII("WebApps");
-  EXPECT_FALSE(file_utils_->DirectoryExists(web_apps_dir));
+  const base::FilePath web_apps_dir = GetWebAppsRootDirectory(profile());
+  const base::FilePath manifest_resources_directory =
+      GetManifestResourcesDirectory(web_apps_dir);
+  EXPECT_FALSE(file_utils_->DirectoryExists(manifest_resources_directory));
 
   const SkColor color = SK_ColorGREEN;
   const int original_icon_size_px = icon_size::k512;
@@ -656,13 +657,14 @@ TEST_F(WebAppInstallTaskTest, WriteDataToDisk) {
 
   const AppId app_id = InstallWebAppFromManifestWithFallback();
 
-  EXPECT_TRUE(file_utils_->DirectoryExists(web_apps_dir));
+  EXPECT_TRUE(file_utils_->DirectoryExists(manifest_resources_directory));
 
   const base::FilePath temp_dir = web_apps_dir.AppendASCII("Temp");
   EXPECT_TRUE(file_utils_->DirectoryExists(temp_dir));
   EXPECT_TRUE(file_utils_->IsDirectoryEmpty(temp_dir));
 
-  const base::FilePath app_dir = web_apps_dir.AppendASCII(app_id);
+  const base::FilePath app_dir =
+      manifest_resources_directory.AppendASCII(app_id);
   EXPECT_TRUE(file_utils_->DirectoryExists(app_dir));
 
   const base::FilePath icons_dir = app_dir.AppendASCII("Icons");
@@ -712,10 +714,11 @@ TEST_F(WebAppInstallTaskTest, WriteDataToDiskFailed) {
                     SK_ColorBLUE, &icons_map);
   SetIconsMapToRetrieve(std::move(icons_map));
 
-  const base::FilePath profile_dir = profile()->GetPath();
-  const base::FilePath web_apps_dir = profile_dir.AppendASCII("WebApps");
+  const base::FilePath web_apps_dir = GetWebAppsRootDirectory(profile());
+  const base::FilePath manifest_resources_directory =
+      GetManifestResourcesDirectory(web_apps_dir);
 
-  EXPECT_TRUE(file_utils_->CreateDirectory(web_apps_dir));
+  EXPECT_TRUE(file_utils_->CreateDirectory(manifest_resources_directory));
 
   // Induce an error: Simulate "Disk Full" for writing icon files.
   file_utils_->SetRemainingDiskSpaceSize(1024);
@@ -742,7 +745,8 @@ TEST_F(WebAppInstallTaskTest, WriteDataToDiskFailed) {
   EXPECT_TRUE(file_utils_->IsDirectoryEmpty(temp_dir));
 
   const AppId app_id = GenerateAppIdFromURL(app_url);
-  const base::FilePath app_dir = web_apps_dir.AppendASCII(app_id);
+  const base::FilePath app_dir =
+      manifest_resources_directory.AppendASCII(app_id);
   EXPECT_FALSE(file_utils_->DirectoryExists(app_dir));
 }