Add new ExtensionManagement preference

Add new ExtensionManagement preference

New ExtensionManagement preference is a policy-controlled dictionary preference for management of extensions.
Current implementation covers all legacy extension management preference (installation_mode, install_sources, allowed_types).

This CL includes extension preference parsing, definition of constants strings for properties/string enums in the new dictionary value, and a test helper class for manipulating it in unit tests.

BUG=177351
Committed: https://crrev.com/b245438121f8dba852268c0bed0359c6f31c50c6
Cr-Commit-Position: refs/heads/master@{#295974}

[binjin, 2014-09-10 00:00:34]

Patchset #3 (id:40001) has been deleted

[binjin, 2014-09-10 00:00:41]

Patchset #2 (id:20001) has been deleted

[binjin, 2014-09-10 00:00:54]

Patchset #1 (id:1) has been deleted

[binjin, 2014-09-17 15:06:23]

binjin@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[binjin, 2014-09-17 15:06:24]

Joao, PTAL at this CL.

[Joao da Silva, 2014-09-18 12:08:22]

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:27: const char
kAllOtherExtension[] = "*";
Rename this to kWildcard

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:47: { "platform_app",
Manifest::TYPE_PLATFORM_APP },
Align the types in the same column

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:50: { NULL,
Manifest::TYPE_UNKNOWN }
Remove the NULL entry

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:51: };
Add the size constant:

const size_t kAllowedTypesMapSize = arraysize(kAllowedTypesMap);

You may try ARRAYSIZE_UNSAFE if that doesn't work.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:72: LOG(WARNING) << "Invalid
value for 'installation_modes'.";
installation_mode

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:79: &update_url)) {
The update_url only matters for FORCED and RECOMMENDED extensions right? Maybe
move this to line 99 below?

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:257: LOG(WARNING) << "Default
extension management settings parsing error.";
Reset() default_settings_ in this case?

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:260: const base::ListValue*
list_value;
= NULL

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:338: break;
Make a helper for this:

bool GetManifestType(const std::string& name, Manifest::Type* type);

Return false if |name| is not valid, else return true and set |type|.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:356: iter.key(),
schema_constants::kUpdateUrlPrefix, true)) {
if (StartsWithASCII(...))
  continue;

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:52: Manifest::Type
manifest_type;
Document these 2 fields

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:55: extern const
AllowedTypesMapType kAllowedTypesMap[];
Add a size constant for this array:

extern const size_t kAllowedTypesMapSize;

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:57: }  // namespace
schema_constants
Move these constants to separate files. This class will be used from many places
and this is just adding noise.

Suggest moving to extension_management_constants.h and .cc

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:94: SCOPE_BY_UPDATE_URL,
This scope is never being used

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:95: SCOPE_INDIVIDUAL,
Document what each value means

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:99: bool Parse(const
base::DictionaryValue* dict, Scope scope);
These methods don't belong in the interface either. WDYT of moving them to
anonymous functions in the .cc file? Something like:

namespace {

void Reset(IndividualSettings* settings) {
  ...
}

void Parse(const base::DictionaryValue* dict, Scope scope, IndividualSettings*
settings) {
  Reset(settings);
  ...
}

Then the Scope can also be moved to the .cc. I think it's a good idea to keep
this header file as simple as possible, since it's what external code will use.
Implementation details should be hidden.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:41:
service_->SetManagedPref(pref_names::kExtensionManagement, pref_.release());
Break this class into 2:

class ExtensionManagementPrefUpdatedBase {
  - has all the utility methods to set the prefs
  - gets the initial pref value from subclasses and modifies it via a pointer
}

template<class TestingPrefService>
class ExtensionManagement : public ExtensionManagementPrefUpdaterBase {
  - only has the constructor that passes a pointer of the pref to the super
class
  - the destructor sets the pref in the service_
}

So most of the utility comes from the Base class, and the implementation can be
moved to a .cc file.

Then the templated class is just a frontend to handle the different
TestingPrefService template types: all it needs to do is get the managed pref
and set it in the dtor.

That will make this interface file much simpler and smaller, and the templated
methods only needs to be generated for the set and get of the pref_service; all
the utility methods can be reused.

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:1939:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
This pref should only be used by ExtensionManagement, so it should be registered
by ExtensionManagementFactory::RegisterProfilePrefs.

See RegisterProfilePrefs in
components/keyed_service/content/browser_context_keyed_base_factory.h. It's a
virtual method of factories that can be used to register preferences. See
chrome/browser/policy/cloud/user_policy_signin_service_factory.h for an example.

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/pref...
extensions/browser/pref_names.h:29: // DEPRECATED: use kExtensionManagement
instead.
Actually no one should read these prefs directly. They should use the
ExtensionManagement class directly.

[Joao da Silva, 2014-09-18 12:09:19]

The CL description should be updated to be more descriptive too. Imagine a
sheriff from another team looking at this CL description; he should be able to
understand what this is about without having seen this code before.

[binjin, 2014-09-18 14:37:28]

Hello Joao,

I updated the description as well, please proof read it.

Mean while, I didn't figure out a simple way to integrating parsing of this
preference into unit tests. Do you have any ideas?

-bjin

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:27: const char
kAllOtherExtension[] = "*";
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Rename this to kWildcard

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:47: { "platform_app",
Manifest::TYPE_PLATFORM_APP },
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Align the types in the same column

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:50: { NULL,
Manifest::TYPE_UNKNOWN }
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Remove the NULL entry

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:51: };
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Add the size constant:
> 
> const size_t kAllowedTypesMapSize = arraysize(kAllowedTypesMap);
> 
> You may try ARRAYSIZE_UNSAFE if that doesn't work.

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:72: LOG(WARNING) << "Invalid
value for 'installation_modes'.";
On 2014/09/18 12:08:21, Joao da Silva wrote:
> installation_mode

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:79: &update_url)) {
On 2014/09/18 12:08:21, Joao da Silva wrote:
> The update_url only matters for FORCED and RECOMMENDED extensions right? Maybe
> move this to line 99 below?

Done with further simplification.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:257: LOG(WARNING) << "Default
extension management settings parsing error.";
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Reset() default_settings_ in this case?

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:260: const base::ListValue*
list_value;
On 2014/09/18 12:08:21, Joao da Silva wrote:
> = NULL

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:338: break;
On 2014/09/18 12:08:21, Joao da Silva wrote:
> Make a helper for this:
> 
> bool GetManifestType(const std::string& name, Manifest::Type* type);
> 
> Return false if |name| is not valid, else return true and set |type|.

Done. I tried to avoid passing pointer here, and return Manifest::TYPE_UNKNOWN
in case of fail.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:356: iter.key(),
schema_constants::kUpdateUrlPrefix, true)) {
On 2014/09/18 12:08:21, Joao da Silva wrote:
> if (StartsWithASCII(...))
>   continue;

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:52: Manifest::Type
manifest_type;
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Document these 2 fields

Done. I also renamed AllowedTypesMapType into AllowedTypesMapEntry.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:55: extern const
AllowedTypesMapType kAllowedTypesMap[];
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Add a size constant for this array:
> 
> extern const size_t kAllowedTypesMapSize;

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:57: }  // namespace
schema_constants
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Move these constants to separate files. This class will be used from many
places
> and this is just adding noise.
> 
> Suggest moving to extension_management_constants.h and .cc

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:94: SCOPE_BY_UPDATE_URL,
On 2014/09/18 12:08:22, Joao da Silva wrote:
> This scope is never being used

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:95: SCOPE_INDIVIDUAL,
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Document what each value means

Done.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:99: bool Parse(const
base::DictionaryValue* dict, Scope scope);
On 2014/09/18 12:08:22, Joao da Silva wrote:
> These methods don't belong in the interface either. WDYT of moving them to
> anonymous functions in the .cc file? Something like:
> 
> namespace {
> 
> void Reset(IndividualSettings* settings) {
>   ...
> }
> 
> void Parse(const base::DictionaryValue* dict, Scope scope, IndividualSettings*
> settings) {
>   Reset(settings);
>   ...
> }
> 
> Then the Scope can also be moved to the .cc. I think it's a good idea to keep
> this header file as simple as possible, since it's what external code will
use.
> Implementation details should be hidden.

Done. I left Reset() though.

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/559603002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:41:
service_->SetManagedPref(pref_names::kExtensionManagement, pref_.release());
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Break this class into 2:
> 
> class ExtensionManagementPrefUpdatedBase {
>   - has all the utility methods to set the prefs
>   - gets the initial pref value from subclasses and modifies it via a pointer
> }
> 
> template<class TestingPrefService>
> class ExtensionManagement : public ExtensionManagementPrefUpdaterBase {
>   - only has the constructor that passes a pointer of the pref to the super
> class
>   - the destructor sets the pref in the service_
> }
> 
> So most of the utility comes from the Base class, and the implementation can
be
> moved to a .cc file.
> 
> Then the templated class is just a frontend to handle the different
> TestingPrefService template types: all it needs to do is get the managed pref
> and set it in the dtor.
> 
> That will make this interface file much simpler and smaller, and the templated
> methods only needs to be generated for the set and get of the pref_service;
all
> the utility methods can be reused.

Done.

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:1939:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2014/09/18 12:08:22, Joao da Silva wrote:
> This pref should only be used by ExtensionManagement, so it should be
registered
> by ExtensionManagementFactory::RegisterProfilePrefs.
> 
> See RegisterProfilePrefs in
> components/keyed_service/content/browser_context_keyed_base_factory.h. It's a
> virtual method of factories that can be used to register preferences. See
> chrome/browser/policy/cloud/user_policy_signin_service_factory.h for an
example.

Done.

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/140001/extensions/browser/pref...
extensions/browser/pref_names.h:29: // DEPRECATED: use kExtensionManagement
instead.
On 2014/09/18 12:08:22, Joao da Silva wrote:
> Actually no one should read these prefs directly. They should use the
> ExtensionManagement class directly.

Removed.

[Joao da Silva, 2014-09-18 15:23:55]

I think the new unit tests add good coverage. What did you have in mind?

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:30: // Parses the default
settings
Terminate sentence with .

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:43: ++iter) {
This doesn't work anymore, because there is no NULL name at the end. Should
iterate with an int now:

for (size_t i = 0; i < kAllowedTypesMapSize; ++i) {
  if (kAllowedTypesMap[i].name == name) { ... }
}

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.h (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.h:40: Manifest::Type
GetManifestType(const std::string& name);
#include <string>

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:22:
make_path(schema::kWildcard, schema::kAllowedTypes);
Don't define std::string globals like this, because they require static
initializers.

Just duplicate and inline the string constants:

const char kInstallSourcesPath[] = "*.install_sources";

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:124:
pref_->Set(path, new base::ListValue());
This also overrides existing lists, so it works for both cases.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:12: #include
"extensions/browser/pref_names.h"
Move to .cc

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:17: // Base class for
essential routines on preference manipulating.
manipulation

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:23: // Helper
functions for 'installation_mode' manipulating.
manipulation

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:29: const
std::string& update_url,
#include <string>

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:36: "  \"%s\": {"  //
kTargetExtension
Just inline the value, it's easier to read

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:110: SetPref(true,
pref_names::kExtensionManagement, parsed->DeepCopy());
parsed.release()

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:373: // update the
new dictionary preference.
Update

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:410: // update the
new dictionary preference.
Update

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:500: ++it) {
Needs to be updated to use an int iterator and kAllowedTypesMapSize

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
extensions/browser/pref_names.h:29: // DEPRECATED: use kExtensionManagement
instead.
Remove this too

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
extensions/browser/pref_names.h:35: // DEPRECATED: use kExtensionManagement
instead.
And this

[binjin, 2014-09-18 15:49:05]

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:30: // Parses the default
settings
On 2014/09/18 15:23:52, Joao da Silva wrote:
> Terminate sentence with .

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:43: ++iter) {
On 2014/09/18 15:23:52, Joao da Silva wrote:
> This doesn't work anymore, because there is no NULL name at the end. Should
> iterate with an int now:
> 
> for (size_t i = 0; i < kAllowedTypesMapSize; ++i) {
>   if (kAllowedTypesMap[i].name == name) { ... }
> }

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.h (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.h:40: Manifest::Type
GetManifestType(const std::string& name);
On 2014/09/18 15:23:53, Joao da Silva wrote:
> #include <string>

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:22:
make_path(schema::kWildcard, schema::kAllowedTypes);
On 2014/09/18 15:23:53, Joao da Silva wrote:
> Don't define std::string globals like this, because they require static
> initializers.
> 
> Just duplicate and inline the string constants:
> 
> const char kInstallSourcesPath[] = "*.install_sources";

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:124:
pref_->Set(path, new base::ListValue());
On 2014/09/18 15:23:53, Joao da Silva wrote:
> This also overrides existing lists, so it works for both cases.

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:12: #include
"extensions/browser/pref_names.h"
On 2014/09/18 15:23:53, Joao da Silva wrote:
> Move to .cc

It's used in template class in header file.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:17: // Base class for
essential routines on preference manipulating.
On 2014/09/18 15:23:53, Joao da Silva wrote:
> manipulation

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:23: // Helper
functions for 'installation_mode' manipulating.
On 2014/09/18 15:23:53, Joao da Silva wrote:
> manipulation

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:29: const
std::string& update_url,
On 2014/09/18 15:23:53, Joao da Silva wrote:
> #include <string>

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:36: "  \"%s\": {"  //
kTargetExtension
On 2014/09/18 15:23:54, Joao da Silva wrote:
> Just inline the value, it's easier to read

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:110: SetPref(true,
pref_names::kExtensionManagement, parsed->DeepCopy());
On 2014/09/18 15:23:54, Joao da Silva wrote:
> parsed.release()

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:373: // update the
new dictionary preference.
On 2014/09/18 15:23:54, Joao da Silva wrote:
> Update

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:410: // update the
new dictionary preference.
On 2014/09/18 15:23:54, Joao da Silva wrote:
> Update

Done.

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/559603002/diff/160001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:500: ++it) {
On 2014/09/18 15:23:54, Joao da Silva wrote:
> Needs to be updated to use an int iterator and kAllowedTypesMapSize

Done.

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
extensions/browser/pref_names.h:29: // DEPRECATED: use kExtensionManagement
instead.
On 2014/09/18 15:23:54, Joao da Silva wrote:
> Remove this too

Done.

https://codereview.chromium.org/559603002/diff/160001/extensions/browser/pref...
extensions/browser/pref_names.h:35: // DEPRECATED: use kExtensionManagement
instead.
On 2014/09/18 15:23:54, Joao da Silva wrote:
> And this

Done.

[Joao da Silva, 2014-09-19 07:27:26]

Almost ready to go.

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:7: #include <string>
This should be in the .h, for the declaration of GetManifestType

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:42: for (size_t
index = 0; index < kAllowedTypesMapSize; index++) {
++index

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.h (right):

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.h:40: Manifest::Type
GetManifestType(const std::string& name);
#include <string>

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:499: index++)
{
++index

[binjin, 2014-09-19 10:06:08]

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:7: #include <string>
On 2014/09/19 07:27:26, Joao da Silva wrote:
> This should be in the .h, for the declaration of GetManifestType

Done.

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:42: for (size_t
index = 0; index < kAllowedTypesMapSize; index++) {
On 2014/09/19 07:27:26, Joao da Silva wrote:
> ++index

Done.

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.h (right):

https://codereview.chromium.org/559603002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.h:40: Manifest::Type
GetManifestType(const std::string& name);
On 2014/09/19 07:27:26, Joao da Silva wrote:
> #include <string>

Done.

[Joao da Silva, 2014-09-19 10:11:17]

lgtm

[binjin, 2014-09-19 10:37:49]

binjin@chromium.org changed reviewers:
+ rockot@chromium.org, scheib@chromium.org

[binjin, 2014-09-19 10:37:50]

rockot, scheib: chrome/browser/extensions/* and
extensions/browser/pref_names.{h,cc}

Thanks!

-bjin

[scheib, 2014-09-19 16:22:14]

lgtm though please scrutinize the LOGs.

https://codereview.chromium.org/559603002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:60: LOG(WARNING) << "Invalid
value for 'installation_mode'.";
http://dev.chromium.org/developers/coding-style#TOC-Logging encourages LOG to be
used quite sparringly. If you believe it will be necessary to ship in Release
builds, shouldn't we be using some shared constant string ~~"Extension Policy
Parsing Error" and optionally some short literal if multiple instances must be
identified? These log messages are more descriptive, but I'm guessing not quite
enough that enterprise users will use them to diagnose what is going wrong with
bad data in the field?

https://codereview.chromium.org/559603002/diff/220001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/220001/extensions/browser/pref...
extensions/browser/pref_names.h:51: // Values are expected to conform to the
schema of the ExtensionManagement
Where is that schema defined?

[binjin, 2014-09-22 13:53:54]

https://codereview.chromium.org/559603002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/559603002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:60: LOG(WARNING) << "Invalid
value for 'installation_mode'.";
On 2014/09/19 16:22:13, scheib wrote:
> http://dev.chromium.org/developers/coding-style#TOC-Logging encourages LOG to
be
> used quite sparringly. If you believe it will be necessary to ship in Release
> builds, shouldn't we be using some shared constant string ~~"Extension Policy
> Parsing Error" and optionally some short literal if multiple instances must be
> identified? These log messages are more descriptive, but I'm guessing not
quite
> enough that enterprise users will use them to diagnose what is going wrong
with
> bad data in the field?

Thanks, I simplified the warning messages in LOG to make them compact and still
useful for diagnosing.

https://codereview.chromium.org/559603002/diff/220001/extensions/browser/pref...
File extensions/browser/pref_names.h (right):

https://codereview.chromium.org/559603002/diff/220001/extensions/browser/pref...
extensions/browser/pref_names.h:51: // Values are expected to conform to the
schema of the ExtensionManagement
On 2014/09/19 16:22:13, scheib wrote:
> Where is that schema defined?

It will be part of the following CL, defined in policy schema.

[binjin, 2014-09-22 13:54:18]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-22 13:54:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/559603002/240001

[commit-bot: I haz the power, 2014-09-22 14:45:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-22 14:45:56]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[binjin, 2014-09-22 14:50:02]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-22 14:50:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/559603002/240001

[commit-bot: I haz the power, 2014-09-22 15:17:52]

Committed patchset #10 (id:240001) as ce498ba520655f72be588aecabf0bb00d74fed8a

[commit-bot: I haz the power, 2014-09-22 15:18:24]

Patchset 10 (id:??) landed as
https://crrev.com/b245438121f8dba852268c0bed0359c6f31c50c6
Cr-Commit-Position: refs/heads/master@{#295974}