Permission messages: Add a bunch of missing combination/suppression rules.

chrome/common/extensions/permissions/chrome_permission_message_rules.cc

Index: chrome/common/extensions/permissions/chrome_permission_message_rules.cc
diff --git a/chrome/common/extensions/permissions/chrome_permission_message_rules.cc b/chrome/common/extensions/permissions/chrome_permission_message_rules.cc
index 61a0e56a78b49730455cef02ffb92cc98ff57669..8535885373c467199c549e50313589b968e266d7 100644
--- a/chrome/common/extensions/permissions/chrome_permission_message_rules.cc
+++ b/chrome/common/extensions/permissions/chrome_permission_message_rules.cc
@@ -311,6 +311,31 @@ class ChromePermissionMessageRule::PermissionIDSetInitializer
     insert(g);
   }
 
+  PermissionIDSetInitializer(APIPermission::ID a,
+                             APIPermission::ID b,
+                             APIPermission::ID c,
+                             APIPermission::ID d,
+                             APIPermission::ID e,
+                             APIPermission::ID f,
+                             APIPermission::ID g,
+                             APIPermission::ID h)
+      : PermissionIDSetInitializer(a, b, c, d, e, f, g) {
+    insert(h);
+  }
+
+  PermissionIDSetInitializer(APIPermission::ID a,

[Devlin, 2015/09/01 16:08:51]

This is getting a little nuts here.  Looking briefly at our style guide, I don't
see anything banning variadic functions (did I miss it?).  Using one variadic
function would make this muuuuuch cleaner...

[Marc Treib, 2015/09/01 16:25:50]

Hm, seems you're right, I also can't find anything about them in the style
guide. OTOH, I haven't seen a variadic function used in Chrome (or anywhere
else, really) except for printf-style functions. I also don't think I've ever
written one.
I actually think the current mass of ctors is easier to parse than a variadic
function (if only because those are so uncommon). I guess the "correct" solution
would be an std::initializer_list, but those aren't allowed yet. A variadic
template might also work, but will also have readability issues.

Would it be acceptable for you to just put in something like
// TODO(treib): Replace all these ctors by a single initializer_list version
when we have those.
for now?

[Devlin, 2015/09/01 16:33:43]

Eh, variadic templates aren't *that* hard to parse, especially when the argument
list is all one type.  And another approach, which would be just as good, is to
pass in a std::set or std::vector of ids which can be brace-initialized, so that
you can still have one constructor, and do PermissionIdSetInitializer({id1, id2,
...idn});, but I think that vector brace initialization is still verboten as
part of uniform initialization (do you know for sure?).

But, in any case, this is mostly just an eyesore, not a safety or performance
concern, so as long as I stop looking at it, we'll be okay. :)

[Marc Treib, 2015/09/02 10:43:55]

Yup, the uniform initialization of set/vector is based on std::initializer_list
which we don't have yet.

But, indeed, variadic templates are less weird-looking than I would've thought.
So I'm now the proud author of my very first variadic template :)

+                             APIPermission::ID b,
+                             APIPermission::ID c,
+                             APIPermission::ID d,
+                             APIPermission::ID e,
+                             APIPermission::ID f,
+                             APIPermission::ID g,
+                             APIPermission::ID h,
+                             APIPermission::ID i)
+      : PermissionIDSetInitializer(a, b, c, d, e, f, g, h) {
+    insert(i);
+  }
+
   virtual ~PermissionIDSetInitializer() {}
 };
 
@@ -389,16 +414,17 @@ ChromePermissionMessageRule::GetAllRules() {
       {IDS_EXTENSION_PROMPT_WARNING_DEBUGGER, {APIPermission::kDebugger}, {}},
       {IDS_EXTENSION_PROMPT_WARNING_FULL_ACCESS,
        {APIPermission::kPlugin},
-       // TODO(treib): Add the other IDs implied by kFullAccess/kHostsAll.
-       {APIPermission::kFullAccess, APIPermission::kHostsAll,
-        APIPermission::kHostsAllReadOnly, APIPermission::kDeclarativeWebRequest,
-        APIPermission::kTopSites, APIPermission::kTab}},
+       {APIPermission::kDeclarativeWebRequest, APIPermission::kFavicon,
+        APIPermission::kFullAccess, APIPermission::kHostsAll,
+        APIPermission::kHostsAllReadOnly, APIPermission::kProcesses,
+        APIPermission::kTab, APIPermission::kTopSites,
+        APIPermission::kWebNavigation}},
       {IDS_EXTENSION_PROMPT_WARNING_FULL_ACCESS,
        {APIPermission::kFullAccess},
-       // TODO(treib): Add the other IDs implied by kHostsAll.
-       {APIPermission::kHostsAll, APIPermission::kHostsAllReadOnly,
-        APIPermission::kDeclarativeWebRequest, APIPermission::kTopSites,
-        APIPermission::kTab}},
+       {APIPermission::kDeclarativeWebRequest, APIPermission::kFavicon,
+        APIPermission::kHostsAll, APIPermission::kHostsAllReadOnly,
+        APIPermission::kProcesses, APIPermission::kTab,
+        APIPermission::kTopSites, APIPermission::kWebNavigation}},
 
       // Hosts permission messages.
       // Full host access already allows DeclarativeWebRequest, reading the list
@@ -409,17 +435,23 @@ ChromePermissionMessageRule::GetAllRules() {
       // message if both permissions are required.
       {IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS,
        {APIPermission::kHostsAll},
-       // TODO(treib): Add kHostReadWrite and kHostReadOnly.
        {APIPermission::kDeclarativeWebRequest, APIPermission::kFavicon,
-        APIPermission::kHostsAllReadOnly, APIPermission::kProcesses,
+        APIPermission::kHostsAllReadOnly, APIPermission::kHostReadOnly,
+        APIPermission::kHostReadWrite, APIPermission::kProcesses,
         APIPermission::kTab, APIPermission::kTopSites,
         APIPermission::kWebNavigation}},
       {IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS_READ_ONLY,
        {APIPermission::kHostsAllReadOnly},
-       // TODO(treib): Add kHostReadOnly.
-       {APIPermission::kFavicon, APIPermission::kProcesses, APIPermission::kTab,
+       {APIPermission::kFavicon, APIPermission::kHostReadOnly,
+        APIPermission::kProcesses, APIPermission::kTab,
         APIPermission::kTopSites, APIPermission::kWebNavigation}},
 
+      {new CommaSeparatedListFormatter(IDS_EXTENSION_PROMPT_WARNING_1_HOST,
+                                       IDS_EXTENSION_PROMPT_WARNING_2_HOSTS,
+                                       IDS_EXTENSION_PROMPT_WARNING_3_HOSTS,
+                                       IDS_EXTENSION_PROMPT_WARNING_HOSTS_LIST),
+       {APIPermission::kHostReadWrite},
+       {}},
       {new CommaSeparatedListFormatter(
            IDS_EXTENSION_PROMPT_WARNING_1_HOST_READ_ONLY,
            IDS_EXTENSION_PROMPT_WARNING_2_HOSTS_READ_ONLY,
@@ -427,41 +459,43 @@ ChromePermissionMessageRule::GetAllRules() {
            IDS_EXTENSION_PROMPT_WARNING_HOSTS_LIST_READ_ONLY),
        {APIPermission::kHostReadOnly},
        {}},
-      {new CommaSeparatedListFormatter(IDS_EXTENSION_PROMPT_WARNING_1_HOST,
-                                       IDS_EXTENSION_PROMPT_WARNING_2_HOSTS,
-                                       IDS_EXTENSION_PROMPT_WARNING_3_HOSTS,
-                                       IDS_EXTENSION_PROMPT_WARNING_HOSTS_LIST),
-       {APIPermission::kHostReadWrite},
-       {}},
 
       // History-related permission messages.
       // History already allows reading favicons, tab access and accessing the
       // list of most frequently visited sites.
       {IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE_AND_SESSIONS,
-       {APIPermission::kSessions, APIPermission::kHistory},
+       {APIPermission::kHistory, APIPermission::kSessions},
        {APIPermission::kFavicon, APIPermission::kProcesses, APIPermission::kTab,
         APIPermission::kTopSites, APIPermission::kWebNavigation}},
-      {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS,
-       {APIPermission::kSessions, APIPermission::kTab},
-       {APIPermission::kFavicon, APIPermission::kProcesses,
-        APIPermission::kTopSites, APIPermission::kWebNavigation}},
       {IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE,
        {APIPermission::kHistory},
        {APIPermission::kFavicon, APIPermission::kProcesses, APIPermission::kTab,
         APIPermission::kTopSites, APIPermission::kWebNavigation}},
+      {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS,
+       {APIPermission::kTab, APIPermission::kSessions},
+       {APIPermission::kFavicon, APIPermission::kProcesses,
+        APIPermission::kTopSites, APIPermission::kWebNavigation}},
+      {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS,
+       {APIPermission::kProcesses, APIPermission::kSessions},

[Devlin, 2015/09/01 16:08:51]

See comment on the other CL - I think I was wrong about these.

[Devlin, 2015/09/01 16:09:53]

Aaaaand, nevermind. :)

+       {APIPermission::kFavicon, APIPermission::kTopSites,
+        APIPermission::kWebNavigation}},
+      {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS,
+       {APIPermission::kWebNavigation, APIPermission::kSessions},
+       {APIPermission::kFavicon, APIPermission::kTopSites}},
       {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ,
        {APIPermission::kTab},
        {APIPermission::kFavicon, APIPermission::kProcesses,
         APIPermission::kTopSites, APIPermission::kWebNavigation}},
-      // TODO(treib): Should we have _AND_SESSIONS versions of these 2 as well?
       {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ,
        {APIPermission::kProcesses},
-       {}},
+       {APIPermission::kFavicon, APIPermission::kTopSites,
+        APIPermission::kWebNavigation}},
       {IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ,
        {APIPermission::kWebNavigation},
-       {}},
+       {APIPermission::kFavicon, APIPermission::kTopSites}},
       {IDS_EXTENSION_PROMPT_WARNING_FAVICON, {APIPermission::kFavicon}, {}},
       {IDS_EXTENSION_PROMPT_WARNING_TOPSITES, {APIPermission::kTopSites}, {}},
+
       {IDS_EXTENSION_PROMPT_WARNING_DECLARATIVE_WEB_REQUEST,
        {APIPermission::kDeclarativeWebRequest},
        {}},
@@ -469,8 +503,8 @@ ChromePermissionMessageRule::GetAllRules() {
       // Messages generated by the sockets permission.
       {IDS_EXTENSION_PROMPT_WARNING_SOCKET_ANY_HOST,
        {APIPermission::kSocketAnyHost},
-       // TODO(treib): Add kSocketDomainHosts and kSocketSpecificHosts.
-       {}},
+       {APIPermission::kSocketDomainHosts,
+        APIPermission::kSocketSpecificHosts}},
       {new SpaceSeparatedListFormatter(
            IDS_EXTENSION_PROMPT_WARNING_SOCKET_HOSTS_IN_DOMAIN,
            IDS_EXTENSION_PROMPT_WARNING_SOCKET_HOSTS_IN_DOMAINS),
@@ -526,21 +560,19 @@ ChromePermissionMessageRule::GetAllRules() {
        {APIPermission::kAccessibilityFeaturesRead},
        {}},
 
-      // TODO(sashab): Add the missing combinations of media galleries
-      // permissions so a valid permission is generated for all combinations.
+      // Media galleries permissions. We don't have strings for every possible
+      // combination, e.g. we don't bother with a special string for "write, but
+      // not read" - just show the "read and write" string instead, etc.
       {IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE_DELETE,
        {APIPermission::kMediaGalleriesAllGalleriesCopyTo,
-        APIPermission::kMediaGalleriesAllGalleriesDelete,
-        APIPermission::kMediaGalleriesAllGalleriesRead},
-       {}},
+        APIPermission::kMediaGalleriesAllGalleriesDelete},
+       {APIPermission::kMediaGalleriesAllGalleriesRead}},
       {IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE,
-       {APIPermission::kMediaGalleriesAllGalleriesCopyTo,
-        APIPermission::kMediaGalleriesAllGalleriesRead},
-       {}},
+       {APIPermission::kMediaGalleriesAllGalleriesCopyTo},
+       {APIPermission::kMediaGalleriesAllGalleriesRead}},
       {IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_DELETE,
-       {APIPermission::kMediaGalleriesAllGalleriesDelete,
-        APIPermission::kMediaGalleriesAllGalleriesRead},
-       {}},
+       {APIPermission::kMediaGalleriesAllGalleriesDelete},
+       {APIPermission::kMediaGalleriesAllGalleriesRead}},
       {IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ,
        {APIPermission::kMediaGalleriesAllGalleriesRead},
        {}},
@@ -629,7 +661,7 @@ ChromePermissionMessageRule::GetAllRules() {
        {}},
       {IDS_EXTENSION_PROMPT_WARNING_INTERCEPT_ALL_KEYS,
        {APIPermission::kInterceptAllKeys},
-       {}},  // TODO(treib): This should probably suppress kInput.
+       {APIPermission::kInput}},
       {IDS_EXTENSION_PROMPT_WARNING_INPUT, {APIPermission::kInput}, {}},
       {IDS_EXTENSION_PROMPT_WARNING_MANAGEMENT,
        {APIPermission::kManagement},