Download WebAPK into a WebAPK-specifc directory, and delete all cached ones.

chrome/browser/android/webapk/webapk_installer.cc

Index: chrome/browser/android/webapk/webapk_installer.cc
diff --git a/chrome/browser/android/webapk/webapk_installer.cc b/chrome/browser/android/webapk/webapk_installer.cc
index 37df154bd8a3115e4a5972973c4ac9b6b9766e7b..fcbbd10da9583bffa73b07a18ff820b4cf5b36e8 100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -52,6 +52,12 @@ const int kWebApkDownloadUrlTimeoutMs = 60000;
 // complete.
 const int kDownloadTimeoutMs = 60000;
 
+const int KBasicPosixPermissions = base::FILE_PERMISSION_READ_BY_USER |

[pkotwicz, 2016/09/26 21:04:25]

Nit: rename |KBasicPosixPermissions| to |kWorldReadableFilePermission|.

I don't think that making a file world readable is the "default" setting for a
file.

Can you move FILE_PERMISSION_EXECUTE_BY_USER to line 153 and out of
|kWorldReadableFilePermission|

(Also lowercase "k". I have done this typo in the past and will probably do it
again)

[Xi Han, 2016/09/26 22:22:17]

Done.

+                                   base::FILE_PERMISSION_WRITE_BY_USER |
+                                   base::FILE_PERMISSION_EXECUTE_BY_USER |
+                                   base::FILE_PERMISSION_READ_BY_GROUP |
+                                   base::FILE_PERMISSION_READ_BY_OTHERS;
+
 // Returns the scope from |info| if it is specified. Otherwise, returns the
 // default scope.
 GURL GetScope(const ShortcutInfo& info) {
@@ -128,6 +134,36 @@ GURL GetServerUrlForUpdate(const GURL& server_url,
               kDefaultWebApkServerUrlResponseType);
 }
 
+// Creates a directory depending on the type of the task, and set permissions.
+// It also creates any parent directory along the path if doesn't exist,
+// and sets permissions as well.
+// The previously downloaded APKs are deleted before downloading a new one,
+// which helps to clean up cached data. Creating different downloaded

[pkotwicz, 2016/09/26 21:04:25]

How about: "Previously downloaded APKs are deleted in order to clean up unused
cached data."

In my version I removed: "before downloading a new one" because this function
does not know about when the download occurs

[Xi Han, 2016/09/26 22:22:17]

Acknowledged.

+// directory for install/update cases is to prevent deleting the APK which is
+// still in use when an install and an update happen at the same time.
+// However, it doesn't help cases of when mutiple installs (or multiple
+// updates) happen at the same time.
+base::FilePath CreateSubDirAndSetPermissionsInBackground(
+    const base::FilePath& output_root_dir,
+    WebApkInstaller::TaskType task_type,
+    const std::string& package_name) {
+  base::FilePath webapk_dir = output_root_dir.AppendASCII("webapks");

[pkotwicz, 2016/09/26 21:04:24]

Move the part of the comment which starts with "Creating different downloaded
directory" here because it is an implementation detail

[Xi Han, 2016/09/26 22:22:17]

Done.

+  base::FilePath output_dir = webapk_dir.AppendASCII(
+      task_type == WebApkInstaller::INSTALL ? "install" : "update");
+  int posix_permissions = KBasicPosixPermissions |
+                          base::FILE_PERMISSION_EXECUTE_BY_OTHERS;
+  if (base::PathExists(output_dir))
+    base::DeleteFile(output_dir, true);
+
+  // Creates the directory to download and sets permissions.
+  if (!base::CreateDirectory(output_dir) ||
+      !base::SetPosixFilePermissions(webapk_dir, posix_permissions) ||
+      !base::SetPosixFilePermissions(output_dir, posix_permissions))
+    return base::FilePath();
+
+  return output_dir;
+}
+
 }  // anonymous namespace
 
 WebApkInstaller::WebApkInstaller(const ShortcutInfo& shortcut_info,
@@ -351,9 +387,21 @@ void WebApkInstaller::OnGotWebApkDownloadUrl(const GURL& download_url,
   base::FilePath output_dir;

[pkotwicz, 2016/09/26 21:04:24]

Nit: Move GetCacheDirectory() to CreateSubDirAndSetPermissionsInBackground()

[Xi Han, 2016/09/26 22:22:17]

Done.

   base::android::GetCacheDirectory(&output_dir);
   webapk_package_ = package_name;
-  // TODO(pkotwicz): Download WebAPKs into WebAPK-specific subdirectory
-  // directory.
-  // TODO(pkotwicz): Figure out when downloaded WebAPK should be deleted.
+
+  base::PostTaskAndReplyWithResult(
+      GetBackgroundTaskRunner().get(), FROM_HERE,
+      base::Bind(&CreateSubDirAndSetPermissionsInBackground,
+                 output_dir, task_type_, package_name),
+      base::Bind(&WebApkInstaller::OnCreateSubDirAndSetPermissions,
+                 weak_ptr_factory_.GetWeakPtr(), download_url));
+}
+
+void WebApkInstaller::OnCreateSubDirAndSetPermissions(
+    const GURL& download_url, const base::FilePath& output_dir) {

[pkotwicz, 2016/09/26 21:04:25]

Nit: OnCreateSubDirAndSetPermissions() -> OnCreatedSubDirAndSetPermissions()

"created" not "create" because the directory would have been created in the past
when this function is called

[Xi Han, 2016/09/26 22:22:17]

Done.

+  if (output_dir.empty()) {
+    OnFailure();
+    return;
+  }
 
   timer_.Start(
       FROM_HERE, base::TimeDelta::FromMilliseconds(download_timeout_ms_),
@@ -375,13 +423,10 @@ void WebApkInstaller::OnWebApkDownloaded(const base::FilePath& file_path,
     return;
   }
 
-  int posix_permissions = base::FILE_PERMISSION_READ_BY_USER |
-                          base::FILE_PERMISSION_WRITE_BY_USER |
-                          base::FILE_PERMISSION_READ_BY_GROUP |
-                          base::FILE_PERMISSION_READ_BY_OTHERS;
   base::PostTaskAndReplyWithResult(
       GetBackgroundTaskRunner().get(), FROM_HERE,
-      base::Bind(&base::SetPosixFilePermissions, file_path, posix_permissions),
+      base::Bind(&base::SetPosixFilePermissions, file_path,
+                 KBasicPosixPermissions),
       base::Bind(&WebApkInstaller::OnWebApkMadeWorldReadable,
                  weak_ptr_factory_.GetWeakPtr(), file_path));
 }