[WebAPKs] Display same text for menu & engagement banner

chrome/browser/android/shortcut_info.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/shortcut_info.h"
 
 ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
     : url(shortcut_url),
       display(blink::kWebDisplayModeBrowser),
       orientation(blink::kWebScreenOrientationLockDefault),
       source(SOURCE_ADD_TO_HOMESCREEN_SHORTCUT),
       theme_color(content::Manifest::kInvalidOrMissingColor),
       background_color(content::Manifest::kInvalidOrMissingColor),
       ideal_splash_image_size_in_px(0),
       minimum_splash_image_size_in_px(0) {}
 
 ShortcutInfo::ShortcutInfo(const ShortcutInfo& other) = default;
 
 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.
   if (manifest.start_url.is_valid())
     url = manifest.start_url;
 
   if (manifest.scope.is_valid())
     scope = manifest.scope;
 
   // Set the display based on the manifest value, if any.
@@ skipping 30 matching lines @@
 
   // Set the icon urls based on the icons in the manifest, if any.
   icon_urls.clear();
   for (const content::Manifest::Icon& icon : manifest.icons)
     icon_urls.push_back(icon.src.spec());
 }
 
 void ShortcutInfo::UpdateSource(const Source new_source) {
   source = new_source;
 }