Use high resolution icons where possible for streamlined hosted app icons.

chrome/browser/ui/web_applications/hosted_app_tab_helper_unittest.cc

+// Copyright 2013 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/ui/web_applications/hosted_app_tab_helper.h"
+
+#include "base/command_line.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/crx_installer.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/extensions/test_extension_service.h"
+#include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/manifest_handlers/icons_handler.h"
+#include "chrome/common/web_application_info.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/common/favicon_url.h"
+#include "content/public/test/test_renderer_host.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+
+using content::RenderViewHostTester;
+
+namespace {
+
+// Creates valid SkBitmaps of the dimensions found in |sizes| and pushes them
+// into |bitmaps|.
+void CreateTestBitmaps(const std::vector<gfx::Size>& sizes,
+                       std::vector<SkBitmap>* bitmaps) {
+  bitmaps->clear();

[tapted, 2013/11/13 07:44:53]

It's pretty common just to return std::vectors for things like this. I'd
probably do this like

  std::vector<SkBitmap> bitmaps(sizes.size());
  for (size_t i = 0; i < sizes.size(); ++i) {
    SkBitmap& bitmap = bitmaps[i];
    ...
  }
  return bitmaps;

[calamity, 2013/11/15 04:25:50]

Done.

+  for (size_t i = 0; i < sizes.size(); ++i) {
+    bitmaps->push_back(SkBitmap());
+    SkBitmap& bitmap = bitmaps->back();
+    bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                      sizes[i].width(),

[tapted, 2013/11/13 07:44:53]

nit: outdent 1 space (and next line)

[calamity, 2013/11/15 04:25:50]

Done.

+                      sizes[i].height());
+    bitmap.allocPixels();
+    bitmap.eraseColor(SK_ColorRED);
+  }
+}
+
+class TestHostedAppTabHelper : public HostedAppTabHelper{

[tapted, 2013/11/13 07:44:53]

nit: space before {

[calamity, 2013/11/15 04:25:50]

Done.

+ public:
+  explicit TestHostedAppTabHelper(content::WebContents* web_contents)
+      : HostedAppTabHelper(web_contents),
+        id_counter_(0) {
+  }
+  virtual ~TestHostedAppTabHelper() {}
+
+  virtual int DownloadImage(const GURL& url) OVERRIDE {
+    return id_counter_++;
+  }
+
+  int id_counter_;

[tapted, 2013/11/13 07:44:53]

nit: private: before

[calamity, 2013/11/15 04:25:50]

Done.

+};

[tapted, 2013/11/13 07:44:53]

nit: DISALLOW_COPY_AND_ASSIGN(..)

[calamity, 2013/11/15 04:25:50]

Done.

+
+class HostedAppTabHelperTest : public ChromeRenderViewHostTestHarness,
+                               public content::NotificationObserver {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    ChromeRenderViewHostTestHarness::SetUp();
+
+    web_app_info_.app_url = GURL("http://www.google.com");
+    web_app_info_.title = UTF8ToUTF16("Hosted App");

[tapted, 2013/11/13 07:44:53]

nit: I usually see ASCIIToUTF16 for string literals.

[calamity, 2013/11/15 04:25:50]

Done.

+    extension_ = NULL;

[tapted, 2013/11/13 07:44:53]

maybe do this in the constructor with an initializer? uninitialized primitives
make me nervous

[calamity, 2013/11/15 04:25:50]

Done.

+
+    // A real extension service is needed for the CrxInstaller.
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    static_cast<extensions::TestExtensionSystem*>(
+        extensions::ExtensionSystem::Get(profile()))->
+        CreateExtensionService(
+            CommandLine::ForCurrentProcess(),
+            temp_dir_.path(),
+            false);
+
+    HostedAppTabHelper::CreateForWebContents(web_contents());
+
+    // Listen for notifications which indicate the hosted app installation has
+    // finished.
+    registrar_.Add(this,
+                   chrome::NOTIFICATION_CRX_INSTALLER_DONE,
+                   content::Source<extensions::CrxInstaller>(NULL));
+
+    registrar_.Add(this,
+                   chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
+                   content::NotificationService::AllSources());
+  }
+
+

[tapted, 2013/11/13 07:44:53]

nit: extra blank line

[calamity, 2013/11/15 04:25:50]

Done.

+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) {

[tapted, 2013/11/13 07:44:53]

OVERRIDE?, and // Overridden from content::NotificationObserver:

[calamity, 2013/11/15 04:25:50]

Done.

+    if (type == chrome::NOTIFICATION_CRX_INSTALLER_DONE)
+      extension_ = content::Details<const extensions::Extension>(details).ptr();
+
+    base::MessageLoop::current()->Quit();
+  }
+

[tapted, 2013/11/13 07:44:53]

private: from here?

[calamity, 2013/11/15 04:25:50]

Done.

[tapted, 2013/11/17 23:28:50]

optional: members on test harnesses can be protected if you like that better
(just not public ;). Accessors are fine too though.

+  const extensions::Extension* extension_;
+  WebApplicationInfo web_app_info_;
+  content::NotificationRegistrar registrar_;
+  base::ScopedTempDir temp_dir_;
+};

[tapted, 2013/11/13 07:44:53]

nit: DISALLOW_COPY_AND_ASSIGN(..)

[calamity, 2013/11/15 04:25:50]

Done.

+
+}  // namespace
+
+TEST_F(HostedAppTabHelperTest, DownloadWindowIcon) {
+  TestHostedAppTabHelper helper(web_contents());
+  // Make the helper fetch icons immediately.
+  helper.SetDelegate(NULL, true);
+
+  std::vector<content::FaviconURL> favicon_urls;
+  favicon_urls.push_back(
+      content::FaviconURL(GURL("http://www.google.com/favicon.ico"),
+                          content::FaviconURL::FAVICON));
+
+  // Updating the favicon should result in the download being initiated.
+  helper.DidUpdateFaviconURL(0, favicon_urls);
+
+  EXPECT_EQ(1, helper.pending_requests());
+
+  std::vector<gfx::Size> sizes;

[tapted, 2013/11/13 07:44:53]

nit: I'd make a vector of size 1 like

std::vector<gfx::Size> sizes(1, gfx::Size(32, 32));

[calamity, 2013/11/15 04:25:50]

Done.

+  sizes.push_back(gfx::Size(32, 32));
+  std::vector<SkBitmap> bitmaps;
+  CreateTestBitmaps(sizes, &bitmaps);
+  helper.DidDownloadFavicon(0, 200, favicon_urls[0].icon_url, bitmaps, sizes);
+  EXPECT_EQ(0, helper.pending_requests());
+
+  EXPECT_EQ(1, helper.image_family().size());

[tapted, 2013/11/13 07:44:53]

I'm surprised you don't need to make the integer literals explicitly unsigned..

i.e. EXPECT_EQ(1u, helper.image_family().size());

Maybe that's a mac thing

[calamity, 2013/11/15 04:25:50]

Done.

+  EXPECT_EQ(gfx::Size(32, 32), helper.image_family().GetBest(0, 0)->Size());
+}
+
+TEST_F(HostedAppTabHelperTest, CreateHostedApp) {
+  TestHostedAppTabHelper helper(web_contents());
+
+  std::vector<content::FaviconURL> favicon_urls;
+  favicon_urls.push_back(
+      content::FaviconURL(GURL("http://www.google.com/favicon.ico"),
+                          content::FaviconURL::FAVICON));
+
+  // Download should not be initiated because |fetch_icons_immediately| is not
+  // set.
+  helper.DidUpdateFaviconURL(0, favicon_urls);
+  EXPECT_EQ(0, helper.pending_requests());
+
+  // Create hosted app which will initiate download.
+  helper.CreateHostedApp(web_app_info_);
+  EXPECT_EQ(1, helper.pending_requests());
+
+  std::vector<gfx::Size> sizes;
+  sizes.push_back(gfx::Size(32, 32));
+  std::vector<SkBitmap> bitmaps;
+  CreateTestBitmaps(sizes, &bitmaps);
+  helper.DidDownloadFavicon(0, 200, favicon_urls[0].icon_url, bitmaps, sizes);
+
+  // Create and install the hosted app.
+  content::RunMessageLoop();
+
+  EXPECT_TRUE(extension_);
+  EXPECT_TRUE(extension_->from_bookmark());
+
+  const ExtensionIconSet& icons = extensions::IconsInfo::GetIcons(extension_);
+  EXPECT_EQ(1, icons.map().size());
+  EXPECT_FALSE(icons.Get(32, ExtensionIconSet::MATCH_EXACTLY).empty());
+}
+
+TEST_F(HostedAppTabHelperTest, CreateHostedAppBeforeFaviconURLUpdate) {
+  TestHostedAppTabHelper helper(web_contents());
+
+  web_app_info_.icons.push_back(WebApplicationInfo::IconInfo());
+  web_app_info_.icons[0].url = GURL("http://www.google.com/favicon2.ico");
+
+  // Initiate creation of hosted app before favicon URLs are loaded.
+  helper.CreateHostedApp(web_app_info_);
+
+  std::vector<content::FaviconURL> favicon_urls;
+  favicon_urls.push_back(
+      content::FaviconURL(GURL("http://www.google.com/favicon.ico"),

[tapted, 2013/11/13 07:44:53]

maybe include favicon2.ico as well to get coverage of the other branch? And an
invalid URL?

[calamity, 2013/11/15 04:25:50]

Done.

+                          content::FaviconURL::FAVICON));
+  helper.DidUpdateFaviconURL(0, favicon_urls);
+
+  std::vector<gfx::Size> sizes_1;
+  std::vector<SkBitmap> bitmaps_1;
+  sizes_1.push_back(gfx::Size(16, 16));
+  CreateTestBitmaps(sizes_1, &bitmaps_1);
+  helper.DidDownloadFavicon(0, 200, favicon_urls[0].icon_url, bitmaps_1,
+                            sizes_1);
+
+  std::vector<gfx::Size> sizes_2;
+  std::vector<SkBitmap> bitmaps_2;
+  sizes_2.push_back(gfx::Size(32, 32));
+  sizes_2.push_back(gfx::Size(64, 64));
+  CreateTestBitmaps(sizes_2, &bitmaps_2);
+  helper.DidDownloadFavicon(1, 200, web_app_info_.icons[0].url, bitmaps_2,
+                            sizes_2);
+
+  // Create and install the hosted app.
+  content::RunMessageLoop();
+
+  EXPECT_TRUE(extension_);
+  EXPECT_TRUE(extension_->from_bookmark());
+
+  const ExtensionIconSet& icons = extensions::IconsInfo::GetIcons(extension_);
+  EXPECT_EQ(3, icons.map().size());
+  EXPECT_FALSE(icons.Get(16, ExtensionIconSet::MATCH_EXACTLY).empty());
+  EXPECT_FALSE(icons.Get(32, ExtensionIconSet::MATCH_EXACTLY).empty());
+  EXPECT_FALSE(icons.Get(64, ExtensionIconSet::MATCH_EXACTLY).empty());
+}