Showing devices transport method in one device

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 26 matching lines @@
                      int suppressing_message,
                      int suppressed_message) {
   typename T::iterator suppressed = FindMessageByID(messages,
                                                     suppressed_message);
   if (suppressed != messages.end() &&
       FindMessageByID(messages, suppressing_message) != messages.end()) {
     messages.erase(suppressed);
   }
 }
 
+template <typename T>
+bool CombineMessage(T& messages, int first_message, int second_message) {

[meacer, 2014/07/03 01:04:11]

This needs a better name. Maybe ContainsMessages?

[mhm, 2014/07/07 21:19:38]

Done.

+  typename T::iterator first = FindMessageByID(messages, first_message);
+  typename T::iterator second = FindMessageByID(messages, second_message);
+  if (first != messages.end() && second != messages.end()) {

[meacer, 2014/07/03 01:04:11]

Cleaner:

return first != messages.end() && second != messages.end();

[mhm, 2014/07/07 21:19:38]

Done.

+    return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 ChromePermissionMessageProvider::ChromePermissionMessageProvider() {
 }
 
 ChromePermissionMessageProvider::~ChromePermissionMessageProvider() {
 }
 
 PermissionMessages ChromePermissionMessageProvider::GetPermissionMessages(
     const PermissionSet* permissions,
@@ skipping 47 matching lines @@
   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);
 
-  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 accessibility_read = false;
-  bool accessibility_write = 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::kAccessibilityFeaturesRead:
-        accessibility_read = true;
-        break;
-      case PermissionMessage::kAccessibilityFeaturesModify:
-        accessibility_write = true;
-        break;
-      default:
-        break;
-    }
+  // Warning for Hid always include warning for USB and/or Bluetooth
+  // access permission.
+  SuppressMessage(messages,
+                  PermissionMessage::kHid,
+                  PermissionMessage::kUsb);
+  SuppressMessage(messages,
+                  PermissionMessage::kBluetooth,
+                  PermissionMessage::kBluetoothDevices);
+  SuppressMessage(messages,
+                  PermissionMessage::kHid,
+                  PermissionMessage::kBluetooth);
+
+  // Replace USB and Bluetooth with HID.
+  if (CombineMessage(
+          messages, PermissionMessage::kUsb, PermissionMessage::kBluetooth)) {

[meacer, 2014/07/03 01:04:11]

As I mentioned on the previous comment on IDS_EXTENSION_PROMPT_WARNING_HID, both
usb and bluetooth can also be non-hid devices.

Actually, hid is the odd case here because it doesn't have it's own hidDevices
list; it uses either usbDevices or bluetoothDevices so it's hard to reason what
the message should be.

Maybe you could ignore "hid" for the time being and combine usb, serial and
bluetooth?

[mhm, 2014/07/07 21:19:38]

Done.

+    messages.push_back(PermissionMessage(
+        PermissionMessage::kHid,
+        l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HID)));
+    messages.erase(FindMessageByID(messages, PermissionMessage::kUsb));
+    messages.erase(FindMessageByID(messages, PermissionMessage::kBluetooth));
   }
 
+  bool media_galleries_read_copy_to =

[meacer, 2014/07/03 01:04:11]

I would just put these inside the if condition where they are used:

if (id == ..GalleriesRead ||
    id == ..GalleriesCopyTo ||
    id == ..GalleriesDelete) {
  ...
  if (CombineMessage(messages, ..Read, ..Delete)) {
    ...
  }
}

[mhm, 2014/07/07 21:19:38]

Done.

+      CombineMessage(messages,
+                     PermissionMessage::kMediaGalleriesAllGalleriesRead,
+                     PermissionMessage::kMediaGalleriesAllGalleriesCopyTo);
+  bool media_galleries_read_delete =

[meacer, 2014/07/03 01:04:11]

Same as media_galleries_copy_to

[mhm, 2014/07/07 21:19:38]

Done.

+      CombineMessage(messages,
+                     PermissionMessage::kMediaGalleriesAllGalleriesRead,
+                     PermissionMessage::kMediaGalleriesAllGalleriesDelete);
   for (PermissionMessages::const_iterator i = messages.begin();
        i != messages.end(); ++i) {
     int id = i->id();
-    if (audio_capture && video_capture) {
+    if ((id == PermissionMessage::kHid || id == PermissionMessage::kSerial) &&
+        CombineMessage(
+            messages, PermissionMessage::kHid, PermissionMessage::kSerial)) {
+      if (id == PermissionMessage::kHid) {
+        message_strings.push_back(
+            l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_DEVICES));
+      }
+      continue;
+    }
+    if ((id == PermissionMessage::kUsb || id == PermissionMessage::kSerial) &&
+        CombineMessage(
+            messages, PermissionMessage::kUsb, PermissionMessage::kSerial)) {
+      if (id == PermissionMessage::kUsb) {

[meacer, 2014/07/03 01:04:11]

This seems cleaner:

if (id == kUsb && ContainsMessages(messages, kUsb, kSerial)) {
  message_strings.push_back(
      l10n_util::GetStringUTF16(
          IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL));
  continue;
}

[mhm, 2014/07/07 21:19:38]

The issue is that we also need to have a continue in the case of:

if (id == Serial && ContainMessages(messages, kUsb, kSerial) {
    continue;
}

+        message_strings.push_back(
+            l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL));
+      }
+      continue;
+    }
+    if ((id == PermissionMessage::kBluetooth ||
+         id == PermissionMessage::kSerial) &&
+        CombineMessage(messages,
+                       PermissionMessage::kBluetooth,
+                       PermissionMessage::kSerial)) {
+      if (id == PermissionMessage::kBluetooth) {

[meacer, 2014/07/03 01:04:11]

Same as above, you could just check for kBluetooth:

if (id == kBluetooth && ContainsMessages(messages, kBluetooth, kSerial)) {
 ...
}

[mhm, 2014/07/07 21:19:39]

Done.

+        message_strings.push_back(l10n_util::GetStringUTF16(
+            IDS_EXTENSION_PROMPT_WARNING_BLUETOOTH_SERIAL));
+      }
+      continue;
+    }
+    if ((id == PermissionMessage::kAccessibilityFeaturesRead ||
+         id == PermissionMessage::kAccessibilityFeaturesModify) &&
+        CombineMessage(messages,
+                       PermissionMessage::kAccessibilityFeaturesRead,
+                       PermissionMessage::kAccessibilityFeaturesModify)) {
+      if (id == PermissionMessage::kAccessibilityFeaturesRead) {
+        message_strings.push_back(l10n_util::GetStringUTF16(
+            IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY));

[meacer, 2014/07/03 01:04:11]

Same:

if (id == Read && CombineMessage(messages, Read, Modify)) {
  ...
}

[mhm, 2014/07/07 21:19:38]

Done.

+      }
+      continue;
+    }
+    if ((id == PermissionMessage::kAudioCapture ||
+         id == PermissionMessage::kVideoCapture) &&
+        CombineMessage(messages,
+                       PermissionMessage::kAudioCapture,
+                       PermissionMessage::kVideoCapture)) {
       if (id == PermissionMessage::kAudioCapture) {

[meacer, 2014/07/03 01:04:11]

Ditto.

[mhm, 2014/07/07 21:19:38]

Done.

         message_strings.push_back(l10n_util::GetStringUTF16(
             IDS_EXTENSION_PROMPT_WARNING_AUDIO_AND_VIDEO_CAPTURE));
-        continue;
-      } else if (id == PermissionMessage::kVideoCapture) {
-        // The combined message will be pushed above.
-        continue;
       }
+      continue;
     }
-    if (accessibility_read && accessibility_write) {
-      if (id == PermissionMessage::kAccessibilityFeaturesRead) {
-        message_strings.push_back(l10n_util::GetStringUTF16(
-            IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY));
-        continue;
-      } else if (id == PermissionMessage::kAccessibilityFeaturesModify) {
-        // The combined message will be pushed above.
-        continue;
+    if ((id == PermissionMessage::kMediaGalleriesAllGalleriesRead ||
+         id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo ||
+         id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) &&
+        (media_galleries_read_delete || media_galleries_read_copy_to)) {
+      if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) {

[meacer, 2014/07/03 01:04:11]

Looks like this is always false, right?

id can only be one of (Read, CopyTo, Delete) in the above if statement.

[meacer, 2014/07/03 01:09:59]

Never mind, my comment is wrong. 

[mhm, 2014/07/07 21:19:39]

Done.

+        if (media_galleries_read_copy_to) {
+          message_strings.push_back(l10n_util::GetStringUTF16(
+              IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE));
+        } else {
+          message_strings.push_back(l10n_util::GetStringUTF16(
+              IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_DELETE));
+        }
       }
-    }
-    if (media_galleries_read &&
-        (media_galleries_copy_to || media_galleries_delete)) {
-      if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) {
-        int m_id = media_galleries_copy_to ?
-            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;
-      }
+      continue;
     }
     if (permissions->HasAPIPermission(APIPermission::kSessions) &&
         id == PermissionMessage::kTabs) {
       message_strings.push_back(l10n_util::GetStringUTF16(
           IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS));
       continue;
     }
     if (permissions->HasAPIPermission(APIPermission::kSessions) &&
         id == PermissionMessage::kBrowsingHistory) {
       message_strings.push_back(l10n_util::GetStringUTF16(
@@ skipping 194 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