Don't ignore manifest icons for sites that don't have a service worker.

chrome/browser/android/webapps/add_to_homescreen_data_fetcher.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/webapps/add_to_homescreen_data_fetcher.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 25 matching lines @@
 const int kDataTimeoutInMilliseconds = 4000;
 
 // Looks up the original, online URL of the site requested.  The URL from the
 // WebContents may be a distilled article which is not appropriate for a home
 // screen shortcut.
 GURL GetShortcutUrl(content::BrowserContext* browser_context,
                     const GURL& actual_url) {
   return dom_distiller::url_utils::GetOriginalUrlFromDistillerUrl(actual_url);
 }
 
-InstallableParams ParamsToPerformInstallableCheck(
+InstallableParams ParamsToPerformManifestAndIconFetch(
     int ideal_icon_size_in_px,
     int minimum_icon_size_in_px,
     int badge_size_in_px,
     bool check_webapk_compatibility) {
   InstallableParams params;
   params.ideal_primary_icon_size_in_px = ideal_icon_size_in_px;
   params.minimum_primary_icon_size_in_px = minimum_icon_size_in_px;
-  params.check_installable = check_webapk_compatibility;
   params.fetch_valid_primary_icon = true;
   if (check_webapk_compatibility) {
+    params.fetch_valid_badge_icon = true;
     params.ideal_badge_icon_size_in_px = badge_size_in_px;
     params.minimum_badge_icon_size_in_px = badge_size_in_px;
-    params.fetch_valid_badge_icon = true;
   }
   return params;
 }
 
+InstallableParams ParamsToPerformInstallableCheck(
+    bool check_webapk_compatibility) {
+  InstallableParams params;
+  params.check_installable = check_webapk_compatibility;
+  return params;
+}
+
 }  // namespace
 
 AddToHomescreenDataFetcher::AddToHomescreenDataFetcher(
     content::WebContents* web_contents,
     int ideal_icon_size_in_px,
     int minimum_icon_size_in_px,
     int ideal_splash_image_size_in_px,
     int minimum_splash_image_size_in_px,
     int badge_size_in_px,
     bool check_webapk_compatibility,
     Observer* observer)
     : content::WebContentsObserver(web_contents),
+      installable_manager_(InstallableManager::FromWebContents(web_contents)),
       weak_observer_(observer),
       shortcut_info_(GetShortcutUrl(web_contents->GetBrowserContext(),
                                     web_contents->GetLastCommittedURL())),
       ideal_icon_size_in_px_(ideal_icon_size_in_px),
       minimum_icon_size_in_px_(minimum_icon_size_in_px),
       ideal_splash_image_size_in_px_(ideal_splash_image_size_in_px),
       minimum_splash_image_size_in_px_(minimum_splash_image_size_in_px),
       badge_size_in_px_(badge_size_in_px),
       check_webapk_compatibility_(check_webapk_compatibility),
       is_waiting_for_web_application_info_(true),
@@ skipping 42 matching lines @@
     case WebApplicationInfo::MOBILE_CAPABLE_APPLE:
       base::RecordAction(
           base::UserMetricsAction("webapps.AddShortcut.AppShortcutApple"));
       break;
     case WebApplicationInfo::MOBILE_CAPABLE_UNSPECIFIED:
       base::RecordAction(
           base::UserMetricsAction("webapps.AddShortcut.Bookmark"));
       break;
   }
 
-  InstallableManager::CreateForWebContents(web_contents());
-  InstallableManager* manager =
-      InstallableManager::FromWebContents(web_contents());
-  DCHECK(manager);
-
   // Kick off a timeout for downloading data. If we haven't finished within the
   // timeout, fall back to using a dynamically-generated launcher icon.
   data_timeout_timer_.Start(
       FROM_HERE, base::TimeDelta::FromMilliseconds(kDataTimeoutInMilliseconds),
       base::Bind(&AddToHomescreenDataFetcher::OnDataTimedout, this));
 
-  manager->GetData(
-      ParamsToPerformInstallableCheck(ideal_icon_size_in_px_,
-                                      minimum_icon_size_in_px_,
-                                      badge_size_in_px_,
-                                      check_webapk_compatibility_),
-      base::Bind(&AddToHomescreenDataFetcher::OnDidPerformInstallableCheck,
-                 this));
+  installable_manager_->GetData(
+      ParamsToPerformManifestAndIconFetch(
+          ideal_icon_size_in_px_, minimum_icon_size_in_px_, badge_size_in_px_,
+          check_webapk_compatibility_),
+      base::Bind(&AddToHomescreenDataFetcher::OnDidGetManifestAndIcons, this));
 }
 
 AddToHomescreenDataFetcher::~AddToHomescreenDataFetcher() {
   DCHECK(!weak_observer_);
 }
 
 bool AddToHomescreenDataFetcher::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* sender) {
   if (!is_waiting_for_web_application_info_)
@@ skipping 15 matching lines @@
     return;
 
   if (!is_installable_check_complete_) {
     is_installable_check_complete_ = true;
     if (check_webapk_compatibility_)
       weak_observer_->OnDidDetermineWebApkCompatibility(false);
     weak_observer_->OnUserTitleAvailable(shortcut_info_.user_title);
   }
 
   badge_icon_.reset();
-  CreateLauncherIcon(SkBitmap());
+  CreateLauncherIcon(raw_primary_icon_);
 }
 
-void AddToHomescreenDataFetcher::OnDidPerformInstallableCheck(
+void AddToHomescreenDataFetcher::OnDidGetManifestAndIcons(
     const InstallableData& data) {
-  data_timeout_timer_.Stop();
-  badge_icon_.reset();
-
   if (!web_contents() || !weak_observer_ || is_installable_check_complete_)
     return;
 
-  is_installable_check_complete_ = true;
-
-  bool webapk_compatible = false;
-  if (check_webapk_compatibility_) {
-    webapk_compatible = (data.error_code == NO_ERROR_DETECTED &&
-                         AreWebManifestUrlsWebApkCompatible(data.manifest));
-    weak_observer_->OnDidDetermineWebApkCompatibility(webapk_compatible);
-
-    if (webapk_compatible) {
-      // WebAPKs are wholly defined by the Web Manifest. Ignore the <meta> tag
-      // data received in OnDidGetWebApplicationInfo().
-      shortcut_info_ = ShortcutInfo(GURL());
-    }
-  }
-
   if (!data.manifest.IsEmpty()) {
     base::RecordAction(base::UserMetricsAction("webapps.AddShortcut.Manifest"));
     shortcut_info_.UpdateFromManifest(data.manifest);
     shortcut_info_.manifest_url = data.manifest_url;
+  }
 
-    if (webapk_compatible) {
-      shortcut_info_.UpdateSource(ShortcutInfo::SOURCE_ADD_TO_HOMESCREEN_PWA);
+  // Do this after updating from the manifest for the case where a site has
+  // a manifest with name and standalone specified, but no icons.
+  if (data.manifest.IsEmpty() || !data.primary_icon) {
+    FetchFavicon();
+    return;
+  }
 
-      if (data.badge_icon && !data.badge_icon->drawsNothing()) {
-        shortcut_info_.best_badge_icon_url = data.badge_icon_url;
-        badge_icon_ = *data.badge_icon;
-      }
-    }
-  }
+  raw_primary_icon_ = *data.primary_icon;
+  shortcut_info_.best_primary_icon_url = data.primary_icon_url;
 
   // Save the splash screen URL for the later download.
   shortcut_info_.splash_image_url =
       content::ManifestIconSelector::FindBestMatchingIcon(
           data.manifest.icons, ideal_splash_image_size_in_px_,
           minimum_splash_image_size_in_px_,
           content::Manifest::Icon::IconPurpose::ANY);
   shortcut_info_.ideal_splash_image_size_in_px = ideal_splash_image_size_in_px_;
   shortcut_info_.minimum_splash_image_size_in_px =
       minimum_splash_image_size_in_px_;
+  if (data.badge_icon) {
+    shortcut_info_.best_badge_icon_url = data.badge_icon_url;
+    badge_icon_ = *data.badge_icon;
+  }
+
+  installable_manager_->GetData(
+      ParamsToPerformInstallableCheck(check_webapk_compatibility_),
+      base::Bind(&AddToHomescreenDataFetcher::OnDidPerformInstallableCheck,
+                 this));
+}
+
+void AddToHomescreenDataFetcher::OnDidPerformInstallableCheck(
+    const InstallableData& data) {
+  data_timeout_timer_.Stop();
+  badge_icon_.reset();

[pkotwicz, 2017/06/23 19:23:19]

Shouldn't this be removed?

[dominickn, 2017/06/26 02:39:17]

Good catch, thanks.

+
+  if (!web_contents() || !weak_observer_ || is_installable_check_complete_)
+    return;
+
+  is_installable_check_complete_ = true;
+
+  bool webapk_compatible = false;
+  if (check_webapk_compatibility_) {
+    webapk_compatible = (data.error_code == NO_ERROR_DETECTED &&
+                         AreWebManifestUrlsWebApkCompatible(data.manifest));
+    weak_observer_->OnDidDetermineWebApkCompatibility(webapk_compatible);
+  }
 
   weak_observer_->OnUserTitleAvailable(shortcut_info_.user_title);
-
-  if (data.primary_icon) {
-    shortcut_info_.best_primary_icon_url = data.primary_icon_url;
-
-    if (webapk_compatible)
-      NotifyObserver(*data.primary_icon);
-    else
-      CreateLauncherIcon(*(data.primary_icon));
-    return;
+  if (webapk_compatible) {
+    shortcut_info_.UpdateSource(ShortcutInfo::SOURCE_ADD_TO_HOMESCREEN_PWA);
+    NotifyObserver(raw_primary_icon_);
+  } else {
+    CreateLauncherIcon(raw_primary_icon_);
   }
-
-  FetchFavicon();
 }
 
 void AddToHomescreenDataFetcher::FetchFavicon() {
   if (!web_contents() || !weak_observer_)
     return;
 
   // Grab the best, largest icon we can find to represent this bookmark.
   // TODO(dfalcantara): Try combining with the new BookmarksHandler once its
   //                    rewrite is further along.
   std::vector<int> icon_types{
@@ skipping 10 matching lines @@
   int threshold_to_get_any_largest_icon = ideal_icon_size_in_px_ - 1;
   favicon_service->GetLargestRawFaviconForPageURL(
       shortcut_info_.url, icon_types, threshold_to_get_any_largest_icon,
       base::Bind(&AddToHomescreenDataFetcher::OnFaviconFetched, this),
       &favicon_task_tracker_);
 }
 
 void AddToHomescreenDataFetcher::OnFaviconFetched(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  data_timeout_timer_.Stop();

[pkotwicz, 2017/06/23 19:23:19]

Won't this cause OnUserTitleAvailable() never to be called?

[dominickn, 2017/06/26 02:39:16]

Added an explicit call in OnDidFetchManifestAndIcons. That should be the one
call path where we don't already call OnUserDataAvailable.

Also fixed up the tests so we can test the time out case properly

 
   if (!web_contents() || !weak_observer_ || is_icon_saved_)
     return;
 
   // The user is waiting for the icon to be processed before they can proceed
   // with add to homescreen. But if we shut down, there's no point starting the
   // image processing. Use USER_VISIBLE with MayBlock and SKIP_ON_SHUTDOWN.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE,
       {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(&AddToHomescreenDataFetcher::
                          CreateLauncherIconFromFaviconInBackground,
                      base::Unretained(this), bitmap_result),
       base::BindOnce(&AddToHomescreenDataFetcher::NotifyObserver,
                      base::RetainedRef(this)));
 }
 
 SkBitmap AddToHomescreenDataFetcher::CreateLauncherIconFromFaviconInBackground(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   base::ThreadRestrictions::AssertIOAllowed();
 
-  SkBitmap raw_icon;
   if (bitmap_result.is_valid()) {
     gfx::PNGCodec::Decode(bitmap_result.bitmap_data->front(),
-                          bitmap_result.bitmap_data->size(), &raw_icon);
+                          bitmap_result.bitmap_data->size(),
+                          &raw_primary_icon_);
   }
 
   shortcut_info_.best_primary_icon_url = bitmap_result.icon_url;
-  return CreateLauncherIconInBackground(raw_icon);
+  return CreateLauncherIconInBackground(raw_primary_icon_);
 }
 
-void AddToHomescreenDataFetcher::CreateLauncherIcon(const SkBitmap& raw_icon) {
+void AddToHomescreenDataFetcher::CreateLauncherIcon(const SkBitmap& icon) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // The user is waiting for the icon to be processed before they can proceed
   // with add to homescreen. But if we shut down, there's no point starting the
   // image processing. Use USER_VISIBLE with MayBlock and SKIP_ON_SHUTDOWN.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE,
       {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(
           &AddToHomescreenDataFetcher::CreateLauncherIconInBackground,
-          base::Unretained(this), raw_icon),
+          base::Unretained(this), raw_primary_icon_),
       base::BindOnce(&AddToHomescreenDataFetcher::NotifyObserver,
                      base::RetainedRef(this)));
 }
 
 SkBitmap AddToHomescreenDataFetcher::CreateLauncherIconInBackground(
-    const SkBitmap& raw_icon) {
+    const SkBitmap& icon) {
   base::ThreadRestrictions::AssertIOAllowed();
 
   SkBitmap primary_icon;
   bool is_generated = false;
   if (weak_observer_) {
     primary_icon = weak_observer_->FinalizeLauncherIconInBackground(
-        raw_icon, shortcut_info_.url, &is_generated);
+        icon, shortcut_info_.url, &is_generated);
   }
 
   if (is_generated)
     shortcut_info_.best_primary_icon_url = GURL();
 
   return primary_icon;
 }
 
 void AddToHomescreenDataFetcher::NotifyObserver(const SkBitmap& primary_icon) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!web_contents() || !weak_observer_ || is_icon_saved_)
     return;
 
   is_icon_saved_ = true;
   primary_icon_ = primary_icon;
   weak_observer_->OnDataAvailable(shortcut_info_, primary_icon_, badge_icon_);
 }