Cleaned up the code for suppressed messages

chrome/common/extensions/permissions/chrome_permission_message_provider.cc

 // Copyright 2013 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/common/extensions/permissions/chrome_permission_message_provider.h"
 
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/permissions/permission_message.h"
@@ skipping 62 matching lines @@
 
 ChromePermissionMessageProvider::ChromePermissionMessageProvider() {
 }
 
 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()) {
     messages.push_back(PermissionMessage(
         PermissionMessage::kFullAccess,
         l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_FULL_ACCESS)));
     return messages;
   }
 
   PermissionMsgSet host_msgs =
       GetHostPermissionMessages(permissions, extension_type);
   PermissionMsgSet api_msgs = GetAPIPermissionMessages(permissions);
   PermissionMsgSet manifest_permission_msgs =
       GetManifestPermissionMessages(permissions);
   messages.insert(messages.end(), host_msgs.begin(), host_msgs.end());
   messages.insert(messages.end(), api_msgs.begin(), api_msgs.end());
   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;
 }
 
 std::vector<base::string16> ChromePermissionMessageProvider::GetWarningMessages(
     const PermissionSet* permissions,
     Manifest::Type extension_type) const {
   std::vector<base::string16> message_strings;
   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();
     // Access to users' devices should provide a single warning message
     // specifying the transport method used; USB, serial and/or Bluetooth.
     if (id == PermissionMessage::kBluetooth ||
         id == PermissionMessage::kSerial ||
         id == PermissionMessage::kUsb) {
       if (ContainsMessages(messages,
                            PermissionMessage::kBluetooth,
@@ skipping 156 matching lines @@
   PermissionMsgSet messages;
   for (APIPermissionSet::const_iterator permission_it =
            permissions->apis().begin();
        permission_it != permissions->apis().end(); ++permission_it) {
     if (permission_it->HasMessages()) {
       PermissionMessages new_messages = permission_it->GetMessages();
       messages.insert(new_messages.begin(), new_messages.end());
     }
   }
 
-  // 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
   // display only the "<all_urls>" warning message if both permissions
   // are required.
   if (permissions->ShouldWarnAllHosts()) {
     messages.erase(
         PermissionMessage(
             PermissionMessage::kDeclarativeWebRequest, base::string16()));
   }
@@ skipping 116 matching lines @@
   std::set<std::string> old_hosts_set(
       permission_message_util::GetDistinctHosts(old_list, false, false));
   std::set<std::string> new_hosts_only =
       base::STLSetDifference<std::set<std::string> >(new_hosts_set,
                                                      old_hosts_set);
 
   return !new_hosts_only.empty();
 }
 
 }  // namespace extensions