Change tabs + sessions permission warning

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 80 matching lines @@
                   PermissionMessage::kBrowsingHistory,
                   PermissionMessage::kTabs);
   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);

[not at google - send to devlin, 2014/06/09 16:57:31]

honestly all this code and its looping is a bit weird and hard to follow.
extensions are unlikely to have >couple of warnings so this optimisation of
gather together bools to look up later is hard to read.

if there were a method bool ContainsWarningMessage(const
std::vector<base::string16>& messages, PermissionMessage::ID){...} which just
did the iteration each time, it would be much easier to read.

(another approach would be to convert to a std::set but that seems like overkill
given the small size of this list).

I don't think this is a cleanup that needs to happen now.

 
   bool audio_capture = false;
   bool video_capture = false;
   bool media_galleries_read = false;
   bool media_galleries_copy_to = false;
   bool media_galleries_delete = false;
+  bool tabs = false;
+  bool history = false;
   for (PermissionMessages::const_iterator i = messages.begin();
        i != messages.end(); ++i) {
     switch (i->id()) {
       case PermissionMessage::kAudioCapture:
         audio_capture = true;
         break;
       case PermissionMessage::kVideoCapture:
         video_capture = true;
         break;
       case PermissionMessage::kMediaGalleriesAllGalleriesRead:
         media_galleries_read = true;
         break;
       case PermissionMessage::kMediaGalleriesAllGalleriesCopyTo:
         media_galleries_copy_to = true;
         break;
       case PermissionMessage::kMediaGalleriesAllGalleriesDelete:
         media_galleries_delete = true;
         break;
+      case PermissionMessage::kTabs:
+        tabs = true;
+        break;
+      case PermissionMessage::kBrowsingHistory:
+        history = true;
+        break;
       default:
         break;
     }
   }
 
   for (PermissionMessages::const_iterator i = messages.begin();
        i != messages.end(); ++i) {
     int id = i->id();
     if (audio_capture && video_capture) {
       if (id == PermissionMessage::kAudioCapture) {
@@ skipping 12 matching lines @@
             IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE :
             IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_DELETE;
         message_strings.push_back(l10n_util::GetStringUTF16(m_id));
         continue;
       } else if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo ||
                  id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) {
         // The combined message will be pushed above.
         continue;
       }
     }
+    if (tabs && permissions->HasAPIPermission(APIPermission::kSessions) &&
+        id == PermissionMessage::kTabs) {

[not at google - send to devlin, 2014/06/09 16:57:31]

am I missing something or is this check and the
id==PermissionMessage::kBrowsingHistory check below pointless, because if tabs
is true then that implies PermissionMessage::kTabs and likewise is history is
true then that implies PermissionMessage::kBrowsingHistory?

[wjywbs, 2014/06/09 20:06:28]

You are right. A lot of lines are not necessary. This is in a loop of all
permission messages, and we change the message when the |id| is the kTabs
message.

+      message_strings.push_back(l10n_util::GetStringUTF16(
+          IDS_EXTENSION_PROMPT_WARNING_TABS_AND_SESSIONS));
+      continue;
+    }
+    if (history && permissions->HasAPIPermission(APIPermission::kSessions) &&
+        id == PermissionMessage::kBrowsingHistory) {
+      message_strings.push_back(l10n_util::GetStringUTF16(
+          IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY_AND_SESSIONS));
+      continue;
+    }
 
     message_strings.push_back(i->message());
   }
 
   return message_strings;
 }
 
 std::vector<base::string16>
 ChromePermissionMessageProvider::GetWarningMessagesDetails(
     const PermissionSet* permissions,
@@ skipping 181 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