Extensions: Switch to new permission message system, part II

chrome/common/extensions/permissions/permission_set_unittest.cc

Index: chrome/common/extensions/permissions/permission_set_unittest.cc
diff --git a/chrome/common/extensions/permissions/permission_set_unittest.cc b/chrome/common/extensions/permissions/permission_set_unittest.cc
index 2be3dda827d221d6128164affd4cfc551342dddf..4b1b5c23b335afab9d6f8f9321290a68403df82b 100644
--- a/chrome/common/extensions/permissions/permission_set_unittest.cc
+++ b/chrome/common/extensions/permissions/permission_set_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -788,6 +789,85 @@ TEST(PermissionsTest, PermissionMessages) {
   }
 }
 
+PermissionIDSet MakePermissionIDSet(APIPermission::ID id1) {

[Devlin, 2015/04/02 21:00:23]

Can you move all of these to the anonymous namespace above?

[Marc Treib, 2015/04/07 11:34:47]

Done.

+  PermissionIDSet set;
+  set.insert(id1);
+  return set;
+}
+
+PermissionIDSet MakePermissionIDSet(APIPermission::ID id1,
+                                    APIPermission::ID id2) {
+  PermissionIDSet set;
+  set.insert(id1);
+  set.insert(id2);
+  return set;
+}
+
+PermissionIDSet MakePermissionIDSet(const APIPermissionSet& permissions) {
+  PermissionIDSet set;
+  for (const APIPermission* permission : permissions)
+    set.insert(permission->id());
+  return set;
+}
+
+std::string LegacyPermissionIDsToString(const PermissionMessageIDs& ids) {
+  if (ids.empty())

[Devlin, 2015/04/02 21:00:23]

nit: JoinString just returns "" if there are no strings, so you don't need the
if (empty) here or below.

[Marc Treib, 2015/04/07 11:34:47]

Okay, removed. We'll get some extra spaces in the brackets this way, but I guess
that doesn't really matter.

+    return "[]";
+  std::vector<std::string> strs;
+  for (const PermissionMessage::ID& id : ids)
+    strs.push_back(base::IntToString(id));
+  return "[ " + JoinString(strs, ", ") + " ]";

[Devlin, 2015/04/02 21:00:23]

nit: please use base::StringPrintf() (as a rule of thumb, any time it needs more
than one '+')

[Marc Treib, 2015/04/07 11:34:47]

Done.

+}
+
+std::string PermissionIDsToString(const PermissionIDSet& ids) {
+  if (ids.empty())
+    return "[]";
+  std::vector<std::string> strs;
+  for (const PermissionID& id : ids)
+    strs.push_back(base::IntToString(id.id()));
+  return "[ " + JoinString(strs, ", ") + " ]";
+}
+
+std::string CoalescedPermissionIDsToString(
+    const CoalescedPermissionMessages& msgs) {
+  std::vector<std::string> strs;
+  for (const CoalescedPermissionMessage& msg : msgs)
+    strs.push_back(PermissionIDsToString(msg.permissions()));
+  return JoinString(strs, " ");
+}
+
+testing::AssertionResult CheckPermissionIDs(

[Devlin, 2015/04/02 21:00:23]

This really only pertains to situations with Hidden/Suppressed permissions,
right? (otherwise we'd need to handle multiple permission messages.)  Should the
name of the function reflect as much?

[Devlin, 2015/04/02 21:00:23]

Add function comments.

[Marc Treib, 2015/04/07 11:34:47]

Done.

[Marc Treib, 2015/04/07 11:34:47]

I've renamed it to PermissionSetProducesMessage. Open for better suggestions :)

+    const PermissionSet* permissions,
+    Manifest::Type extension_type,
+    PermissionMessage::ID expected_legacy_id,
+    const PermissionIDSet& expected_ids) {
+  PermissionMessageIDs legacy_ids =
+      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
+          permissions, extension_type);
+  if (legacy_ids.size() != 1 || expected_legacy_id != legacy_ids[0]) {
+    return testing::AssertionFailure()
+           << "Expected single legacy permission ID " << expected_legacy_id

[Devlin, 2015/04/02 21:00:23]

indentation off? (Also at other returns)

[Marc Treib, 2015/04/07 11:34:47]

This is what "git cl format" produced (weird though it looks), and presubmit
even complains if I format it differently.

[Devlin, 2015/04/07 15:55:56]

:_( Why 8-space indentation?  Grrr.... Well, if git cl format does it.

[Marc Treib, 2015/04/07 16:15:55]

I think there's some special case thingie to line stuff up after the "return"...

+           << " but got " << LegacyPermissionIDsToString(legacy_ids);
+  }
+
+  const PermissionMessageProvider* provider = PermissionMessageProvider::Get();

[Devlin, 2015/04/02 21:00:23]

Cache this above, since you also use it on line 845

[Marc Treib, 2015/04/07 11:34:47]

Done.

+  CoalescedPermissionMessages msgs = provider->GetCoalescedPermissionMessages(
+      provider->GetAllPermissionIDs(permissions, extension_type));
+  if (msgs.size() != 1) {
+    return testing::AssertionFailure()
+           << "Expected single permission message with IDs "
+           << PermissionIDsToString(expected_ids) << " but got " << msgs.size()
+           << " messages: " << CoalescedPermissionIDsToString(msgs);
+  }
+  if (!msgs.front().permissions().Equals(expected_ids)) {
+    return testing::AssertionFailure()
+           << "Expected permission IDs " << PermissionIDsToString(expected_ids)
+           << " but got " << PermissionIDsToString(msgs.front().permissions());
+  }
+
+  return testing::AssertionSuccess();
+}
+
 TEST(PermissionsTest, FileSystemPermissionMessages) {
   APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemWrite);
@@ -795,11 +875,9 @@ TEST(PermissionsTest, FileSystemPermissionMessages) {
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, ManifestPermissionSet(),
                         URLPatternSet(), URLPatternSet()));
-  PermissionMessageIDs ids =
-      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-          permissions.get(), Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, ids.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemDirectory, ids[0]);
+  EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_PLATFORM_APP,
+                                 PermissionMessage::kFileSystemDirectory,
+                                 MakePermissionIDSet(api_permissions)));
 }
 
 // The file system permissions have a special-case hack to show a warning for
@@ -842,11 +920,9 @@ TEST(PermissionsTest, HiddenFileSystemPermissionMessages) {
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, ManifestPermissionSet(),
                         URLPatternSet(), URLPatternSet()));
-  PermissionMessageIDs ids =
-      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-          permissions.get(), Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, ids.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemWriteDirectory, ids[0]);
+  EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_PLATFORM_APP,
+                                 PermissionMessage::kFileSystemWriteDirectory,
+                                 MakePermissionIDSet(api_permissions)));
 }
 
 TEST(PermissionsTest, SuppressedPermissionMessages) {
@@ -860,11 +936,9 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kTabs, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION, PermissionMessage::kTabs,
+        MakePermissionIDSet(APIPermission::kTab, APIPermission::kFavicon)));
   }
   {
     // History warning suppresses favicon warning.
@@ -876,53 +950,51 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kBrowsingHistory, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kBrowsingHistory,
+        MakePermissionIDSet(APIPermission::kHistory, APIPermission::kFavicon)));
   }
   {
     // All sites warning suppresses tabs warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kTab);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kTab);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kHostsAll,
+        MakePermissionIDSet(APIPermission::kHostsAll, APIPermission::kTab)));
   }
   {
     // All sites warning suppresses topSites warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kTopSites);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kTopSites);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(
+        CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                           PermissionMessage::kHostsAll,
+                           MakePermissionIDSet(APIPermission::kHostsAll,
+                                               APIPermission::kTopSites)));
   }
   {
     // All sites warning suppresses declarativeWebRequest warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kDeclarativeWebRequest);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kDeclarativeWebRequest);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kHostsAll,
+        MakePermissionIDSet(APIPermission::kHostsAll,
+                            APIPermission::kDeclarativeWebRequest)));
   }
   {
     // BrowsingHistory warning suppresses all history read/write warnings.
@@ -935,11 +1007,9 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           URLPatternSet(), URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kBrowsingHistory, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                                   PermissionMessage::kBrowsingHistory,
+                                   MakePermissionIDSet(api_permissions)));
   }
   {
     // Tabs warning suppresses all read-only history warnings.
@@ -951,11 +1021,9 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           URLPatternSet(), URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kTabs, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                                   PermissionMessage::kTabs,
+                                   MakePermissionIDSet(api_permissions)));
   }
 }
 
@@ -1718,6 +1786,9 @@ TEST(PermissionsTest, ChromeURLs) {
                         allowed_hosts, URLPatternSet()));
   PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
       permissions.get(), Manifest::TYPE_EXTENSION);
+  PermissionMessageProvider::Get()->GetCoalescedPermissionMessages(
+      PermissionMessageProvider::Get()->GetAllPermissionIDs(
+          permissions.get(), Manifest::TYPE_EXTENSION));
 }
 
 TEST(PermissionsTest, IsPrivilegeIncrease_DeclarativeWebRequest) {