Ensure there are always nice icons for bookmark apps.

chrome/browser/extensions/bookmark_app_helper.cc

 // Copyright 2014 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/extensions/bookmark_app_helper.h"
 
 #include <cctype>
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/crx_installer.h"
@@ skipping 94 matching lines @@
        ++it) {
     WebApplicationInfo::IconInfo icon_info;
     icon_info.data = *it->ToSkBitmap();
     icon_info.width = icon_info.data.width();
     icon_info.height = icon_info.data.height();
     web_app_info.icons.push_back(icon_info);
   }
   callback.Run(web_app_info);
 }
 
+std::set<int> SizesToGenerate() {
+  // Generate container icons from smaller icons.
+  const int kIconSizesToGenerate[] = {
+      extension_misc::EXTENSION_ICON_SMALL,
+      extension_misc::EXTENSION_ICON_MEDIUM,
+  };
+  return std::set<int>(kIconSizesToGenerate,
+                       kIconSizesToGenerate + arraysize(kIconSizesToGenerate));
+}
+
+void GenerateIcons(std::set<int> generate_sizes,
+                   const GURL& app_url,
+                   SkColor generated_icon_color,
+                   std::map<int, SkBitmap>* bitmap_map) {
+  // The letter that will be painted on the generated icon.
+  char icon_letter = ' ';
+  std::string domain_and_registry(
+      net::registry_controlled_domains::GetDomainAndRegistry(
+          app_url,
+          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
+  if (!domain_and_registry.empty()) {
+    icon_letter = domain_and_registry[0];
+  } else if (!app_url.host().empty()) {
+    icon_letter = app_url.host()[0];
+  }
+
+  // If no color has been specified, use a dark gray so it will stand out on the
+  // black shelf.
+  if (generated_icon_color == SK_ColorTRANSPARENT)
+    generated_icon_color = SK_ColorDKGRAY;
+
+  for (std::set<int>::const_iterator it = generate_sizes.begin();
+       it != generate_sizes.end(); ++it) {
+    extensions::BookmarkAppHelper::GenerateIcon(
+        bitmap_map, *it, generated_icon_color, icon_letter);
+    // Also generate the 2x resource for this size.
+    extensions::BookmarkAppHelper::GenerateIcon(
+        bitmap_map, *it * 2, generated_icon_color, icon_letter);
+  }
+}
+
+void ReplaceWebAppIcons(std::map<int, SkBitmap> bitmap_map,
+                        WebApplicationInfo* web_app_info) {
+  web_app_info->icons.clear();
+
+  // Populate the icon data into the WebApplicationInfo we are using to
+  // install the bookmark app.
+  for (std::map<int, SkBitmap>::const_iterator bitmap_map_it =
+           bitmap_map.begin();
+       bitmap_map_it != bitmap_map.end(); ++bitmap_map_it) {
+    WebApplicationInfo::IconInfo icon_info;
+    icon_info.data = bitmap_map_it->second;
+    icon_info.width = icon_info.data.width();
+    icon_info.height = icon_info.data.height();
+    web_app_info->icons.push_back(icon_info);
+  }
+}
+
 }  // namespace
 
 namespace extensions {
 
 // static
 void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
     const content::Manifest& manifest,
     WebApplicationInfo* web_app_info) {
   if (!manifest.short_name.is_null())
     web_app_info->title = manifest.short_name.string();
@@ skipping 128 matching lines @@
     bool success,
     const std::map<GURL, std::vector<SkBitmap> >& bitmaps) {
   // The tab has navigated away during the icon download. Cancel the bookmark
   // app creation.
   if (!success) {
     favicon_downloader_.reset();
     callback_.Run(NULL, web_app_info_);
     return;
   }
 
-  // Add the downloaded icons. Extensions only allow certain icon sizes. First
-  // populate icons that match the allowed sizes exactly and then downscale
-  // remaining icons to the closest allowed size that doesn't yet have an icon.
-  std::set<int> allowed_sizes(extension_misc::kExtensionIconSizes,
-                              extension_misc::kExtensionIconSizes +
-                                  extension_misc::kNumExtensionIconSizes);
   std::vector<SkBitmap> downloaded_icons;
   for (FaviconDownloader::FaviconMap::const_iterator map_it = bitmaps.begin();
        map_it != bitmaps.end();
        ++map_it) {
     for (std::vector<SkBitmap>::const_iterator bitmap_it =
              map_it->second.begin();
          bitmap_it != map_it->second.end();
          ++bitmap_it) {
       if (bitmap_it->empty() || bitmap_it->width() != bitmap_it->height())
         continue;
 
       downloaded_icons.push_back(*bitmap_it);
     }
   }
 
   // Add all existing icons from WebApplicationInfo.
   for (std::vector<WebApplicationInfo::IconInfo>::const_iterator it =
            web_app_info_.icons.begin();
        it != web_app_info_.icons.end();
        ++it) {
     const SkBitmap& icon = it->data;
     if (!icon.drawsNothing() && icon.width() == icon.height())
       downloaded_icons.push_back(icon);
   }
 
-  web_app_info_.icons.clear();
+  // Add the downloaded icons. Extensions only allow certain icon sizes. First
+  // populate icons that match the allowed sizes exactly and then downscale
+  // remaining icons to the closest allowed size that doesn't yet have an icon.
+  std::set<int> allowed_sizes(extension_misc::kExtensionIconSizes,
+                              extension_misc::kExtensionIconSizes +
+                                  extension_misc::kNumExtensionIconSizes);
 
   // If there are icons that don't match the accepted icon sizes, find the
   // closest bigger icon to the accepted sizes and resize the icon to it. An
   // icon will be resized and used for at most one size.
   std::map<int, SkBitmap> resized_bitmaps(
-      ConstrainBitmapsToSizes(downloaded_icons, allowed_sizes));
+      extensions::BookmarkAppHelper::ConstrainBitmapsToSizes(downloaded_icons,
+                                                             allowed_sizes));

[calamity, 2014/12/09 03:46:11]

Hmm. Technically, this is now unnecessary work. You can probably get away with
just using the downloaded_icons vector. Maybe you'd want to sort it by size or
by color_util::CalculateBoringScore(). You can use lambdas to do this sort now!

[benwells, 2014/12/09 08:28:35]

OK. I'm just going to use the first one we find, I don't think we need to sort
it as the order should be deterministic (although fairly meaningless).

 
-  // Generate container icons from smaller icons.
-  const int kIconSizesToGenerate[] = {extension_misc::EXTENSION_ICON_SMALL,
-                                      extension_misc::EXTENSION_ICON_MEDIUM, };
-  const std::set<int> generate_sizes(
-      kIconSizesToGenerate,
-      kIconSizesToGenerate + arraysize(kIconSizesToGenerate));
-
-  // Only generate icons if larger icons don't exist. This means the app
-  // launcher and the taskbar will do their best downsizing large icons and
-  // these icons are only generated as a last resort against upscaling a smaller
-  // icon.
-  if (resized_bitmaps.lower_bound(*generate_sizes.rbegin()) ==
-      resized_bitmaps.end()) {
-    GURL app_url = web_app_info_.app_url;
-
-    // The letter that will be painted on the generated icon.
-    char icon_letter = ' ';
-    std::string domain_and_registry(
-        net::registry_controlled_domains::GetDomainAndRegistry(
-            app_url,
-            net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
-    if (!domain_and_registry.empty()) {
-      icon_letter = domain_and_registry[0];
-    } else if (!app_url.host().empty()) {
-      icon_letter = app_url.host()[0];
-    }
-
-    // The color that will be used for the icon's background.
-    SkColor background_color = SK_ColorBLACK;
-    if (resized_bitmaps.size()) {
-      color_utils::GridSampler sampler;
-      background_color = color_utils::CalculateKMeanColorOfBitmap(
-          resized_bitmaps.begin()->second);
-    }
-
-    for (std::set<int>::const_iterator it = generate_sizes.begin();
-         it != generate_sizes.end();
-         ++it) {
-      GenerateIcon(&resized_bitmaps, *it, background_color, icon_letter);
-      // Also generate the 2x resource for this size.
-      GenerateIcon(&resized_bitmaps, *it * 2, background_color, icon_letter);
-    }
+  web_app_info_.generated_icon_color = SK_ColorTRANSPARENT;
+  // Determine the color that will be used for the icon's background/

[calamity, 2014/12/09 03:46:11]

s/d\//./

[benwells, 2014/12/09 08:28:35]

Done.

+  if (resized_bitmaps.size()) {
+    color_utils::GridSampler sampler;
+    web_app_info_.generated_icon_color =
+        color_utils::CalculateKMeanColorOfBitmap(
+            resized_bitmaps.begin()->second);
   }
 
-  // Populate the icon data into the WebApplicationInfo we are using to
-  // install the bookmark app.
-  for (std::map<int, SkBitmap>::const_iterator resized_bitmaps_it =
-           resized_bitmaps.begin();
-       resized_bitmaps_it != resized_bitmaps.end();
-       ++resized_bitmaps_it) {
-    WebApplicationInfo::IconInfo icon_info;
-    icon_info.data = resized_bitmaps_it->second;
-    icon_info.width = icon_info.data.width();
-    icon_info.height = icon_info.data.height();
-    web_app_info_.icons.push_back(icon_info);
-  }
+  std::set<int> generate_sizes = SizesToGenerate();
+
+  // Icons are always generated, replacing the icons that were downloaded. This
+  // is done so that the icons are consistent across machines.
+  // TODO(benwells): Use blob sync once it is available to sync the downloaded
+  // icons, and then only generate when there are required sizes missing.
+  resized_bitmaps.clear();
+  GenerateIcons(generate_sizes, web_app_info_.app_url,
+                web_app_info_.generated_icon_color, &resized_bitmaps);
+
+  ReplaceWebAppIcons(resized_bitmaps, &web_app_info_);
 
   // Install the app.
   crx_installer_->InstallWebApp(web_app_info_);
   favicon_downloader_.reset();
 }
 
 void BookmarkAppHelper::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
   switch (type) {
     case extensions::NOTIFICATION_CRX_INSTALLER_DONE: {
       const Extension* extension =
           content::Details<const Extension>(details).ptr();
       DCHECK(extension);
       DCHECK_EQ(AppLaunchInfo::GetLaunchWebURL(extension),
                 web_app_info_.app_url);
       callback_.Run(extension, web_app_info_);
       break;
     }
     case extensions::NOTIFICATION_EXTENSION_INSTALL_ERROR:
       callback_.Run(NULL, web_app_info_);
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
 void CreateOrUpdateBookmarkApp(ExtensionService* service,
                                WebApplicationInfo& web_app_info) {

[calamity, 2014/12/09 03:46:11]

non-const ref? Can this be changed to a pointer?

[benwells, 2014/12/09 08:28:35]

Nice spot, done.

   scoped_refptr<extensions::CrxInstaller> installer(
       extensions::CrxInstaller::CreateSilent(service));
   installer->set_error_on_unsupported_requirements(true);
+  if (web_app_info.icons.empty()) {
+    std::map<int, SkBitmap> bitmap_map;
+    GenerateIcons(SizesToGenerate(), web_app_info.app_url,
+                  web_app_info.generated_icon_color, &bitmap_map);
+    ReplaceWebAppIcons(bitmap_map, &web_app_info);
+  }
+
   installer->InstallWebApp(web_app_info);
 }
 
 void GetWebApplicationInfoFromApp(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,
     const base::Callback<void(const WebApplicationInfo&)> callback) {
   if (!extension->from_bookmark()) {
     callback.Run(WebApplicationInfo());
     return;
@@ skipping 23 matching lines @@
       extension, info_list, base::Bind(&OnIconsLoaded, web_app_info, callback));
 }
 
 bool IsValidBookmarkAppUrl(const GURL& url) {
   URLPattern origin_only_pattern(Extension::kValidWebExtentSchemes);
   origin_only_pattern.SetMatchAllURLs(true);
   return url.is_valid() && origin_only_pattern.MatchesURL(url);
 }
 
 }  // namespace extensions