[WebAPKs] Display same text for menu & engagement banner

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 <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
@@ skipping 23 matching lines @@
 using base::android::ConvertUTF16ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;
 
 namespace banners {
 
 // static
 bool AppBannerInfoBarDelegateAndroid::Create(
     content::WebContents* web_contents,
     base::WeakPtr<AppBannerManager> weak_manager,
-    const base::string16& app_title,
     std::unique_ptr<ShortcutInfo> shortcut_info,
     const SkBitmap& primary_icon,
     const SkBitmap& badge_icon,
     int event_request_id,
     bool is_webapk,
     webapk::InstallSource webapk_install_source) {
   DCHECK(shortcut_info);
   const GURL url = shortcut_info->url;
   if (url.is_empty())
     return false;
 
   auto infobar_delegate =
       base::WrapUnique(new banners::AppBannerInfoBarDelegateAndroid(
-          weak_manager, app_title, std::move(shortcut_info), primary_icon,
-          badge_icon, event_request_id, is_webapk, webapk_install_source));
+          weak_manager, std::move(shortcut_info), primary_icon, badge_icon,
+          event_request_id, is_webapk, webapk_install_source));
   auto* raw_delegate = infobar_delegate.get();
   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) {
     if (webapk_install_source == webapk::INSTALL_SOURCE_MENU) {
       webapk::TrackInstallInfoBarShown(
@@ skipping 126 matching lines @@
   Java_AppBannerInfoBarDelegateAndroid_setWebApkInstallingState(
       env, java_delegate_, false);
   Java_AppBannerInfoBarDelegateAndroid_setWebApkPackageName(
       env, java_delegate_, java_webapk_package_name);
   UpdateInstallState(env, nullptr);
   install_state_ = INSTALLED;
 }
 
 AppBannerInfoBarDelegateAndroid::AppBannerInfoBarDelegateAndroid(
     base::WeakPtr<AppBannerManager> weak_manager,
-    const base::string16& app_title,
     std::unique_ptr<ShortcutInfo> shortcut_info,
     const SkBitmap& primary_icon,
     const SkBitmap& badge_icon,
     int event_request_id,
     bool is_webapk,
     webapk::InstallSource webapk_install_source)
     : weak_manager_(weak_manager),
-      app_title_(app_title),
+      app_title_(shortcut_info->name),
       shortcut_info_(std::move(shortcut_info)),
       primary_icon_(primary_icon),
       badge_icon_(badge_icon),
       event_request_id_(event_request_id),
       has_user_interaction_(false),
       is_webapk_(is_webapk),
       install_state_(INSTALL_NOT_STARTED),
       webapk_install_source_(webapk_install_source),
       weak_ptr_factory_(this) {
   CreateJavaDelegate();
@@ skipping 48 matching lines @@
   return was_opened;
 }
 
 bool AppBannerInfoBarDelegateAndroid::AcceptWebApp(
     content::WebContents* web_contents) {
   TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED);
 
   AppBannerSettingsHelper::RecordBannerInstallEvent(
       web_contents, shortcut_info_->url.spec(), AppBannerSettingsHelper::WEB);
 
-  ShortcutHelper::AddToLauncherWithSkBitmap(web_contents, *shortcut_info_,
-                                            primary_icon_);
+  ShortcutHelper::AddToLauncherWithSkBitmap(
+      web_contents, *shortcut_info_, shortcut_info_->short_name, primary_icon_);
 
   SendBannerAccepted();
   return true;
 }
 
 bool AppBannerInfoBarDelegateAndroid::AcceptWebApk(
     content::WebContents* web_contents) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
   // If the WebAPK is installed and the "Open" button is clicked, open the
@@ skipping 75 matching lines @@
     return;
 
   // If the install didn't definitely fail, we don't add a shortcut. This could
   // happen if Play was busy with another install and this one is still queued
   // (and hence might succeed in the future).
   if (result == WebApkInstallResult::FAILURE) {
     content::WebContents* web_contents =
         InfoBarService::WebContentsFromInfoBar(infobar());
     // Add webapp shortcut to the homescreen.
     ShortcutHelper::AddToLauncherWithSkBitmap(web_contents, *shortcut_info_,
+                                              shortcut_info_->short_name,

[dominickn, 2017/06/16 01:32:32]

This is a behaviour change for app banners. Previously, app banners would prefer
|name| over |short_name| if both were present (see the title setting logic in
app_banner_manager.cc). Now it will prefer |short_name| if both are present. In
fact, our documentation explicitly calls out the |name| field as being used in
the banner itself, while |short_name| is used on the homescreen, so we need to
preserve this default behaviour
(https://developers.google.com/web/fundamentals/engage-and-retain/app-install-...)

I'll reiterate the first comment I made: 
I think it makes more sense to just reverse the logic in UpdateFromManifest() to
match that used in app banners. The other thing that UpdateFromManifest()
doesn't do it check if the string() is empty as opposed to non-null. This means
you could set short_name: "" and end up with a WebAPK banner with an empty title
(we had this issue recently with app banners for m.forbes.com I think).

[pkotwicz, 2017/06/17 00:17:23]

Thank you for your question.

This is actually incorrect. The banner title and the name of the installed web
app used to come from two different places:
The banner title used to come from AppBannerManager::app_title_
The name of the install web app used to come from CreateShortcutInfo() in
app_banner_manager_android.cc

If I changed the logic in ShortcutInfo::UpdateFromManifest() to match the banner
code, this would change home screen shortcuts to use the "name" instead of the
"short_name". This would be a change in behavior for both home screen shortcuts
added from the app menu and home screen shortcuts added from the banner. As far
as I can tell, the "short_name" has been used for home screen shortcuts for at
least a year.

[dominickn, 2017/06/17 01:05:44]

Yes, we should still use the |short_name| for the home screen shortcut, so my
proposed solution there would break that.

The behaviour we want is to use |name| for the banner title if it's present,
rather than |short_name|. This means devs can use a longer, full name in the
banner and have the truncated one be in the actual shortcut if the real name is
too long. In short:

A2HS Dialog:
  - if not WebAPK: prefer short_name (actual text that will go in the shortcut)
  - if WebAPK: prefer name in the banner, but use short_name for shortcut

Banner:
 - prefer name in the banner, but use short_name for shortcut

                                               primary_icon_);
   }
 
   infobar()->RemoveSelf();
 }
 
 void AppBannerInfoBarDelegateAndroid::TrackWebApkInstallationDismissEvents(
     InstallState install_state) {
   if (install_state == INSTALL_NOT_STARTED) {
     webapk::TrackInstallEvent(webapk::INFOBAR_DISMISSED_BEFORE_INSTALLATION);
@@ skipping 66 matching lines @@
 
   TrackDismissEvent(DISMISS_EVENT_BANNER_CLICK);
   return true;
 }
 
 bool RegisterAppBannerInfoBarDelegateAndroid(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace banners