Skip installation process if WebAPK is already installed.

chrome/browser/android/banners/app_banner_infobar_delegate_android.cc

 // Copyright 2015 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/banners/app_banner_infobar_delegate_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/guid.h"
 #include "base/location.h"
@@ skipping 20 matching lines @@
 #include "ui/gfx/android/java_bitmap.h"
 #include "url/gurl.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ConvertUTF16ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;
 
+// TODO(zpeng): Reorganize control flow to allow itemized metrics.

[dominickn, 2016/09/28 02:10:25]

Nit: don't have a TODO without a crbug link.

[F, 2016/09/30 18:02:01]

Done. Thanks for the information!

+
 namespace {
 
 bool IsInfoEmpty(const std::unique_ptr<ShortcutInfo>& info) {
   return !info || info->url.is_empty();
 }
 
 }  // anonymous namespace
 
 namespace banners {
 
@@ skipping 136 matching lines @@
     // Open the WebAPK.
     ScopedJavaLocalRef<jstring> java_webapk_package_name =
         base::android::ConvertUTF8ToJavaString(env, webapk_package_name_);
     Java_AppBannerInfoBarDelegateAndroid_openWebApk(env, java_delegate_,
                                                     java_webapk_package_name);
     webapk::TrackUserAction(webapk::USER_ACTION_INSTALLED_OPEN);
     SendBannerAccepted(web_contents, "web");
     return true;
   }
 
-  // Request install the WebAPK.
-  install_state_ = INSTALLING;
-  TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);
-  AppBannerSettingsHelper::RecordBannerInstallEvent(
-      web_contents, shortcut_info_->url.spec(), AppBannerSettingsHelper::WEB);
+  // Check whether the WebAPK has been installed.
+  std::string installed_webapk_package_name =
+      ShortcutHelper::QueryWebApkPackage(web_contents->GetLastCommittedURL());
+  if (installed_webapk_package_name.empty()) {
+    // Request install the WebAPK.
+    install_state_ = INSTALLING;
+    TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);
+    AppBannerSettingsHelper::RecordBannerInstallEvent(
+        web_contents, shortcut_info_->url.spec(), AppBannerSettingsHelper::WEB);
 
-  Java_AppBannerInfoBarDelegateAndroid_setWebApkInstallingState(
-      env, java_delegate_, true);
-  UpdateInstallState(env, nullptr);
-  WebApkInstaller::FinishCallback callback =
-      base::Bind(&AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished,
-                  weak_ptr_factory_.GetWeakPtr());
-  ShortcutHelper::InstallWebApkWithSkBitmap(web_contents->GetBrowserContext(),
-                                            *shortcut_info_,
-                                            *icon_.get(), callback);
-  SendBannerAccepted(web_contents, "web");
+    Java_AppBannerInfoBarDelegateAndroid_setWebApkInstallingState(
+        env, java_delegate_, true);
+    UpdateInstallState(env, nullptr);
+    WebApkInstaller::FinishCallback callback =
+        base::Bind(&AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished,
+                    weak_ptr_factory_.GetWeakPtr());
+    ShortcutHelper::InstallWebApkWithSkBitmap(web_contents->GetBrowserContext(),
+                                              *shortcut_info_,
+                                              *icon_.get(), callback);
+    SendBannerAccepted(web_contents, "web");
 
-  // Prevent the infobar from disappearing, because the infobar will show
-  // "Adding" during the installation process.
-  return false;
+    // Prevent the infobar from disappearing, because the infobar will show
+    // "Adding" during the installation process.
+    return false;
+  } else {

[dominickn, 2016/09/28 02:10:25]

Nit: remove the else clause (no else after return) - just have the contents
outside of the else.

[F, 2016/09/30 18:02:01]

Done. Thanks for the information!

+    // Bypass the installation process.
+    TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);

[pkotwicz, 2016/09/29 14:18:54]

Shouldn't you log:
webapk::TrackUserAction(webapk::USER_ACTION_OPEN) instead of
TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED)

It is not clear that we shouldn't log the USER_RESPONSE_WEB_APP_ACCEPTED
histogram. It feels like the histogram should only be logged when the "user has
accepted a WebAPK install"

[F, 2016/09/30 18:02:01]

Resolved in offline discussion. Filed new issue
https://bugs.chromium.org/p/chromium/issues/detail?id=651615.

+    weak_ptr_factory_.GetWeakPtr()->OnWebApkInstallFinished(
+        true, installed_webapk_package_name);

[pkotwicz, 2016/09/29 14:18:54]

Calling AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished() is not
completely right because OnWebApkInstallFinished() logs the
INSTALL_EVENT_WEB_APP_INSTALLED metric (which in my opinion we do not want
because no installation took place)

My proposal is to move lines 406 - 414 to their own function:
SetCanOpenWebApk(const std::string& webapk_package_name)

[F, 2016/09/30 18:02:01]

Resolved in offline discussion. See above.

+    return false;
+  }
 }
 
 AppBannerInfoBarDelegateAndroid::AppBannerInfoBarDelegateAndroid(
     base::WeakPtr<AppBannerManager> weak_manager,
     const base::string16& app_title,
     std::unique_ptr<ShortcutInfo> shortcut_info,
     std::unique_ptr<SkBitmap> icon,
     int event_request_id,
     bool is_webapk)
     : weak_manager_(weak_manager),
@@ skipping 178 matching lines @@
 void AppBannerInfoBarDelegateAndroid::TrackWebApkInstallationDismissEvents(
     InstallState install_state) {
   if (install_state == INSTALL_NOT_STARTED) {
     webapk::TrackInstallEvent(webapk::INFOBAR_DISMISSED_BEFORE_INSTALLATION);
   } else if (install_state == INSTALLING) {
     webapk::TrackInstallEvent(webapk::INFOBAR_DISMISSED_DURING_INSTALLATION);
   } else if (install_state == INSTALLED) {
     // When |install_state| is INSTALLED, the install Event will be recorded in
     // OnWebApkInstallFinished().
     webapk::TrackUserAction(webapk::USER_ACTION_INSTALLED_OPEN_DISMISS);
   }

[pkotwicz, 2016/09/29 14:18:54]

According to the TODO in
https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_met...
we should be recording USER_ACTION_OPEN_DISMISS when the WebAPK is already
installed.

For now, I don't mind if you add a new InstallState, ALREADY_INSTALLED, in order
to determine which metric to track. I agree that passing whether the WebAPK is
already installed into AppBannerInfoBarDelegateAndroid::Create() is the correct
long term solution

[F, 2016/09/30 18:02:01]

Resolved in offline discussion. See above.

 }
 
 bool AppBannerInfoBarDelegateAndroid::LinkClicked(
     WindowOpenDisposition disposition) {
   if (native_app_data_.is_null())
     return false;
 
   // Try to show the details for the native app.
   JNIEnv* env = base::android::AttachCurrentThread();
 
   content::WebContents* web_contents =
       InfoBarService::WebContentsFromInfoBar(infobar());
   TabAndroid* tab = TabAndroid::FromWebContents(web_contents);
   DCHECK(tab);
 
   Java_AppBannerInfoBarDelegateAndroid_showAppDetails(
       env, java_delegate_, tab->GetJavaObject(), native_app_data_);
 
   TrackDismissEvent(DISMISS_EVENT_BANNER_CLICK);
   return true;
 }
 
 bool RegisterAppBannerInfoBarDelegateAndroid(JNIEnv* env) {
  return RegisterNativesImpl(env);
 }
 
 }  // namespace banners