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

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 17 matching lines @@
 #include "content/public/common/manifest.h"
 #include "jni/AppBannerInfoBarDelegateAndroid_jni.h"
 #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;

[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() {
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   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);
   }
 
   TrackDismissEvent(DISMISS_EVENT_DISMISSED);
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_AppBannerInfoBarDelegateAndroid_destroy(env, java_delegate_);
   java_delegate_.Reset();
 }
 
 void AppBannerInfoBarDelegateAndroid::UpdateInstallState(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   if (native_app_data_.is_null() && !is_webapk_)
     return;
 
   int newState = Java_AppBannerInfoBarDelegateAndroid_determineInstallState(

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

Nit: |new_state| (this isn't Java)

Or just inline this below.

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

Done.

       env, java_delegate_, native_app_data_);
   static_cast<AppBannerInfoBarAndroid*>(infobar())
       ->OnInstallStateChanged(newState);
 }
 
 void AppBannerInfoBarDelegateAndroid::OnInstallIntentReturned(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jboolean jis_installing) {
   if (!infobar())

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

How can this ever be null?  A delegate should never have no infobar.  (2 places)

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

Add DCHECK instead.

     return;
 
   content::WebContents* web_contents =
       InfoBarService::WebContentsFromInfoBar(infobar());
   if (!web_contents)

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

Can this really be called when the infobar is unowned?  You shouldn't be
checking this unless you need to (e.g. if this callback can occur while the
infobar is animating closed).  If you do need to, you should probably note why.

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

Removed.

     return;
 
   if (jis_installing) {
     AppBannerSettingsHelper::RecordBannerEvent(
         web_contents,
         web_contents->GetURL(),
         native_app_package_,
         AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
         AppBannerManager::GetCurrentTime());
 
@@ skipping 15 matching lines @@
 
   if (success) {
     TrackInstallEvent(INSTALL_EVENT_NATIVE_APP_INSTALL_COMPLETED);
     UpdateInstallState(env, obj);
   } else if (infobar()->owner()) {
     TrackDismissEvent(DISMISS_EVENT_INSTALL_TIMEOUT);
     infobar()->owner()->RemoveInfoBar(infobar());
   }
 }
 
+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(
       env,
       reinterpret_cast<intptr_t>(this)));
 }
 
 void AppBannerInfoBarDelegateAndroid::SendBannerAccepted(
     content::WebContents* web_contents,
     const std::string& platform) {
   web_contents->GetMainFrame()->Send(
       new ChromeViewMsg_AppBannerAccepted(
           web_contents->GetMainFrame()->GetRoutingID(),
           event_request_id_,
           platform));
 }
 
 infobars::InfoBarDelegate::InfoBarIdentifier
 AppBannerInfoBarDelegateAndroid::GetIdentifier() const {
   return APP_BANNER_INFOBAR_DELEGATE_ANDROID;
 }
 
 gfx::Image AppBannerInfoBarDelegateAndroid::GetIcon() const {
   return gfx::Image::CreateFrom1xBitmap(*icon_.get());
 }
 
 void AppBannerInfoBarDelegateAndroid::InfoBarDismissed() {
   has_user_interaction_ = true;
 
   content::WebContents* web_contents =
       InfoBarService::WebContentsFromInfoBar(infobar());
   if (!web_contents)

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

How can this ever be null?  We should never be getting InfoBarDismissed() while
not owned.

Similar question applies to Accept() below.

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

Done.

     return;
 
   web_contents->GetMainFrame()->Send(
       new ChromeViewMsg_AppBannerDismissed(
           web_contents->GetMainFrame()->GetRoutingID(),
           event_request_id_));
 
   if (!native_app_data_.is_null()) {
     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);
   }
 }
 
 base::string16 AppBannerInfoBarDelegateAndroid::GetMessageText() const {
   return app_title_;
 }
 
 int AppBannerInfoBarDelegateAndroid::GetButtons() const {
   return BUTTON_OK;
 }
 
 bool AppBannerInfoBarDelegateAndroid::Accept() {
   has_user_interaction_ = true;
 
   content::WebContents* web_contents =
       InfoBarService::WebContentsFromInfoBar(infobar());
   if (!web_contents) {
     TrackDismissEvent(DISMISS_EVENT_ERROR);
     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(
     content::WebContents* web_contents) {
   TrackUserResponse(USER_RESPONSE_NATIVE_APP_ACCEPTED);
   JNIEnv* env = base::android::AttachCurrentThread();
 
   TabAndroid* tab = TabAndroid::FromWebContents(web_contents);
   if (tab == nullptr) {

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

Can this be null?  When?

Nit: "if (!tab)" is probably a more common Chromium idiom. (2 places)

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

It looks like the tab shouldn't be null, but I am not 100 percent sure. Add a
DCHECK here.

     TrackDismissEvent(DISMISS_EVENT_ERROR);
     return true;
   }
   ScopedJavaLocalRef<jstring> jreferrer(
       ConvertUTF8ToJavaString(env, referrer_));
 
   bool was_opened =
       Java_AppBannerInfoBarDelegateAndroid_installOrOpenNativeApp(
           env, java_delegate_, tab->GetJavaObject(),
           native_app_data_, jreferrer);
 
   if (was_opened) {

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

Nit: No {}

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

Done.

     TrackDismissEvent(DISMISS_EVENT_APP_OPEN);
   } else {
     TrackInstallEvent(INSTALL_EVENT_NATIVE_APP_INSTALL_TRIGGERED);
   }
   SendBannerAccepted(web_contents, "play");
   return was_opened;
 }
 
 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) {
   JNIEnv* env = base::android::AttachCurrentThread();
   if (!success) {
     // The installation failed.
     if (infobar())
       infobar()->RemoveSelf();

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

Nit: Might be safer to do this after the next function call since this call can
delete |this|?  I don't know if that next call can reach back to anything in
this object.

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

In the UI, we would like to see a toast shown after the infobar disappears, that
is main reason the RemoveSelf is called earlier. The next one is a static
function in Java, so I assume it would be fine.

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

Are these orders perceptibly different to the user?  RemoveSelf() should start
the bar animating closed and return immediately.  It doesn't block.

     Java_AppBannerInfoBarDelegateAndroid_showWebApkInstallFailureToast(env);
     DVLOG(1) << "The WebAPK installation failed.";

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

Nit: Similarly, avoid this logging unless it's actively helping you debug an
issue.

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

Personally I still prefer to have this log which helps identify whether the
callback is called even if the installation fails.

     return;
   }
 
   webapk_package_name_ = webapk_package_name;
   ScopedJavaLocalRef<jstring> java_webapk_package_name =
       base::android::ConvertUTF8ToJavaString(env, webapk_package_name);
   Java_AppBannerInfoBarDelegateAndroid_setWebApkInstallingState(
       env, java_delegate_, false);
   Java_AppBannerInfoBarDelegateAndroid_setWebApkPackageName(
       env, java_delegate_, java_webapk_package_name);
   UpdateInstallState(env, nullptr);
 }
 
 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 = web_contents ? TabAndroid::FromWebContents(web_contents)
                                  : nullptr;
   if (tab == nullptr) {

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

Again, can this be null?  When?

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

Add a DCHECK instead.

     TrackDismissEvent(DISMISS_EVENT_ERROR);
     return true;
   }
 
   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