Break out manifest icon logic from ShortcutHelper

chrome/browser/android/manifest_icon_selector_unittest.cc

-// Copyright 2014 The Chromium Authors. All rights reserved.
+// 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_helper.h"
+#include "chrome/browser/android/manifest_icon_selector.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/screen.h"
 #include "ui/gfx/screen_type_delegate.h"
 
-// A dummy implementation of gfx::Screen, since ShortcutHelper needs access to
-// a gfx::Display's device scale factor.
+// A dummy implementation of gfx::Screen, since ManifestIconSelector needs
+// access to a gfx::Display's device scale factor.
 // This is inspired by web_contents_video_capture_device_unittest.cc
 // A bug has been opened to merge all those mocks: http://crbug.com/417227
 class FakeScreen : public gfx::Screen {
  public:
   FakeScreen() : display_(0x1337, gfx::Rect(0, 0, 2560, 1440)) {
   }
   virtual ~FakeScreen() {}
 
   void SetDisplayDeviceScaleFactor(float device_scale_factor) {
     display_.set_device_scale_factor(device_scale_factor);
@@ skipping 25 matching lines @@
   }
   virtual void AddObserver(gfx::DisplayObserver* observer) override {}
   virtual void RemoveObserver(gfx::DisplayObserver* observer) override {}
 
  private:
   gfx::Display display_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeScreen);
 };
 
-class ShortcutHelperTest : public ChromeRenderViewHostTestHarness  {
+class ManifestIconSelectorTest : public ChromeRenderViewHostTestHarness  {
  protected:
-  ShortcutHelperTest() : shortcut_helper_(NULL) {}
-  virtual ~ShortcutHelperTest() {}
+  ManifestIconSelectorTest() {}
+  virtual ~ManifestIconSelectorTest() {}

[mlamouri (slow - plz ping), 2015/01/30 22:32:13]

I think you should use override instead of virtual.

[gone, 2015/01/30 22:43:30]

Fixed your usage of virtual ~FakeScreen too.

 
-  static jobject CreateShortcutHelperJava(JNIEnv* env) {
-    jclass clazz = env->FindClass("org/chromium/chrome/browser/ShortcutHelper");
-    jmethodID constructor =
-        env->GetMethodID(clazz, "<init>",
-                         "(Landroid/content/Context;"
-                             "Lorg/chromium/chrome/browser/Tab;)V");
-    return env->NewObject(clazz, constructor, jobject(), jobject());
-  }
-
-  void ResetShorcutHelper() {
-    if (shortcut_helper_)
-      delete shortcut_helper_;
-
-    JNIEnv* env = base::android::AttachCurrentThread();
-    shortcut_helper_ =
-        new ShortcutHelper(env, CreateShortcutHelperJava(env), web_contents());
+  void ResetManifestIconSelector() {
+    manifest_icon_selector_.reset(new ManifestIconSelector(
+      web_contents(),
+      GetPreferredIconSizeInDp()));

[mlamouri (slow - plz ping), 2015/01/30 22:32:13]

I guess you should update this. It probably doesn't compile anymore.

[gone, 2015/01/30 22:43:30]

Yeah, the joys of juggling multiple CLs.

   }
 
   virtual void SetUp() override {
     gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, &fake_screen_);
     ASSERT_EQ(&fake_screen_, gfx::Screen::GetNativeScreen());
-
     ChromeRenderViewHostTestHarness::SetUp();
-
-    ResetShorcutHelper();
+    ResetManifestIconSelector();
   }
 
   virtual void TearDown() override {
-    delete shortcut_helper_;
-    shortcut_helper_ = NULL;
-
+    manifest_icon_selector_.reset(NULL);

[mlamouri (slow - plz ping), 2015/01/30 22:32:13]

Feel free to s/NULL/nullptr/ in this file.

[gone, 2015/01/30 22:43:30]

Done.

     ChromeRenderViewHostTestHarness::TearDown();
-
     gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, NULL);
   }
 
   GURL FindBestMatchingIcon(const std::vector<content::Manifest::Icon>& icons) {
-    return shortcut_helper_->FindBestMatchingIcon(icons);
+    return manifest_icon_selector_->FindBestMatchingIcon(icons, web_contents());
   }
 
   void SetDisplayDeviceScaleFactor(float device_scale_factor) {
     fake_screen_.SetDisplayDeviceScaleFactor(device_scale_factor);
 
-    ResetShorcutHelper();
+    ResetManifestIconSelector();
   }
 
   static int GetPreferredIconSizeInDp() {
-    return ShortcutHelper::kPreferredIconSizeInDp;
+    return 48;

[mlamouri (slow - plz ping), 2015/01/30 22:32:13]

It might be better to use a const here even if you declare it in the test class.

[gone, 2015/01/30 22:43:30]

Done.

   }
 
   static content::Manifest::Icon CreateIcon(
       const std::string& url,
       const std::string& type,
       double density,
       const std::vector<gfx::Size> sizes) {
     content::Manifest::Icon icon;
     icon.src = GURL(url);
     if (!type.empty())
       icon.type = base::NullableString16(base::UTF8ToUTF16(type), false);
     icon.density = density;
     icon.sizes = sizes;
 
     return icon;
   }
 
  private:
-  ShortcutHelper* shortcut_helper_;
+  scoped_ptr<ManifestIconSelector> manifest_icon_selector_;
   FakeScreen fake_screen_;
 
-  DISALLOW_COPY_AND_ASSIGN(ShortcutHelperTest);
+  DISALLOW_COPY_AND_ASSIGN(ManifestIconSelectorTest);
 };
 
-TEST_F(ShortcutHelperTest, NoIcons) {
+TEST_F(ManifestIconSelectorTest, NoIcons) {
   // No icons should return the empty URL.
   std::vector<content::Manifest::Icon> icons;
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_TRUE(url.is_empty());
 }
 
-TEST_F(ShortcutHelperTest, NoSizes) {
+TEST_F(ManifestIconSelectorTest, NoSizes) {
   // Icon with no sizes are ignored.
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(
       CreateIcon("http://foo.com/icon.png", "", 1.0, std::vector<gfx::Size>()));
 
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_TRUE(url.is_empty());
 }
 
-TEST_F(ShortcutHelperTest, MIMETypeFiltering) {
+TEST_F(ManifestIconSelectorTest, MIMETypeFiltering) {
   // Icons with type specified to a MIME type that isn't a valid image MIME type
   // are ignored.
   std::vector<gfx::Size> sizes;
   sizes.push_back(gfx::Size(10, 10));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(
       CreateIcon("http://foo.com/icon.png", "image/foo_bar", 1.0, sizes));
   icons.push_back(CreateIcon("http://foo.com/icon.png", "image/", 1.0, sizes));
   icons.push_back(CreateIcon("http://foo.com/icon.png", "image/", 1.0, sizes));
@@ skipping 15 matching lines @@
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon.png", url.spec());
 
   icons.clear();
   icons.push_back(
       CreateIcon("http://foo.com/icon.png", "image/jpeg", 1.0, sizes));
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon.png", url.spec());
 }
 
-TEST_F(ShortcutHelperTest, PreferredSizeOfCurrentDensityIsUsedFirst) {
+TEST_F(ManifestIconSelectorTest, PreferredSizeOfCurrentDensityIsUsedFirst) {
   // This test has three icons each are marked with sizes set to the preferred
   // icon size for the associated density.
   std::vector<gfx::Size> sizes_1;
   sizes_1.push_back(gfx::Size(GetPreferredIconSizeInDp(),
                               GetPreferredIconSizeInDp()));
 
   std::vector<gfx::Size> sizes_2;
   sizes_2.push_back(gfx::Size(GetPreferredIconSizeInDp() * 2,
                               GetPreferredIconSizeInDp() * 2));
 
@@ skipping 12 matching lines @@
 
   SetDisplayDeviceScaleFactor(2.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x2.png", url.spec());
 
   SetDisplayDeviceScaleFactor(3.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x3.png", url.spec());
 }
 
-TEST_F(ShortcutHelperTest, PreferredSizeOfDefaultDensityIsUsedSecond) {
+TEST_F(ManifestIconSelectorTest, PreferredSizeOfDefaultDensityIsUsedSecond) {
   // This test has three icons. The first one is of density zero and is marked
   // with three sizes which are the preferred icon size for density 1, 2 and 3.
   // The icon for density 2 and 3 have a size set to 2x2 and 3x3.
   // Regardless of the device scale factor, the icon of density 1 is going to be
   // used because it matches the preferred size.
   std::vector<gfx::Size> sizes_1;
   sizes_1.push_back(gfx::Size(GetPreferredIconSizeInDp(),
                               GetPreferredIconSizeInDp()));
   sizes_1.push_back(gfx::Size(GetPreferredIconSizeInDp() * 2,
                               GetPreferredIconSizeInDp() * 2));
@@ skipping 17 matching lines @@
 
   SetDisplayDeviceScaleFactor(2.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x1.png", url.spec());
 
   SetDisplayDeviceScaleFactor(3.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x1.png", url.spec());
 }
 
-TEST_F(ShortcutHelperTest, DeviceDensityFirst) {
+TEST_F(ManifestIconSelectorTest, DeviceDensityFirst) {
   // If there is no perfect icon but an icon of the current device density is
   // present, it will be picked.
   // This test has three icons each are marked with sizes set to the preferred
   // icon size for the associated density.
   std::vector<gfx::Size> sizes;
   sizes.push_back(gfx::Size(2, 2));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
   icons.push_back(CreateIcon("http://foo.com/icon_x2.png", "", 2.0, sizes));
   icons.push_back(CreateIcon("http://foo.com/icon_x3.png", "", 3.0, sizes));
 
   SetDisplayDeviceScaleFactor(1.0f);
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x1.png", url.spec());
 
   SetDisplayDeviceScaleFactor(2.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x2.png", url.spec());
 
   SetDisplayDeviceScaleFactor(3.0f);
   url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x3.png", url.spec());
 }
 
-TEST_F(ShortcutHelperTest, DeviceDensityFallback) {
+TEST_F(ManifestIconSelectorTest, DeviceDensityFallback) {
   // If there is no perfect icon but and no icon of the current display density,
   // an icon of density 1.0 will be used.
   std::vector<gfx::Size> sizes;
   sizes.push_back(gfx::Size(2, 2));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
   icons.push_back(CreateIcon("http://foo.com/icon_x2.png", "", 2.0, sizes));
 
   SetDisplayDeviceScaleFactor(3.0f);
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_EQ("http://foo.com/icon_x1.png", url.spec());
 }
 
-TEST_F(ShortcutHelperTest, DoNotUseOtherDensities) {
+TEST_F(ManifestIconSelectorTest, DoNotUseOtherDensities) {
   // If there are only icons of densities that are not the current display
   // density or the default density, they are ignored.
   std::vector<gfx::Size> sizes;
   sizes.push_back(gfx::Size(2, 2));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon_x2.png", "", 2.0, sizes));
 
   SetDisplayDeviceScaleFactor(3.0f);
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_TRUE(url.is_empty());
 }
 
-TEST_F(ShortcutHelperTest, NotSquareIconsAreIgnored) {
+TEST_F(ManifestIconSelectorTest, NotSquareIconsAreIgnored) {
   std::vector<gfx::Size> sizes;
   sizes.push_back(gfx::Size(20, 2));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon.png", "", 1.0, sizes));
 
   GURL url = FindBestMatchingIcon(icons);
   EXPECT_TRUE(url.is_empty());
 }
 
-TEST_F(ShortcutHelperTest, ClosestIconToPreferred) {
-  // This test verifies ShortcutHelper::FindBestMatchingIcon by passing
+TEST_F(ManifestIconSelectorTest, ClosestIconToPreferred) {
+  // This test verifies ManifestIconSelector::FindBestMatchingIcon by passing
   // different icon sizes and checking which one is picked.
   // The Device Scale Factor is 1.0 and the preferred icon size is returned by
   // GetPreferredIconSizeInDp().
   int very_small = GetPreferredIconSizeInDp() / 4;
   int small = GetPreferredIconSizeInDp() / 2;
   int bit_small = GetPreferredIconSizeInDp() - 1;
   int bit_big = GetPreferredIconSizeInDp() + 1;
   int big = GetPreferredIconSizeInDp() * 2;
   int very_big = GetPreferredIconSizeInDp() * 4;
 
@@ skipping 95 matching lines @@
 
     std::vector<content::Manifest::Icon> icons;
     icons.push_back(CreateIcon("http://foo.com/icon_no.png", "", 1.0, sizes_1));
     icons.push_back(CreateIcon("http://foo.com/icon.png", "", 1.0, sizes_2));
 
     GURL url = FindBestMatchingIcon(icons);
     EXPECT_EQ("http://foo.com/icon.png", url.spec());
   }
 }
 
-TEST_F(ShortcutHelperTest, UseAnyIfNoPreferredSize) {
+TEST_F(ManifestIconSelectorTest, UseAnyIfNoPreferredSize) {
   // 'any' (ie. gfx::Size(0,0)) should be used if there is no icon of a
   // preferred size. An icon with the current device scale factor is preferred
   // over one with the default density.
 
   // 'any' with preferred size => preferred size
   {
     std::vector<gfx::Size> sizes_1;
     sizes_1.push_back(gfx::Size(GetPreferredIconSizeInDp(),
                                 GetPreferredIconSizeInDp()));
     std::vector<gfx::Size> sizes_2;
@@ skipping 30 matching lines @@
 
     std::vector<content::Manifest::Icon> icons;
     icons.push_back(CreateIcon("http://foo.com/icon_no.png", "", 1.0, sizes));
     icons.push_back(CreateIcon("http://foo.com/icon.png", "", 3.0, sizes));
 
     SetDisplayDeviceScaleFactor(3.0f);
     GURL url = FindBestMatchingIcon(icons);
     EXPECT_EQ("http://foo.com/icon.png", url.spec());
   }
 }