Cleaned up the code for suppressed messages

chrome/common/extensions/permissions/chrome_permission_message_provider.cc

Index: chrome/common/extensions/permissions/chrome_permission_message_provider.cc
diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc
index 3e8c56f21d48f4cff1d1d97d32b818309ccf2bf8..341895e027ec0869f221b440b6632a8930f51186 100644
--- a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc
+++ b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc
@@ -80,6 +80,23 @@ ChromePermissionMessageProvider::~ChromePermissionMessageProvider() {
 PermissionMessages ChromePermissionMessageProvider::GetPermissionMessages(
     const PermissionSet* permissions,
     Manifest::Type extension_type) const {
+  const std::multimap<PermissionMessage::ID,
+                      PermissionMessage::ID> SuppressList = {

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

should be kSuppressList I suppose?

[mhm, 2014/07/18 21:29:12]

Done.

+      // Some warnings are more generic and/or powerful and superseed other

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

this comment applies to the whole variable right, not to the Bookmarks
suppression specifically? could you move it out... and actually describe what
the mapping means? does the former suppress the latter, or do they suppress each
other, or...?

[mhm, 2014/07/18 21:29:12]

Done.

+      // warnings. In that case, suppress the superseeded warning.
+      {PermissionMessage::kBookmarks, PermissionMessage::kOverrideBookmarksUI},
+      {PermissionMessage::kTabs, PermissionMessage::kFavicon},
+      {PermissionMessage::kBrowsingHistory, PermissionMessage::kFavicon},
+      {PermissionMessage::kBrowsingHistory, PermissionMessage::kTabs},
+      {PermissionMessage::kHostsAll, PermissionMessage::kTabs},
+      {PermissionMessage::kHostsAll, PermissionMessage::kDeclarativeWebRequest},
+      {PermissionMessage::kBluetooth, PermissionMessage::kBluetoothDevices},
+      // A special hack: If kFileSystemWriteDirectory would be displayed, hide
+      // kFileSystemDirectory as the write directory message implies it.
+      // TODO(sammc): Remove this. See http://crbug.com/284849.
+      {PermissionMessage::kFileSystemWriteDirectory,
+       PermissionMessage::kFileSystemDirectory}};

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

can you sort this list?

[mhm, 2014/07/18 21:29:12]

Done.

+
   PermissionMessages messages;
 
   if (permissions->HasEffectiveFullAccess()) {
@@ -99,32 +116,13 @@ PermissionMessages ChromePermissionMessageProvider::GetPermissionMessages(
   messages.insert(messages.end(), manifest_permission_msgs.begin(),
                   manifest_permission_msgs.end());
 
-  // Some warnings are more generic and/or powerful and superseed other
-  // warnings. In that case, suppress the superseeded warning.
-  SuppressMessage(messages,
-                  PermissionMessage::kBookmarks,
-                  PermissionMessage::kOverrideBookmarksUI);
-  // Both tabs and history already allow reading favicons.

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

but maybe you should retain these comments on that object?

[mhm, 2014/07/18 21:29:12]

Done.

-  SuppressMessage(messages,
-                  PermissionMessage::kTabs,
-                  PermissionMessage::kFavicon);
-  SuppressMessage(messages,
-                  PermissionMessage::kBrowsingHistory,
-                  PermissionMessage::kFavicon);
-  // Warning for history permission already covers warning for tabs permission.
-  SuppressMessage(messages,
-                  PermissionMessage::kBrowsingHistory,
-                  PermissionMessage::kTabs);
-  // Warning for full access permission already covers warning for tabs
-  // permission.
-  SuppressMessage(messages,
-                  PermissionMessage::kHostsAll,
-                  PermissionMessage::kTabs);
-  // Warning for full access already covers warning for DeclarativeWebRequest
-  // permission.
-  SuppressMessage(messages,
-                  PermissionMessage::kHostsAll,
-                  PermissionMessage::kDeclarativeWebRequest);
+  for (std::multimap<PermissionMessage::ID,
+                     PermissionMessage::ID>::const_iterator it =
+           SuppressList.begin();
+       it != SuppressList.end();
+       it++) {
+    SuppressMessage(messages, it->first, it->second);
+  }
 
   return messages;
 }
@@ -136,10 +134,6 @@ std::vector<base::string16> ChromePermissionMessageProvider::GetWarningMessages(
   PermissionMessages messages =
       GetPermissionMessages(permissions, extension_type);
 
-  SuppressMessage(messages,

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

this change isn't obvious, and it doesn't seem related to this CL. what is the
effect?

[mhm, 2014/07/18 21:29:12]

This was added in another CL when showing the messages for devices access (bug
384353).

[not at google - send to devlin, 2014/07/18 21:36:33]

I mean that the other changes you've made are in GetPermissionMessages and are
clearly a no-op logic wise, but this isn't clearly a no-op because it's in a
different function. I don't really know why it's here in the first place though,
seems like all the other SuppressMessage calls are in GetPermissionMessages.

-                  PermissionMessage::kBluetooth,
-                  PermissionMessage::kBluetoothDevices);
-
   for (PermissionMessages::const_iterator i = messages.begin();
        i != messages.end(); ++i) {
     int id = i->id();
@@ -316,12 +310,6 @@ ChromePermissionMessageProvider::GetAPIPermissionMessages(
     }
   }
 
-  // A special hack: If kFileSystemWriteDirectory would be displayed, hide
-  // kFileSystemDirectory as the write directory message implies it.
-  // TODO(sammc): Remove this. See http://crbug.com/284849.
-  SuppressMessage(messages,
-                  PermissionMessage::kFileSystemWriteDirectory,
-                  PermissionMessage::kFileSystemDirectory);
   // A special hack: The warning message for declarativeWebRequest
   // permissions speaks about blocking parts of pages, which is a
   // subset of what the "<all_urls>" access allows. Therefore we