Extensions: Switch to new permission message system, part V

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 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/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 63 matching lines @@
 #include "extensions/browser/updater/extension_cache.h"
 #include "extensions/browser/updater/extension_downloader.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/background_info.h"
 #include "extensions/common/manifest_url_handlers.h"
 #include "extensions/common/one_shot_event.h"
+#include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permission_message_provider.h"
 #include "extensions/common/permissions/permissions_data.h"
 
 #if defined(ENABLE_SUPERVISED_USERS)
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/extensions/install_limiter.h"
 #include "storage/browser/fileapi/file_system_backend.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #endif
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::DevToolsAgentHost;
+using extensions::APIPermission;
 using extensions::CrxInstaller;
 using extensions::Extension;
 using extensions::ExtensionIdSet;
 using extensions::ExtensionInfo;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::FeatureSwitch;
 using extensions::InstallVerifier;
 using extensions::ManagementPolicy;
 using extensions::Manifest;
+using extensions::PermissionID;
+using extensions::PermissionIDSet;
 using extensions::PermissionMessage;
 using extensions::PermissionMessageIDs;
 using extensions::PermissionSet;
 using extensions::SharedModuleInfo;
 using extensions::SharedModuleService;
 using extensions::UnloadedExtensionInfo;
 
 namespace {
 
 // Wait this many seconds after an extensions becomes idle before updating it.
@@ skipping 612 matching lines @@
   if (extension_sync_service_ &&
       reason != extensions::UNINSTALL_REASON_REINSTALL) {
      sync_change = extension_sync_service_->PrepareToSyncUninstallExtension(
         extension.get(), is_ready());
   }
 
   system_->install_verifier()->Remove(extension->id());
 
   UMA_HISTOGRAM_ENUMERATION("Extensions.UninstallType",
                             extension->GetType(), 100);
-  RecordPermissionMessagesHistogram(extension.get(),
-                                    "Extensions.Permissions_Uninstall2");
+  RecordPermissionMessagesHistogram(extension.get(), "Uninstall");
 
   // Unload before doing more cleanup to ensure that nothing is hanging on to
   // any of these resources.
   UnloadExtension(extension->id(), UnloadedExtensionInfo::REASON_UNINSTALL);
 
   // Tell the backend to start deleting installed extensions on the file thread.
   if (!Manifest::IsUnpackedLocation(extension->location())) {
     if (!GetFileTaskRunner()->PostTask(
             FROM_HERE,
             base::Bind(&ExtensionService::UninstallExtensionOnFileThread,
@@ skipping 232 matching lines @@
 
   for (const scoped_refptr<const Extension>& extension : *to_unblock) {
     registry_->RemoveBlocked(extension->id());
     AddExtension(extension.get());
   }
 }
 
 void ExtensionService::GrantPermissionsAndEnableExtension(
     const Extension* extension) {
   GrantPermissions(extension);
-  RecordPermissionMessagesHistogram(extension,
-                                    "Extensions.Permissions_ReEnable2");
+  RecordPermissionMessagesHistogram(extension, "ReEnable");
   extension_prefs_->SetDidExtensionEscalatePermissions(extension, false);
   EnableExtension(extension->id());
 }
 
 void ExtensionService::GrantPermissions(const Extension* extension) {
   CHECK(extension);
   extensions::PermissionsUpdater(profile()).GrantActivePermissions(extension);
 }
 
 // static
 void ExtensionService::RecordPermissionMessagesHistogram(
     const Extension* extension, const char* histogram) {
   // Since this is called from multiple sources, and since the histogram macros
   // use statics, we need to manually lookup the histogram ourselves.
-  base::HistogramBase* counter = base::LinearHistogram::FactoryGet(
-      histogram,
+  std::string histogram_name_base =

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

Is there a reason to record both 2 and 3?  (It doesn't look like we bother
recording the original anymore...)

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

I just didn't want to break the existing ones until I'm sure the new ones
fulfill everyone's needs. After having both for a while (say, a week or two on
Dev at least), I'll remove the old stuff.

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

Yeah, I understand the motivation, but I'm not sure it's right to "pollute" the
old, if we have our concerns at all.  The alternative it to pass in a bool for
whether or not to record old metrics, and only record if we're not using the new
system.

I'm fine either way, though - just making sure you've thought of it.  Use your
best judgment. :)

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

What do you mean by "pollute"? The old histograms shouldn't change at all with
this CL.

[Devlin, 2015/04/29 15:42:24]

Ah, misread part of this.  Nevermind; should be fine.

+      std::string("Extensions.Permissions_") + histogram;

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

nit: + string concatenation is pretty slow - let's use a StringPrintf here.
std::string legacy_histogram_name =
base::StringPrintf("Extensions.Permissions_%s2", histogram);
std::string histogram_name = base::StringPrintf("Extensions.Permissions_%s3",
histogram);

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

Okay, done. Is "+"-style concatenation really that bad, though?!

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

Depends on whom you ask. ;)  It's tremendously worse than StringPrintf for > 1
concatenations (here, you have two - the root + the histogram name + the number
suffix), but it's also not like this is performance-critical code.  But, a good
habit, nonetheless, and it doesn't (IMO) hurt readability at all.

+  base::HistogramBase* legacy_counter = base::LinearHistogram::FactoryGet(
+      histogram_name_base + "2",
       1,
       PermissionMessage::kEnumBoundary,
       PermissionMessage::kEnumBoundary + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
 
-  PermissionMessageIDs permissions =
+  PermissionMessageIDs legacy_permissions =
       extension->permissions_data()->GetLegacyPermissionMessageIDs();
+  if (legacy_permissions.empty()) {
+    legacy_counter->Add(PermissionMessage::kNone);
+  } else {
+    for (PermissionMessage::ID id : legacy_permissions)
+      legacy_counter->Add(id);
+  }
+
+  base::HistogramBase* counter = base::LinearHistogram::FactoryGet(
+      histogram_name_base + "3",
+      1,
+      APIPermission::kEnumBoundary,
+      APIPermission::kEnumBoundary + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+
+  PermissionIDSet permissions =
+      extensions::PermissionMessageProvider::Get()->GetAllPermissionIDs(
+          extension->permissions_data()->active_permissions().get(),
+          extension->GetType());
   if (permissions.empty()) {
-    counter->Add(PermissionMessage::kNone);
+    counter->Add(APIPermission::kNone);
   } else {
-    for (PermissionMessage::ID id : permissions)
-      counter->Add(id);
+    for (const PermissionID& id : permissions)
+      counter->Add(id.id());
   }
 }
 
 void ExtensionService::NotifyExtensionLoaded(const Extension* extension) {
   // The URLRequestContexts need to be first to know that the extension
   // was loaded, otherwise a race can arise where a renderer that is created
   // for the extension may try to load an extension URL with an extension id
   // that the request context doesn't yet know about. The profile is responsible
   // for ensuring its URLRequestContexts appropriately discover the loaded
   // extension.
@@ skipping 621 matching lines @@
   // Extension has changed permissions significantly. Disable it. A
   // notification should be sent by the caller. If the extension is already
   // disabled because it was installed remotely, don't add another disable
   // reason, but instead always set the "did escalate permissions" flag, to
   // ensure enabling it will always show a warning.
   if (disable_reasons == Extension::DISABLE_REMOTE_INSTALL) {
     extension_prefs_->SetDidExtensionEscalatePermissions(extension, true);
   } else if (is_privilege_increase) {
     disable_reasons |= Extension::DISABLE_PERMISSIONS_INCREASE;
     if (!extension_prefs_->DidExtensionEscalatePermissions(extension->id())) {
-      RecordPermissionMessagesHistogram(extension,
-                                        "Extensions.Permissions_AutoDisable2");
+      RecordPermissionMessagesHistogram(extension, "AutoDisable");
     }
     extension_prefs_->SetExtensionState(extension->id(), Extension::DISABLED);
     extension_prefs_->SetDidExtensionEscalatePermissions(extension, true);
 
 #if defined(ENABLE_SUPERVISED_USERS)
     // If a custodian-installed extension is disabled for a supervised user due
     // to a permissions increase, send a request to the custodian, since the
     // supervised user themselves can't re-enable the extension.
     if (extensions::util::IsExtensionSupervised(extension, profile_)) {
       SupervisedUserService* supervised_user_service =
@@ skipping 102 matching lines @@
     UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.SilentInstall",
                               extension->location(),
                               Manifest::NUM_LOCATIONS);
   }
 
   if (!GetInstalledExtension(extension->id())) {
     UMA_HISTOGRAM_ENUMERATION("Extensions.InstallType",
                               extension->GetType(), 100);
     UMA_HISTOGRAM_ENUMERATION("Extensions.InstallSource",
                               extension->location(), Manifest::NUM_LOCATIONS);
-    RecordPermissionMessagesHistogram(extension,
-                                      "Extensions.Permissions_Install2");
+    RecordPermissionMessagesHistogram(extension, "Install");
   } else {
     UMA_HISTOGRAM_ENUMERATION("Extensions.UpdateType",
                               extension->GetType(), 100);
     UMA_HISTOGRAM_ENUMERATION("Extensions.UpdateSource",
                               extension->location(), Manifest::NUM_LOCATIONS);
 
     // A fully installed app cannot be demoted to an ephemeral app.
     if ((install_flags & extensions::kInstallFlagIsEphemeral) &&
         !extension_prefs_->IsEphemeralApp(id)) {
       install_flags &= ~static_cast<int>(extensions::kInstallFlagIsEphemeral);
@@ skipping 778 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }