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

chrome/browser/android/banners/app_banner_manager_android.cc

Index: chrome/browser/android/banners/app_banner_manager_android.cc
diff --git a/chrome/browser/android/banners/app_banner_manager_android.cc b/chrome/browser/android/banners/app_banner_manager_android.cc
index d7ba9c2dc61d2f8311e06b84d11a1c68bc62bbea..aafbbe9aa4407e9ab2bbcefccc5db53340f2642a 100644
--- a/chrome/browser/android/banners/app_banner_manager_android.cc
+++ b/chrome/browser/android/banners/app_banner_manager_android.cc
@@ -15,10 +15,8 @@
 #include "chrome/browser/android/webapk/chrome_webapk_host.h"
 #include "chrome/browser/android/webapk/webapk_web_manifest_checker.h"
 #include "chrome/browser/banners/app_banner_metrics.h"
-#include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/manifest/manifest_icon_downloader.h"
 #include "chrome/browser/manifest/manifest_icon_selector.h"
-#include "chrome/browser/ui/android/infobars/app_banner_infobar_android.h"
 #include "chrome/common/chrome_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
@@ -40,6 +38,20 @@ const char kReferrerName[] = "referrer";
 const char kIdName[] = "id";
 const char kPlayInlineReferrer[] = "playinline=chrome_inline";

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

Nit: Each of these temps is used in only one function.  Define them in the
functions in which they're used.  Or just inline them where used, if the
variable names don't add much.

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

Personally I would prefer to declare them at the beginning of the class, in case
they will be used again in the future. Especially the fourth one is used twice.
Since the first one is inline now, just remove it, but leave the other three.

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

Twice yes, but in adjacent statements in the same function.

http://google.github.io/styleguide/cppguide.html#Local_Variables says
"...declare [variables] in as local a scope as possible, and as close to the
first use as possible."  While arguably constants are not the same as variables,
declaring them somewhere widely separate from their use means that code
referencing them now requires the reader to scan to a different place to see
what the constant value actually is.  It also makes it easier to have these left
around after their uses are deleted.  And if these are used in multiple places
in the future they can always be factored out then.

More directly in this case, I think kPlayInlineReferrer deserves a named
constant anyway, but the kXXXName ones don't -- those string constants should
just be inlined directly into the code that uses them, since the constant names
just add verbosity to the actual strings without adding any clarity or
explanation.  So I'd inline those two and then make a decision about how to
handle the last one based on how compelling you think my above arguments are :)

 
+std::unique_ptr<ShortcutInfo> CreateShortcutInfo(
+    const GURL& manifest_url,
+    const content::Manifest& manifest,
+    const GURL& icon_url) {
+  std::unique_ptr<ShortcutInfo> shortcut_info(new ShortcutInfo(GURL()));

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

Nit: Prefer "= base::MakeUnique<ShortcutInfo>(GURL())" to raw new.

Feel free to use auto as needed to avoid re-spelling the type name.

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

Done.

+  if (!manifest.IsEmpty()) {
+    shortcut_info->UpdateFromManifest(manifest);
+    shortcut_info->manifest_url = manifest_url;
+    shortcut_info->icon_url = icon_url;
+    shortcut_info->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER);
+  }
+  return shortcut_info;
+}
+
 }  // anonymous namespace
 
 namespace banners {
@@ -185,17 +197,13 @@ void AppBannerManagerAndroid::ShowBanner() {
   content::WebContents* contents = web_contents();
   DCHECK(contents);
 
-  infobars::InfoBar* infobar = nullptr;
   if (native_app_data_.is_null()) {
     bool is_webapk = ChromeWebApkHost::AreWebApkEnabled();

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

Nit: Just inline below

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

Done.

-    std::unique_ptr<AppBannerInfoBarDelegateAndroid> delegate(
-        new AppBannerInfoBarDelegateAndroid(
-            GetWeakPtr(), app_title_, manifest_url_, manifest_, icon_url_,
-            std::move(icon_), event_request_id(), is_webapk));
-
-    infobar = new AppBannerInfoBarAndroid(
-        std::move(delegate), manifest_.start_url, is_webapk);
-    if (infobar) {
+    if (AppBannerInfoBarDelegateAndroid::Create(
+            contents, GetWeakPtr(), app_title_,
+            CreateShortcutInfo(manifest_url_, manifest_, icon_url_),
+            std::move(icon_), event_request_id(), is_webapk,
+            false /* start_install_webapk*/)) {

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

Nit: Need space before */ (or just remove the comment, people should have param
names in their IDE cross-references or codesearch)

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

Done.

       RecordDidShowBanner("AppBanner.WebApp.Shown");
       TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_CREATED);
       ReportStatus(contents, SHOWING_WEB_APP_BANNER);
@@ -203,13 +211,9 @@ void AppBannerManagerAndroid::ShowBanner() {
       ReportStatus(contents, FAILED_TO_CREATE_BANNER);
     }
   } else {
-    std::unique_ptr<AppBannerInfoBarDelegateAndroid> delegate(
-        new AppBannerInfoBarDelegateAndroid(
-            app_title_, native_app_data_, std::move(icon_), native_app_package_,
-            referrer_, event_request_id()));
-    infobar =
-        new AppBannerInfoBarAndroid(std::move(delegate), native_app_data_);
-    if (infobar) {
+    if (AppBannerInfoBarDelegateAndroid::Create(
+            contents, app_title_, native_app_data_, std::move(icon_),
+            native_app_package_, referrer_, event_request_id())) {
       RecordDidShowBanner("AppBanner.NativeApp.Shown");
       TrackDisplayEvent(DISPLAY_EVENT_NATIVE_APP_BANNER_CREATED);
       ReportStatus(contents, SHOWING_NATIVE_APP_BANNER);
@@ -217,11 +221,6 @@ void AppBannerManagerAndroid::ShowBanner() {
       ReportStatus(contents, FAILED_TO_CREATE_BANNER);
     }
   }
-
-  if (infobar) {
-    InfoBarService::FromWebContents(contents)->AddInfoBar(
-        base::WrapUnique(infobar));
-  }
 }
 
 bool AppBannerManagerAndroid::CanHandleNonWebApp(const std::string& platform,