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

chrome/browser/android/webapk/webapk_installer.cc

 // Copyright 2016 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 "chrome/browser/android/webapk/webapk_installer.h"
 
 #include "base/android/build_info.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/path_utils.h"
@@ skipping 34 matching lines @@
 const char kProtoMimeType[] = "application/x-protobuf";
 
 // The default number of milliseconds to wait for the WebAPK download URL from
 // the WebAPK server.
 const int kWebApkDownloadUrlTimeoutMs = 60000;
 
 // The default number of milliseconds to wait for the WebAPK download to
 // 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) {
   return (info.scope.is_valid())
       ? info.scope
       : ShortcutHelper::GetScopeFromURL(info.url);
 }
 
 // Converts a color from the format specified in content::Manifest to a CSS
 // string.
@@ skipping 56 matching lines @@
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
 }
 
 GURL GetServerUrlForUpdate(const GURL& server_url,
                            const std::string& webapk_package) {
   // crbug.com/636552. Simplify the server URL.
   return GURL(server_url.spec() + webapk_package + "/" +
               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,
                                  const SkBitmap& shortcut_icon)
     : shortcut_info_(shortcut_info),
       shortcut_icon_(shortcut_icon),
       webapk_download_url_timeout_ms_(kWebApkDownloadUrlTimeoutMs),
       download_timeout_ms_(kDownloadTimeoutMs),
       task_type_(UNDEFINED),
       weak_ptr_factory_(this) {
@@ skipping 200 matching lines @@
   url_fetcher_ = net::URLFetcher::Create(server_url, request_type, this);
   url_fetcher_->SetRequestContext(request_context_getter_);
   std::string serialized_request;
   request_proto->SerializeToString(&serialized_request);
   url_fetcher_->SetUploadData(kProtoMimeType, serialized_request);
   url_fetcher_->Start();
 }
 
 void WebApkInstaller::OnGotWebApkDownloadUrl(const GURL& download_url,
                                              const std::string& package_name) {
   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_),
       base::Bind(&WebApkInstaller::OnTimeout, weak_ptr_factory_.GetWeakPtr()));
 
   base::FilePath output_path = output_dir.AppendASCII(webapk_package_);
   downloader_.reset(new FileDownloader(
       download_url, output_path, true, request_context_getter_,
       base::Bind(&WebApkInstaller::OnWebApkDownloaded,
                  weak_ptr_factory_.GetWeakPtr(), output_path)));
 }
 
 void WebApkInstaller::OnWebApkDownloaded(const base::FilePath& file_path,
                                          FileDownloader::Result result) {
   timer_.Stop();
 
   if (result != FileDownloader::DOWNLOADED) {
     OnFailure();
     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));
 }
 
 void WebApkInstaller::OnWebApkMadeWorldReadable(
     const base::FilePath& file_path,
     bool change_permission_success) {
   if (!change_permission_success) {
     OnFailure();
     return;
@@ skipping 30 matching lines @@
   std::string webapk_package = webapk_package_;
   delete this;
   callback.Run(true, webapk_package);
 }
 
 void WebApkInstaller::OnFailure() {
   FinishCallback callback = finish_callback_;
   delete this;
   callback.Run(false, "");
 }