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..44e62e0ab38b1459c9465f6fa4198aa96a957add
--- /dev/null
+++ b/chrome/browser/extensions/extension_action_manager_unittest.cc
@@ -0,0 +1,233 @@
+// 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 "base/strings/string_number_conversions.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 {
+
+namespace {
+
+const char kBrowserAction[] = "browser_action";
+const char kPageAction[] = "page_action";
+
+}  // namespace
+
+class ExtensionActionManagerTest : public testing::Test {
+ public:
+  ExtensionActionManagerTest();
+
+ protected:
+  // Build an extension, populating |action_type| key with |action|, and
+  // "icons" key with |extension_icons|.
+  scoped_refptr<Extension> BuildExtension(DictionaryBuilder& extension_icons,
+                                          DictionaryBuilder& action,
+                                          const char* action_type);
+
+  // Returns true if |action|'s title matches |extension|'s name.
+  bool TitlesMatch(const Extension& extension, const ExtensionAction& action);
+
+  // Returns true if |action|'s icon for size |action_key| matches
+  // |extension|'s icon for size |extension_key|;
+  bool IconsMatch(const Extension& extension,
+                  int extension_key,
+                  const ExtensionAction& action,
+                  int action_key);
+
+  // Tests that values that are missing from the |action_type| key are properly
+  // populated with values from the other keys in the manifest (e.g.
+  // "default_icon" key of |action_type| is populated with "icons" key).
+  void TestPopulateMissingValues(const char* action_type);
+
+  ExtensionActionManager* manager() { return manager_; }
+
+ private:
+  int curr_id_;
+  ExtensionActionManager* manager_;
+  scoped_ptr<TestingProfile> profile_;
+};
+
+ExtensionActionManagerTest::ExtensionActionManagerTest()
+    : curr_id_(0),
+      profile_(new TestingProfile) {
+  manager_ = ExtensionActionManager::Get(profile_.get());
+}
+
+scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
+    DictionaryBuilder& extension_icons,
+    DictionaryBuilder& action,
+    const char* action_type) {
+  std::string id = base::IntToString(curr_id_++);
+  return ExtensionBuilder()
+      .SetManifest(DictionaryBuilder().Set("version", "1")
+          .Set("manifest_version", 2)
+          .Set("icons", extension_icons)
+          .Set(action_type, action)
+          .Set("name",
+              std::string("Test Extension").append(id)))
+      .SetID(id)
+      .Build();
+}
+
+bool ExtensionActionManagerTest::TitlesMatch(const Extension& extension,
+                                             const ExtensionAction& action) {
+  return action.GetTitle(ExtensionAction::kDefaultTabId) == extension.name();
+}
+
+bool ExtensionActionManagerTest::IconsMatch(const Extension& extension,
+                                            int extension_key,
+                                            const ExtensionAction& action,
+                                            int action_key) {
+  return action.default_icon()->Get(action_key,
+                                    ExtensionIconSet::MATCH_EXACTLY) ==
+      IconsInfo::GetIcons(&extension).Get(extension_key,
+                                          ExtensionIconSet::MATCH_EXACTLY);
+}
+
+void ExtensionActionManagerTest::TestPopulateMissingValues(
+    const char* action_type) {
+  // Determine the appropriate getter method for |action_type|.
+  const base::Callback<ExtensionAction*(const Extension&)> getter = base::Bind(
+      (action_type == kBrowserAction) ?

[not at google - send to devlin, 2014/08/07 16:03:37]

why not just pass ExtensionActionManager::Get{Browser,Page}Action into this
method directly?

[gpdavis, 2014/08/07 18:31:00]

Well, I figured since we already need to pass |action_type| in for setting the
extension manifests, there's no need to have to pass in the callback, since it
can be determined by |action_type|.  No good?

[not at google - send to devlin, 2014/08/07 20:50:18]

ah, fair enough. I didn't notice you needed to pass the action type in to
construct these.

in that case using a callback is a rather roundabout way to do it. perhaps you
can just define a method like GetAction(action_type) and use that rather than
getter.Run().

[gpdavis, 2014/08/07 21:28:18]

Sure, I can dig this idea.

+          &ExtensionActionManager::GetBrowserAction :
+          &ExtensionActionManager::GetPageAction,
+      base::Unretained(manager_));
+
+  // Test that the largest icon from the extension's "icons" key is chosen as a
+  // replacement for missing action default_icons keys.
+  scoped_ptr<DictionaryBuilder> extension_icons(new DictionaryBuilder());
+  extension_icons->Set("48", "icon48.png")
+                 .Set("128", "icon128.png");
+  scoped_refptr<Extension> extension = BuildExtension(
+      extension_icons->Pass(),

[not at google - send to devlin, 2014/08/07 16:03:37]

there are a lot of instances in this test where you can use the builder pattern
more effectively. e.g. here, you can do

scoped_refptr<Extension> extension = BuildExtension(
    DictionaryBuilder().Set("48", "icon48.png")
                       .Set("128", "icon128.png")));

or whatever makes that compile.

same elsewhere. basically the point of DictionaryBuilder is to avoid local
variables as much as possible.

[gpdavis, 2014/08/07 18:31:00]

I was primarily concerned with the readability and formatting of this code,
since nesting DictionaryBuilder constructors can be really messy to format. 
I'll do it that way if you'd prefer, but I like the structure of this function
because it follows a simple pattern: for each test, build the extension_icons
dictionary, build the action dictionary (if applicable), build the extension,
get the action, and make assertions.  I figure that's pretty simple to
understand, but you're right, having to reset those pointers all over the place
isn't exactly necessary.

Your call :)

[not at google - send to devlin, 2014/08/07 20:50:18]

yeah it looks odd to me, for example |extension_icons| being reset every time
with a dictionary builder. most of the time these builders are supposed to be
temporary objects, you might as well just construct using a plain
DictionaryValue instead.

anyway, I do prefer using as few temporary variables as possible.

[gpdavis, 2014/08/07 21:28:18]

Alrighty.  Will rewrite with inline DictionaryBuilders instead of local
variables.

+      DictionaryBuilder().Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  const ExtensionAction* action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_TRUE(TitlesMatch(*extension, *action));
+  ASSERT_TRUE(IconsMatch(*extension, 128, *action, 19));
+  ASSERT_TRUE(IconsMatch(*extension, 128, *action, 38));
+
+  // Test that the action's missing default_icons are not replaced with smaller
+  // icons.
+  extension_icons.reset(new DictionaryBuilder());
+  extension_icons->Set("16", "icon16.png");
+  extension = BuildExtension(
+      extension_icons->Pass(),
+      DictionaryBuilder().Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_FALSE(IconsMatch(*extension, 16, *action, 19));
+  ASSERT_FALSE(IconsMatch(*extension, 16, *action, 38));
+
+  // Test that an action's 38px icon is used as a replacement for the 19px icon
+  // if it is missing. No icons from the "icons" key should be used instead.
+  extension_icons.reset(new DictionaryBuilder());
+  extension_icons->Set("128", "icon128.png");
+  scoped_ptr<DictionaryBuilder> action_manifest(new DictionaryBuilder());
+  action_manifest->Set("default_icon", DictionaryBuilder()
+                             .Set("38", "action38.png"));
+  extension = BuildExtension(
+      extension_icons->Pass(),
+      action_manifest->Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 19));
+  ASSERT_EQ(action->default_icon()->Get(19, ExtensionIconSet::MATCH_EXACTLY),
+            action->default_icon()->Get(38, ExtensionIconSet::MATCH_EXACTLY));
+
+  // Test that existing default_icons and default_title are not replaced.
+  extension_icons.reset(new DictionaryBuilder());
+  extension_icons->Set("128", "icon128.png");
+  action_manifest.reset(new DictionaryBuilder());
+  action_manifest->Set("default_title", "Action!")
+      .Set("default_icon", DictionaryBuilder()
+                             .Set("19", "action19.png")
+                             .Set("38", "action38.png"));
+  extension = BuildExtension(
+      extension_icons->Pass(),
+      action_manifest->Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = getter.Run(*extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_FALSE(TitlesMatch(*extension, *action));
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 19));
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 38));
+}
+
+namespace {
+
+TEST_F(ExtensionActionManagerTest, PopulateBrowserAction) {
+  TestPopulateMissingValues(kBrowserAction);
+}
+
+TEST_F(ExtensionActionManagerTest, PopulatePageAction) {
+  TestPopulateMissingValues(kPageAction);
+}
+
+TEST_F(ExtensionActionManagerTest, GetBestFitActionTest) {
+  // Create an extension with page action defaults.
+  scoped_ptr<DictionaryBuilder> extension_icons(new DictionaryBuilder());
+  extension_icons->Set("48", "icon48.png");
+  scoped_ptr<DictionaryBuilder> action_manifest(new DictionaryBuilder());
+  action_manifest->Set("default_title", "Action!")
+      .Set("default_icon", DictionaryBuilder()
+                             .Set("38", "action38.png"));
+  scoped_refptr<Extension> extension = BuildExtension(extension_icons->Pass(),
+                                                      action_manifest->Pass(),
+                                                      kPageAction);
+  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));

[not at google - send to devlin, 2014/08/07 16:03:37]

it's better to ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) I
think. as in, sure, this assertion is good, but this would also pass if the
title is empty. better to assert exactly what it should be.

[gpdavis, 2014/08/07 18:31:00]

I'm a little confused-- why would this pass if the title is empty?  It should
only pass if both titles are empty.

[not at google - send to devlin, 2014/08/07 20:50:18]

TitlesMatch("Test Extension", "") --> false --> ASSERT_FALSE passes?

[gpdavis, 2014/08/07 21:28:18]

Ah, you said ASSERT_TRUE in the first comment.  That's why I was confused. 
ASSERT_TRUE(TitlesMatch(*extension, /* get page action */)) would be checking to
see that the extension name matches the page action title.  Are you suggesting
we ought to check equality between the returned browser action's title and the
page action's title as per the manifest?  Because that would be something more
along the lines of...

ASSERT_EQ(manager()->GetPageAction(*extension)->GetTitle(ExtensionAction::kDefaultTabId),
          action->GetTitle(ExtensionAction::kDefaultTabId));

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

yep, that's pretty much what I'm suggesting. though the first part of that can
be just "Action!" since you're setting it to "Action!" earlier in the method.

+  ASSERT_FALSE(IconsMatch(*extension, 48, *action, 19));
+  ASSERT_FALSE(IconsMatch(*extension, 48, *action, 38));
+
+  // Create a new extension without page action defaults.
+  extension_icons.reset(new DictionaryBuilder());
+  extension_icons->Set("48", "icon48.png");
+  extension = BuildExtension(
+      extension_icons->Pass(),
+      DictionaryBuilder().Pass(),
+      kPageAction);
+  ASSERT_TRUE(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, 48, *action, 19));
+  ASSERT_TRUE(IconsMatch(*extension, 48, *action, 38));
+}
+
+}  // namespace
+}  // namespace extensions