[WebAPKs] Display same text for menu & engagement banner

chrome/browser/android/shortcut_info.cc

Index: chrome/browser/android/shortcut_info.cc
diff --git a/chrome/browser/android/shortcut_info.cc b/chrome/browser/android/shortcut_info.cc
index 7811a601cb7a8f2c9a65c99b17b0d4303967fe1c..87d4c3f012570c29c894f9f0dc949743d365ff2f 100644
--- a/chrome/browser/android/shortcut_info.cc
+++ b/chrome/browser/android/shortcut_info.cc
@@ -20,16 +20,12 @@ ShortcutInfo::~ShortcutInfo() {
 }
 
 void ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {

[dominickn, 2017/06/02 01:42:25]

Please add:

DCHECK(!short_name.empty() || !name.empty());

[pkotwicz, 2017/06/02 15:53:53]

UpdateManifest() might be called with an empty short_name and an empty name in
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck()

[dominickn, 2017/06/06 00:20:58]

Good catch. That's not great - it means we'll show the add to homescreen dialog
with an empty title if name = "" and short_name = "" (and I think this might
have done the same before this patch).

Ideally, we'd have one piece of code that works out the best title to use. That
one location should be in InstallableManager because it's already doing that
work. What do you think about returning the title as another member of the
InstallableData struct? Then all of this code could just use that title and we
wouldn't have the title selection logic duplicated everywhere. If you're happy
with that solution I can put up a patch for it.

Otherwise, the simpler stop gap is to guard the call in
AddToHomescreenDataFetcher() with an if check on the short_name and name (which
is a bit yuck and adds more duplication).

[pkotwicz, 2017/06/07 02:22:33]

Ok, I have changed the logic to be a no-op if the manifest's short_name and name
are empty. I have added a unit test
(ShortcutInfoTest::IgnoreEmptyNameAndShortName)

I don't think that changing the logic in InstallableManager makes sense because
AddToHomescreenDataFetcher::OnDidGetWebApplicationInfo() is unique to the
app-menu code path.

Guarding the calling to ShortcutInfo::UpdateFromManifest()  will break creating
a full screen PWA for a page:
- with a Web Manifest with display="standalone" and empty name and short_name
- currently the name and short_name are picked up from the page's <title> tag

-  if (!manifest.short_name.is_null())
-    short_name = manifest.short_name.string();
-  if (!manifest.name.is_null())
-    name = manifest.name.string();
-  if (manifest.short_name.is_null() != manifest.name.is_null()) {
-    if (manifest.short_name.is_null())
-      short_name = name;
-    else
-      name = short_name;
-  }
+  short_name = manifest.short_name.string();
+  name = manifest.name.string();
+  if (short_name.empty())
+    short_name = name;
+  else if (name.empty())
+    name = short_name;
   user_title = short_name;
 
   // Set the url based on the manifest value, if any.