Add more management policy checking after extension installed

chrome/browser/extensions/extension_service.h

 // Copyright (c) 2012 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 CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 502 matching lines @@
 
   // Removes the extension with the given id from the list of
   // terminated extensions if it is there.
   void UntrackTerminatedExtension(const std::string& id);
 
   // Update preferences for a new or updated extension; notify observers that
   // the extension is installed, e.g., to update event handlers on background
   // pages; and perform other extension install tasks before calling
   // AddExtension.
   // |install_flags| is a bitmask of extensions::InstallFlags.
-  void AddNewOrUpdatedExtension(const extensions::Extension* extension,
-                                extensions::Extension::State initial_state,
-                                int install_flags,
-                                const syncer::StringOrdinal& page_ordinal,
-                                const std::string& install_parameter);
+  void AddNewOrUpdatedExtension(
+      const extensions::Extension* extension,
+      extensions::Extension::State initial_state,
+      extensions::Extension::DisableReason initial_disable_reason,

[Finnur, 2014/11/11 22:58:03]

I think |initial_state| and |initial_disable_reason| deserve documentation. For
example, if |initial_state| is ENABLED, what should |initial_disable_reason| be
set to?

[not at google - send to devlin, 2014/11/12 00:54:19]

This is pretty weird to look at (and looking at the files earlier, having to
pass in a "not disabled" flag every time you say "enabled", is very strange).

2 proposals:

- is it really that bad for whatever place needs to set a different disable
reason to add in a DISABLED state then later set the real disable state?

- perhaps look into replacing the initial state argument with the disable reason
flag? Still weird, but not quite as weird. The states are basically enabled,
disabled, and external-extension-uninstalled, the latter of which is a pretty
weird thing in the first place. I'd question why it needs to be passed in here
(is it still used?) and/or whether it should *really* be a disable reason.

[binjin, 2014/11/12 16:33:18]

Yes, I now followed your first proposal.

[not at google - send to devlin, 2014/11/12 16:38:42]

Cool! If you're going with this, it would be ideal to split this CL into 2
parts, firstly doing the parameter type change, then adding your feature.

[not at google - send to devlin, 2014/11/12 16:40:33]

And perhaps a 3rd earlier to do the rename that Finnur suggested with the
DISABLED_NONE enum.

+      int install_flags,
+      const syncer::StringOrdinal& page_ordinal,
+      const std::string& install_parameter);
 
   // Handles sending notification that |extension| was loaded.
   void NotifyExtensionLoaded(const extensions::Extension* extension);
 
   // Handles sending notification that |extension| was unloaded.
   void NotifyExtensionUnloaded(
       const extensions::Extension* extension,
       extensions::UnloadedExtensionInfo::Reason reason);
 
   // Common helper to finish installing the given extension. |was_ephemeral|
   // should be true if the extension was previously installed and ephemeral.
   void FinishInstallation(const extensions::Extension* extension,
                           bool was_ephemeral);
 
   // Disables the extension if the privilege level has increased
   // (e.g., due to an upgrade).
   void CheckPermissionsIncrease(const extensions::Extension* extension,
                                 bool is_extension_installed);
 
   // Helper that updates the active extension list used for crash reporting.
   void UpdateActiveExtensionsInCrashReporter();
 
   // Helper to determine whether we should initially enable an installed
-  // (or upgraded) extension.
-  bool ShouldEnableOnInstall(const extensions::Extension* extension);
+  // (or upgraded) extension. If we should not initially enable the extension
+  // and |disable_reason| is not null, set it to be DisableReason if
+  // applicable.

[Finnur, 2014/11/11 22:58:03]

This comment is a bit confusing. How about:

|disable_reason| is ignored if NULL, but otherwise contains why |extension|
should not be enabled.

[binjin, 2014/11/12 16:33:18]

Done.

+  bool ShouldEnableOnInstall(
+      const extensions::Extension* extension,
+      extensions::Extension::DisableReason* disable_reason);
 
   // Helper to determine if updating an extensions should proceed immediately,
   // or if we should delay the update until further notice.
   bool ShouldDelayExtensionUpdate(const std::string& extension_id,
                                   bool install_immediately) const;
 
   // Manages the blacklisted extensions, intended as callback from
   // Blacklist::GetBlacklistedIDs.
   void ManageBlacklist(
       const extensions::Blacklist::BlacklistStateMap& blacklisted_ids);
@@ skipping 165 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            WillNotLoadBlacklistedExtensionsFromDirectory);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, ReloadBlacklistedExtension);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, BlacklistedInPrefsFromStartup);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            GreylistedExtensionDisabled);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            GreylistDontEnableManuallyDisabled);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            GreylistUnknownDontChange);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
+                           ManagementPolicyProhibitsEnableOnInstalled);
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionService);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_