Trigger app banner when add to homescreen is pressed and WebAPKs are enabled.

chrome/browser/android/banners/app_banner_infobar_delegate_android.cc

Index: chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
diff --git a/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc b/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
index 1e2e973cbef929860fbd4ce586fd014ffe1bb2ee..8c444f7b09c38655b8de3be43dfea1589e6e9dad 100644
--- a/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
+++ b/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
@@ -37,48 +37,62 @@ using base::android::ConvertUTF16ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Be cautious with using directives; prefer to use them only when it
noticeably improves readability or shortens code.

My rule of thumb is, if I can avoid the directive and the file is not longer, I
avoid it.

[Xi Han, 2016/09/08 15:49:09]

I will keep this in mind.

 
+namespace {
+
+bool IsInfoEmpty(const ShortcutInfo* info) {

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Take a const unique_ptr& and you won't have to say .get() on all the
callers.

[Xi Han, 2016/09/08 15:49:10]

It can't be a unique_ptr since when this function is called, we need to pass
std::move(shortcut_info_). 

[Peter Kasting, 2016/09/08 20:57:05]

I said "const unique_ptr&", not "unique_ptr".

+  return !info || info->url.is_empty();

[Peter Kasting, 2016/09/08 04:56:57]

Note that if you split this class in the future, I think one type of delegate
will always have a null pointer and the other always a non-null one, so this
function can be simplified or even removed.  Just something to keep in mind for
later.

[Xi Han, 2016/09/08 15:49:09]

Acknowledged.

+}
+
+}  // anonymous namespace

[Peter Kasting, 2016/09/08 04:56:56]

Nit: No need for this comment

[Xi Han, 2016/09/08 15:49:10]

Done.

+
 namespace banners {
 
-AppBannerInfoBarDelegateAndroid::AppBannerInfoBarDelegateAndroid(
+// static
+bool AppBannerInfoBarDelegateAndroid::Create(
+    content::WebContents* web_contents,
     base::WeakPtr<AppBannerManager> weak_manager,
     const base::string16& app_title,
-    const GURL& manifest_url,
-    const content::Manifest& manifest,
-    const GURL& icon_url,
+    std::unique_ptr<ShortcutInfo> shortcut_info,
     std::unique_ptr<SkBitmap> icon,
     int event_request_id,
-    bool is_webapk)
-    : weak_manager_(weak_manager),
-      app_title_(app_title),
-      manifest_url_(manifest_url),
-      manifest_(manifest),
-      icon_url_(icon_url),
-      icon_(std::move(icon)),
-      event_request_id_(event_request_id),
-      has_user_interaction_(false),
-      is_webapk_(is_webapk),
-      weak_ptr_factory_(this) {
-  DCHECK(!manifest.IsEmpty());
-  CreateJavaDelegate();
+    bool is_webapk,
+    bool start_install_webapk) {
+  const GURL& url = shortcut_info->url;

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Inline this into the caller below (even if you wanted a temp, you'd want to
declare it down there where it's used anyway)

[Xi Han, 2016/09/08 15:49:09]

It has to be done before std::move(shortcut_info) is called, otherwise the
pointer shortcut_info will be invalid.

[Peter Kasting, 2016/09/08 20:57:05]

Oh bah.

+  auto infobar_delegate =
+      base::WrapUnique(new banners::AppBannerInfoBarDelegateAndroid(
+          weak_manager, app_title, std::move(shortcut_info), std::move(icon),
+          event_request_id, is_webapk));
+  auto raw_delegate = infobar_delegate.get();
+

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Can't really figure out the logic on when this function does and doesn't
insert newlines.  I might remove this one and the next, and add one before the
final "return true"?  That way we have one block for trying to create the
infobar, and then one set-off block for optionally calling AcceptWebApk().

[Xi Han, 2016/09/08 15:49:09]

Sure, updated.

+  auto infobar = base::MakeUnique<AppBannerInfoBarAndroid>(
+      std::move(infobar_delegate), url, is_webapk);
+
+  if (!InfoBarService::FromWebContents(web_contents)
+           ->AddInfoBar(std::move(infobar)))
+    return false;
+
+  if (is_webapk && start_install_webapk)
+    raw_delegate->AcceptWebApk(web_contents);
+  return true;
 }
 
-AppBannerInfoBarDelegateAndroid::AppBannerInfoBarDelegateAndroid(
+// static
+bool AppBannerInfoBarDelegateAndroid::Create(
+    content::WebContents* web_contents,
     const base::string16& app_title,
     const base::android::ScopedJavaGlobalRef<jobject>& native_app_data,
     std::unique_ptr<SkBitmap> icon,
     const std::string& native_app_package,
     const std::string& referrer,
-    int event_request_id)
-    : app_title_(app_title),
-      native_app_data_(native_app_data),
-      icon_(std::move(icon)),
-      native_app_package_(native_app_package),
-      referrer_(referrer),
-      event_request_id_(event_request_id),
-      has_user_interaction_(false),
-      weak_ptr_factory_(this) {
-  DCHECK(!native_app_data_.is_null());
-  CreateJavaDelegate();
+    int event_request_id) {
+  auto infobar_delegate = base::WrapUnique(new AppBannerInfoBarDelegateAndroid(
+      app_title, native_app_data, std::move(icon), native_app_package, referrer,
+      event_request_id));
+  auto infobar = base::MakeUnique<AppBannerInfoBarAndroid>(
+      std::move(infobar_delegate), native_app_data);

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Might want to inline this below (or even both temps, but that's pretty darn
ugly)

[Xi Han, 2016/09/08 15:49:09]

Inline the infobar one only, since the delegate one is too long.

+

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Maybe remove this newline?

[Xi Han, 2016/09/08 15:49:10]

Done.

+  return InfoBarService::FromWebContents(web_contents)
+      ->AddInfoBar(std::move(infobar));
 }
 
 AppBannerInfoBarDelegateAndroid::~AppBannerInfoBarDelegateAndroid() {
@@ -87,7 +101,7 @@ AppBannerInfoBarDelegateAndroid::~AppBannerInfoBarDelegateAndroid() {
   if (!has_user_interaction_) {
     if (!native_app_data_.is_null())
       TrackUserResponse(USER_RESPONSE_NATIVE_APP_IGNORED);
-    else if (!manifest_.IsEmpty())
+    else if (!IsInfoEmpty(shortcut_info_.get()))
       TrackUserResponse(USER_RESPONSE_WEB_APP_IGNORED);
   }
 
@@ -154,6 +168,88 @@ void AppBannerInfoBarDelegateAndroid::OnInstallFinished(
   }
 }
 
+bool AppBannerInfoBarDelegateAndroid::AcceptWebApk(
+    content::WebContents* web_contents) {
+  if (IsInfoEmpty(shortcut_info_.get()))
+    return true;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  // |webapk_package_name_| is set when the WebAPK has finished installing.
+  // If the |webapk_package_name_| is empty, it means the "Add to Homescreen"
+  // button is pressed, so request WebAPK installation. Otherwise, it means
+  // the "Open" button is pressed, then open the installed WebAPK.

[Peter Kasting, 2016/09/08 04:56:57]

Nit: then -> so

[Xi Han, 2016/09/08 15:49:10]

Done.

+  if (webapk_package_name_.empty()) {
+    // Request install the WebAPK.
+    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());
+    DVLOG(1) << "Trigger the installation of the WebAPK.";

[Peter Kasting, 2016/09/08 04:56:56]

Nit: Avoid checking in logging if possible.

[Xi Han, 2016/09/08 15:49:09]

Done.

+    ShortcutHelper::InstallWebApkWithSkBitmap(web_contents->GetBrowserContext(),
+                                              *shortcut_info_.get(),

[Peter Kasting, 2016/09/08 04:56:57]

Nit: .get() not necessary when dereferencing a smart pointer with '*' (3 places)

[Xi Han, 2016/09/08 15:49:09]

Done.

+                                              *icon_.get(), callback);
+
+    SendBannerAccepted(web_contents, "web");
+    // Returns false to prevent the infobar from disappearing.

[Peter Kasting, 2016/09/08 04:56:57]

Nit: We can see it returns false; maybe instead say "Prevent the infobar from
disappearing, because..." (or "so...") with an explanation of why this matters.

Put a newline above comments like this since they describe the subsequenct block
of code.

[Xi Han, 2016/09/08 15:49:09]

Done.

+    return false;
+  }
+
+  // Open the WebAPK.

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Consider making this shorter arm the one in the indented case above, just
to minimize the amount of code that gets indented.

[Xi Han, 2016/09/08 15:49:09]

Done.

+  ScopedJavaLocalRef<jstring> java_webapk_package_name =
+      base::android::ConvertUTF8ToJavaString(env, webapk_package_name_);
+  Java_AppBannerInfoBarDelegateAndroid_openWebApk(env, java_delegate_,
+                                                  java_webapk_package_name);
+
+  SendBannerAccepted(web_contents, "web");
+  return true;
+}
+
+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),
+      app_title_(app_title),
+      shortcut_info_(std::move(shortcut_info)),
+      icon_(std::move(icon)),
+      event_request_id_(event_request_id),
+      has_user_interaction_(false),
+      is_webapk_(is_webapk),
+      weak_ptr_factory_(this) {
+  DCHECK(!IsInfoEmpty(shortcut_info_.get()));
+  CreateJavaDelegate();
+}
+
+AppBannerInfoBarDelegateAndroid::AppBannerInfoBarDelegateAndroid(
+    const base::string16& app_title,
+    const base::android::ScopedJavaGlobalRef<jobject>& native_app_data,
+    std::unique_ptr<SkBitmap> icon,
+    const std::string& native_app_package,
+    const std::string& referrer,
+    int event_request_id)
+    : app_title_(app_title),
+      native_app_data_(native_app_data),
+      icon_(std::move(icon)),
+      native_app_package_(native_app_package),
+      referrer_(referrer),
+      event_request_id_(event_request_id),
+      has_user_interaction_(false),
+      weak_ptr_factory_(this) {
+  DCHECK(!native_app_data_.is_null());
+  CreateJavaDelegate();
+}
+
 void AppBannerInfoBarDelegateAndroid::CreateJavaDelegate() {
   JNIEnv* env = base::android::AttachCurrentThread();

[Peter Kasting, 2016/09/08 04:56:57]

Nit: Just inline this below

[Xi Han, 2016/09/08 15:49:10]

Done.

   java_delegate_.Reset(Java_AppBannerInfoBarDelegateAndroid_create(
@@ -197,11 +293,10 @@ void AppBannerInfoBarDelegateAndroid::InfoBarDismissed() {
     TrackUserResponse(USER_RESPONSE_NATIVE_APP_DISMISSED);
     AppBannerSettingsHelper::RecordBannerDismissEvent(
         web_contents, native_app_package_, AppBannerSettingsHelper::NATIVE);
-  } else if (!manifest_.IsEmpty()) {
+  } else if (!IsInfoEmpty(shortcut_info_.get())) {

[Peter Kasting, 2016/09/08 04:56:57]

Can both these conditions fail, or is one always true?

[Xi Han, 2016/09/08 15:49:09]

I believe one is always true, see the DCHECH in the Create() function. Yes, it's
better to use else her.

     TrackUserResponse(USER_RESPONSE_WEB_APP_DISMISSED);
     AppBannerSettingsHelper::RecordBannerDismissEvent(
-        web_contents, manifest_.start_url.spec(),
-        AppBannerSettingsHelper::WEB);
+        web_contents, shortcut_info_->url.spec(), AppBannerSettingsHelper::WEB);
   }
 }
 
@@ -223,12 +318,12 @@ bool AppBannerInfoBarDelegateAndroid::Accept() {
     return true;
   }
 
-  if (!native_app_data_.is_null()) {
+  if (!native_app_data_.is_null())
     return AcceptNativeApp(web_contents);
-  } else if (is_webapk_) {
+  else if (is_webapk_)

[Peter Kasting, 2016/09/08 04:56:56]

Nit: No else after return (2 places)

[Xi Han, 2016/09/08 15:49:09]

Done.

     return AcceptWebApk(web_contents);
-  }
-  return AcceptWebApp(web_contents);
+  else
+    return AcceptWebApp(web_contents);
 }
 
 bool AppBannerInfoBarDelegateAndroid::AcceptNativeApp(
@@ -260,81 +355,24 @@ bool AppBannerInfoBarDelegateAndroid::AcceptNativeApp(
 
 bool AppBannerInfoBarDelegateAndroid::AcceptWebApp(
     content::WebContents* web_contents) {
-  if (manifest_.IsEmpty())
+  if (IsInfoEmpty(shortcut_info_.get()))
     return true;
   TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);
 
   AppBannerSettingsHelper::RecordBannerInstallEvent(
-      web_contents, manifest_.start_url.spec(),
-      AppBannerSettingsHelper::WEB);
+      web_contents, shortcut_info_->url.spec(), AppBannerSettingsHelper::WEB);
 
   if (weak_manager_) {
-    ShortcutInfo info(GURL::EmptyGURL());
-    info.UpdateFromManifest(manifest_);
-    info.manifest_url = manifest_url_;
-    info.icon_url = icon_url_;
-    info.UpdateSource(ShortcutInfo::SOURCE_APP_BANNER);
-
     const std::string& uid = base::GenerateGUID();
     ShortcutHelper::AddToLauncherWithSkBitmap(
-        web_contents->GetBrowserContext(), info, uid, *icon_.get(),
-        weak_manager_->FetchWebappSplashScreenImageCallback(uid));
+        web_contents->GetBrowserContext(), *shortcut_info_.get(), uid,
+        *icon_.get(), weak_manager_->FetchWebappSplashScreenImageCallback(uid));
   }
 
   SendBannerAccepted(web_contents, "web");
   return true;
 }
 
-bool AppBannerInfoBarDelegateAndroid::AcceptWebApk(
-    content::WebContents* web_contents) {
-  if (manifest_.IsEmpty())
-    return true;
-
-  JNIEnv* env = base::android::AttachCurrentThread();
-  // |webapk_package_name_| is set when the WebAPK has finished installing.
-  // If the |webapk_package_name_| is empty, it means the "Add to Homescreen"
-  // button is pressed, so request WebAPK installation. Otherwise, it means
-  // the "Open" button is pressed, then open the installed WebAPK.
-  if (webapk_package_name_.empty()) {
-    // Request install the WebAPK.
-    TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);
-
-    AppBannerSettingsHelper::RecordBannerInstallEvent(
-        web_contents, manifest_.start_url.spec(),
-        AppBannerSettingsHelper::WEB);
-
-    ShortcutInfo info(GURL::EmptyGURL());
-    info.UpdateFromManifest(manifest_);
-    info.manifest_url = manifest_url_;
-    info.icon_url = icon_url_;
-    info.UpdateSource(ShortcutInfo::SOURCE_APP_BANNER);
-
-    Java_AppBannerInfoBarDelegateAndroid_setWebApkInstallingState(
-        env, java_delegate_, true);
-    UpdateInstallState(env, nullptr);
-
-    WebApkInstaller::FinishCallback callback = base::Bind(
-        &AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished,
-        weak_ptr_factory_.GetWeakPtr());
-    DVLOG(1) << "Trigger the installation of the WebAPK.";
-    ShortcutHelper::InstallWebApkWithSkBitmap(
-        web_contents->GetBrowserContext(), info, *icon_.get(), callback);
-
-    SendBannerAccepted(web_contents, "web");
-    // Returns false to prevent the infobar from disappearing.
-    return false;
-  }
-
-  // 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);
-
-  SendBannerAccepted(web_contents, "web");
-  return true;
-}
-
 void AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished(
     bool success,
     const std::string& webapk_package_name) {