Make the internal storage of policy::Schemas ref counted.

Make the internal storage policy::Schemas ref counted.

The policy Schemas are used to represent the expected data types of policy
values. Making their internal storage refcounted will allow passing them to background threads
that use them to validate data being loaded, while the UI thread may be
concurrently loading a new Schema for the same component.

This change makes Schema directly reference the InternalStorage struct, so that
SchemaOwner is not necessary anymore.

BUG=270667
R=dconnelly@chromium.org, kalman@chromium.org

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

[Joao da Silva, 2013-10-29 14:55:39]

Please review:

@Daniel: whole thing. Note that it builds on
https://codereview.chromium.org/31273002, and these and other changes will go in
together after everything looks sane in the end.

@Benjamin: extensions code

@Bartosz: FYI. This makes the Schemas refcounted, to enable the larger
refactoring we discussed this morning.

Thanks!

[not at google - send to devlin, 2013-10-29 15:05:30]

extensions lgtm

[dconnelly, 2013-10-29 16:43:57]

lgtm

https://codereview.chromium.org/47513018/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/47513018/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/storage/storage_schema_manifest_handler.h:24: // If
the schema is invalid then the failure reason is stored in |error|.
mention what is returned if it's invalid

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema.cc:58: : public
base::RefCountedThreadSafe<InternalStorage> {
Can you update the CL description to explain that it isn't Schema that's
refcounted, but Schema::InternalStorage?

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

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema.h:37: Schema(const Schema& schema);
Can you add a comment explaining that the new instance and the old instance
point to the same InternalStorage?

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema_unittest.cc:108: // This test shouldn't
leak nor use invalid memory.
this relies on the sanitizer bots, right?

[Joao da Silva, 2013-10-29 20:38:41]

Thanks for the review!

https://codereview.chromium.org/47513018/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/47513018/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/storage/storage_schema_manifest_handler.h:24: // If
the schema is invalid then the failure reason is stored in |error|.
On 2013/10/29 16:43:58, dconnelly wrote:
> mention what is returned if it's invalid

Done.

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema.cc:58: : public
base::RefCountedThreadSafe<InternalStorage> {
On 2013/10/29 16:43:58, dconnelly wrote:
> Can you update the CL description to explain that it isn't Schema that's
> refcounted, but Schema::InternalStorage?

Done.

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

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema.h:37: Schema(const Schema& schema);
On 2013/10/29 16:43:58, dconnelly wrote:
> Can you add a comment explaining that the new instance and the old instance
> point to the same InternalStorage?

Done.

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/47513018/diff/1/components/policy/core/common...
components/policy/core/common/schema_unittest.cc:108: // This test shouldn't
leak nor use invalid memory.
On 2013/10/29 16:43:58, dconnelly wrote:
> this relies on the sanitizer bots, right?

Yes. There are 2 sides to this: one is that this isn't accessing invalid memory
(the test would crash), and the 2nd is that nothing is getting leaked (which the
bots would/will catch).

[Joao da Silva, 2013-11-07 12:43:58]

Committed patchset #6 manually as r233596 (presubmit successful).