Change default mode of extension install verification

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 527 matching lines @@
 
     SetReadyAndNotifyListeners();
 
     // TODO(erikkay) this should probably be deferred to a future point
     // rather than running immediately at startup.
     CheckForExternalUpdates();
 
     InstallVerifier* verifier =
         extensions::ExtensionSystem::Get(profile_)->install_verifier();
     if (verifier->NeedsBootstrap())
-      VerifyAllExtensions();
+      VerifyAllExtensions(true /* bootstrap */);

[Finnur, 2014/01/29 10:35:42]

nit: I thought we generally shied away from multi-line comments? 
Can you change this to:
VerifyAllExtensions(true);  // bootstrap=true.

[asargent_no_longer_on_chrome, 2014/01/29 19:52:51]

Done.

     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ExtensionService::GarbageCollectExtensions, AsWeakPtr()),
         base::TimeDelta::FromSeconds(kGarbageCollectStartupDelay));
 
     if (extension_prefs_->NeedsStorageGarbageCollection()) {
       GarbageCollectIsolatedStorage();
       extension_prefs_->SetNeedsStorageGarbageCollection(false);
     }
     system_->management_policy()->RegisterProvider(
         shared_module_policy_provider_.get());
   }
 
   UMA_HISTOGRAM_TIMES("Extensions.ExtensionServiceInitTime",
                       base::Time::Now() - begin_time);
 }
 
-void ExtensionService::VerifyAllExtensions() {
+void ExtensionService::VerifyAllExtensions(bool bootstrap) {
   ExtensionIdSet to_add;
   scoped_ptr<ExtensionSet> all_extensions = GenerateInstalledExtensionsSet();
 
   for (ExtensionSet::const_iterator i = all_extensions->begin();
        i != all_extensions->end(); ++i) {
     const Extension& extension = **i;
 
     if (InstallVerifier::NeedsVerification(extension))
       to_add.insert(extension.id());
   }
   extensions::ExtensionSystem::Get(profile_)->install_verifier()->AddMany(
       to_add, base::Bind(&ExtensionService::FinishVerifyAllExtensions,
-                         AsWeakPtr()));
+                         AsWeakPtr(), bootstrap));
 }
 
-void ExtensionService::FinishVerifyAllExtensions(bool success) {
+namespace {
+
+enum VerifyAllSuccess {
+  VERIFY_ALL_BOOTSTRAP_SUCCESS,

[Finnur, 2014/01/29 10:35:42]

Nit: We're not consistent on this throughout Chrome, but I tend to explicitly
give the first enum the value of 0. I read somewhere that it is up to the
compiler what values it uses if the developer doesn't specify it and that could
be bad for the histograms if it picked different values on different
platforms/used negative numbers. Anyway, up to you.

[asargent_no_longer_on_chrome, 2014/01/29 19:52:51]

Good advice, done. 

+  VERIFY_ALL_BOOTSTRAP_FAILURE,
+  VERIFY_ALL_NON_BOOTSTRAP_SUCCESS,
+  VERIFY_ALL_NON_BOOTSTRAP_FAILURE,
+
+  // Used in histograms. Do not remove/reorder any entries above, and the below
+  // MAX entry should always come last.
+
+  VERIFY_ALL_SUCCESS_MAX
+};
+
+void LogVerifyAllSuccessHistogram(bool bootstrap, bool success) {
+  VerifyAllSuccess result;
+  if (bootstrap && success)
+    result = VERIFY_ALL_BOOTSTRAP_SUCCESS;
+  else if (bootstrap && !success)
+    result = VERIFY_ALL_BOOTSTRAP_FAILURE;
+  else if (!bootstrap && success)
+    result = VERIFY_ALL_NON_BOOTSTRAP_SUCCESS;
+  else
+    result = VERIFY_ALL_NON_BOOTSTRAP_FAILURE;
+
+  UMA_HISTOGRAM_ENUMERATION("ExtensionService.VerifyAllSuccess",
+                            result, VERIFY_ALL_SUCCESS_MAX);
+}
+
+}  // namespace
+
+void ExtensionService::FinishVerifyAllExtensions(bool bootstrap, bool success) {
+  LogVerifyAllSuccessHistogram(bootstrap, success);
   if (success) {
     // Check to see if any currently unverified extensions became verified.
     InstallVerifier* verifier =
         extensions::ExtensionSystem::Get(profile_)->install_verifier();
     const ExtensionSet& disabled_extensions = registry_->disabled_extensions();
     for (ExtensionSet::const_iterator i = disabled_extensions.begin();
          i != disabled_extensions.end(); ++i) {
       const Extension& extension = **i;
       int disable_reasons = extension_prefs_->GetDisableReasons(extension.id());
       if (disable_reasons & Extension::DISABLE_NOT_VERIFIED &&
@@ skipping 1578 matching lines @@
       delayed_installs_.Insert(extension);
     }
   } else {
     AddNewOrUpdatedExtension(extension,
                              initial_state,
                              blacklist_state,
                              page_ordinal);
   }
 }
 
+namespace {
+
+void LogAddVerifiedSuccess(bool success) {
+  UMA_HISTOGRAM_BOOLEAN("ExtensionService.AddVerified", success);
+}

[Finnur, 2014/01/29 10:35:42]

nit: I would probably prefer to group these utility functions along with the
rest (in the anonymous namespace at the top), instead of interspersed between
member functions. Same goes for LogVerifyAllSuccessHistogram and the enum above
it. 

[asargent_no_longer_on_chrome, 2014/01/29 19:52:51]

Done.

+
+}  // namespace
+
 void ExtensionService::AddNewOrUpdatedExtension(
     const Extension* extension,
     Extension::State initial_state,
     extensions::BlacklistState blacklist_state,
     const syncer::StringOrdinal& page_ordinal) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const bool blacklisted_for_malware =
       blacklist_state == extensions::BLACKLISTED_MALWARE;
   extension_prefs_->OnExtensionInstalled(extension,
                                          initial_state,
                                          blacklisted_for_malware,
                                          page_ordinal);
   delayed_installs_.Remove(extension->id());
   if (InstallVerifier::NeedsVerification(*extension)) {
     extensions::ExtensionSystem::Get(profile_)->install_verifier()->Add(
-        extension->id(), InstallVerifier::AddResultCallback());
+        extension->id(), base::Bind(LogAddVerifiedSuccess));
   }
   FinishInstallation(extension);
 }
 
 void ExtensionService::MaybeFinishDelayedInstallation(
     const std::string& extension_id) {
   // Check if the extension already got installed.
   if (!delayed_installs_.Contains(extension_id))
     return;
   extensions::ExtensionPrefs::DelayReason reason =
@@ skipping 569 matching lines @@
 void ExtensionService::UnloadAllExtensionsInternal() {
   profile_->GetExtensionSpecialStoragePolicy()->RevokeRightsForAllExtensions();
 
   registry_->ClearAll();
   system_->runtime_data()->ClearAll();
 
   // TODO(erikkay) should there be a notification for this?  We can't use
   // EXTENSION_UNLOADED since that implies that the extension has been disabled
   // or uninstalled.
 }