Removed the PolicyDefinitionList.

Removed the PolicyDefinitionList.

This list was generated at compile time and included policy information
extracted from the policy templates file that was useful at runtime, such as
the policy names, types, deprecation status, etc.

Most of this information is now available via the Chrome policy schema, so
this list was largely redundant.

The chrome-specific data in this list is now available in a list of
PolicyDetails, also generated at compile time and kept in sync with the Chrome
schema data.

Refactored all the remaining users of PolicyDefinitionList to use the schema
and/or PolicyDetails now. Also introduced a test helper to mock out
PolicyDetails in tests.

Cleanup: moved policy_test_utils.h to policy/test/

BUG=270667
R=bartfab@chromium.org, dubroy@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234515

[Joao da Silva, 2013-11-04 19:32:03]

Congratulations Mario, you made it to the final CL! :-)

This one is mostly a cleanup and refactorings to use the new classes. The rest
of the work will be done in each provider separately, to use the new Schemas to
load policy for Chrome and for components.

One more to review, please have a look. Thanks!

[bartfab (slow), 2013-11-05 18:18:33]

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/cloud_external_data_manager_base.cc:10: 
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:9: 
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_external_data_manager.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_external_data_manager.cc:6: 
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:38:
#include "policy/policy_constants.h"
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:48: #include
"policy/policy_constants.h"
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager_browsertest.cc:14: #include
"chrome/browser/policy/test/policy_test_utils.h"
Nit: chrome/browser/policy/test/policy_test_utils.h now implicitly pulls in
"callback_forward.h", so all implementation files using it must #include
"base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc:23: #include
"chrome/browser/policy/test/policy_test_utils.h"
Nit: chrome/browser/policy/test/policy_test_utils.h now implicitly pulls in
"callback_forward.h", so all implementation files using it must #include
"base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/configu...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/configu...
chrome/browser/policy/configuration_policy_handler_list.cc:31: #include
"policy/policy_constants.h"
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/generat...
File chrome/browser/policy/generate_policy_source_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/generat...
chrome/browser/policy/generate_policy_source_unittest.cc:118: //
TODO(joaodasilva): add a test that verifies a max_external_data_size larger
You can mark that as TODO(bartfab).

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_mac.cc:90:
schema_map()->GetSchema(PolicyNamespace(POLICY_DOMAIN_CHROME, ""));
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_mac_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_mac_unittest.cc:20: #include
"policy/policy_constants.h"
Nit: Is this still used?

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:37: #include
"policy/policy_constants.h"
Nit 1: Is this still needed?

Nit 2: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:327:
schema_map()->GetSchema(PolicyNamespace(POLICY_DOMAIN_CHROME, ""));
#include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win_unittest.cc:38: #include
"policy/policy_constants.h"
Nit: Is this still needed?

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:37: #include
"policy/policy_constants.h"
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:395:
CHECK(chrome_schema.valid());
Nit: #include "base/logging.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:420: // the keys() returned
are the same.
Nit: s/keys()/key()s/

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:458: return Advance();
This is the post-increment operator. Its return value should be the
unincremented iterator.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_statistics_collector.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector.cc:9: 
Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h", so
all implementation files using it must #include "base/callback.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector.cc:75: NOTREACHED();
Nit: #include "base/logging.h"

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_statistics_collector_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector_unittest.cc:25: #include
"components/policy/core/common/schema.h"
Nit: Already included by header file.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/ui/webui/polic...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/ui/webui/polic...
chrome/browser/ui/webui/policy_ui.cc:549: policy::SchemaRegistry* registry =
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:391: f.write('#include
<algorithm>\n'
Nit 1: The header includes policy_details.h, which includes callback_forward.h.
This means that the implementation should include callback.h.

Nit 2: #include "policy/policy_constants.h" should be first since this is the
implementation file for that header.

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:454: '  // First index in
kPropertyNodes of the Chrome policies.\n'
Nit: If you document begin_index, you should document end_index as well.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
File components/policy/core/common/policy_details.h (right):

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:20: // True if this policy is a
ChromeOS device policy.
Nit: s/ChromeOS/Chrome OS/

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:25: int id;
Do we ever use this for anything?

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:27: // If this policy references
external data to be downloaded then
Could you drop the part "to be downloaded"? As Mattias pointed out in our
earliest discussions about external policy data, we may want to support
platform-specific references in the future that do not require downloading, e.g.
UNC paths.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:30: size_t
max_external_data_size;
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:35: // function into objects, so
that it can be easily mocked for
Nice mocking mechanism.

[Joao da Silva, 2013-11-07 20:27:26]

@Bartosz: PTAL

@Patrick: please check webui/

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/cloud_external_data_manager_base.cc:10: 
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:9: 
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_external_data_manager.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_external_data_manager.cc:6: 
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

This code doesn't actually use the Callback class, and a forward declaration is
enough here (it just passed a const& along)

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:38:
#include "policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

See previous reply

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:48: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

policy_constants.h is not pulling callback_forward.h anymore. Also, this file
doesn't use the Callback class so a forward decl is enough

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager_browsertest.cc:14: #include
"chrome/browser/policy/test/policy_test_utils.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: chrome/browser/policy/test/policy_test_utils.h now implicitly pulls in
> "callback_forward.h", so all implementation files using it must #include
> "base/callback.h"

This file doesn't use the Callback interface

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc:23: #include
"chrome/browser/policy/test/policy_test_utils.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: chrome/browser/policy/test/policy_test_utils.h now implicitly pulls in
> "callback_forward.h", so all implementation files using it must #include
> "base/callback.h"

This file doesn't use the Callback interface

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/configu...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/configu...
chrome/browser/policy/configuration_policy_handler_list.cc:31: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

This file doesn't use the Callback interface

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/generat...
File chrome/browser/policy/generate_policy_source_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/generat...
chrome/browser/policy/generate_policy_source_unittest.cc:118: //
TODO(joaodasilva): add a test that verifies a max_external_data_size larger
On 2013/11/05 18:18:33, bartfab wrote:
> You can mark that as TODO(bartfab).

Great! Done :-)

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_mac.cc:90:
schema_map()->GetSchema(PolicyNamespace(POLICY_DOMAIN_CHROME, ""));
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Already in .h, for the PolicyDomain type

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_mac_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_mac_unittest.cc:20: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: Is this still used?

Good catch, removed

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:37: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit 1: Is this still needed?
> 
> Nit 2: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
> so all implementation files using it must #include "base/callback.h"

It's not used, removed

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:327:
schema_map()->GetSchema(PolicyNamespace(POLICY_DOMAIN_CHROME, ""));
On 2013/11/05 18:18:33, bartfab wrote:
> #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win_unittest.cc:38: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: Is this still needed?

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:37: #include
"policy/policy_constants.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

The interface is not actually used, so forward-decl is enough

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:395:
CHECK(chrome_schema.valid());
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:420: // the keys() returned
are the same.
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: s/keys()/key()s/

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_prefs_browsertest.cc:458: return Advance();
On 2013/11/05 18:18:33, bartfab wrote:
> This is the post-increment operator. Its return value should be the
> unincremented iterator.

Ah right, done

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_statistics_collector.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector.cc:9: 
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: policy/policy_constants.h now implicitly pulls in "callback_forward.h",
so
> all implementation files using it must #include "base/callback.h"

This file does use Run(), so added callback.h :-)

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector.cc:75: NOTREACHED();
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_statistics_collector_unittest.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_statistics_collector_unittest.cc:25: #include
"components/policy/core/common/schema.h"
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: Already included by header file.

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/browser/ui/webui/polic...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/58313002/diff/1/chrome/browser/ui/webui/polic...
chrome/browser/ui/webui/policy_ui.cc:549: policy::SchemaRegistry* registry =
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:391: f.write('#include
<algorithm>\n'
On 2013/11/05 18:18:33, bartfab wrote:
> Nit 1: The header includes policy_details.h, which includes
callback_forward.h.
> This means that the implementation should include callback.h.
> 

But it's not used, it's just the return type of Bind(); so a forward-decl is
enough

> Nit 2: #include "policy/policy_constants.h" should be first since this is the
> implementation file for that header.

Done

https://codereview.chromium.org/58313002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:454: '  // First index in
kPropertyNodes of the Chrome policies.\n'
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: If you document begin_index, you should document end_index as well.

Done.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
File components/policy/core/common/policy_details.h (right):

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:20: // True if this policy is a
ChromeOS device policy.
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: s/ChromeOS/Chrome OS/

Done.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:25: int id;
On 2013/11/05 18:18:33, bartfab wrote:
> Do we ever use this for anything?

It's what the PolicyStatisticsCollector samples for UMA. histograms.xml then
maps these IDs to the policy names for diplaying

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:27: // If this policy references
external data to be downloaded then
On 2013/11/05 18:18:33, bartfab wrote:
> Could you drop the part "to be downloaded"? As Mattias pointed out in our
> earliest discussions about external policy data, we may want to support
> platform-specific references in the future that do not require downloading,
e.g.
> UNC paths.

Done.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:30: size_t
max_external_data_size;
On 2013/11/05 18:18:33, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/58313002/diff/1/components/policy/core/common...
components/policy/core/common/policy_details.h:35: // function into objects, so
that it can be easily mocked for
On 2013/11/05 18:18:33, bartfab wrote:
> Nice mocking mechanism.

Daniel and I have remarked how the component refactoring is improving the code
in other ways. This was another choice "forced" by the DEPS constrains of the
component.

[Patrick Dubroy, 2013-11-08 08:22:49]

lgtm

[bartfab (slow), 2013-11-08 12:40:37]

lgtm

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:13:
#include "base/logging.h"
Nit: Where is this used?

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:29:
#include "components/policy/core/common/policy_details.h"
Nit: Already included by header file.

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:395:
CHECK(chrome_schema.valid());
Nit: #include "base/logging.h"

[Joao da Silva, 2013-11-08 14:26:54]

Thanks for the reviews!

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:13:
#include "base/logging.h"
On 2013/11/08 12:40:37, bartfab wrote:
> Nit: Where is this used?

Oops, removed

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:29:
#include "components/policy/core/common/policy_details.h"
On 2013/11/08 12:40:37, bartfab wrote:
> Nit: Already included by header file.

Done.

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/58313002/diff/630001/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:395:
CHECK(chrome_schema.valid());
On 2013/11/08 12:40:37, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

[Joao da Silva, 2013-11-12 14:52:28]

Committed patchset #8 manually as r234515 (presubmit successful).