Fails InstallableManager if a selected badge icon cannot be fetched.

chrome/browser/installable/installable_manager.cc

 // Copyright 2016 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/installable/installable_manager.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/manifest/manifest_icon_downloader.h"
@@ skipping 159 matching lines @@
 
   if (params.check_installable && installable_->error != NO_ERROR_DETECTED)
     return installable_->error;
 
   if (params.fetch_valid_primary_icon) {
     IconProperty& icon = icons_[ParamsForPrimaryIcon(params)];
     if (icon.error != NO_ERROR_DETECTED)
       return icon.error;
   }
 
-  // Do not report badge icon's error because badge icon is optional.
+  if (params.fetch_valid_badge_icon) {
+    IconProperty& icon = icons_[ParamsForBadgeIcon(params)];
+    if (icon.error != NO_ERROR_DETECTED)
+      return icon.error;

[dominickn, 2017/04/05 02:57:24]

Can you edit the following error messages in installable_logging so they're more
generic?

kCannotDownloadIconMessage -> "could not download a required icon from the
manifest"

kNoIconAvailableMessage -> "downloaded manifest icon was empty or corrupted"

[F, 2017/04/06 15:35:35]

Done.

+  }
+
   return NO_ERROR_DETECTED;
 }
 
 InstallableStatusCode InstallableManager::manifest_error() const {
   return manifest_->error;
 }
 
 InstallableStatusCode InstallableManager::installable_error() const {
   return installable_->error;
 }
@@ skipping 107 matching lines @@
     StartNextTask();
     return;
   }
 
   if (!manifest_->fetched) {
     FetchManifest();
   } else if (params.check_installable && !installable_->fetched) {
     CheckInstallable();
   } else if (params.fetch_valid_primary_icon &&
              !IsIconFetched(ParamsForPrimaryIcon(params))) {
-    CheckAndFetchBestIcon(ParamsForPrimaryIcon(params));
+    CheckAndFetchBestIcon(ParamsForPrimaryIcon(params), true);
   } else if (params.fetch_valid_badge_icon &&
              !IsIconFetched(ParamsForBadgeIcon(params))) {
-    CheckAndFetchBestIcon(ParamsForBadgeIcon(params));
+    CheckAndFetchBestIcon(ParamsForBadgeIcon(params), false);
   } else {
     NOTREACHED();
   }
 }
 
 void InstallableManager::FetchManifest() {
   DCHECK(!manifest_->fetched);
 
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
@@ skipping 106 matching lines @@
     case content::ServiceWorkerCapability::NO_SERVICE_WORKER:
       installable_->installable = false;
       installable_->error = NO_MATCHING_SERVICE_WORKER;
       break;
   }
 
   installable_->fetched = true;
   WorkOnTask();
 }
 
-void InstallableManager::CheckAndFetchBestIcon(const IconParams& params) {
+void InstallableManager::CheckAndFetchBestIcon(const IconParams& params,
+                                               bool is_required) {
   DCHECK(!manifest().IsEmpty());
 
   int ideal_icon_size_in_px = std::get<0>(params);
   int minimum_icon_size_in_px = std::get<1>(params);
   IconPurpose icon_purpose = std::get<2>(params);
 
   IconProperty& icon = icons_[params];
   icon.fetched = true;
 
   GURL icon_url = ManifestIconSelector::FindBestMatchingIcon(
       manifest().icons, ideal_icon_size_in_px, minimum_icon_size_in_px,
       icon_purpose);
 
   if (icon_url.is_empty()) {
-    icon.error = NO_ACCEPTABLE_ICON;
+    if (is_required)

[dominickn, 2017/04/05 02:57:24]

I misinterpreted why you did it this way the first time I read this.

Instead of having is_required here, can you amend the conditional in
GetErrorCode() to be:

// If the error is NO_ACCEPTABLE_ICON, there is no icon suitable as a badge in
the
// manifest. Ignore this case since we only want to fail the check if there was
a
// suitable badge icon specified and we couldn't fetch it.
if (icon.error != NO_ERROR_DETECTED && icon.error != NO_ACCEPTABLE_ICON)
  return icon.error;

That way, we don't have the misleading NO_ERROR_DETECTED - we just ignore the
error when it's caused by a suitable badge icon not being present.

[F, 2017/04/06 15:35:35]

Done. This is a much better solution!

+      icon.error = NO_ACCEPTABLE_ICON;
   } else {
     bool can_download_icon = ManifestIconDownloader::Download(
         GetWebContents(), icon_url, ideal_icon_size_in_px,
         minimum_icon_size_in_px,
         base::Bind(&InstallableManager::OnIconFetched,
                    weak_factory_.GetWeakPtr(), icon_url, params));
     if (can_download_icon)
       return;
     icon.error = CANNOT_DOWNLOAD_ICON;
   }
@@ skipping 35 matching lines @@
   return manifest_->url;
 }
 
 const content::Manifest& InstallableManager::manifest() const {
   return manifest_->manifest;
 }
 
 bool InstallableManager::is_installable() const {
   return installable_->installable;
 }