[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part 1/3

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 199c48fa8bb98cd261dc0cb3fa947461663b0a28..0aa893ab3e70bee8d7eedcc11783dda228a07200 100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/android/webapk/chrome_webapk_host.h"
 #include "chrome/browser/android/webapk/webapk.pb.h"
 #include "chrome/browser/android/webapk/webapk_icon_hasher.h"
+#include "chrome/browser/android/webapk/webapk_install_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/version_info/version_info.h"
@@ -271,11 +272,8 @@ void WebApkInstaller::SetTimeoutMs(int timeout_ms) {
 void WebApkInstaller::OnInstallFinished(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
-    jboolean success) {
-  if (success)
-    OnSuccess();
-  else
-    OnFailure();
+    jint webApkInstallResult) {
+  OnResult(static_cast<WebApkInstallResult>(webApkInstallResult));
 }
 
 void WebApkInstaller::BuildWebApkProtoInBackgroundForTesting(
@@ -328,6 +326,11 @@ void WebApkInstaller::InstallOrUpdateWebApkFromGooglePlay(
       java_title, java_token, java_url);
 }
 
+void WebApkInstaller::OnResult(WebApkInstallResult result) {
+  finish_callback_.Run(result, relax_updates_, webapk_package_);

[dominickn, 2017/03/13 03:22:24]

Nit: this should call weak_ptr_factory_.InvalidateWeakPtrs() first to make sure
no callbacks will be invoked.

[pkotwicz, 2017/03/14 22:38:57]

Doesn't the destructor call WeakPtrFactory::InvalidateWeakPtrs() implicitly?

[dominickn, 2017/03/14 23:25:19]

Yes, but finish_callback_ could potentially be bound to something that triggers
one of the callbacks, or it could do some blocking work on a different thread
that allows one of the weak pointer calls to squeeze in before the destructor is
run. Invalidating the weak pointers first here makes sure none of that will
happen.

[pkotwicz, 2017/03/14 23:43:29]

Fair enough

+  delete this;
+}
+
 WebApkInstaller::WebApkInstaller(content::BrowserContext* browser_context,
                                  const ShortcutInfo& shortcut_info,
                                  const SkBitmap& shortcut_icon)
@@ -392,7 +395,7 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
       source->GetResponseCode() != net::HTTP_OK) {
     LOG(WARNING) << base::StringPrintf(
         "WebAPK server returned response code %d.", source->GetResponseCode());
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -402,7 +405,7 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
   std::unique_ptr<webapk::WebApkResponse> response(new webapk::WebApkResponse);
   if (!response->ParseFromString(response_string)) {
     LOG(WARNING) << "WebAPK server did not return proto.";
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -411,13 +414,13 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
     // https://crbug.com/680131. The server sends an empty URL if the server
     // does not have a newer WebAPK to update to.
     relax_updates_ = response->relax_updates();
-    OnSuccess();
+    OnResult(WebApkInstallResult::SUCCESS);
     return;
   }
 
   if (!signed_download_url.is_valid() || response->package_name().empty()) {
     LOG(WARNING) << "WebAPK server returned incomplete proto.";
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -435,13 +438,14 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
 void WebApkInstaller::DownloadAppIconAndComputeMurmur2Hash() {
   // Safeguard. WebApkIconHasher crashes if asked to fetch an invalid URL.
   if (!shortcut_info_.best_primary_icon_url.is_valid()) {
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
   timer_.Start(
       FROM_HERE, base::TimeDelta::FromMilliseconds(download_timeout_ms_),
-      base::Bind(&WebApkInstaller::OnTimeout, weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&WebApkInstaller::OnResult, weak_ptr_factory_.GetWeakPtr(),
+                 WebApkInstallResult::FAILURE));
 
   icon_hasher_.reset(new WebApkIconHasher());
   icon_hasher_->DownloadAndComputeMurmur2Hash(
@@ -457,7 +461,7 @@ void WebApkInstaller::OnGotIconMurmur2Hash(
 
   // An empty hash indicates that |icon_hasher_| encountered an error.
   if (icon_murmur2_hash.empty()) {
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -495,7 +499,8 @@ void WebApkInstaller::SendRequest(std::unique_ptr<webapk::WebApk> request_proto,
   timer_.Start(
       FROM_HERE,
       base::TimeDelta::FromMilliseconds(webapk_download_url_timeout_ms_),
-      base::Bind(&WebApkInstaller::OnTimeout, weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&WebApkInstaller::OnResult, weak_ptr_factory_.GetWeakPtr(),
+                 WebApkInstallResult::FAILURE));
 
   url_fetcher_ =
       net::URLFetcher::Create(server_url, net::URLFetcher::POST, this);
@@ -526,7 +531,7 @@ void WebApkInstaller::OnCreatedSubDirAndSetPermissions(
     const GURL& download_url,
     const base::FilePath& output_dir) {
   if (output_dir.empty()) {
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -539,7 +544,8 @@ void WebApkInstaller::DownloadWebApk(const base::FilePath& output_path,
                                      bool retry_if_fails) {
   timer_.Start(
       FROM_HERE, base::TimeDelta::FromMilliseconds(download_timeout_ms_),
-      base::Bind(&WebApkInstaller::OnTimeout, weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&WebApkInstaller::OnResult, weak_ptr_factory_.GetWeakPtr(),
+                 WebApkInstallResult::FAILURE));
 
   downloader_.reset(new FileDownloader(
       download_url, output_path, true, request_context_getter_,
@@ -557,7 +563,7 @@ void WebApkInstaller::OnWebApkDownloaded(const base::FilePath& file_path,
 
   if (result != FileDownloader::DOWNLOADED) {
     if (!retry_if_fails) {
-      OnFailure();
+      OnResult(WebApkInstallResult::FAILURE);
       return;
     }
 
@@ -584,7 +590,7 @@ void WebApkInstaller::OnWebApkMadeWorldReadable(
     const base::FilePath& file_path,
     bool change_permission_success) {
   if (!change_permission_success) {
-    OnFailure();
+    OnResult(WebApkInstallResult::FAILURE);
     return;
   }
 
@@ -595,17 +601,3 @@ void WebApkInstaller::OnWebApkMadeWorldReadable(
       base::android::ConvertUTF8ToJavaString(env, webapk_package_);
   InstallOrUpdateDownloadedWebApk(env, java_file_path, java_package_name);
 }
-
-void WebApkInstaller::OnTimeout() {
-  OnFailure();
-}
-
-void WebApkInstaller::OnSuccess() {
-  finish_callback_.Run(true, relax_updates_, webapk_package_);
-  delete this;
-}
-
-void WebApkInstaller::OnFailure() {
-  finish_callback_.Run(false, relax_updates_, webapk_package_);
-  delete this;
-}