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

chrome/browser/android/banners/app_banner_manager_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 <utility>
 
 #include "chrome/browser/android/banners/app_banner_manager_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/android/banners/app_banner_infobar_delegate_android.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #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"
 #include "jni/AppBannerManager_jni.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;

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

Nit: See earlier nit on using directives.

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

Acknowledged.

 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(banners::AppBannerManagerAndroid);
 
 namespace {
 
 const char kPlayPlatform[] = "play";
 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 {
 
 AppBannerManagerAndroid::AppBannerManagerAndroid(
     content::WebContents* web_contents)
     : AppBannerManager(web_contents) {
   CreateJavaBannerManager();
 }
 
@@ skipping 125 matching lines @@
     return;
 
   icon_.reset(new SkBitmap(bitmap));
   SendBannerPromptRequest();
 }
 
 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);
     } else {
       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);
     } else {
       ReportStatus(contents, FAILED_TO_CREATE_BANNER);
     }
   }
-
-  if (infobar) {
-    InfoBarService::FromWebContents(contents)->AddInfoBar(
-        base::WrapUnique(infobar));
-  }
 }
 
 bool AppBannerManagerAndroid::CanHandleNonWebApp(const std::string& platform,
                                                  const GURL& url,
                                                  const std::string& id) {
   if (!CheckPlatformAndId(platform, id))
     return false;
 
   banners::TrackDisplayEvent(DISPLAY_EVENT_NATIVE_APP_BANNER_REQUESTED);
 
@@ skipping 36 matching lines @@
 bool AppBannerManagerAndroid::CheckPlatformAndId(const std::string& platform,
                                                  const std::string& id) {
   if (platform != kPlayPlatform) {
     ReportStatus(web_contents(), PLATFORM_NOT_SUPPORTED_ON_ANDROID);
     return false;
   }
   if (id.empty()) {
     ReportStatus(web_contents(), NO_ID_SPECIFIED);
     return false;
   }
   return true;

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

Nit: Shorter:

  const bool correct_platform = (platform == "play");
  if (correct_platform && !id.empty())
    return true;
  ReportStatus(correct_platform ? NO_ID_SPECIFIED
                                : PLATFORM_NOT_SUPPORTED_ON_ANDROID);
  return false;

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

Done.

 }
 
 std::string AppBannerManagerAndroid::ExtractQueryValueForName(
     const GURL& url,
     const std::string& name) {
   url::Component query = url.parsed_for_possibly_invalid_spec().query;
   url::Component key, value;
   const char* url_spec = url.spec().c_str();
 
   while (url::ExtractQueryKeyValue(url_spec, &query, &key, &value)) {
     std::string key_str(url_spec, key.begin, key.len);
     std::string value_str(url_spec, value.begin, value.len);
     if (key_str == name)
       return value_str;

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

Nit: Shorter, more readable, and more efficient:

  for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) {
    if (it.GetKey() == name)
      return it.GetValue();
  }

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

Done.

   }
   return "";

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

Nit: "" -> std::string()

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

Done.

 }
 
 // static
 bool AppBannerManagerAndroid::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 // static
 ScopedJavaLocalRef<jobject> GetJavaBannerManagerForWebContents(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,
     const JavaParamRef<jobject>& java_web_contents) {
   AppBannerManagerAndroid* manager = AppBannerManagerAndroid::FromWebContents(
       content::WebContents::FromJavaWebContents(java_web_contents));
   if (!manager)
     return ScopedJavaLocalRef<jobject>();
 
   return ScopedJavaLocalRef<jobject>(manager->GetJavaBannerManager());

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

Nit: Seems like this could be:

  return manager ? manager->GetJavaBannerManager()
                 : ScopedJavaLocalRef<jobject>();

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

Done.

 }
 
 // static
 void DisableSecureSchemeCheckForTesting(JNIEnv* env,
                                         const JavaParamRef<jclass>& clazz) {
   AppBannerManager::DisableSecureSchemeCheckForTesting();
 }
 
 // static
 void SetEngagementWeights(JNIEnv* env,
                           const JavaParamRef<jclass>& clazz,
                           jdouble direct_engagement,
                           jdouble indirect_engagement) {
   AppBannerManager::SetEngagementWeights(direct_engagement,
                                          indirect_engagement);
 }
 
 // static
 void SetTimeDeltaForTesting(JNIEnv* env,
                             const JavaParamRef<jclass>& clazz,
                             jint days) {
   AppBannerManager::SetTimeDeltaForTesting(days);
 }
 
 }  // namespace banners