Add more management policy checking after extension installed

chrome/browser/extensions/extension_service.h

Index: chrome/browser/extensions/extension_service.h
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index 556adb0025afdec9c42b162005a9cf4879179df8..17e00fac2c5e2a34c544f602c3f53277818bfdcc 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -520,11 +520,13 @@ class ExtensionService
   // 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);
@@ -548,8 +550,12 @@ class ExtensionService
   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.
@@ -735,6 +741,8 @@ class ExtensionService
                            GreylistDontEnableManuallyDisabled);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            GreylistUnknownDontChange);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
+                           ManagementPolicyProhibitsEnableOnInstalled);
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionService);
 };