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..714c63bf40d79d514e19816fcc107c9f9489cca9
--- /dev/null
+++ b/chrome/browser/extensions/extension_action_manager_unittest.cc
@@ -0,0 +1,155 @@
+// 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/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_service_test_base.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 ExtensionServiceTestBase {
+ public:
+  // Build an extension, populating "name" and "icons" keys for the extension,
+  // and "default_title" and "default_icon" keys for the extension's page
+  // action.
+  scoped_refptr<Extension> BuildExtension(const char* extension_name,
+                                          const char* extension_icon,
+                                          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);
+};
+
+scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
+    const char* extension_name,
+    const char* extension_icon,
+    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("page_action", action);
+  if (extension_name)
+    manifest.Set("name", extension_name);
+  else

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

why this? seems odd, and you never pass in NULL anyway.

[gpdavis, 2014/08/05 18:44:15]

Well, I figured I'd add a default name because it's the only one of the four
keys passed in that's mandatory in the manifest.  But you're right; I always
pass it in, so it's not really necessary.  I just wanted it to be valid if I
passed all NULL values into this method.

+    manifest.Set("name", "Test Extension");
+  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);
+}
+
+namespace {
+
+TEST_F(ExtensionActionManagerTest, PopulateMissingValuesTest) {
+  InitializeEmptyExtensionService();

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

ok let's fix this. there's no good reason for ExtensionActionManager to depend
on an ExtensionService, it can just depend on ExtensionRegistry. replace this:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

with
ExtensionRegistry::Get(profile)->enabled_extensions().Contains(extension_id);

then this test can extend ExtensionsTest not ExtensionServiceTestBase.

[gpdavis, 2014/08/05 18:44:15]

I already changed the code you're referring to, since we wanted to pass in an
extension instead of an ID.  I have it passing in an Extension*, and I just NULL
check it (this is okay, right?)

Can this test be an ExtensionsTest?  The header says to only use this class in
extensions_unittests:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

Also, I need a profile to get the ExtensionActionManager, and ExtensionsTest
provides a browser_context. I got a segfault when I tried to use
Profile::FromBrowserContext().  I also had to case the TestBrowserContext to a
BrowserContext, and it seemed like a recipe for disaster anyway.

Advice?

+  ExtensionActionManager* manager = ExtensionActionManager::Get(profile_.get());
+  ASSERT_TRUE(manager);
+
+  // Create an extension without page action defaults.
+  scoped_refptr<Extension> extension = BuildExtension("Test Extension 1",
+                                                      "test_icon.png",
+                                                      NULL,
+                                                      NULL);
+  ASSERT_TRUE(extension.get());
+  service_->AddExtension(extension.get());
+
+  const ExtensionAction* action = manager->GetPageAction(*extension);

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

and it would be nice to test both page and browser actions here. an idea:

1. Pull this whole test into a function which takes a Callback.
2. This Callback takes an Extension then returns an ExtensionAction.
3. Run that Callback instead of calling manager->GetPageAction.

it would look something like this:

void TestPopulateMissingValues(
    const base::Callback<const ExtensionAction*(const Extension*)>& getter) {
  ... the stuff you already have, then...
  const ExtensionAction* action = getter.Run(extension.get());
  ... etc
}

then you have

TEST_F(ExtensionActionManagerTest, PopulatePageAction) {
  TestPopulateMissingValues(base::Bind(
      &ExtensionActionManager::GetPageAction,
      base::Unretained(manager())));
}

similar for PopulateBrowserAction.

you should add a manager() or action_manager or whatever method as a shortcut.

[gpdavis, 2014/08/05 18:44:15]

I'm having trouble getting the callback to work.  I think it has to do with
reference counting:

https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=73

ExtensionActionManager doesn't appear to support RefCounted, and I'm getting a
compiler error.  Is there a way around this, or am I misunderstanding something?

[not at google - send to devlin, 2014/08/05 18:48:53]

are you using base::Unretained()?

[gpdavis, 2014/08/05 18:50:57]

Yep.  I was doing it just as you said, except that GetPageAction takes a const
Extension&, not an Extension*, so I adjusted the callback accordingly.

[gpdavis, 2014/08/05 19:07:59]

Ah, I got it.  GetPageAction doesn't return a const pointer.  That was the
problem, and the cryptic nature of the compiler errors with these callbacks made
it difficult to determine that that was the problem.

Still need advice on switching to ExtensionsTest.  Can't those only be run from
the extensions/ directory?

[gpdavis, 2014/08/05 19:33:50]

Double nevermind.  Sorry for the repeated comments.

I added a testing profile to this class, and made it a subclass of testing::Test
instead of ExtensionsTest (hope that's okay).  Everything seems to be
functioning properly now.  Have a look and let me know what you think.

+  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 page action defaults.
+  extension = BuildExtension("Test Extension 2",
+                             "Test Action",
+                             "test_icon.png",
+                             "test_action_icon.png");
+  ASSERT_TRUE(extension);
+  service_->AddExtension(extension.get());
+
+  action = manager->GetPageAction(*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));
+}
+
+TEST_F(ExtensionActionManagerTest, GetBestFitActionTest) {
+  InitializeEmptyExtensionService();
+  ExtensionActionManager* manager = ExtensionActionManager::Get(profile_.get());
+  ASSERT_TRUE(manager);
+
+  // Create an extension with page action defaults.
+  scoped_refptr<Extension> extension = BuildExtension("Test Extension",
+                                                      "test_icon.png",
+                                                      "Test Action",
+                                                      "test_action_icon.png");
+  ASSERT_TRUE(extension.get());
+  service_->AddExtension(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));
+
+  // Create a new extension without page action defaults.
+  extension = BuildExtension("Test Extension 2",
+                             "Test Action",
+                             NULL,
+                             NULL);
+  ASSERT_TRUE(extension);
+  service_->AddExtension(extension.get());
+
+  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));
+}
+
+}  // namespace
+}  // namespace extensions