Refactored users of PolicySchema to use the new policy::Schema class.

Refactored users of PolicySchema to use the new policy::Schema class.

PolicySchema was used to contain the JSON schema describing the valid values for
each known policy. This is now done by the policy::Schema class, which supports
loading the Chrome policy schema at startup without having to parse a JSON blob.

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

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

[Joao da Silva, 2013-09-23 15:46:50]

Please have a look:

@bartfab: the whole thing

@kalman: extensions modifications (it's a mechanical refactoring)

@joi: components/components_tests.gypi (should we have more owners for this
file?)

Thanks!

[Jói, 2013-09-23 15:58:43]

LGTM for components. Agreed we should have more owners for the
components_tests.gypi file.

On Mon, Sep 23, 2013 at 3:46 PM,  <joaodasilva@chromium.org> wrote:
> Reviewers: bartfab, kalman, Jói,
>
> Message:
> Please have a look:
>
> @bartfab: the whole thing
>
> @kalman: extensions modifications (it's a mechanical refactoring)
>
> @joi: components/components_tests.gypi (should we have more owners for this
> file?)
>
> Thanks!
>
> Description:
> Refactored users of PolicySchema to use the new policy::Schema class.
>
> PolicySchema was used to contain the JSON schema describing the valid values
> for
> each known policy. This is now done by the policy::Schema class, which
> supports
> loading the Chrome policy schema at startup without having to parse a JSON
> blob.
>
> BUG=270667
>
> Please review this at https://codereview.chromium.org/24367003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+141, -579 lines):
>   M chrome/browser/extensions/api/storage/managed_value_store_cache.cc
>   M chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc
>   M chrome/browser/policy/policy_domain_descriptor.h
>   M chrome/browser/policy/policy_domain_descriptor.cc
>   M chrome/browser/policy/policy_domain_descriptor_unittest.cc
>   M chrome/browser/policy/policy_loader_mac.h
>   M chrome/browser/policy/policy_loader_mac.cc
>   M chrome/browser/policy/policy_service_impl_unittest.cc
>   M chrome/browser/ui/webui/policy_ui.cc
>   M chrome/common/extensions/api/storage/storage_schema_manifest_handler.h
>   M chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc
>   M components/components_tests.gypi
>   M components/policy.gypi
>   D components/policy/core/common/policy_schema.h
>   D components/policy/core/common/policy_schema.cc
>   D components/policy/core/common/policy_schema_unittest.cc
>   M components/policy/core/common/schema.h
>   M components/policy/core/common/schema.cc
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2013-09-23 16:00:45]

extensions lgtm.

[bartfab (slow), 2013-09-24 10:03:54]

lgtm with nits

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

https://codereview.chromium.org/24367003/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:16: #include
"components/policy/core/common/schema.h"
Nit: No need for that here. policy_domain_descriptor.h already includes
schema.h.

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

https://codereview.chromium.org/24367003/diff/1/chrome/browser/ui/webui/polic...
chrome/browser/ui/webui/policy_ui.cc:585: for (; !it_policies.IsAtEnd();
it_policies.Advance())
You don't use it_policies outside the for loop, why not constrain it to that
scope?

[Joao da Silva, 2013-09-24 10:16:31]

Thanks for the reviews!

+dubroy to review chrome/browser/ui/webui/

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

https://codereview.chromium.org/24367003/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:16: #include
"components/policy/core/common/schema.h"
On 2013/09/24 10:03:55, bartfab wrote:
> Nit: No need for that here. policy_domain_descriptor.h already includes
> schema.h.

Done.

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

https://codereview.chromium.org/24367003/diff/1/chrome/browser/ui/webui/polic...
chrome/browser/ui/webui/policy_ui.cc:585: for (; !it_policies.IsAtEnd();
it_policies.Advance())
On 2013/09/24 10:03:55, bartfab wrote:
> You don't use it_policies outside the for loop, why not constrain it to that
> scope?

Done.

[Patrick Dubroy, 2013-09-24 10:19:16]

On 2013/09/24 10:16:31, Joao da Silva wrote:
> Thanks for the reviews!
> 
> +dubroy to review chrome/browser/ui/webui/
> 
>
https://codereview.chromium.org/24367003/diff/1/chrome/browser/policy/policy_...
> File chrome/browser/policy/policy_domain_descriptor_unittest.cc (right):
> 
>
https://codereview.chromium.org/24367003/diff/1/chrome/browser/policy/policy_...
> chrome/browser/policy/policy_domain_descriptor_unittest.cc:16: #include
> "components/policy/core/common/schema.h"
> On 2013/09/24 10:03:55, bartfab wrote:
> > Nit: No need for that here. policy_domain_descriptor.h already includes
> > schema.h.
> 
> Done.
> 
>
https://codereview.chromium.org/24367003/diff/1/chrome/browser/ui/webui/polic...
> File chrome/browser/ui/webui/policy_ui.cc (right):
> 
>
https://codereview.chromium.org/24367003/diff/1/chrome/browser/ui/webui/polic...
> chrome/browser/ui/webui/policy_ui.cc:585: for (; !it_policies.IsAtEnd();
> it_policies.Advance())
> On 2013/09/24 10:03:55, bartfab wrote:
> > You don't use it_policies outside the for loop, why not constrain it to that
> > scope?
> 
> Done.

lgtm

[commit-bot: I haz the power, 2013-09-24 10:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/24367003/14001

[commit-bot: I haz the power, 2013-09-24 15:37:23]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-24 16:09:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/24367003/14001

[commit-bot: I haz the power, 2013-09-24 20:18:56]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-25 07:20:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/24367003/14001

[commit-bot: I haz the power, 2013-09-25 07:57:10]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-25 08:07:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/24367003/14001

[commit-bot: I haz the power, 2013-09-25 09:15:40]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-25 11:43:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/24367003/14001

[commit-bot: I haz the power, 2013-09-25 13:02:09]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Joao da Silva, 2013-09-25 13:19:15]

Committed patchset #2 manually as r225181 (presubmit successful).