Improve extension icon prediction

chrome/browser/extensions/extension_action_manager_unittest.cc

Index: chrome/browser/extensions/extension_action_manager_unittest.cc
diff --git a/chrome/browser/extensions/extension_action_manager_unittest.cc b/chrome/browser/extensions/extension_action_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..20fa997a196c00144b67e245019911b5bde33b44
--- /dev/null
+++ b/chrome/browser/extensions/extension_action_manager_unittest.cc
@@ -0,0 +1,178 @@
+// 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/extension_action_manager.h"
+
+#include "chrome/browser/extensions/extension_action.h"
+#include "chrome/test/base/testing_profile.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+class ExtensionActionManagerTest : public testing::Test {
+ public:
+  ExtensionActionManagerTest();

[not at google - send to devlin, 2014/08/05 21:20:41]

everything from here down can be protected: I think.

[gpdavis, 2014/08/05 21:41:46]

Done.

+  // Build an extension, populating "name" and "icons" keys for the extension,
+  // and "default_title" and "default_icon" keys for the extension's
+  // |action_type| key.
+  scoped_refptr<Extension> BuildExtension(const char* extension_name,
+                                          const char* extension_icon,
+                                          const char* action_type,
+                                          const char* action_title,
+                                          const char* action_icon);
+
+  // Returns true if |action|'s title matches |extension|'s name.
+  bool TitlesMatch(const Extension& extension, const ExtensionAction& action);
+
+  // Returns true if |action|'s default icon path matches |extension|'s.
+  bool IconsMatch(const Extension& extension, const ExtensionAction& action);
+
+  // Tests that values that are missing from the |action_type| key are
+  // populated with values from the other keys in the manifest (e.g.
+  // "default_icon" key of |action_type| is populated with "icons" key).
+  // Uses |getter| to retrieve the action.
+  void TestPopulateMissingValues(
+      const base::Callback<ExtensionAction*(const Extension&)>& getter,
+      const char* action_type);
+
+  ExtensionActionManager* manager() { return manager_; }
+
+ private:
+  ExtensionActionManager* manager_;
+  scoped_ptr<TestingProfile> profile_;
+};
+
+ExtensionActionManagerTest::ExtensionActionManagerTest()
+    : profile_(new TestingProfile) {
+  manager_ = ExtensionActionManager::Get(profile_.get());
+}
+
+scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
+    const char* extension_name,
+    const char* extension_icon,
+    const char* action_type,
+    const char* action_title,
+    const char* action_icon) {
+  DictionaryBuilder action;
+  if (action_title)
+    action.Set("default_title", action_title);
+  if (action_icon) {
+    action.Set("default_icon", DictionaryBuilder()
+        .Set("38", action_icon));
+  }
+
+  DictionaryBuilder manifest;
+  manifest.Set("version", "1")
+      .Set("manifest_version", 2)
+      .Set(action_type, action);
+  if (extension_name)
+    manifest.Set("name", extension_name);
+  if (extension_icon) {
+    manifest.Set("icons", DictionaryBuilder()
+        .Set("48", extension_icon));
+  }
+  return ExtensionBuilder().SetManifest(manifest).Build();
+}
+
+bool ExtensionActionManagerTest::TitlesMatch(const Extension& extension,
+                                             const ExtensionAction& action) {
+  return action.GetTitle(ExtensionAction::kDefaultTabId) == extension.name();
+}
+
+bool ExtensionActionManagerTest::IconsMatch(const Extension& extension,
+                                            const ExtensionAction& action) {
+  return action.default_icon()->Get(38, ExtensionIconSet::MATCH_EXACTLY) ==
+      IconsInfo::GetIcons(&extension).Get(48, ExtensionIconSet::MATCH_EXACTLY);
+}
+

[not at google - send to devlin, 2014/08/05 21:20:40]

somewhere in here we should also test having multiple icons, and only icons that
are smaller, and both. that way we won't regress that bug filed ages ago about
low-res icons.

[gpdavis, 2014/08/05 21:41:46]

Currently, I have it grabbing the largest icon it can find in the "icons" key,
and using that to replace any keys in the action's "default_icon" that are
missing.  The only instance in which I can imagine this being a problem is here:

"icons": {
  "16": "icon16.png"
},
"page_action": {
  "default_icon": {
    "19": "icon19.png"
  }
}

In this case, the 38px icon for the page_action would be populated with
icon16.png, when it would be better to use icon19.png.  Should we worry much
about that?

What cases are important to test here?  Something like the above situation?

[not at google - send to devlin, 2014/08/05 21:45:31]

Yep, and we should test that it does pick the largest.
yep that's exactly what we should test - the default icon of the page action
shouldn't be overridden by a smaller icon.

however, even if that default icon wasn't specified it *still* shouldn't be
overridden by that 16px icon, because that's smaller than the page action size.

[gpdavis, 2014/08/06 00:01:40]

So we prefer the default puzzle piece icon to a 16px icon resized up?

+void ExtensionActionManagerTest::TestPopulateMissingValues(
+    const base::Callback<ExtensionAction*(const Extension&)>& getter,
+    const char* action_type) {
+  // Create an extension without action defaults.
+  scoped_refptr<Extension> extension = BuildExtension("Test Extension 1",
+                                                      "test_icon.png",
+                                                      action_type,
+                                                      NULL,
+                                                      NULL);
+  ASSERT_TRUE(extension.get());

[not at google - send to devlin, 2014/08/05 21:20:40]

it's better if you make this assertion inside the BuildExtension method so that
you don't need to keep doing ASSERT_TRUE(extension.get())

[gpdavis, 2014/08/05 21:41:46]

Done.

[gpdavis, 2014/08/06 00:01:40]

Shouldn't have given you a premature done here-- I just didn't anticipate
anything going wrong with this.

I can't do this check within the method because it returns a
scoped_refptr<Extension>, and the macro can return. I get a compiler error about
how there's "no viable conversion from void to
scoped_refptr<extensions::Extension>".

Is there a way around this?

+  const ExtensionAction* action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  // Ensure that |action|'s title and default icon match |extension|'s name and
+  // icon.
+  ASSERT_TRUE(TitlesMatch(*extension, *action));
+  ASSERT_TRUE(IconsMatch(*extension, *action));
+
+  // Create a new extension with action defaults.
+  extension = BuildExtension("Test Extension 2",
+                             "test_icon.png",
+                             action_type,
+                             "Test Action",
+                             "test_action_icon.png");
+  ASSERT_TRUE(extension);
+  action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  // The titles and icons should no longer match since the page action has
+  // explicitly set default values.
+  ASSERT_FALSE(TitlesMatch(*extension, *action));
+  ASSERT_FALSE(IconsMatch(*extension, *action));
+}
+
+namespace {
+
+TEST_F(ExtensionActionManagerTest, PopulatePageAction) {
+  TestPopulateMissingValues(base::Bind(
+      &ExtensionActionManager::GetPageAction,
+      base::Unretained(manager())),
+      "page_action");
+}
+
+TEST_F(ExtensionActionManagerTest, PopulateBrowserAction) {
+  TestPopulateMissingValues(base::Bind(
+      &ExtensionActionManager::GetBrowserAction,
+      base::Unretained(manager())),
+      "browser_action");
+}
+
+
+TEST_F(ExtensionActionManagerTest, GetBestFitActionTest) {
+  // Create an extension with page action defaults.
+  scoped_refptr<Extension> extension = BuildExtension("Test Extension",
+                                                      "test_icon.png",
+                                                      "page_action",
+                                                      "Test Action",
+                                                      "test_action_icon.png");
+  ASSERT_TRUE(extension.get());
+
+  // Get a "best fit" browser action for |extension|.
+  scoped_ptr<ExtensionAction> action =
+      manager()->GetBestFitAction(*extension, ActionInfo::TYPE_BROWSER);
+  ASSERT_TRUE(action.get());
+
+  // |action|'s title and default icon should not match |extension|'s because
+  // the data should be pulled from |extension|'s page action manifest key.
+  ASSERT_FALSE(TitlesMatch(*extension, *action));
+  ASSERT_FALSE(IconsMatch(*extension, *action));
+

[not at google - send to devlin, 2014/08/05 21:20:41]

assert that it matches the page action?

[gpdavis, 2014/08/05 21:41:46]

Is this a documentation suggestion, or are you suggesting that I use
GetPageAction to pull the page action out and compare it to |action|?

+  // Create a new extension without page action defaults.
+  extension = BuildExtension("Test Extension 2",
+                             "test_icon.png",
+                             "page_action",
+                             NULL,
+                             NULL);
+  ASSERT_TRUE(extension);
+  action = manager()->GetBestFitAction(*extension, ActionInfo::TYPE_BROWSER);
+
+  // Now these values match because |extension| does not have page action
+  // defaults.
+  ASSERT_TRUE(TitlesMatch(*extension, *action));
+  ASSERT_TRUE(IconsMatch(*extension, *action));

[not at google - send to devlin, 2014/08/05 21:20:40]

assert that it matches the page action?

+}
+
+}  // namespace
+}  // namespace extensions