Extensions: Switch to new permission message system, part V

extensions/common/permissions/api_permission.h

 // 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.
 
 #ifndef EXTENSIONS_COMMON_PERMISSIONS_API_PERMISSION_H_
 #define EXTENSIONS_COMMON_PERMISSIONS_API_PERMISSION_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 16 matching lines @@
 class ChromeAPIPermissions;
 
 // APIPermission is for handling some complex permissions. Please refer to
 // extensions::SocketPermission as an example.
 // There is one instance per permission per loaded extension.
 class APIPermission {
  public:
   // The IDs of all permissions available to apps. Add as many permissions here
   // as needed to generate meaningful permission messages. Add the rules for the
   // messages to ChromePermissionMessageProvider.
-  // Remove permissions from this list if they have no longer have a
-  // corresponding API permission and no permission message.
+  // Do not reorder this enumeration or remove any entries. If you need to add a
+  // new entry, add it just prior to kEnumBoundary.
   // TODO(sashab): Move this to a more central location, and rename it to
   // PermissionID.
   enum ID {
     // Error codes.
-    kInvalid = -2,
-    kUnknown = -1,
+    kInvalid,
+    kUnknown,
+
+    // For use in histograms.
+    kNone,

[Marc Treib, 2015/04/17 13:09:33]

Not quite sure about this: The existing histograms record a special "none" value
when an extension requires no permissions, or one value *per permission*
otherwise. So e.g. we can't just add all the numbers to get the total number of
installations. Maybe it'd be better to not have a "none" entry here, and instead
make separate bool histograms ("NeedsPermissions")?

[Devlin, 2015/04/27 17:58:03]

I agree - it's weird to have "None" as a unique permission type.  I like the
idea of a boolean histogram.  We should maintain the status quo for the
existing/legacy one, though.

[Marc Treib, 2015/04/28 12:31:32]

I've removed the "None" type for the new histograms, and instead added new
boolean "HasPermissions" histograms.

 
     // Real permissions.
     kAccessibilityFeaturesModify,
     kAccessibilityFeaturesRead,
     kAccessibilityPrivate,
     kActiveTab,
     kActivityLogPrivate,
     kAlarms,
     kAlphaEnabled,
     kAlwaysOnTopWindows,
@@ skipping 153 matching lines @@
     kScreenlockPrivate,
     kSystemCpu,
     kSystemMemory,
     kSystemNetwork,
     kSystemInfoCpu,
     kSystemInfoMemory,
 
     // Permission message IDs that are not currently valid permissions on their
     // own, but are needed by various manifest permissions to represent their
     // permission message rule combinations.
     // TODO(sashab): Move these in-line with the other permission IDs.

[Marc Treib, 2015/04/17 13:09:33]

We'd have to do this reordering right now, before making the new histograms. But
I think we shouldn't bother, since we won't be able to keep the list sorted
alphabetically anyway.

[Devlin, 2015/04/27 17:58:03]

Yeah, there's no way we can keep them in order - but I don't know if we should
keep this section here at all.  Otherwise, we'll be at a stage of
enum {
  <alphabetical>
  <pseudo-random-section-of-invalid-permissions>
  <everything else>
}

Might be best just to inline it now so we at least get
enum {
  <alphabetical>
  <everything else>
}

[Marc Treib, 2015/04/28 12:31:32]

I'm a bit concerned that when a developer sees an alphabetical list, they'll be
tempted to insert their new entry in the "proper" place in the middle. So maybe
a random_shuffle would be better :D

[Devlin, 2015/04/28 16:16:36]

I was actually toying with the same idea... and it might not be bad.  But we
shouldn't leave this section in there; otherwise it forces new entries under
this weird section of "not valid permissions".  So, we should take out the
section (or, at bare minimum, the comment), and if you leave them in
alphabetical order, maybe put a comment at the end saying "Initial batch as of
<date>; new entries below."

Also, I know we have presubmits for things like ExtensionFunction histograms -
might be worth looking into if we can just give it another source file/enum to
protect?

[Marc Treib, 2015/04/29 11:24:02]

Good idea! I've added a presubmit check for extensions/common, essentially
copied from extensions/browser.
I've also removed the "not valid permissions" comment, and updated the comment
at the top accordingly.

     kBluetooth,
     kBluetoothDevices,
     kFavicon,
     kFullAccess,
     kHostReadOnly,
     kHostReadWrite,
     kHostsAll,
     kHostsAllReadOnly,
     kMediaGalleriesAllGalleriesCopyTo,
     kMediaGalleriesAllGalleriesDelete,
     kMediaGalleriesAllGalleriesRead,
     kNetworkState,
     kOverrideBookmarksUI,
     kShouldWarnAllHosts,
     kSocketAnyHost,
     kSocketDomainHosts,
     kSocketSpecificHosts,
     kUsbDeviceList,
     kUsbDeviceUnknownProduct,
     kUsbDeviceUnknownVendor,
 
+    // Last entry: Add new entries above and ensure to update the
+    // "ExtensionPermission3" enum in tools/metrics/histograms/histograms.xml
+    // (by running update_extension_functions.py).
     kEnumBoundary
   };
 
   struct CheckParam {
   };
 
   explicit APIPermission(const APIPermissionInfo* info);
 
   virtual ~APIPermission();
 
@@ skipping 196 matching lines @@
   const char* const name_;
   const int flags_;
   const int l10n_message_id_;
   const PermissionMessage::ID message_id_;
   const APIPermissionConstructor api_permission_constructor_;
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_COMMON_PERMISSIONS_API_PERMISSION_H_